RE: [PATCH v2 04/10] vdpa-dev: implement the instance_init/class_init interface

[longpeng2--- via <qemu-devel@nongnu.org>]

> -----Original Message-----
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Monday, March 7, 2022 5:13 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 04/10] vdpa-dev: implement the instance_init/class_init
> interface
> 
> On Mon, Mar 07, 2022 at 08:55:35AM +0000, Longpeng (Mike, Cloud Infrastructure
> Service Product Dept.) wrote:
> >
> >
> >> -----Original Message-----
> >> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> >> Sent: Monday, March 7, 2022 4:10 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 04/10] vdpa-dev: implement the
> instance_init/class_init
> >> interface
> >>
> >> Hi Longpeng,
> >>
> >> On Sat, Mar 05, 2022 at 06:06:42AM +0000, Longpeng (Mike, Cloud Infrastructure
> >> Service Product Dept.) wrote:
> >> >Hi Stefano,
> >> >
> >> >> -----Original Message-----
> >> >> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> >> >> Sent: Wednesday, January 19, 2022 7: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 04/10] vdpa-dev: implement the
> >> instance_init/class_init
> >> >> interface
> >> >>
> >> >> On Mon, Jan 17, 2022 at 08:43:25PM +0800, Longpeng(Mike) via wrote:
> >> >> >From: Longpeng <longpeng2@huawei.com>
> >> >> >
> >> >> >Implements the .instance_init and the .class_init interface.
> >> >> >
> >> >> >Signed-off-by: Longpeng <longpeng2@huawei.com>
> >> >> >---
> >> >> > hw/virtio/vdpa-dev-pci.c     | 52 ++++++++++++++++++++++-
> >> >> > hw/virtio/vdpa-dev.c         | 81 +++++++++++++++++++++++++++++++++++-
> >> >> > include/hw/virtio/vdpa-dev.h |  5 +++
> >> >> > 3 files changed, 134 insertions(+), 4 deletions(-)
> >> >> >
> >> >> >diff --git a/hw/virtio/vdpa-dev-pci.c b/hw/virtio/vdpa-dev-pci.c
> >> >> >index a5a7b528a9..257538dbdd 100644
> >> >> >--- a/hw/virtio/vdpa-dev-pci.c
> >> >> >+++ b/hw/virtio/vdpa-dev-pci.c
> >> >> >@@ -25,12 +25,60 @@ struct VhostVdpaDevicePCI {
> >> >> >
> >> >> > static void vhost_vdpa_device_pci_instance_init(Object *obj)
> >> >> > {
> >> >> >-    return;
> >> >> >+    VhostVdpaDevicePCI *dev = VHOST_VDPA_DEVICE_PCI(obj);
> >> >> >+
> >> >> >+    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> >> >> >+                                TYPE_VHOST_VDPA_DEVICE);
> >> >> >+    object_property_add_alias(obj, "bootindex", OBJECT(&dev->vdev),
> >> >> >+                              "bootindex");
> >> >> >+}
> >> >> >+
> >> >> >+static Property vhost_vdpa_device_pci_properties[] = {
> >> >> >+    DEFINE_PROP_END_OF_LIST(),
> >> >> >+};
> >> >> >+
> >> >> >+static void
> >> >> >+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);
> >> >>
> >> >> We should use `vdpa_dev_fd` if the user set it, and I think we should
> >> >> also check that `vdpa_dev` is not null.
> >> >>
> >> >
> >> >The user can set both 'vdpa_dev_fd' and 'vdpa_dev' now, but how
> >> >to make sure the 'vdpa_dev_fd' is really a FD of the 'vdpa_dev' ?
> >> >Maybe we should remove 'vdpa_dev_fd' from
> >> >'vhost_vdpa_device_properties',
> >> >so the user can only set 'vdpa_dev'.
> >>
> >> This is the same problem that would happen if the user passed a path any
> >> file or device (e.g. /dev/null). I believe that on the first operation
> >> on it (e.g. an ioctl) we would get an error and exit.
> >>
> >
> >Yes, but how about the 'vdpa_dev_fd' refers to /dev/vhost-vdpa-0 but
> >the 'vdpa_dev' refers to /dev/vhost-vdpa-1 ? Should we need to consider
> >this case ?
> 
> I think we can do as we already do in vhost-scsi and vhost-vsock. If fd
> is set we use that otherwise we use path.
> 
> If we want we can also print an error and exit if both are set, since
> IMHO should be set only one of the two.
> 

Make sense, I'll try. Thanks.

> Thanks,
> Stefano