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 <i.maxim...@samsung.com> >>>> --- >>>> >>>> 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 >>> >>> > >