On Thu, Jan 06, 2022 at 03:23:07AM +0000, Longpeng (Mike, Cloud Infrastructure
Service Product Dept.) wrote:
-----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.
Yep, if it could be useful, we can keep it.
I just realize that I forgot to free s->dev.vqs here...
Oh right, I miss it too.
We should free it also in the error path of vhost_vdpa_device_realize().
Thanks,
Stefano
