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

[Halil Pasic]

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
>