On Tue, Mar 08, 2022 at 03:19:55AM +0000, Longpeng (Mike, Cloud Infrastructure
Service Product Dept.) wrote:
-----Original Message-----
From: Stefano Garzarella [mailto:sgarz...@redhat.com]
Sent: Monday, March 7, 2022 8:14 PM
To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
<longpe...@huawei.com>
Cc: stefa...@redhat.com; m...@redhat.com; coh...@redhat.com;
pbonz...@redhat.com; Gonglei (Arei) <arei.gong...@huawei.com>; Yechuan
<yech...@huawei.com>; Huangzhichao <huangzhic...@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:sgarz...@redhat.com]
>> Sent: Monday, March 7, 2022 4:24 PM
>> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
>> <longpe...@huawei.com>
>> Cc: stefa...@redhat.com; m...@redhat.com; coh...@redhat.com;
>> pbonz...@redhat.com; Gonglei (Arei) <arei.gong...@huawei.com>; Yechuan
>> <yech...@huawei.com>; Huangzhichao <huangzhic...@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:sgarz...@redhat.com]
>> >> Sent: Wednesday, January 19, 2022 7:31 PM
>> >> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
>> >> <longpe...@huawei.com>
>> >> Cc: stefa...@redhat.com; m...@redhat.com; coh...@redhat.com;
>> >> pbonz...@redhat.com; Gonglei (Arei) <arei.gong...@huawei.com>; Yechuan
>> >> <yech...@huawei.com>; Huangzhichao <huangzhic...@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 <longpe...@huawei.com>
>> >> >
>> >> >Implements the .realize interface.
>> >> >
>> >> >Signed-off-by: Longpeng <longpe...@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.
Yeah, I was not thinking of modifying virtio or virtio_pci core either.
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);
}
...
}
I was honestly thinking of something simpler: call qdev_realize() to
realize the VhostVdpaDevice object and then query VhostVdpaDevice for
the id and number of queues.
Something like this (untested):
diff --git a/include/hw/virtio/vdpa-dev.h b/include/hw/virtio/vdpa-dev.h
index e0482035cf..9d5f90eacc 100644
--- a/include/hw/virtio/vdpa-dev.h
+++ b/include/hw/virtio/vdpa-dev.h
@@ -25,5 +25,7 @@ struct VhostVdpaDevice {
};
uint32_t vhost_vdpa_device_get_u32(int fd, unsigned long int cmd, Error
**errp);
+uint32_t vhost_vdpa_device_get_vdev_id(VhostVdpaDevice *s);
+uint32_t vhost_vdpa_device_get_num_queues(VhostVdpaDevice *s);
#endif
diff --git a/hw/virtio/vdpa-dev-pci.c b/hw/virtio/vdpa-dev-pci.c
index 257538dbdd..5eace2f79e 100644
--- a/hw/virtio/vdpa-dev-pci.c
+++ b/hw/virtio/vdpa-dev-pci.c
@@ -43,32 +43,16 @@ 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);
- if (*errp) {
+ if (!qdev_realize(vdev, BUS(&vpci_dev->bus), 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);
- if (*errp) {
- qemu_close(fd);
- return;
- }
-
- dev->vdev.vdpa_dev_fd = fd;
+ vdev_id = vhost_vdpa_device_get_vdev_id(&dev->vdev);
vpci_dev->class_code = virtio_pci_get_class_id(vdev_id);
vpci_dev->trans_devid = virtio_pci_get_trans_devid(vdev_id);
/* one for config interrupt, one per vq */
- vpci_dev->nvectors = num_queues + 1;
- qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
+ vpci_dev->nvectors = vhost_vdpa_device_get_num_queues(&dev->vdev) + 1;
}
static void vhost_vdpa_device_pci_class_init(ObjectClass *klass, void *data)
diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
index 65511243f9..3bf3040e26 100644
--- a/hw/virtio/vdpa-dev.c
+++ b/hw/virtio/vdpa-dev.c
@@ -27,6 +27,14 @@ uint32_t vhost_vdpa_device_get_u32(int fd, unsigned long int
cmd, Error **errp)
return val;
}
+uint32_t vhost_vdpa_device_get_vdev_id(VhostVdpaDevice *s) {
+ return s->vdev_id;
+}
+
+uint32_t vhost_vdpa_device_get_num_queues(VhostVdpaDevice *s) {
+ return s->num_queues;
+}
+
static void
vhost_vdpa_device_dummy_handle_output(VirtIODevice *vdev, VirtQueue *vq)
{
Cheers,
Stefano
