> -----Original Message-----
> From: Stefano Garzarella [mailto:sgarz...@redhat.com]
> Sent: Tuesday, March 8, 2022 4:41 PM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <longpe...@huawei.com>
> Cc: stefa...@redhat.com; m...@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: [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:sgarz...@redhat.com]
> >> Sent: Monday, March 7, 2022 8:14 PM
> >> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> >> <longpe...@huawei.com>
> >> Cc: stefa...@redhat.com; m...@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: [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:sgarz...@redhat.com]
> >> >> Sent: Monday, March 7, 2022 4:24 PM
> >> >> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> >> >> <longpe...@huawei.com>
> >> >> Cc: stefa...@redhat.com; m...@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: [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:sgarz...@redhat.com]
> >> >> >> Sent: Wednesday, January 19, 2022 7:31 PM
> >> >> >> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> >> >> >> <longpe...@huawei.com>
> >> >> >> Cc: stefa...@redhat.com; m...@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: [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 <longpe...@huawei.com>
> >> >> >> >
> >> >> >> >Implements the .realize interface.
> >> >> >> >
> >> >> >> >Signed-off-by: Longpeng <longpe...@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().
> static void vhost_vdpa_device_pci_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
> index 65511243f9..3bf3040e26 100644
> --- a/hw/virtio/vdpa-dev.c
> +++ b/hw/virtio/vdpa-dev.c
> @@ -27,6 +27,14 @@ uint32_t vhost_vdpa_device_get_u32(int fd, unsigned long
> int
> cmd, Error **errp)
> return val;
> }
>
> +uint32_t vhost_vdpa_device_get_vdev_id(VhostVdpaDevice *s) {
> + return s->vdev_id;
> +}
> +
> +uint32_t vhost_vdpa_device_get_num_queues(VhostVdpaDevice *s) {
> + return s->num_queues;
> +}
> +
> static void
> vhost_vdpa_device_dummy_handle_output(VirtIODevice *vdev, VirtQueue *vq)
> {
>
> Cheers,
> Stefano
