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

[Ilya Maximets]

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.

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