RE: [RFC 06/10] vdpa-dev: implement the unrealize interface

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

> -----Original Message-----
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, January 5, 2022 7:16 PM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <longpeng2@huawei.com>
> Cc: stefanha@redhat.com; mst@redhat.com; jasowang@redhat.com;
> cohuck@redhat.com; pbonzini@redhat.com; Gonglei (Arei)
> <arei.gonglei@huawei.com>; Yechuan <yechuan@huawei.com>; Huangzhichao
> <huangzhichao@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 <longpeng2@huawei.com>
> >
> >Implements the .unrealize interface.
> >
> >Signed-off-by: Longpeng <longpeng2@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
> >