RE: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature

[Wentao Jia]

Hi, Eugenio

Thanks for you comments

It is a dilemma, our team mainly work on smartNIC vDPA, features implementation 
in the QEMU emulated devices is a certain workload for us.
I have a proposal, clear these features except vhost, it will not affect 
emulated devices, do you agree the change?

partial codes for clear these features
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 7a2846fa1c..f4cf8b74da 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -822,6 +822,8 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, 
uint64_t features,
     }

     if (!get_vhost_net(nc->peer)) {
+        virtio_clear_feature(&features, VIRTIO_F_IN_ORDER);
+        virtio_clear_feature(&features, VIRTIO_F_NOTIFICATION_DATA);
         return features;
     }

Best Regards
Wentao Jia

-----Original Message-----
From: Eugenio Perez Martin <eperezma@redhat.com> 
Sent: Saturday, January 27, 2024 2:04 AM
To: Wentao Jia <wentao.jia@nephogine.com>
Cc: Rick Zhong <zhaoyong.zhong@nephogine.com>; qemu-devel@nongnu.org; 
mst@redhat.com; Jason Wang <jasowang@redhat.com>; Peter Xu <peterx@redhat.com>; 
Guo Zhi <qtxuning1999@sjtu.edu.cn>; Xinying Yu <xinying.yu@nephogine.com>
Subject: Re: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and 
VIRTIO_F_NOTIFICATION_DATA feature

On Fri, Jan 26, 2024 at 9:59 AM Wentao Jia <wentao.jia@nephogine.com> wrote:
>
> Hi, Eugenio
>
> Thanks for you comments, Our team has made new change about the patch, 
> these features in hw/virtio/virtio.h:DEFINE_VIRTIO_COMMON_FEATURES,
> they are turned off by default , and can be turned on from at qemu 
> command line Do you have comments about this patch?
>

If the commandline is set to =on on an emulated device, we're back at square 
one: The guest will try to use these features in the emulator device and the 
kick or the descriptors exchange will fail.

Maybe we can propose their implementation in the emulated devices on Google 
Summer of Code? Would you be interested in mentoring this? I can help with it 
for sure.

On the other hand I'm not sure about the benefits of notification_data for 
emulated devices or even vhost-kernel. My understanding is that the data 
written cannot be passed with the eventfd, so QEMU should fully vmexit to the 
iowrite (which probably is slower in the event of a lot of notifications). 
Unless we can transmit the avail idx, the device must read the avail ring 
anyway.

So the question for MST / Jason is, Is this enough justification to maybe fail 
the initialization of virtio-net-pci devices with backends different than 
vhost-user of vdpa if notification_data=on? Should this be backed by profiled 
data?

In my opinion the emulated device should implement it and be =off by default, 
just for testing the driver implementation. But maybe it can be done on top 
after the early failure?

Thanks!

> Best Regards
> Wentao Jia
>
>
> VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature are important 
> feature for dpdk vdpa packets transmitting performance, add these 
> features at vhost-user front-end to negotiation with backend.
>
> In this patch, these features are turned off by default, turn on the 
> features at qemu command line.
> ... notification_data=on,in_order=on ...
>
> Signed-off-by: Wentao Jia <wentao.jia@corigine.com>
> Signed-off-by: Xinying Yu <xinying.yu@corigine.com>
> Signed-off-by: Kyle Xu <zhenbing.xu@corigine.com>
> Reviewed-by:   Shujing Dong <shujing.dong@corigine.com>
> Reviewed-by:   Rick Zhong <zhaoyong.zhong@corigine.com>
> ---
>  hw/core/machine.c          | 2 ++
>  hw/net/vhost_net.c         | 2 ++
>  include/hw/virtio/virtio.h | 6 +++++-
>  3 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/machine.c b/hw/core/machine.c index 
> fb5afdcae4..40489c23a6 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -40,6 +40,7 @@ GlobalProperty hw_compat_8_1[] = {
>      { "ramfb", "x-migrate", "off" },
>      { "vfio-pci-nohotplug", "x-ramfb-migrate", "off" },
>      { "igb", "x-pcie-flr-init", "off" },
> +    { "virtio-device", "notification_data", "off"},
>  };
>  const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
>
> @@ -65,6 +66,7 @@ GlobalProperty hw_compat_7_1[] = {
>      { "virtio-rng-pci", "vectors", "0" },
>      { "virtio-rng-pci-transitional", "vectors", "0" },
>      { "virtio-rng-pci-non-transitional", "vectors", "0" },
> +    { "virtio-device", "in_order", "off"},
>  };
>  const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index 
> e8e1661646..211ca859a6 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -76,6 +76,8 @@ static const int user_feature_bits[] = {
>      VIRTIO_F_IOMMU_PLATFORM,
>      VIRTIO_F_RING_PACKED,
>      VIRTIO_F_RING_RESET,
> +    VIRTIO_F_IN_ORDER,
> +    VIRTIO_F_NOTIFICATION_DATA,
>      VIRTIO_NET_F_RSS,
>      VIRTIO_NET_F_HASH_REPORT,
>      VIRTIO_NET_F_GUEST_USO4,
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h 
> index c8f72850bc..e6aa10f01b 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -368,7 +368,11 @@ typedef struct VirtIORNGConf VirtIORNGConf;
>      DEFINE_PROP_BIT64("packed", _state, _field, \
>                        VIRTIO_F_RING_PACKED, false), \
>      DEFINE_PROP_BIT64("queue_reset", _state, _field, \
> -                      VIRTIO_F_RING_RESET, true)
> +                      VIRTIO_F_RING_RESET, true), \
> +    DEFINE_PROP_BIT64("in_order", _state, _field, \
> +                      VIRTIO_F_IN_ORDER, false), \
> +    DEFINE_PROP_BIT64("notification_data", _state, _field, \
> +                      VIRTIO_F_NOTIFICATION_DATA, false)
>
>  hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);  bool 
> virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
> --
> 2.31.1
>
> -----Original Message-----
> From: Rick Zhong <zhaoyong.zhong@nephogine.com>
> Sent: Friday, January 19, 2024 6:39 PM
> To: Eugenio Perez Martin <eperezma@redhat.com>; Wentao Jia 
> <wentao.jia@nephogine.com>
> Cc: qemu-devel@nongnu.org; mst@redhat.com; Jason Wang 
> <jasowang@redhat.com>; Peter Xu <peterx@redhat.com>; Guo Zhi 
> <qtxuning1999@sjtu.edu.cn>
> Subject: 回复: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and 
> VIRTIO_F_NOTIFICATION_DATA feature
>
> Hi Eugenio,
>
> Thanks for your comments. Very helpful. Wentao and I will discuss and get 
> back to you later.
>
> Also welcome for any comments from other guys.
>
> Best Regards,
> Rick Zhong
>
> -----邮件原件-----
> 发件人: Eugenio Perez Martin <eperezma@redhat.com>
> 发送时间: 2024年1月19日 18:26
> 收件人: Wentao Jia <wentao.jia@nephogine.com>
> 抄送: qemu-devel@nongnu.org; mst@redhat.com; Rick Zhong 
> <zhaoyong.zhong@nephogine.com>; Jason Wang <jasowang@redhat.com>; 
> Peter Xu <peterx@redhat.com>; Guo Zhi <qtxuning1999@sjtu.edu.cn>
> 主题: Re: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and 
> VIRTIO_F_NOTIFICATION_DATA feature
>
> On Fri, Jan 19, 2024 at 7:42 AM Wentao Jia <wentao.jia@nephogine.com> wrote:
> >
> >
> > VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature are 
> > important feature for dpdk vdpa packets transmitting performance, 
> > add the 2 features at vhost-user front-end to negotiation with backend.
> >
> > Signed-off-by: Kyle Xu <zhenbing.xu@corigine.com>
> > Signed-off-by: Wentao Jia <wentao.jia@corigine.com>
> > Reviewed-by:   Xinying Yu <xinying.yu@corigine.com>
> > Reviewed-by:   Shujing Dong <shujing.dong@corigine.com>
> > Reviewed-by:   Rick Zhong <zhaoyong.zhong@corigine.com>
> > ---
> >  hw/core/machine.c   | 2 ++
> >  hw/net/vhost_net.c  | 2 ++
> >  hw/net/virtio-net.c | 4 ++++
> >  3 files changed, 8 insertions(+)
> >
> > diff --git a/hw/core/machine.c b/hw/core/machine.c index
> > fb5afdcae4..e620f5e7d0 100644
> > --- a/hw/core/machine.c
> > +++ b/hw/core/machine.c
> > @@ -40,6 +40,7 @@ GlobalProperty hw_compat_8_1[] = {
> >      { "ramfb", "x-migrate", "off" },
> >      { "vfio-pci-nohotplug", "x-ramfb-migrate", "off" },
> >      { "igb", "x-pcie-flr-init", "off" },
> > +    { TYPE_VIRTIO_NET, "notification_data", "off"},
> >  };
>
> Assuming the default "true" in
> hw/net/virtio-net.c:virtio_net_properties is valid, this needs to be appended 
> to the array of the QEMU version that introduced the property in the 
> virtio_net_properties array, not the one that imported the macro from the 
> kernel. This allows QEMU to know that old versions have these features 
> disabled although the default set in 
> hw/net/virtio-net.c:virtio_net_properties is true when migrating from / to 
> these versions.
>
> You can check that this is added properly by migrating from / to a previous 
> version of QEMU, with the combinations of true and false.
>
> You have an example in [1] with blk devices multiqueue. CCing Peter Xu as he 
> knows more than me about this.
>
> This is very easy to miss when adding new features. Somebody who knows perl 
> should add a test in checkpath.pl similar to the warning "added, moved or 
> deleted file(s), does MAINTAINERS need updating?" when virtio properties are 
> modified :).
>
> >  const size_t hw_compat_8_1_len = G_N_ELEMENTS(hw_compat_8_1);
> >
> > @@ -65,6 +66,7 @@ GlobalProperty hw_compat_7_1[] = {
> >      { "virtio-rng-pci", "vectors", "0" },
> >      { "virtio-rng-pci-transitional", "vectors", "0" },
> >      { "virtio-rng-pci-non-transitional", "vectors", "0" },
> > +    { TYPE_VIRTIO_NET, "in_order", "off"},
> >  };
> >  const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);
> >
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index
> > e8e1661646..211ca859a6 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -76,6 +76,8 @@ static const int user_feature_bits[] = {
> >      VIRTIO_F_IOMMU_PLATFORM,
> >      VIRTIO_F_RING_PACKED,
> >      VIRTIO_F_RING_RESET,
> > +    VIRTIO_F_IN_ORDER,
> > +    VIRTIO_F_NOTIFICATION_DATA,
> >      VIRTIO_NET_F_RSS,
> >      VIRTIO_NET_F_HASH_REPORT,
> >      VIRTIO_NET_F_GUEST_USO4,
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index
> > 7a2846fa1c..dc0a028934 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -3949,6 +3949,10 @@ static Property virtio_net_properties[] = {
> >                        VIRTIO_NET_F_GUEST_USO6, true),
> >      DEFINE_PROP_BIT64("host_uso", VirtIONet, host_features,
> >                        VIRTIO_NET_F_HOST_USO, true),
> > +    DEFINE_PROP_BIT64("in_order", VirtIONet, host_features,
> > +                      VIRTIO_F_IN_ORDER, true),
> > +    DEFINE_PROP_BIT64("notification_data", VirtIONet, host_features,
> > +                      VIRTIO_F_NOTIFICATION_DATA, true),
>
> This default=true is wrong, and makes emulated devices show these features as 
> available when they're not. You can test it by running qemu with the 
> parameters:
>
> -netdev tap,id=hostnet0,vhost=off -device virtio-net-pci,netdev=hostnet0,...
>
> The emulated device must support both features before making them tunnables.
>
> On the other hand, all kinds of virtio devices can use in_order and 
> notification_data, so they should be in 
> include/hw/virtio/virtio.h:DEFINE_VIRTIO_COMMON_FEATURES. But not all of them 
> benefit from in_order. One example of this is virtio-blk. It is usual that 
> requests are completed out of order by the backend device, so my impression 
> is that in_order will hurt its performance.
> I've never profiled it though, so I may be wrong :).
>
> Long story short: Maybe in_order should be false by default, and enabled just 
> in virtio-net?
>
> You can see previous attempts of implementing this feature in qemu in [2]. 
> CCing Guo too, as I don't know if he plans to continue this work soon.
>
> Please let me know if you need any help with these!
>
> Thanks!
>
> [1] 
> https://www.qemu.org/docs/master/devel/migration/compatibility.html#ho
> w-backwards-compatibility-works [2] 
> https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02772.html
>
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > --
> >
>