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

[Stefano Garzarella]

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.

I think we should allow to specify an fd (as we already do for other 
vhost devices), because it's a common use case when qemu is launched 
from higher layers (e.g. libvirt).

> > >+    if (*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);
> >                                                   ^
> > The build fails here, I think this should be VHOST_VDPA_GET_VQS_COUNT
>
> Yes, I sent a wrong version, I'll send v3 later.

Thanks,
Stefano