Re: [Qemu-devel] [PATCH] virtio: add support for in-order feature

[Ilya Maximets]

On 13.08.2018 12:56, Michael S. Tsirkin wrote:
> On Mon, Aug 13, 2018 at 10:55:23AM +0300, Ilya Maximets wrote:
>> On 10.08.2018 22:19, Michael S. Tsirkin wrote:
>>> On Fri, Aug 10, 2018 at 02:04:47PM +0300, Ilya Maximets wrote:
>>>> On 10.08.2018 12:34, Michael S. Tsirkin wrote:
>>>>> On Fri, Aug 10, 2018 at 11:28:47AM +0300, Ilya Maximets wrote:
>>>>>> On 10.08.2018 01:51, Michael S. Tsirkin wrote:
>>>>>>> On Thu, Aug 09, 2018 at 07:54:37PM +0300, Ilya Maximets wrote:
>>>>>>>> New feature bit for in-order feature of the upcoming
>>>>>>>> virtio 1.1. It's already supported by DPDK vhost-user
>>>>>>>> and virtio implementations. These changes required to
>>>>>>>> allow feature negotiation.
>>>>>>>>
>>>>>>>> Signed-off-by: Ilya Maximets <address@hidden>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> I just wanted to test this new feature in DPDK but failed
>>>>>>>> to found required patch for QEMU side. So, I implemented it.
>>>>>>>> At least it will be helpful for someone like me, who wants
>>>>>>>> to evaluate VIRTIO_F_IN_ORDER with DPDK.
>>>>>>>>
>>>>>>>>  hw/net/vhost_net.c                             |  1 +
>>>>>>>>  include/hw/virtio/virtio.h                     | 12 +++++++-----
>>>>>>>>  include/standard-headers/linux/virtio_config.h |  7 +++++++
>>>>>>>>  3 files changed, 15 insertions(+), 5 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>>>>>>>> index e037db6..86879c5 100644
>>>>>>>> --- a/hw/net/vhost_net.c
>>>>>>>> +++ b/hw/net/vhost_net.c
>>>>>>>> @@ -78,6 +78,7 @@ static const int user_feature_bits[] = {
>>>>>>>>      VIRTIO_NET_F_MRG_RXBUF,
>>>>>>>>      VIRTIO_NET_F_MTU,
>>>>>>>>      VIRTIO_F_IOMMU_PLATFORM,
>>>>>>>> +    VIRTIO_F_IN_ORDER,
>>>>>>>>  
>>>>>>>>      /* This bit implies RARP isn't sent by QEMU out of band */
>>>>>>>>      VIRTIO_NET_F_GUEST_ANNOUNCE,
>>>>>>>> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
>>>>>>>> index 9c1fa07..a422025 100644
>>>>>>>> --- a/include/hw/virtio/virtio.h
>>>>>>>> +++ b/include/hw/virtio/virtio.h
>>>>>>>> @@ -254,16 +254,18 @@ typedef struct virtio_input_conf 
>>>>>>>> virtio_input_conf;
>>>>>>>>  typedef struct VirtIOSCSIConf VirtIOSCSIConf;
>>>>>>>>  typedef struct VirtIORNGConf VirtIORNGConf;
>>>>>>>>  
>>>>>>>> -#define DEFINE_VIRTIO_COMMON_FEATURES(_state, _field) \
>>>>>>>> +#define DEFINE_VIRTIO_COMMON_FEATURES(_state, _field)     \
>>>>>>>>      DEFINE_PROP_BIT64("indirect_desc", _state, _field,    \
>>>>>>>>                        VIRTIO_RING_F_INDIRECT_DESC, true), \
>>>>>>>>      DEFINE_PROP_BIT64("event_idx", _state, _field,        \
>>>>>>>>                        VIRTIO_RING_F_EVENT_IDX, true),     \
>>>>>>>>      DEFINE_PROP_BIT64("notify_on_empty", _state, _field,  \
>>>>>>>> -                      VIRTIO_F_NOTIFY_ON_EMPTY, true), \
>>>>>>>> -    DEFINE_PROP_BIT64("any_layout", _state, _field, \
>>>>>>>> -                      VIRTIO_F_ANY_LAYOUT, true), \
>>>>>>>> -    DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
>>>>>>>> +                      VIRTIO_F_NOTIFY_ON_EMPTY, true),    \
>>>>>>>> +    DEFINE_PROP_BIT64("any_layout", _state, _field,       \
>>>>>>>> +                      VIRTIO_F_ANY_LAYOUT, true),         \
>>>>>>>> +    DEFINE_PROP_BIT64("in_order", _state, _field,         \
>>>>>>>> +                      VIRTIO_F_IN_ORDER, true),           \
>>>>>>>> +    DEFINE_PROP_BIT64("iommu_platform", _state, _field,   \
>>>>>>>>                        VIRTIO_F_IOMMU_PLATFORM, false)
>>>>>>>
>>>>>>> Is in_order really right for all virtio devices?
>>>>>>
>>>>>> I see nothing device specific in this feature. It just specifies
>>>>>> some restrictions on the descriptors handling. All virtio devices
>>>>>> could use it to have performance benefits. Also, upcoming packed
>>>>>> rings should give a good performance boost in case of enabled
>>>>>> in-order feature. And packed rings RFC [1] implements
>>>>>> VIRTIO_F_RING_PACKED for all virtio devices. So, I see no issues
>>>>>> in enabling in-order negotiation for all of them.
>>>>>>
>>>>>> What do you think?
>>>>>>
>>>>>> [1] https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg01028.html
>>>>>>
>>>>>> Best regards, Ilya Maximets.
>>>>>
>>>>> If guest assumes in-order use of buffers but device uses them out of
>>>>> order then guest will crash. So there's a missing piece where
>>>>> you actually make devices use buffers in order when the flag is set.
>>>>
>>>> I thought that feature negotiation is the mechanism that should
>>>> protect us from situations like that. Isn't it?
>>>> If device negotiates in-order feature, when it MUST (as described
>>>> in spec) use buffers in the same order in which they have been
>>>> available.
>>>
>>> Exactly. And your patch does nothing to ensure that,
> 
> Let me elaborate. Your patch adds an in order property to
> all virtio devices. When set, guests will assume that
> all buffers are used in the order they have been made
> available. However, IIUC current virtio code in qemu
> sometimes uses buffers out of order. Therefore
> with your patch applied devices behave out of spec.
> 
> You need to either make sure the flag forces in-order
> behaviour, or limit the flag to devices which are
> in-order.
> 

OK. Finally, I got your point. Thanks for explanation.
So, we have a bunch of qemu virtio devices that doesn't
support this option.

I see 2 options that could be not so hard to implement:

1. Set the default value of 'in_order' property to 'false'.
   In this case, It'll be the users responsibility to ensure
   that device that he wants to use supports this feature.

2. Remove the "in_order" property bit and enable the feature
   for now only for virtio-net device somewhere in
   "virtio_net_get_features". Probably, only for vhost enabled
   devices, i.e. just before "vhost_net_get_features".

What do you think ?
 
> 
>> Are you requesting to validate every single ring operation?
>>
>> Anyway,
>> Buggy/malicious device is able to crash guest in a variety of ways.
>> Device that flags support of the feature, but breaks this promise,
>> IMHO, is buggy or malicious. So, why we need to protect from these
>> devices only for this particular feature flag?
>>
>> If your HW is broken, you're replacing it with a better one.
> 
> 
> This isn't what I was trying to say but in any case I'd like to point
> out that generally we would prefer guests to be able to validate device
> input at least optionally. It's useful for use-cases such as SEV.
> 
> Besides, a guest crashing in the field isn't necessarily user or
> developer friendly.
> 
> However it's a separate discussion about guest behaviours that
> would refer to a guest not host change.
> 
>> Do you have an example where both (device and driver) are compliant
>> to virtio spec and something goes wrong?
> 
> My point was that with your patch qemu isn't compliant.
> 
>>> or limit to devices which use buffers in order.
>> Do you have a full list?
>>
>> Negotiation works well with current patch applied.
> 
> I think that's only true because guests ignore the in-order
> flag even if negotiated. Once guests start relying on the
> in-order flag.
> 
>> If device doesn't
>> support feature, the driver is not able to negotiate it. If device
>> supports it, the driver is able to use this feature.
>> So, what is the point?
> 
> The point is that the feature implies a new promise about
> device behaviour. You set the flag but don't change qemu
> so it does not fulfill the promise.
> 
>> The feature flag VIRTIO_F_IN_ORDER is a common flag for all the
>> devices. If not, maybe you need to fix the virtio spec?
> 
> Any device can make the promise. The point is that current qemu
> code doesn't fulfill it for all device.
> 
>>>
>>>>>
>>>>>>>
>>>>>>>>  hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
>>>>>>>> diff --git a/include/standard-headers/linux/virtio_config.h 
>>>>>>>> b/include/standard-headers/linux/virtio_config.h
>>>>>>>> index b777069..d20398c 100644
>>>>>>>> --- a/include/standard-headers/linux/virtio_config.h
>>>>>>>> +++ b/include/standard-headers/linux/virtio_config.h
>>>>>>>> @@ -71,4 +71,11 @@
>>>>>>>>   * this is for compatibility with legacy systems.
>>>>>>>>   */
>>>>>>>>  #define VIRTIO_F_IOMMU_PLATFORM               33
>>>>>>>> +
>>>>>>>> +/*
>>>>>>>> + * Inorder feature indicates that all buffers are used by the device
>>>>>>>> + * in the same order in which they have been made available.
>>>>>>>> + */
>>>>>>>> +#define VIRTIO_F_IN_ORDER 35
>>>>>>>> +
>>>>>>>>  #endif /* _LINUX_VIRTIO_CONFIG_H */
>>>>>>>> -- 
>>>>>>>> 2.7.4
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>>
> 
>