Re: [PATCH v2 05/10] vdpa-dev: implement the realize interface

[Stefano Garzarella <sgarzare@redhat.com>]

On Tue, Mar 08, 2022 at 09:42:25AM +0000, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote:
>
>
>> -----Original Message-----
>> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
>> Sent: Tuesday, March 8, 2022 4:41 PM
>> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
>> <longpeng2@huawei.com>
>> Cc: stefanha@redhat.com; mst@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: [PATCH v2 05/10] vdpa-dev: implement the realize interface
>>
>> On Tue, Mar 08, 2022 at 03:19:55AM +0000, Longpeng (Mike, Cloud Infrastructure
>> Service Product Dept.) wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
>> >> Sent: Monday, March 7, 2022 8:14 PM
>> >> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
>> >> <longpeng2@huawei.com>
>> >> Cc: stefanha@redhat.com; mst@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: [PATCH v2 05/10] vdpa-dev: implement the realize interface
>> >>
>> >> On Mon, Mar 07, 2022 at 11:13:02AM +0000, Longpeng (Mike, Cloud Infrastructure
>> >> Service Product Dept.) wrote:
>> >> >
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
>> >> >> Sent: Monday, March 7, 2022 4:24 PM
>> >> >> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
>> >> >> <longpeng2@huawei.com>
>> >> >> Cc: stefanha@redhat.com; mst@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: [PATCH v2 05/10] vdpa-dev: implement the realize interface
>> >> >>
>> >> >> On Sat, Mar 05, 2022 at 07:07:54AM +0000, Longpeng (Mike, Cloud
>> Infrastructure
>> >> >> Service Product Dept.) wrote:
>> >> >> >
>> >> >> >
>> >> >> >> -----Original Message-----
>> >> >> >> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
>> >> >> >> Sent: Wednesday, January 19, 2022 7:31 PM
>> >> >> >> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
>> >> >> >> <longpeng2@huawei.com>
>> >> >> >> Cc: stefanha@redhat.com; mst@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: [PATCH v2 05/10] vdpa-dev: implement the realize interface
>> >> >> >>
>> >> >> >> On Mon, Jan 17, 2022 at 08:43:26PM +0800, Longpeng(Mike) via wrote:
>> >> >> >> >From: Longpeng <longpeng2@huawei.com>
>> >> >> >> >
>> >> >> >> >Implements the .realize interface.
>> >> >> >> >
>> >> >> >> >Signed-off-by: Longpeng <longpeng2@huawei.com>
>> >> >> >> >---
>> >> >> >> > hw/virtio/vdpa-dev.c         | 101
>> +++++++++++++++++++++++++++++++++++
>> >> >> >> > include/hw/virtio/vdpa-dev.h |   8 +++
>> >> >> >> > 2 files changed, 109 insertions(+)
>> >> >> >> >
>> >> >> >> >diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
>> >> >> >> >index b103768f33..bd28cf7a15 100644
>> >> >> >> >--- a/hw/virtio/vdpa-dev.c
>> >> >> >> >+++ b/hw/virtio/vdpa-dev.c
>> >> >> >> >@@ -27,9 +27,109 @@ uint32_t vhost_vdpa_device_get_u32(int fd, unsigned
>> >> long
>> >> >> >> int cmd, Error **errp)
>> >> >> >> >     return val;
>> >> >> >> > }
>> >> >> >> >
>> >> >> >> >+static void
>> >> >> >> >+vhost_vdpa_device_dummy_handle_output(VirtIODevice *vdev, VirtQueue
>> >> *vq)
>> >> >> >> >+{
>> >> >> >> >+    /* Nothing to do */
>> >> >> >> >+}
>> >> >> >> >+
>> >> >> >> > static void vhost_vdpa_device_realize(DeviceState *dev, Error **errp)
>> >> >> >> > {
>> >> >> >> >+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>> >> >> >> >+    VhostVdpaDevice *s = VHOST_VDPA_DEVICE(vdev);
>> >> >> >> >+    uint32_t vdev_id, max_queue_size;
>> >> >> >> >+    struct vhost_virtqueue *vqs;
>> >> >> >> >+    int i, ret;
>> >> >> >> >+
>> >> >> >> >+    if (s->vdpa_dev_fd == -1) {
>> >> >> >> >+        s->vdpa_dev_fd = qemu_open(s->vdpa_dev, O_RDWR, errp);
>> >> >> >>
>> >> >> >> So, here we are re-opening the `vdpa_dev` again (without checking if
>> it
>> >> >> >> is NULL).
>> >> >> >>
>> >> >> >> And we re-do the same ioctls already done in
>> >> >> >> vhost_vdpa_device_pci_realize(), so I think we should do them in a
>> >> >> >> single place, and that place should be here.
>> >> >> >>
>> >> >> >> So, what about doing all the ioctls here, setting appropriate fields
>> in
>> >> >> >> VhostVdpaDevice, then using that fields in
>> >> >> >> vhost_vdpa_device_pci_realize() after qdev_realize() to set
>> >> >> >> `class_code`, `trans_devid`, and `nvectors`?
>> >> >> >>
>> >> >> >
>> >> >> >vhost_vdpa_device_pci_realize()
>> >> >> >  qdev_realize()
>> >> >> >    virtio_device_realize()
>> >> >> >      vhost_vdpa_device_realize()
>> >> >> >      virtio_bus_device_plugged()
>> >> >> >        virtio_pci_device_plugged()
>> >> >> >
>> >> >> >These three fields would be used in virtio_pci_device_plugged(), so it's
>> >> too
>> >> >> >late to set them after qdev_realize().  And they belong to VirtIOPCIProxy,
>> >> so
>> >> >> >we cannot set them in vhost_vdpa_device_realize() which is
>> >> >> >transport layer
>> >> >> >independent.
>> >> >>
>> >> >> Maybe I expressed myself wrong, I was saying to open the file and make
>> >> >> ioctls in vhost_vdpa_device_realize(). Save the values we use on both
>> >> >> sides in VhostVdpaDevice (e.g. num_queues, queue_size) and use these
>> >> >> saved values in virtio_pci_device_plugged() without re-opening the file
>> >> >> again.
>> >> >>
>> >> >
>> >> >This means we need to access VhostVdpaDevice in virtio_pci_device_plugged()?
>> >>
>> >> Yep, or implement some functions to get those values.
>> >>
>> >
>> >I prefer not to modify the VIRTIO or the VIRTIO_PCI core too much.
>>
>> Yeah, I was not thinking of modifying virtio or virtio_pci core either.
>>
>> >How about the following proposal?
>> >
>> >struct VhostVdpaDevice {
>> >    ...
>> >    void (*post_init)(VhostVdpaDevice *vdpa_dev);
>> >    ...
>> >}
>> >
>> >vhost_vdpa_device_pci_post_init(VhostVdpaDevice *vdpa_dev)
>> >{
>> >    ...
>> >    vpci_dev->class_code = virtio_pci_get_class_id(vdpa_dev->vdev_id);
>> >    vpci_dev->trans_devid =
>> >    virtio_pci_get_trans_devid(vdpa_dev->vdev_id);
>> >    vpci_dev->nvectors = vdpa_dev->num_queues + 1;
>> >    ...
>> >}
>> >
>> >vhost_vdpa_device_pci_realize():
>> >    post_init = vhost_vdpa_device_pci_post_init;
>> >
>> >vhost_vdpa_device_realize()
>> >{
>> >    ...
>> >    Open the file.
>> >    Set vdpa_dev->vdev_id, vdpa_dev->vdev_id, vdpa_dev->num_queues
>> >    ...
>> >    if (vdpa_dev->post_init) {
>> >        vdpa_dev->post_init(vdpa_dev);
>> >    }
>> >    ...
>> >}
>>
>> I was honestly thinking of something simpler: call qdev_realize() to
>> realize the VhostVdpaDevice object and then query VhostVdpaDevice for
>> the id and number of queues.
>>
>> Something like this (untested):
>>
>> diff --git a/include/hw/virtio/vdpa-dev.h b/include/hw/virtio/vdpa-dev.h
>> index e0482035cf..9d5f90eacc 100644
>> --- a/include/hw/virtio/vdpa-dev.h
>> +++ b/include/hw/virtio/vdpa-dev.h
>> @@ -25,5 +25,7 @@ struct VhostVdpaDevice {
>>   };
>>
>>   uint32_t vhost_vdpa_device_get_u32(int fd, unsigned long int cmd, Error
>> **errp);
>> +uint32_t vhost_vdpa_device_get_vdev_id(VhostVdpaDevice *s);
>> +uint32_t vhost_vdpa_device_get_num_queues(VhostVdpaDevice *s);
>>
>>   #endif
>> diff --git a/hw/virtio/vdpa-dev-pci.c b/hw/virtio/vdpa-dev-pci.c
>> index 257538dbdd..5eace2f79e 100644
>> --- a/hw/virtio/vdpa-dev-pci.c
>> +++ b/hw/virtio/vdpa-dev-pci.c
>> @@ -43,32 +43,16 @@ vhost_vdpa_device_pci_realize(VirtIOPCIProxy *vpci_dev,
>> Error **errp)
>>       VhostVdpaDevicePCI *dev = VHOST_VDPA_DEVICE_PCI(vpci_dev);
>>       DeviceState *vdev = DEVICE(&dev->vdev);
>>       uint32_t vdev_id;
>> -    uint32_t num_queues;
>> -    int fd;
>>
>> -    fd = qemu_open(dev->vdev.vdpa_dev, O_RDWR, errp);
>> -    if (*errp) {
>> +    if (!qdev_realize(vdev, BUS(&vpci_dev->bus), errp)) {
>>           return;
>>       }
>>
>> -    vdev_id = vhost_vdpa_device_get_u32(fd, VHOST_VDPA_GET_DEVICE_ID, errp);
>> -    if (*errp) {
>> -        qemu_close(fd);
>> -        return;
>> -    }
>> -
>> -    num_queues = vhost_vdpa_device_get_u32(fd, VHOST_VDPA_GET_VQS_NUM, errp);
>> -    if (*errp) {
>> -        qemu_close(fd);
>> -        return;
>> -    }
>> -
>> -    dev->vdev.vdpa_dev_fd = fd;
>> +    vdev_id = vhost_vdpa_device_get_vdev_id(&dev->vdev);
>>       vpci_dev->class_code = virtio_pci_get_class_id(vdev_id);
>>       vpci_dev->trans_devid = virtio_pci_get_trans_devid(vdev_id);
>>       /* one for config interrupt, one per vq */
>> -    vpci_dev->nvectors = num_queues + 1;
>> -    qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
>> +    vpci_dev->nvectors = vhost_vdpa_device_get_num_queues(&dev->vdev) + 1;
>>   }
>>
>
>It may be too late to set these fields.
>
>In fact, qdev_realize() calls vhost_vdpa_device_realize() first and then
>calls virtio_pci_device_plugged() which would use class_code, trans_devid
>and nvectors, so we need to make sure they're set before invoking
>virtio_pci_device_plugged().

Right, so let's go with your solution unless someone has a better idea.

Thanks,
Stefano