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

[Halil Pasic <pasic@linux.vnet.ibm.com>]

On 05/17/2017 01:10 PM, Cornelia Huck wrote:
> On Wed, 17 May 2017 12:33:20 +0200
> Halil Pasic <pasic@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
>>>
> 
>