Re: [Qemu-devel] [RFC v1 6/9] virtio-crypto: rework virtio_crypto_handle_request

[Cornelia Huck]

On Wed, 17 May 2017 12:33:20 +0200
Halil Pasic <address@hidden> 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
> >