| Message ID | 20210602033127.40149-1-jasowang@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] vhost-vdpa: don't initialize backend_features | expand |
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
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 >
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>
在 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> >
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;
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(-)