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

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

[Wentao Jia]

[-- Attachment #1: Type: text/plain, Size: 1943 bytes --]

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<mailto:zhenbing.xu@corigine.com>
Signed-off-by: Wentao Jia wentao.jia@corigine.com<mailto:wentao.jia@corigine.com>
Reviewed-by:   Xinying Yu xinying.yu@corigine.com<mailto:xinying.yu@corigine.com>
Reviewed-by:   Shujing Dong shujing.dong@corigine.com<mailto:shujing.dong@corigine.com>
Reviewed-by:   Rick Zhong zhaoyong.zhong@corigine.com<mailto:zhaoyong.zhong@corigine.com>
---
hw/net/vhost_net.c         | 2 ++
include/hw/virtio/virtio.h | 4 ++++
2 files changed, 6 insertions(+)

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..3880b6764c 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -369,6 +369,10 @@ typedef struct VirtIORNGConf VirtIORNGConf;
                       VIRTIO_F_RING_PACKED, false), \
     DEFINE_PROP_BIT64("queue_reset", _state, _field, \
                       VIRTIO_F_RING_RESET, true)
+    DEFINE_PROP_BIT64("notification_data", _state, _field, \
+                      VIRTIO_F_NOTIFICATION_DATA, true), \
+    DEFINE_PROP_BIT64("in_order", _state, _field, \
+                      VIRTIO_F_IN_ORDER, true)

hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
bool virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
--

[-- Attachment #2: Type: text/html, Size: 7088 bytes --]

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

[Wentao Jia]

---
 hw/net/vhost_net.c | 2 ++
 1 file changed, 2 insertions(+)

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

-----Original Message-----
From: Wentao Jia 
Sent: Tuesday, January 2, 2024 1:38 PM
To: Jason Wang <jasowang@redhat.com>
Cc: mst@redhat.com; Rick Zhong <zhaoyong.zhong@nephogine.com>
Subject: RE: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature

Hi, Jason

It is good just change feature bits, I will commit a new patch, thanks

Wentao Jia

-----Original Message-----
From: Jason Wang <jasowang@redhat.com> 
Sent: Tuesday, January 2, 2024 11:24 AM
To: Wentao Jia <wentao.jia@nephogine.com>
Cc: mst@redhat.com; Rick Zhong <zhaoyong.zhong@nephogine.com>
Subject: Re: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature

On Tue, Jan 2, 2024 at 10:26 AM Wentao Jia <wentao.jia@nephogine.com> wrote:
>
> Hi, Michael  and Jason
>
>
>
> please review the patch at your convenience, thank you
>
> vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature - Patchwork (kernel.org)
>
>
>
> Wentao Jia
>
>
>
> From: Wentao Jia
> Sent: Friday, December 1, 2023 6:11 PM
> To: qemu-devel@nongnu.org
> Subject: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature
>
>
>
> 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/net/vhost_net.c         | 2 ++
>
> include/hw/virtio/virtio.h | 4 ++++
>
> 2 files changed, 6 insertions(+)
>
>
>
> 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..3880b6764c 100644
>
> --- a/include/hw/virtio/virtio.h
>
> +++ b/include/hw/virtio/virtio.h
>
> @@ -369,6 +369,10 @@ typedef struct VirtIORNGConf VirtIORNGConf;
>
>                        VIRTIO_F_RING_PACKED, false), \
>
>      DEFINE_PROP_BIT64("queue_reset", _state, _field, \
>
>                        VIRTIO_F_RING_RESET, true)
>
> +    DEFINE_PROP_BIT64("notification_data", _state, _field, \
>
> +                      VIRTIO_F_NOTIFICATION_DATA, true), \
>
> +    DEFINE_PROP_BIT64("in_order", _state, _field, \
>
> +                      VIRTIO_F_IN_ORDER, true)

Do we want compatibility support for those?

Thanks

>
>
>
> hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
>
> bool virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
>
> --

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

[Wentao Jia]

Hi, Michael and Jason

Do you have any other comments? 
Is there a schedule for merge the patch into the community?
Thank you 

Wentao

-----Original Message-----
From: Wentao Jia 
Sent: Tuesday, January 2, 2024 1:57 PM
To: qemu-devel@nongnu.org
Cc: 'mst@redhat.com' <mst@redhat.com>; Rick Zhong <zhaoyong.zhong@nephogine.com>; 'Jason Wang' <jasowang@redhat.com>
Subject: RE: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature


---
 hw/net/vhost_net.c | 2 ++
 1 file changed, 2 insertions(+)

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

-----Original Message-----
From: Wentao Jia
Sent: Tuesday, January 2, 2024 1:38 PM
To: Jason Wang <jasowang@redhat.com>
Cc: mst@redhat.com; Rick Zhong <zhaoyong.zhong@nephogine.com>
Subject: RE: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature

Hi, Jason

It is good just change feature bits, I will commit a new patch, thanks

Wentao Jia

-----Original Message-----
From: Jason Wang <jasowang@redhat.com>
Sent: Tuesday, January 2, 2024 11:24 AM
To: Wentao Jia <wentao.jia@nephogine.com>
Cc: mst@redhat.com; Rick Zhong <zhaoyong.zhong@nephogine.com>
Subject: Re: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature

On Tue, Jan 2, 2024 at 10:26 AM Wentao Jia <wentao.jia@nephogine.com> wrote:
>
> Hi, Michael  and Jason
>
>
>
> please review the patch at your convenience, thank you
>
> vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA 
> feature - Patchwork (kernel.org)
>
>
>
> Wentao Jia
>
>
>
> From: Wentao Jia
> Sent: Friday, December 1, 2023 6:11 PM
> To: qemu-devel@nongnu.org
> Subject: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and 
> VIRTIO_F_NOTIFICATION_DATA feature
>
>
>
> 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/net/vhost_net.c         | 2 ++
>
> include/hw/virtio/virtio.h | 4 ++++
>
> 2 files changed, 6 insertions(+)
>
>
>
> 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..3880b6764c 100644
>
> --- a/include/hw/virtio/virtio.h
>
> +++ b/include/hw/virtio/virtio.h
>
> @@ -369,6 +369,10 @@ typedef struct VirtIORNGConf VirtIORNGConf;
>
>                        VIRTIO_F_RING_PACKED, false), \
>
>      DEFINE_PROP_BIT64("queue_reset", _state, _field, \
>
>                        VIRTIO_F_RING_RESET, true)
>
> +    DEFINE_PROP_BIT64("notification_data", _state, _field, \
>
> +                      VIRTIO_F_NOTIFICATION_DATA, true), \
>
> +    DEFINE_PROP_BIT64("in_order", _state, _field, \
>
> +                      VIRTIO_F_IN_ORDER, true)

Do we want compatibility support for those?

Thanks

>
>
>
> hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
>
> bool virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
>
> --

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

[Jason Wang]

On Fri, Jan 12, 2024 at 4:18 PM Wentao Jia <wentao.jia@nephogine.com> wrote:
>
> Hi, Michael and Jason
>
> Do you have any other comments?
> Is there a schedule for merge the patch into the community?
> Thank you

I think as discussed, we need to add compatibility support for those features.

Thanks

>
> Wentao
>
> -----Original Message-----
> From: Wentao Jia
> Sent: Tuesday, January 2, 2024 1:57 PM
> To: qemu-devel@nongnu.org
> Cc: 'mst@redhat.com' <mst@redhat.com>; Rick Zhong <zhaoyong.zhong@nephogine.com>; 'Jason Wang' <jasowang@redhat.com>
> Subject: RE: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature
>
>
> ---
>  hw/net/vhost_net.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> 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,
> --
>
> -----Original Message-----
> From: Wentao Jia
> Sent: Tuesday, January 2, 2024 1:38 PM
> To: Jason Wang <jasowang@redhat.com>
> Cc: mst@redhat.com; Rick Zhong <zhaoyong.zhong@nephogine.com>
> Subject: RE: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature
>
> Hi, Jason
>
> It is good just change feature bits, I will commit a new patch, thanks
>
> Wentao Jia
>
> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Tuesday, January 2, 2024 11:24 AM
> To: Wentao Jia <wentao.jia@nephogine.com>
> Cc: mst@redhat.com; Rick Zhong <zhaoyong.zhong@nephogine.com>
> Subject: Re: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature
>
> On Tue, Jan 2, 2024 at 10:26 AM Wentao Jia <wentao.jia@nephogine.com> wrote:
> >
> > Hi, Michael  and Jason
> >
> >
> >
> > please review the patch at your convenience, thank you
> >
> > vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA
> > feature - Patchwork (kernel.org)
> >
> >
> >
> > Wentao Jia
> >
> >
> >
> > From: Wentao Jia
> > Sent: Friday, December 1, 2023 6:11 PM
> > To: qemu-devel@nongnu.org
> > Subject: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and
> > VIRTIO_F_NOTIFICATION_DATA feature
> >
> >
> >
> > 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/net/vhost_net.c         | 2 ++
> >
> > include/hw/virtio/virtio.h | 4 ++++
> >
> > 2 files changed, 6 insertions(+)
> >
> >
> >
> > 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..3880b6764c 100644
> >
> > --- a/include/hw/virtio/virtio.h
> >
> > +++ b/include/hw/virtio/virtio.h
> >
> > @@ -369,6 +369,10 @@ typedef struct VirtIORNGConf VirtIORNGConf;
> >
> >                        VIRTIO_F_RING_PACKED, false), \
> >
> >      DEFINE_PROP_BIT64("queue_reset", _state, _field, \
> >
> >                        VIRTIO_F_RING_RESET, true)
> >
> > +    DEFINE_PROP_BIT64("notification_data", _state, _field, \
> >
> > +                      VIRTIO_F_NOTIFICATION_DATA, true), \
> >
> > +    DEFINE_PROP_BIT64("in_order", _state, _field, \
> >
> > +                      VIRTIO_F_IN_ORDER, true)
>
> Do we want compatibility support for those?
>
> Thanks
>
> >
> >
> >
> > hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
> >
> > bool virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
> >
> > --
>

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

[Wentao Jia]

Hi, Jason

I just add two features in vhost user feature bits, The patch was tested in my environment
I do not know what the compatibility mean

Wentao

-----Original Message-----
From: Jason Wang <jasowang@redhat.com> 
Sent: Monday, January 15, 2024 8:18 AM
To: Wentao Jia <wentao.jia@nephogine.com>
Cc: qemu-devel@nongnu.org; mst@redhat.com; Rick Zhong <zhaoyong.zhong@nephogine.com>
Subject: Re: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature

On Fri, Jan 12, 2024 at 4:18 PM Wentao Jia <wentao.jia@nephogine.com> wrote:
>
> Hi, Michael and Jason
>
> Do you have any other comments?
> Is there a schedule for merge the patch into the community?
> Thank you

I think as discussed, we need to add compatibility support for those features.

Thanks

>
> Wentao
>
> -----Original Message-----
> From: Wentao Jia
> Sent: Tuesday, January 2, 2024 1:57 PM
> To: qemu-devel@nongnu.org
> Cc: 'mst@redhat.com' <mst@redhat.com>; Rick Zhong 
> <zhaoyong.zhong@nephogine.com>; 'Jason Wang' <jasowang@redhat.com>
> Subject: RE: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and 
> VIRTIO_F_NOTIFICATION_DATA feature
>
>
> ---
>  hw/net/vhost_net.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> 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,
> --
>
> -----Original Message-----
> From: Wentao Jia
> Sent: Tuesday, January 2, 2024 1:38 PM
> To: Jason Wang <jasowang@redhat.com>
> Cc: mst@redhat.com; Rick Zhong <zhaoyong.zhong@nephogine.com>
> Subject: RE: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and 
> VIRTIO_F_NOTIFICATION_DATA feature
>
> Hi, Jason
>
> It is good just change feature bits, I will commit a new patch, thanks
>
> Wentao Jia
>
> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Tuesday, January 2, 2024 11:24 AM
> To: Wentao Jia <wentao.jia@nephogine.com>
> Cc: mst@redhat.com; Rick Zhong <zhaoyong.zhong@nephogine.com>
> Subject: Re: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and 
> VIRTIO_F_NOTIFICATION_DATA feature
>
> On Tue, Jan 2, 2024 at 10:26 AM Wentao Jia <wentao.jia@nephogine.com> wrote:
> >
> > Hi, Michael  and Jason
> >
> >
> >
> > please review the patch at your convenience, thank you
> >
> > vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA 
> > feature - Patchwork (kernel.org)
> >
> >
> >
> > Wentao Jia
> >
> >
> >
> > From: Wentao Jia
> > Sent: Friday, December 1, 2023 6:11 PM
> > To: qemu-devel@nongnu.org
> > Subject: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and 
> > VIRTIO_F_NOTIFICATION_DATA feature
> >
> >
> >
> > 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/net/vhost_net.c         | 2 ++
> >
> > include/hw/virtio/virtio.h | 4 ++++
> >
> > 2 files changed, 6 insertions(+)
> >
> >
> >
> > 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..3880b6764c 100644
> >
> > --- a/include/hw/virtio/virtio.h
> >
> > +++ b/include/hw/virtio/virtio.h
> >
> > @@ -369,6 +369,10 @@ typedef struct VirtIORNGConf VirtIORNGConf;
> >
> >                        VIRTIO_F_RING_PACKED, false), \
> >
> >      DEFINE_PROP_BIT64("queue_reset", _state, _field, \
> >
> >                        VIRTIO_F_RING_RESET, true)
> >
> > +    DEFINE_PROP_BIT64("notification_data", _state, _field, \
> >
> > +                      VIRTIO_F_NOTIFICATION_DATA, true), \
> >
> > +    DEFINE_PROP_BIT64("in_order", _state, _field, \
> >
> > +                      VIRTIO_F_IN_ORDER, true)
>
> Do we want compatibility support for those?
>
> Thanks
>
> >
> >
> >
> > hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
> >
> > bool virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
> >
> > --
>

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

[Jason Wang]

On Tue, Jan 16, 2024 at 9:57 AM Wentao Jia <wentao.jia@nephogine.com> wrote:
>
> Hi, Jason
>
> I just add two features in vhost user feature bits, The patch was tested in my environment
> I do not know what the compatibility mean

For example, if you don't do that,

Migrating from 9.0 to 8.2 will break as 8.2 doesn't have those new
features at all.

Please refer hw_compat_8_2.

Thanks

>
> Wentao
>
> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Monday, January 15, 2024 8:18 AM
> To: Wentao Jia <wentao.jia@nephogine.com>
> Cc: qemu-devel@nongnu.org; mst@redhat.com; Rick Zhong <zhaoyong.zhong@nephogine.com>
> Subject: Re: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature
>
> On Fri, Jan 12, 2024 at 4:18 PM Wentao Jia <wentao.jia@nephogine.com> wrote:
> >
> > Hi, Michael and Jason
> >
> > Do you have any other comments?
> > Is there a schedule for merge the patch into the community?
> > Thank you
>
> I think as discussed, we need to add compatibility support for those features.
>
> Thanks
>
> >
> > Wentao
> >
> > -----Original Message-----
> > From: Wentao Jia
> > Sent: Tuesday, January 2, 2024 1:57 PM
> > To: qemu-devel@nongnu.org
> > Cc: 'mst@redhat.com' <mst@redhat.com>; Rick Zhong
> > <zhaoyong.zhong@nephogine.com>; 'Jason Wang' <jasowang@redhat.com>
> > Subject: RE: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and
> > VIRTIO_F_NOTIFICATION_DATA feature
> >
> >
> > ---
> >  hw/net/vhost_net.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > 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,
> > --
> >
> > -----Original Message-----
> > From: Wentao Jia
> > Sent: Tuesday, January 2, 2024 1:38 PM
> > To: Jason Wang <jasowang@redhat.com>
> > Cc: mst@redhat.com; Rick Zhong <zhaoyong.zhong@nephogine.com>
> > Subject: RE: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and
> > VIRTIO_F_NOTIFICATION_DATA feature
> >
> > Hi, Jason
> >
> > It is good just change feature bits, I will commit a new patch, thanks
> >
> > Wentao Jia
> >
> > -----Original Message-----
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Tuesday, January 2, 2024 11:24 AM
> > To: Wentao Jia <wentao.jia@nephogine.com>
> > Cc: mst@redhat.com; Rick Zhong <zhaoyong.zhong@nephogine.com>
> > Subject: Re: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and
> > VIRTIO_F_NOTIFICATION_DATA feature
> >
> > On Tue, Jan 2, 2024 at 10:26 AM Wentao Jia <wentao.jia@nephogine.com> wrote:
> > >
> > > Hi, Michael  and Jason
> > >
> > >
> > >
> > > please review the patch at your convenience, thank you
> > >
> > > vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA
> > > feature - Patchwork (kernel.org)
> > >
> > >
> > >
> > > Wentao Jia
> > >
> > >
> > >
> > > From: Wentao Jia
> > > Sent: Friday, December 1, 2023 6:11 PM
> > > To: qemu-devel@nongnu.org
> > > Subject: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and
> > > VIRTIO_F_NOTIFICATION_DATA feature
> > >
> > >
> > >
> > > 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/net/vhost_net.c         | 2 ++
> > >
> > > include/hw/virtio/virtio.h | 4 ++++
> > >
> > > 2 files changed, 6 insertions(+)
> > >
> > >
> > >
> > > 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..3880b6764c 100644
> > >
> > > --- a/include/hw/virtio/virtio.h
> > >
> > > +++ b/include/hw/virtio/virtio.h
> > >
> > > @@ -369,6 +369,10 @@ typedef struct VirtIORNGConf VirtIORNGConf;
> > >
> > >                        VIRTIO_F_RING_PACKED, false), \
> > >
> > >      DEFINE_PROP_BIT64("queue_reset", _state, _field, \
> > >
> > >                        VIRTIO_F_RING_RESET, true)
> > >
> > > +    DEFINE_PROP_BIT64("notification_data", _state, _field, \
> > >
> > > +                      VIRTIO_F_NOTIFICATION_DATA, true), \
> > >
> > > +    DEFINE_PROP_BIT64("in_order", _state, _field, \
> > >
> > > +                      VIRTIO_F_IN_ORDER, true)
> >
> > Do we want compatibility support for those?
> >
> > Thanks
> >
> > >
> > >
> > >
> > > hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
> > >
> > > bool virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
> > >
> > > --
> >
>

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

[Wentao Jia]

Hi, Jason 

The two features was defined before version 8.2, we can found them in qemu 8.2

VIRTIO_F_NOTIFICATION_DATA
https://github.com/qemu/qemu/commit/d0bf492f3877d4187d2f7d0c0abb3a2bf3104392
VIRTIO_F_IN_ORDER
https://github.com/qemu/qemu/commit/e4082063e47e9731dbeb1c26174c17f6038f577f

thank you

Wentao

-----Original Message-----
From: Jason Wang <jasowang@redhat.com> 
Sent: Tuesday, January 16, 2024 10:20 AM
To: Wentao Jia <wentao.jia@nephogine.com>
Cc: qemu-devel@nongnu.org; mst@redhat.com; Rick Zhong <zhaoyong.zhong@nephogine.com>
Subject: Re: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature

On Tue, Jan 16, 2024 at 9:57 AM Wentao Jia <wentao.jia@nephogine.com> wrote:
>
> Hi, Jason
>
> I just add two features in vhost user feature bits, The patch was 
> tested in my environment I do not know what the compatibility mean

For example, if you don't do that,

Migrating from 9.0 to 8.2 will break as 8.2 doesn't have those new features at all.

Please refer hw_compat_8_2.

Thanks

>
> Wentao
>
> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Monday, January 15, 2024 8:18 AM
> To: Wentao Jia <wentao.jia@nephogine.com>
> Cc: qemu-devel@nongnu.org; mst@redhat.com; Rick Zhong 
> <zhaoyong.zhong@nephogine.com>
> Subject: Re: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and 
> VIRTIO_F_NOTIFICATION_DATA feature
>
> On Fri, Jan 12, 2024 at 4:18 PM Wentao Jia <wentao.jia@nephogine.com> wrote:
> >
> > Hi, Michael and Jason
> >
> > Do you have any other comments?
> > Is there a schedule for merge the patch into the community?
> > Thank you
>
> I think as discussed, we need to add compatibility support for those features.
>
> Thanks
>
> >
> > Wentao
> >
> > -----Original Message-----
> > From: Wentao Jia
> > Sent: Tuesday, January 2, 2024 1:57 PM
> > To: qemu-devel@nongnu.org
> > Cc: 'mst@redhat.com' <mst@redhat.com>; Rick Zhong 
> > <zhaoyong.zhong@nephogine.com>; 'Jason Wang' <jasowang@redhat.com>
> > Subject: RE: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and 
> > VIRTIO_F_NOTIFICATION_DATA feature
> >
> >
> > ---
> >  hw/net/vhost_net.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > 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,
> > --
> >
> > -----Original Message-----
> > From: Wentao Jia
> > Sent: Tuesday, January 2, 2024 1:38 PM
> > To: Jason Wang <jasowang@redhat.com>
> > Cc: mst@redhat.com; Rick Zhong <zhaoyong.zhong@nephogine.com>
> > Subject: RE: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and 
> > VIRTIO_F_NOTIFICATION_DATA feature
> >
> > Hi, Jason
> >
> > It is good just change feature bits, I will commit a new patch, 
> > thanks
> >
> > Wentao Jia
> >
> > -----Original Message-----
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Tuesday, January 2, 2024 11:24 AM
> > To: Wentao Jia <wentao.jia@nephogine.com>
> > Cc: mst@redhat.com; Rick Zhong <zhaoyong.zhong@nephogine.com>
> > Subject: Re: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and 
> > VIRTIO_F_NOTIFICATION_DATA feature
> >
> > On Tue, Jan 2, 2024 at 10:26 AM Wentao Jia <wentao.jia@nephogine.com> wrote:
> > >
> > > Hi, Michael  and Jason
> > >
> > >
> > >
> > > please review the patch at your convenience, thank you
> > >
> > > vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA 
> > > feature - Patchwork (kernel.org)
> > >
> > >
> > >
> > > Wentao Jia
> > >
> > >
> > >
> > > From: Wentao Jia
> > > Sent: Friday, December 1, 2023 6:11 PM
> > > To: qemu-devel@nongnu.org
> > > Subject: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and 
> > > VIRTIO_F_NOTIFICATION_DATA feature
> > >
> > >
> > >
> > > 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/net/vhost_net.c         | 2 ++
> > >
> > > include/hw/virtio/virtio.h | 4 ++++
> > >
> > > 2 files changed, 6 insertions(+)
> > >
> > >
> > >
> > > 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..3880b6764c 100644
> > >
> > > --- a/include/hw/virtio/virtio.h
> > >
> > > +++ b/include/hw/virtio/virtio.h
> > >
> > > @@ -369,6 +369,10 @@ typedef struct VirtIORNGConf VirtIORNGConf;
> > >
> > >                        VIRTIO_F_RING_PACKED, false), \
> > >
> > >      DEFINE_PROP_BIT64("queue_reset", _state, _field, \
> > >
> > >                        VIRTIO_F_RING_RESET, true)
> > >
> > > +    DEFINE_PROP_BIT64("notification_data", _state, _field, \
> > >
> > > +                      VIRTIO_F_NOTIFICATION_DATA, true), \
> > >
> > > +    DEFINE_PROP_BIT64("in_order", _state, _field, \
> > >
> > > +                      VIRTIO_F_IN_ORDER, true)
> >
> > Do we want compatibility support for those?
> >
> > Thanks
> >
> > >
> > >
> > >
> > > hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
> > >
> > > bool virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);
> > >
> > > --
> >
>

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

[Wentao Jia]

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"},
 };
 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),
     DEFINE_PROP_END_OF_LIST(),
 };

--

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

[Eugenio Perez Martin]

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#how-backwards-compatibility-works
[2] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02772.html

>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> --
>

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

[Rick Zhong]

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#how-backwards-compatibility-works
[2] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02772.html

>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> --
>

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

[Wentao Jia]

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?

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#how-backwards-compatibility-works
[2] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02772.html

>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> --
>

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

[Eugenio Perez Martin]

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#how-backwards-compatibility-works
> [2] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02772.html
>
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > --
> >
>

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(),
> >  };
> >
> > --
> >
>

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

[Eugenio Perez Martin]

On Thu, Feb 1, 2024 at 11:48 AM Wentao Jia <wentao.jia@nephogine.com> wrote:
>
> 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.

That's understandable.

> I have a proposal, clear these features except vhost, it will not affect emulated devices, do you agree the change?
>

I'm not sure. Personally I think the best is to set them off by
default and raise an error if they are on and there is no vhost
backend, so QEMU knows the reason why the guest does not see the
feature flags. But maybe a warning is enough.

MST, Jason, what do you think?

> 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(),
> > >  };
> > >
> > > --
> > >
> >
>

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

[Michael S. Tsirkin]

On Thu, Feb 01, 2024 at 10:47:56AM +0000, Wentao Jia wrote:
> 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.

In this case the implementation is mostly trivial I have no idea
why argue about it - just do it.

Implementing it in software will make it possible to e.g. test
the driver without the device. Which is of value even to yourself.
IOW - tough, you want a feature in please make it consumable
by implementing a software fallback.





> 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(),
> > >  };
> > >
> > > --
> > >
> >
> 

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

[Rick Zhong]

Hi Eugenio and Michael,

Let me make it more clear about the target for this patch. Currently Corigine is developing the vDPA features on NIC which are based on the QEMU vhost-vdpa/vhost-user backend. These two virtio features are helpful in data plane performance.

In my understanding, these two virtio features are defined as part of the basic facilities of a common virtio device, which means they can be utilized by virtio-net, virtio-blk, virtio-fs... whatever backend. To implement, it is beyond the team's knowledge to handle these for all kinds of backends. So I'd prefer to set them off by default and raise an warning for other type of backends, except vhost-vdpa/vhost-user.

Best Regards,
Rick Zhong

-----邮件原件-----
发件人: Michael S. Tsirkin <mst@redhat.com> 
发送时间: 2024年2月1日 20:59
收件人: Wentao Jia <wentao.jia@nephogine.com>
抄送: Eugenio Perez Martin <eperezma@redhat.com>; Rick Zhong <zhaoyong.zhong@nephogine.com>; qemu-devel@nongnu.org; Jason Wang <jasowang@redhat.com>; Peter Xu <peterx@redhat.com>; Guo Zhi <qtxuning1999@sjtu.edu.cn>; Xinying Yu <xinying.yu@nephogine.com>
主题: Re: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature

On Thu, Feb 01, 2024 at 10:47:56AM +0000, Wentao Jia wrote:
> 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.

In this case the implementation is mostly trivial I have no idea why argue about it - just do it.

Implementing it in software will make it possible to e.g. test the driver without the device. Which is of value even to yourself.
IOW - tough, you want a feature in please make it consumable by implementing a software fallback.





> 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(),
> > >  };
> > >
> > > --
> > >
> >
> 

Re: 回复: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature

[Michael S. Tsirkin]

On Fri, Feb 02, 2024 at 10:27:33AM +0000, Rick Zhong wrote:
> Hi Eugenio and Michael,
> 
> Let me make it more clear about the target for this patch. Currently Corigine is developing the vDPA features on NIC which are based on the QEMU vhost-vdpa/vhost-user backend. These two virtio features are helpful in data plane performance.
> 
> In my understanding, these two virtio features are defined as part of the basic facilities of a common virtio device, which means they can be utilized by virtio-net, virtio-blk, virtio-fs... whatever backend. To implement, it is beyond the team's knowledge to handle these for all kinds of backends. So I'd prefer to set them off by default and raise an warning for other type of backends, except vhost-vdpa/vhost-user.
> 
> Best Regards,
> Rick Zhong

Yes, you should set it off by default.  No, just skipping implementation
won't cut it.  It is understandable that you just want your own use-case
addressed and it is annoying to get roped in to do some work on qemu.
However, such is the cost of doing this business.  If instead we add
hacks like the warning you mention then the codebase quickly becomes a
mess of special cases.  If you need this feature, you have to make it
nicely orthogonal and palatable to everyone. It is really not a lot
of coding work, mostly testing.

-- 
MST

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

[Rick Zhong]

Hi Michael,

Understood. Thanks.

Best Regards,
Rick Zhong

-----邮件原件-----
发件人: Michael S. Tsirkin <mst@redhat.com> 
发送时间: 2024年2月13日 17:53
收件人: Rick Zhong <zhaoyong.zhong@nephogine.com>
抄送: Eugenio Perez Martin <eperezma@redhat.com>; qemu-devel@nongnu.org; Jason Wang <jasowang@redhat.com>; Peter Xu <peterx@redhat.com>; Guo Zhi <qtxuning1999@sjtu.edu.cn>; Xinying Yu <xinying.yu@nephogine.com>; Wentao Jia <wentao.jia@nephogine.com>; Shujing Dong <shujing.dong@nephogine.com>; Kyle Xu <zhenbing.xu@nephogine.com>
主题: Re: 回复: FW: [PATCH] vhost-user: add VIRTIO_F_IN_ORDER and VIRTIO_F_NOTIFICATION_DATA feature

On Fri, Feb 02, 2024 at 10:27:33AM +0000, Rick Zhong wrote:
> Hi Eugenio and Michael,
> 
> Let me make it more clear about the target for this patch. Currently Corigine is developing the vDPA features on NIC which are based on the QEMU vhost-vdpa/vhost-user backend. These two virtio features are helpful in data plane performance.
> 
> In my understanding, these two virtio features are defined as part of the basic facilities of a common virtio device, which means they can be utilized by virtio-net, virtio-blk, virtio-fs... whatever backend. To implement, it is beyond the team's knowledge to handle these for all kinds of backends. So I'd prefer to set them off by default and raise an warning for other type of backends, except vhost-vdpa/vhost-user.
> 
> Best Regards,
> Rick Zhong

Yes, you should set it off by default.  No, just skipping implementation won't cut it.  It is understandable that you just want your own use-case addressed and it is annoying to get roped in to do some work on qemu.
However, such is the cost of doing this business.  If instead we add hacks like the warning you mention then the codebase quickly becomes a mess of special cases.  If you need this feature, you have to make it nicely orthogonal and palatable to everyone. It is really not a lot of coding work, mostly testing.

--
MST