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