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! 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 >
