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

[Stefano Garzarella <sgarzare@redhat.com>]

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