[PATCH 1/2] vhost-vdpa: don't initialize backend_features

[PATCH 1/2] vhost-vdpa: don't initialize backend_features

[Jason Wang]

We used to initialize backend_features during vhost_vdpa_init()
regardless whether or not it was supported by vhost. This will lead
the unsupported features like VIRTIO_F_IN_ORDER to be included and set
to the vhost-vdpa during vhost_dev_start. Because the
VIRTIO_F_IN_ORDER is not supported by vhost-vdpa so it won't be
advertised to guest which will break the datapath.

Fix this by not initializing the backend_features, so the
acked_features could be built only from guest features via
vhost_net_ack_features().

Fixes: 108a64818e69b ("vhost-vdpa: introduce vhost-vdpa backend")
Cc: qemu-stable@nongnu.org
Cc: Gautam Dawar <gdawar@xilinx.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/vhost-vdpa.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 01d2101d09..5fe43a4eb5 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -275,15 +275,12 @@ static void vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)
 static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque)
 {
     struct vhost_vdpa *v;
-    uint64_t features;
     assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
     trace_vhost_vdpa_init(dev, opaque);
 
     v = opaque;
     v->dev = dev;
     dev->opaque =  opaque ;
-    vhost_vdpa_call(dev, VHOST_GET_FEATURES, &features);
-    dev->backend_features = features;
     v->listener = vhost_vdpa_memory_listener;
     v->msg_type = VHOST_IOTLB_MSG_V2;
 
-- 
2.25.1

[PATCH 2/2] vhost-vdpa: remove the unused vhost_vdpa_get_acked_features()

[Jason Wang]

No user for this helper, let's remove it.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 include/net/vhost-vdpa.h | 1 -
 net/vhost-vdpa.c         | 9 ---------
 2 files changed, 10 deletions(-)

diff --git a/include/net/vhost-vdpa.h b/include/net/vhost-vdpa.h
index 45e34b7cfc..b81f9a6f2a 100644
--- a/include/net/vhost-vdpa.h
+++ b/include/net/vhost-vdpa.h
@@ -15,7 +15,6 @@
 #define TYPE_VHOST_VDPA "vhost-vdpa"
 
 struct vhost_net *vhost_vdpa_get_vhost_net(NetClientState *nc);
-uint64_t vhost_vdpa_get_acked_features(NetClientState *nc);
 
 extern const int vdpa_feature_bits[];
 
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index fe659ec9e2..8dc86332a6 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -66,15 +66,6 @@ VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
     return s->vhost_net;
 }
 
-uint64_t vhost_vdpa_get_acked_features(NetClientState *nc)
-{
-    VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
-    assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
-    s->acked_features = vhost_net_get_acked_features(s->vhost_net);
-
-    return s->acked_features;
-}
-
 static int vhost_vdpa_net_check_device_id(struct vhost_net *net)
 {
     uint32_t device_id;
-- 
2.25.1

RE: [PATCH 1/2] vhost-vdpa: don't initialize backend_features

[Gautam Dawar]

Hi Jason,

Pls see my comments inline marked by GD>>

Regards,
Gautam

-----Original Message-----
From: Jason Wang <jasowang@redhat.com> 
Sent: Wednesday, June 2, 2021 9:01 AM
To: mst@redhat.com; qemu-devel@nongnu.org
Cc: Gautam Dawar <gdawar@xilinx.com>; lulu@redhat.com; Jason Wang <jasowang@redhat.com>; qemu-stable@nongnu.org
Subject: [PATCH 1/2] vhost-vdpa: don't initialize backend_features

We used to initialize backend_features during vhost_vdpa_init() regardless whether or not it was supported by vhost. This will lead the unsupported features like VIRTIO_F_IN_ORDER to be included and set to the vhost-vdpa during vhost_dev_start. Because the VIRTIO_F_IN_ORDER is not supported by vhost-vdpa so it won't be advertised to guest which will break the datapath.

Fix this by not initializing the backend_features, so the acked_features could be built only from guest features via vhost_net_ack_features().

Fixes: 108a64818e69b ("vhost-vdpa: introduce vhost-vdpa backend")
Cc: qemu-stable@nongnu.org
Cc: Gautam Dawar <gdawar@xilinx.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/virtio/vhost-vdpa.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index 01d2101d09..5fe43a4eb5 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -275,15 +275,12 @@ static void vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)  static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque)  {
     struct vhost_vdpa *v;
-    uint64_t features;
     assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
     trace_vhost_vdpa_init(dev, opaque);
 
     v = opaque;
     v->dev = dev;
     dev->opaque =  opaque ;
-    vhost_vdpa_call(dev, VHOST_GET_FEATURES, &features);
-    dev->backend_features = features;
[GD>>] Should this be initialized with 0 here? I am not sure if  memory allocated for struct vhost_dev is initialized with 0.
     v->listener = vhost_vdpa_memory_listener;
     v->msg_type = VHOST_IOTLB_MSG_V2;
 
--
2.25.1

Re: [PATCH 1/2] vhost-vdpa: don't initialize backend_features

[Jason Wang]

Hi Gautam:

在 2021/6/2 下午3:38, Gautam Dawar 写道:
> Hi Jason,
>
> Pls see my comments inline marked by GD>>
>
> Regards,
> Gautam
>
> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, June 2, 2021 9:01 AM
> To: mst@redhat.com; qemu-devel@nongnu.org
> Cc: Gautam Dawar <gdawar@xilinx.com>; lulu@redhat.com; Jason Wang <jasowang@redhat.com>; qemu-stable@nongnu.org
> Subject: [PATCH 1/2] vhost-vdpa: don't initialize backend_features
>
> We used to initialize backend_features during vhost_vdpa_init() regardless whether or not it was supported by vhost. This will lead the unsupported features like VIRTIO_F_IN_ORDER to be included and set to the vhost-vdpa during vhost_dev_start. Because the VIRTIO_F_IN_ORDER is not supported by vhost-vdpa so it won't be advertised to guest which will break the datapath.
>
> Fix this by not initializing the backend_features, so the acked_features could be built only from guest features via vhost_net_ack_features().
>
> Fixes: 108a64818e69b ("vhost-vdpa: introduce vhost-vdpa backend")
> Cc: qemu-stable@nongnu.org
> Cc: Gautam Dawar <gdawar@xilinx.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>   hw/virtio/vhost-vdpa.c | 3 ---
>   1 file changed, 3 deletions(-)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index 01d2101d09..5fe43a4eb5 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -275,15 +275,12 @@ static void vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)  static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque)  {
>       struct vhost_vdpa *v;
> -    uint64_t features;
>       assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
>       trace_vhost_vdpa_init(dev, opaque);
>   
>       v = opaque;
>       v->dev = dev;
>       dev->opaque =  opaque ;
> -    vhost_vdpa_call(dev, VHOST_GET_FEATURES, &features);
> -    dev->backend_features = features;
> [GD>>] Should this be initialized with 0 here? I am not sure if  memory allocated for struct vhost_dev is initialized with 0.


See vhost_net_init:

     struct vhost_net *net = g_new0(struct vhost_net, 1);

vhost_dev is embedded in the vhost_net structure. So I think it should 
be zero.

Thanks


>       v->listener = vhost_vdpa_memory_listener;
>       v->msg_type = VHOST_IOTLB_MSG_V2;
>   
> --
> 2.25.1
>

RE: [PATCH 1/2] vhost-vdpa: don't initialize backend_features

[Gautam Dawar]

Hi Jason,

-----Original Message-----
From: Jason Wang <jasowang@redhat.com> 
Sent: Wednesday, June 2, 2021 2:18 PM
To: Gautam Dawar <gdawar@xilinx.com>; mst@redhat.com; qemu-devel@nongnu.org
Cc: lulu@redhat.com; qemu-stable@nongnu.org
Subject: Re: [PATCH 1/2] vhost-vdpa: don't initialize backend_features

Hi Gautam:

在 2021/6/2 下午3:38, Gautam Dawar 写道:
> Hi Jason,
>
> Pls see my comments inline marked by GD>>
>
> Regards,
> Gautam
>
> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, June 2, 2021 9:01 AM
> To: mst@redhat.com; qemu-devel@nongnu.org
> Cc: Gautam Dawar <gdawar@xilinx.com>; lulu@redhat.com; Jason Wang 
> <jasowang@redhat.com>; qemu-stable@nongnu.org
> Subject: [PATCH 1/2] vhost-vdpa: don't initialize backend_features
>
> We used to initialize backend_features during vhost_vdpa_init() regardless whether or not it was supported by vhost. This will lead the unsupported features like VIRTIO_F_IN_ORDER to be included and set to the vhost-vdpa during vhost_dev_start. Because the VIRTIO_F_IN_ORDER is not supported by vhost-vdpa so it won't be advertised to guest which will break the datapath.
>
> Fix this by not initializing the backend_features, so the acked_features could be built only from guest features via vhost_net_ack_features().
>
> Fixes: 108a64818e69b ("vhost-vdpa: introduce vhost-vdpa backend")
> Cc: qemu-stable@nongnu.org
> Cc: Gautam Dawar <gdawar@xilinx.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>   hw/virtio/vhost-vdpa.c | 3 ---
>   1 file changed, 3 deletions(-)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index 
> 01d2101d09..5fe43a4eb5 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -275,15 +275,12 @@ static void vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)  static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque)  {
>       struct vhost_vdpa *v;
> -    uint64_t features;
>       assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
>       trace_vhost_vdpa_init(dev, opaque);
>   
>       v = opaque;
>       v->dev = dev;
>       dev->opaque =  opaque ;
> -    vhost_vdpa_call(dev, VHOST_GET_FEATURES, &features);
> -    dev->backend_features = features;
> [GD>>] Should this be initialized with 0 here? I am not sure if  memory allocated for struct vhost_dev is initialized with 0.


See vhost_net_init:

     struct vhost_net *net = g_new0(struct vhost_net, 1);

vhost_dev is embedded in the vhost_net structure. So I think it should be zero.

[GD>>]  That's correct. The embedded vhost_dev structure is indeed getting cleared to 0 in vhost_net_init().
Thanks


>       v->listener = vhost_vdpa_memory_listener;
>       v->msg_type = VHOST_IOTLB_MSG_V2;
>   
> --
> 2.25.1
>

[GD>>] 
Signed-off-by: Gautam Dawar <gdawar@xilinx.com>
Acked-by: Gautam Dawar <gdawar@xilinx.com>

RE: [PATCH 2/2] vhost-vdpa: remove the unused vhost_vdpa_get_acked_features()

[Gautam Dawar]

No user for this helper, let's remove it.

[GD>>]  These patches seem unrelated to me. Do you think they should be part of one patch series?

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 include/net/vhost-vdpa.h | 1 -
 net/vhost-vdpa.c         | 9 ---------
 2 files changed, 10 deletions(-)

diff --git a/include/net/vhost-vdpa.h b/include/net/vhost-vdpa.h index 45e34b7cfc..b81f9a6f2a 100644
--- a/include/net/vhost-vdpa.h
+++ b/include/net/vhost-vdpa.h
@@ -15,7 +15,6 @@
 #define TYPE_VHOST_VDPA "vhost-vdpa"
 
 struct vhost_net *vhost_vdpa_get_vhost_net(NetClientState *nc); -uint64_t vhost_vdpa_get_acked_features(NetClientState *nc);
 
 extern const int vdpa_feature_bits[];
 
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c index fe659ec9e2..8dc86332a6 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -66,15 +66,6 @@ VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
     return s->vhost_net;
 }
 
-uint64_t vhost_vdpa_get_acked_features(NetClientState *nc) -{
-    VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
-    assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
-    s->acked_features = vhost_net_get_acked_features(s->vhost_net);
-
-    return s->acked_features;
-}
-
 static int vhost_vdpa_net_check_device_id(struct vhost_net *net)  {
     uint32_t device_id;
--
2.25.1

Re: [PATCH 1/2] vhost-vdpa: don't initialize backend_features

[Jason Wang]

在 2021/6/4 下午5:10, Gautam Dawar 写道:
> Hi Jason,
>
> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, June 2, 2021 2:18 PM
> To: Gautam Dawar <gdawar@xilinx.com>; mst@redhat.com; qemu-devel@nongnu.org
> Cc: lulu@redhat.com; qemu-stable@nongnu.org
> Subject: Re: [PATCH 1/2] vhost-vdpa: don't initialize backend_features
>
> Hi Gautam:
>
> 在 2021/6/2 下午3:38, Gautam Dawar 写道:
>> Hi Jason,
>>
>> Pls see my comments inline marked by GD>>
>>
>> Regards,
>> Gautam
>>
>> -----Original Message-----
>> From: Jason Wang <jasowang@redhat.com>
>> Sent: Wednesday, June 2, 2021 9:01 AM
>> To: mst@redhat.com; qemu-devel@nongnu.org
>> Cc: Gautam Dawar <gdawar@xilinx.com>; lulu@redhat.com; Jason Wang
>> <jasowang@redhat.com>; qemu-stable@nongnu.org
>> Subject: [PATCH 1/2] vhost-vdpa: don't initialize backend_features
>>
>> We used to initialize backend_features during vhost_vdpa_init() regardless whether or not it was supported by vhost. This will lead the unsupported features like VIRTIO_F_IN_ORDER to be included and set to the vhost-vdpa during vhost_dev_start. Because the VIRTIO_F_IN_ORDER is not supported by vhost-vdpa so it won't be advertised to guest which will break the datapath.
>>
>> Fix this by not initializing the backend_features, so the acked_features could be built only from guest features via vhost_net_ack_features().
>>
>> Fixes: 108a64818e69b ("vhost-vdpa: introduce vhost-vdpa backend")
>> Cc: qemu-stable@nongnu.org
>> Cc: Gautam Dawar <gdawar@xilinx.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>    hw/virtio/vhost-vdpa.c | 3 ---
>>    1 file changed, 3 deletions(-)
>>
>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index
>> 01d2101d09..5fe43a4eb5 100644
>> --- a/hw/virtio/vhost-vdpa.c
>> +++ b/hw/virtio/vhost-vdpa.c
>> @@ -275,15 +275,12 @@ static void vhost_vdpa_add_status(struct vhost_dev *dev, uint8_t status)  static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque)  {
>>        struct vhost_vdpa *v;
>> -    uint64_t features;
>>        assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
>>        trace_vhost_vdpa_init(dev, opaque);
>>    
>>        v = opaque;
>>        v->dev = dev;
>>        dev->opaque =  opaque ;
>> -    vhost_vdpa_call(dev, VHOST_GET_FEATURES, &features);
>> -    dev->backend_features = features;
>> [GD>>] Should this be initialized with 0 here? I am not sure if  memory allocated for struct vhost_dev is initialized with 0.
>
> See vhost_net_init:
>
>       struct vhost_net *net = g_new0(struct vhost_net, 1);
>
> vhost_dev is embedded in the vhost_net structure. So I think it should be zero.
>
> [GD>>]  That's correct. The embedded vhost_dev structure is indeed getting cleared to 0 in vhost_net_init().
> Thanks


Ok, I've queued this patch.

Thanks


>
>
>>        v->listener = vhost_vdpa_memory_listener;
>>        v->msg_type = VHOST_IOTLB_MSG_V2;
>>    
>> --
>> 2.25.1
>>
> [GD>>]
> Signed-off-by: Gautam Dawar <gdawar@xilinx.com>
> Acked-by: Gautam Dawar <gdawar@xilinx.com>
>

Re: [PATCH 2/2] vhost-vdpa: remove the unused vhost_vdpa_get_acked_features()

[Jason Wang]

在 2021/6/6 上午1:19, Gautam Dawar 写道:
> No user for this helper, let's remove it.
>
> [GD>>]  These patches seem unrelated to me. Do you think they should be part of one patch series?


It doesn't harm.

I've queued this patch.

Thanks


>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>   include/net/vhost-vdpa.h | 1 -
>   net/vhost-vdpa.c         | 9 ---------
>   2 files changed, 10 deletions(-)
>
> diff --git a/include/net/vhost-vdpa.h b/include/net/vhost-vdpa.h index 45e34b7cfc..b81f9a6f2a 100644
> --- a/include/net/vhost-vdpa.h
> +++ b/include/net/vhost-vdpa.h
> @@ -15,7 +15,6 @@
>   #define TYPE_VHOST_VDPA "vhost-vdpa"
>   
>   struct vhost_net *vhost_vdpa_get_vhost_net(NetClientState *nc); -uint64_t vhost_vdpa_get_acked_features(NetClientState *nc);
>   
>   extern const int vdpa_feature_bits[];
>   
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c index fe659ec9e2..8dc86332a6 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -66,15 +66,6 @@ VHostNetState *vhost_vdpa_get_vhost_net(NetClientState *nc)
>       return s->vhost_net;
>   }
>   
> -uint64_t vhost_vdpa_get_acked_features(NetClientState *nc) -{
> -    VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> -    assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> -    s->acked_features = vhost_net_get_acked_features(s->vhost_net);
> -
> -    return s->acked_features;
> -}
> -
>   static int vhost_vdpa_net_check_device_id(struct vhost_net *net)  {
>       uint32_t device_id;
> --
> 2.25.1
>