> -----Original Message-----
> From: Stefano Garzarella [mailto:sgarz...@redhat.com]
> Sent: Wednesday, January 5, 2022 7:16 PM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <longpe...@huawei.com>
> Cc: stefa...@redhat.com; m...@redhat.com; jasow...@redhat.com;
> coh...@redhat.com; pbonz...@redhat.com; Gonglei (Arei)
> <arei.gong...@huawei.com>; Yechuan <yech...@huawei.com>; Huangzhichao
> <huangzhic...@huawei.com>; qemu-devel@nongnu.org
> Subject: Re: [RFC 06/10] vdpa-dev: implement the unrealize interface
>
> On Wed, Jan 05, 2022 at 08:58:56AM +0800, Longpeng(Mike) wrote:
> >From: Longpeng <longpe...@huawei.com>
> >
> >Implements the .unrealize interface.
> >
> >Signed-off-by: Longpeng <longpe...@huawei.com>
> >---
> > hw/virtio/vdpa-dev.c | 22 +++++++++++++++++++++-
> > 1 file changed, 21 insertions(+), 1 deletion(-)
> >
> >diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
> >index 2d534d837a..4e4dd3d201 100644
> >--- a/hw/virtio/vdpa-dev.c
> >+++ b/hw/virtio/vdpa-dev.c
> >@@ -133,9 +133,29 @@ out:
> > close(fd);
> > }
> >
> >+static void vhost_vdpa_vdev_unrealize(VhostVdpaDevice *s)
> >+{
> >+ VirtIODevice *vdev = VIRTIO_DEVICE(s);
> >+ int i;
> >+
> >+ for (i = 0; i < s->num_queues; i++) {
> ^
> `s->num_queues` seems uninitialized to me, maybe we could just remove
> the num_queues field from VhostVdpaDevice, and use `s->dev.nvqs` as in
> vhost_vdpa_device_realize().
>
Good catch, I'll fix the bug.
But I think we should keep the num_queues field, we need it if we support
migration in the next step, right?
> >+ virtio_delete_queue(s->virtqs[i]);
> >+ }
> >+ g_free(s->virtqs);
> >+ virtio_cleanup(vdev);
> >+
> >+ g_free(s->config);
> >+}
> >+
> > static void vhost_vdpa_device_unrealize(DeviceState *dev)
> > {
> >- return;
> >+ VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >+ VhostVdpaDevice *s = VHOST_VDPA_DEVICE(vdev);
> >+
> >+ virtio_set_status(vdev, 0);
> >+ vhost_dev_cleanup(&s->dev);
>
> If we will use `s->dev.nvqs` in vhost_vdpa_vdev_unrealize(), we should
> call vhost_dev_cleanup() after it, just before close() as we already do
> in the error path of vhost_vdpa_device_realize().
>
I'll try to fix the above bug first if you agree that we should keep the
num_queues field.
I just realize that I forgot to free s->dev.vqs here...
> >+ vhost_vdpa_vdev_unrealize(s);
> >+ close(s->vdpa.device_fd);
> > }
> >
> > static void
> >--
> >2.23.0
> >
