On 05/17/2017 01:10 PM, Cornelia Huck wrote: > On Wed, 17 May 2017 12:33:20 +0200 > Halil Pasic <pa...@linux.vnet.ibm.com> wrote: > >> On 05/17/2017 12:13 PM, Gonglei (Arei) wrote: >>>> >>>> On 05/17/2017 11:12 AM, Gonglei (Arei) wrote: >>>>>>>> By the way, I'm having a hard time understing how is the requirement >>>> form >>>>>>>> >>>>>> >>>> http://docs.oasis-open.org/virtio/virtio/v1.0/cs04/virtio-v1.0-cs04.html#x1-260 >>>>>>>> 004 >>>>>>>> (2.4.4 Message Framing) satisfied by this code. Could you explain this >>>>>>>> to me please? >>>>>>>> >>>>>>> Sorry for my bad English, >>>>>>> I don't know which normative formulation the code violates? >>>>>> I'm not sure it's actually violated, but I'm concerned about the >>>>>> following >>>>>> normative statement: "The device MUST NOT make assumptions about the >>>>>> particular >>>>>> arrangement of descriptors. The device MAY have a reasonable limit of >>>>>> descriptors it will allow in a chain." >>>>>> >>>>>> Please also read the explanatory part I've linked, because the normative >>>>>> statement is pretty abstract. >>>>>> >>>>>> In my understanding, the spec says, that e.g. the virti-crypto device >>>>>> should not fail if a single request is split up into let's say two chunks >>>>>> and transmitted by the means of two top level descriptors. >>>>>> >>>>>> Do you agree with my reading of the spec? >>>>>> >>>>> Yes, I agree. But what's the relationship about the request here? >>>>> We don't assume the request have to use one desc entity, it can >>>>> use scatter-gather list for one request header. >>>>> The device can cover the situation in the QEMU. >>>>> >>>>>> What does the virtio-crypto device do if it encounters such a situation? >>>>>> >>>>> This isn't a problem. Pls see blow code segment: >>>>> >>>>> virtio_crypto_handle_request() >>>>> {... >>>>> if (unlikely(iov_to_buf(out_iov, out_num, 0, &req, sizeof(req)) >>>>> != sizeof(req))) { >>>>> virtio_error(vdev, "virtio-crypto request outhdr too short"); >>>>> return -1; >>>>> } >>>>> iov_discard_front(&out_iov, &out_num, sizeof(req)); >>>>> ... >>>>> >>>> >>>> Thats exactly what worries me. I see a call to virtio_error there... >>>> >>>> >>>> void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice *vdev, const char *fmt, >>>> ...) >>>> { >>>> va_list ap; >>>> >>>> va_start(ap, fmt); >>>> error_vreport(fmt, ap); >>>> va_end(ap); >>>> >>>> vdev->broken = true; >>>> >>>> if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) { >>>> virtio_set_status(vdev, vdev->status | >>>> VIRTIO_CONFIG_S_NEEDS_RESET); >>>> virtio_notify_config(vdev); >>>> } >>>> } >>>> >>>> This however seems to make the device 'broken'. Or am I missing something? >>>> >>> Yes, if the parse process failed, the device will broke. >>> But This is a exception scenario IMHO, which is the same situation >>> with other virtio devices. >> >> I know that virtio-blk does the same. I'm not sure my reading of the >> spec is correct. Maybe Stefan, Michael or Connie can clarify this >> for us! > > The spec says for NEEDS_RESET: > > "Indicates that the device has experienced an error from which it can't > recover." > > For me, this means: > - If this is something that might happen in the normal course of events > and there's a good recovery path, just recover. > - Else, use virtio_error() and require the driver to recover via reset. > > If the driver is sending requests in an unexpected format, I'd use > virtio_error(). The format needs to be properly described, though. >
All-clear, my bad. After re-reading the spec and virtio_pop I have realized that virtio_pop pops full descriptor chains. These however correspond to single requests. Thus at the given point the full request has to be placed into the queue. Sorry! >> >> By the way for virtio-blk the current handling was introduced by >> commit 20ea686a0 (by Greg Kurz), but before we were failing even >> harder. >> >> Regards, >> Halil >> >>> >>> Stefan introduced the virtio_error(). >>> >>> Thanks, >>> -Gonglei >>> > >
