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 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.
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);
    }
    ...
}

> Stefano