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

From: Stefano Garzarella <sgarzare@redhat.com>
To: "Longpeng (Mike,
	Cloud Infrastructure Service Product Dept.)"
	<longpeng2@huawei.com>
Cc: "mst@redhat.com" <mst@redhat.com>,
	"cohuck@redhat.com" <cohuck@redhat.com>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
	Yechuan <yechuan@huawei.com>,
	"Gonglei \(Arei\)" <arei.gonglei@huawei.com>,
	Huangzhichao <huangzhichao@huawei.com>,
	"stefanha@redhat.com" <stefanha@redhat.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>
Subject: Re: [PATCH v2 04/10] vdpa-dev: implement the instance_init/class_init interface
Date: Mon, 7 Mar 2022 10:13:21 +0100	[thread overview]
Message-ID: <20220307091321.3mw2wsinl7xakoos@sgarzare-redhat> (raw)
In-Reply-To: <40c8c74f3bfb4527934c9e082f848593@huawei.com>

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.

Thanks,
Stefano