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>