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

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

> -----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()?

> Can't we set `class_code`, `trans_devid`, and `nvectors` after calling
> qdev_realize()?
> 
> Thanks,
> Stefano