On 10.08.2018 11:28, 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 <[email protected]> >>> --- >>> >>> 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. >
Correction: RFC [1] does not enable VIRTIO_F_RING_PACKED by default, but VIRTIO_F_IN_ORDER makes no harm if packed rings disabled (actually, could give some performance improvement) and should give a good performance boost if packed rings enabled. Every user of packed rings will likely want to have in-order feature. So, IMHO, VIRTIO_F_IN_ORDER should be available by default. This will save the cmdline length. > 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 >> >>
