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

[Ilya Maximets]

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.

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