diff mbox series

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

Message ID 20220105005900.860-7-longpeng2@huawei.com (mailing list archive)
State New, archived
Headers show
Series add generic vDPA device support | expand

Commit Message

Zhijian Li (Fujitsu)" via Jan. 5, 2022, 12:58 a.m. UTC 
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(-)

Comments

Stefano Garzarella Jan. 5, 2022, 11:16 a.m. UTC | #1 
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().

>+        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().

>+    vhost_vdpa_vdev_unrealize(s);
>+    close(s->vdpa.device_fd);
> }
>
> static void
>-- 
>2.23.0
>
Zhijian Li (Fujitsu)" via Jan. 6, 2022, 3:23 a.m. UTC | #2 
> -----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
> >
Stefano Garzarella Jan. 10, 2022, 9:38 a.m. UTC | #3 
On Thu, Jan 06, 2022 at 03:23:07AM +0000, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote:
>
>
>> -----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.

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
diff mbox series

Patch

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++) {
+        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);
+    vhost_vdpa_vdev_unrealize(s);
+    close(s->vdpa.device_fd);
 }
 
 static void