Re: [Qemu-devel] [PATCH] virtio: add support for in-order feature
On 13.08.2018 18:35, Michael S. Tsirkin wrote:
> On Mon, Aug 13, 2018 at 06:28:06PM +0300, Ilya Maximets wrote:
>> 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
>> ---
>>
>> 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
Re: [Qemu-devel] [PATCH] virtio: add support for in-order feature
On Mon, Aug 13, 2018 at 06:28:06PM +0300, Ilya Maximets wrote:
> 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
> ---
>
> 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,
> >
> >
Re: [Qemu-devel] [PATCH] virtio: add support for in-order feature
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
---
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
Re: [Qemu-devel] [PATCH] virtio: add support for in-order feature
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
> >> ---
> >>
> >> 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
Re: [Qemu-devel] [PATCH] virtio: add support for in-order feature
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
>> ---
>>
>> 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,
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.
Do you have an example where both (device and driver) are compliant
to virtio spec and something goes wrong?
> or limit to devices which use buffers in order.
Do you have a full
Re: [Qemu-devel] [PATCH] virtio: add support for in-order feature
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
> ---
>
> 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,
or limit to devices which use buffers in order.
> >
> >>>
> 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
Re: [Qemu-devel] [PATCH] virtio: add support for in-order feature
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 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
>>>
>>>
>
>
Re: [Qemu-devel] [PATCH] virtio: add support for in-order feature
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 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.
> >
> >> 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
> >
> >
Re: [Qemu-devel] [PATCH] virtio: add support for in-order feature
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
>>> ---
>>>
>>> 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_PLATFORM33
>>> +
>>> +/*
>>> + * 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
>>
>>
Re: [Qemu-devel] [PATCH] virtio: add support for in-order feature
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 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
>
>
Re: [Qemu-devel] [PATCH] virtio: add support for in-order feature
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 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?
> 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
[Qemu-devel] [PATCH] virtio: add support for in-order feature
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 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)
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_PLATFORM33
+
+/*
+ * 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
