From: Longpeng <longpeng2@huawei.com>

Implements the .realize interface.

Signed-off-by: Longpeng <longpeng2@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);
+        if (*errp) {
+            return;
+        }
+    }
+    s->vdpa.device_fd = s->vdpa_dev_fd;
+
+    max_queue_size = vhost_vdpa_device_get_u32(s->vdpa_dev_fd,
+                                               VHOST_VDPA_GET_VRING_NUM, errp);
+    if (*errp) {
+        goto out;
+    }
+
+    if (s->queue_size > max_queue_size) {
+        error_setg(errp, "vhost-vdpa-device: invalid queue_size: %d (max:%d)",
+                   s->queue_size, max_queue_size);
+        goto out;
+    } else if (!s->queue_size) {
+        s->queue_size = max_queue_size;
+    }
+
+    s->num_queues = vhost_vdpa_device_get_u32(s->vdpa_dev_fd,
+                                              VHOST_VDPA_GET_VQS_NUM, errp);
+    if (*errp) {
+        goto out;
+    }
+
+    if (!s->num_queues || s->num_queues > VIRTIO_QUEUE_MAX) {
+        error_setg(errp, "invalid number of virtqueues: %u (max:%u)",
+                   s->num_queues, VIRTIO_QUEUE_MAX);
+        goto out;
+    }
+
+    s->dev.nvqs = s->num_queues;
+    vqs = g_new0(struct vhost_virtqueue, s->dev.nvqs);
+    s->dev.vqs = vqs;
+    s->dev.vq_index = 0;
+    s->dev.vq_index_end = s->dev.nvqs;
+    s->dev.backend_features = 0;
+    s->started = false;
+
+    ret = vhost_dev_init(&s->dev, &s->vdpa, VHOST_BACKEND_TYPE_VDPA, 0, NULL);
+    if (ret < 0) {
+        error_setg(errp, "vhost-vdpa-device: vhost initialization failed: %s",
+                   strerror(-ret));
+        goto free_vqs;
+    }
+
+    vdev_id = vhost_vdpa_device_get_u32(s->vdpa_dev_fd,
+                                        VHOST_VDPA_GET_DEVICE_ID, errp);
+    if (ret < 0) {
+        error_setg(errp, "vhost-vdpa-device: vhost get device id failed: %s",
+                   strerror(-ret));
+        goto vhost_cleanup;
+    }
+
+    s->config_size = vhost_vdpa_device_get_u32(s->vdpa_dev_fd,
+                                               VHOST_VDPA_GET_CONFIG_SIZE, errp);
+    if (*errp) {
+        goto vhost_cleanup;
+    }
+    s->config = g_malloc0(s->config_size);
+
+    ret = vhost_dev_get_config(&s->dev, s->config, s->config_size, NULL);
+    if (ret < 0) {
+        error_setg(errp, "vhost-vdpa-device: get config failed");
+        goto free_config;
+    }
+
+    virtio_init(vdev, "vhost-vdpa", vdev_id, s->config_size);
+
+    s->virtqs = g_new0(VirtQueue *, s->dev.nvqs);
+    for (i = 0; i < s->dev.nvqs; i++) {
+        s->virtqs[i] = virtio_add_queue(vdev, s->queue_size,
+                                        vhost_vdpa_device_dummy_handle_output);
+    }
+
     return;
+
+free_config:
+    g_free(s->config);
+vhost_cleanup:
+    vhost_dev_cleanup(&s->dev);
+free_vqs:
+    g_free(vqs);
+out:
+    qemu_close(s->vdpa_dev_fd);
+    s->vdpa_dev_fd = -1;
 }
 
 static void vhost_vdpa_device_unrealize(DeviceState *dev)
@@ -64,6 +164,7 @@ static void vhost_vdpa_device_set_status(VirtIODevice *vdev, uint8_t status)
 static Property vhost_vdpa_device_properties[] = {
     DEFINE_PROP_STRING("vdpa-dev", VhostVdpaDevice, vdpa_dev),
     DEFINE_PROP_INT32("vdpa-dev-fd", VhostVdpaDevice, vdpa_dev_fd, -1),
+    DEFINE_PROP_UINT16("queue-size", VhostVdpaDevice, queue_size, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/virtio/vdpa-dev.h b/include/hw/virtio/vdpa-dev.h
index e7ad349113..e0482035cf 100644
--- a/include/hw/virtio/vdpa-dev.h
+++ b/include/hw/virtio/vdpa-dev.h
@@ -14,6 +14,14 @@ struct VhostVdpaDevice {
     char *vdpa_dev;
     int vdpa_dev_fd;
     int32_t bootindex;
+    struct vhost_dev dev;
+    struct vhost_vdpa vdpa;
+    VirtQueue **virtqs;
+    uint8_t *config;
+    int config_size;
+    uint32_t num_queues;
+    uint16_t queue_size;
+    bool started;
 };
 
 uint32_t vhost_vdpa_device_get_u32(int fd, unsigned long int cmd, Error **errp);
-- 
2.23.0

On Mon, Jan 17, 2022 at 08:43:26PM +0800, Longpeng(Mike) via wrote:
>From: Longpeng <longpeng2@huawei.com>
>
>Implements the .realize interface.
>
>Signed-off-by: Longpeng <longpeng2@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`?

>+        if (*errp) {
>+            return;
>+        }
>+    }
>+    s->vdpa.device_fd = s->vdpa_dev_fd;
>+
>+    max_queue_size = vhost_vdpa_device_get_u32(s->vdpa_dev_fd,
>+                                               VHOST_VDPA_GET_VRING_NUM, errp);
>+    if (*errp) {
>+        goto out;
>+    }
>+
>+    if (s->queue_size > max_queue_size) {
>+        error_setg(errp, "vhost-vdpa-device: invalid queue_size: %d (max:%d)",
>+                   s->queue_size, max_queue_size);
>+        goto out;
>+    } else if (!s->queue_size) {
>+        s->queue_size = max_queue_size;
>+    }
>+
>+    s->num_queues = vhost_vdpa_device_get_u32(s->vdpa_dev_fd,
>+                                              VHOST_VDPA_GET_VQS_NUM, errp);
                                                 ^
VHOST_VDPA_GET_VQS_COUNT

>+    if (*errp) {
>+        goto out;
>+    }
>+
>+    if (!s->num_queues || s->num_queues > VIRTIO_QUEUE_MAX) {
>+        error_setg(errp, "invalid number of virtqueues: %u (max:%u)",
>+                   s->num_queues, VIRTIO_QUEUE_MAX);
>+        goto out;
>+    }
>+
>+    s->dev.nvqs = s->num_queues;
>+    vqs = g_new0(struct vhost_virtqueue, s->dev.nvqs);
>+    s->dev.vqs = vqs;
>+    s->dev.vq_index = 0;
>+    s->dev.vq_index_end = s->dev.nvqs;
>+    s->dev.backend_features = 0;
>+    s->started = false;
>+
>+    ret = vhost_dev_init(&s->dev, &s->vdpa, VHOST_BACKEND_TYPE_VDPA, 0, NULL);
>+    if (ret < 0) {
>+        error_setg(errp, "vhost-vdpa-device: vhost initialization failed: %s",
>+                   strerror(-ret));
>+        goto free_vqs;
>+    }
>+
>+    vdev_id = vhost_vdpa_device_get_u32(s->vdpa_dev_fd,
>+                                        VHOST_VDPA_GET_DEVICE_ID, errp);
>+    if (ret < 0) {
>+        error_setg(errp, "vhost-vdpa-device: vhost get device id failed: %s",
>+                   strerror(-ret));
>+        goto vhost_cleanup;
>+    }
>+
>+    s->config_size = vhost_vdpa_device_get_u32(s->vdpa_dev_fd,
>+                                               VHOST_VDPA_GET_CONFIG_SIZE, errp);
>+    if (*errp) {
>+        goto vhost_cleanup;
>+    }
>+    s->config = g_malloc0(s->config_size);
>+
>+    ret = vhost_dev_get_config(&s->dev, s->config, s->config_size, NULL);
>+    if (ret < 0) {
>+        error_setg(errp, "vhost-vdpa-device: get config failed");
>+        goto free_config;
>+    }
>+
>+    virtio_init(vdev, "vhost-vdpa", vdev_id, s->config_size);
>+
>+    s->virtqs = g_new0(VirtQueue *, s->dev.nvqs);
>+    for (i = 0; i < s->dev.nvqs; i++) {
>+        s->virtqs[i] = virtio_add_queue(vdev, s->queue_size,
>+                                        vhost_vdpa_device_dummy_handle_output);
>+    }
>+
>     return;
>+
>+free_config:
>+    g_free(s->config);
>+vhost_cleanup:
>+    vhost_dev_cleanup(&s->dev);
>+free_vqs:
>+    g_free(vqs);
>+out:
>+    qemu_close(s->vdpa_dev_fd);
>+    s->vdpa_dev_fd = -1;
> }
>
> static void vhost_vdpa_device_unrealize(DeviceState *dev)
>@@ -64,6 +164,7 @@ static void vhost_vdpa_device_set_status(VirtIODevice *vdev, uint8_t status)
> static Property vhost_vdpa_device_properties[] = {
>     DEFINE_PROP_STRING("vdpa-dev", VhostVdpaDevice, vdpa_dev),
>     DEFINE_PROP_INT32("vdpa-dev-fd", VhostVdpaDevice, vdpa_dev_fd, -1),
>+    DEFINE_PROP_UINT16("queue-size", VhostVdpaDevice, queue_size, 0),
>     DEFINE_PROP_END_OF_LIST(),
> };
>
>diff --git a/include/hw/virtio/vdpa-dev.h b/include/hw/virtio/vdpa-dev.h
>index e7ad349113..e0482035cf 100644
>--- a/include/hw/virtio/vdpa-dev.h
>+++ b/include/hw/virtio/vdpa-dev.h
>@@ -14,6 +14,14 @@ struct VhostVdpaDevice {
>     char *vdpa_dev;
>     int vdpa_dev_fd;
>     int32_t bootindex;
>+    struct vhost_dev dev;
>+    struct vhost_vdpa vdpa;
>+    VirtQueue **virtqs;
>+    uint8_t *config;
>+    int config_size;
>+    uint32_t num_queues;
>+    uint16_t queue_size;
>+    bool started;
> };
>
> uint32_t vhost_vdpa_device_get_u32(int fd, unsigned long int cmd, Error **errp);
>-- 
>2.23.0
>
>

> -----Original Message-----
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, January 19, 2022 7:31 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 05/10] vdpa-dev: implement the realize interface
> 
> On Mon, Jan 17, 2022 at 08:43:26PM +0800, Longpeng(Mike) via wrote:
> >From: Longpeng <longpeng2@huawei.com>
> >
> >Implements the .realize interface.
> >
> >Signed-off-by: Longpeng <longpeng2@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.

> >+        if (*errp) {
> >+            return;
> >+        }
> >+    }
> >+    s->vdpa.device_fd = s->vdpa_dev_fd;
> >+
> >+    max_queue_size = vhost_vdpa_device_get_u32(s->vdpa_dev_fd,
> >+                                               VHOST_VDPA_GET_VRING_NUM, errp);
> >+    if (*errp) {
> >+        goto out;
> >+    }
> >+
> >+    if (s->queue_size > max_queue_size) {
> >+        error_setg(errp, "vhost-vdpa-device: invalid queue_size: %d
> (max:%d)",
> >+                   s->queue_size, max_queue_size);
> >+        goto out;
> >+    } else if (!s->queue_size) {
> >+        s->queue_size = max_queue_size;
> >+    }
> >+
> >+    s->num_queues = vhost_vdpa_device_get_u32(s->vdpa_dev_fd,
> >+                                              VHOST_VDPA_GET_VQS_NUM, errp);
>                                                  ^
> VHOST_VDPA_GET_VQS_COUNT
> 

OK, thanks :)

> >+    if (*errp) {
> >+        goto out;
> >+    }
> >+
> >+    if (!s->num_queues || s->num_queues > VIRTIO_QUEUE_MAX) {
> >+        error_setg(errp, "invalid number of virtqueues: %u (max:%u)",
> >+                   s->num_queues, VIRTIO_QUEUE_MAX);
> >+        goto out;
> >+    }
> >+
> >+    s->dev.nvqs = s->num_queues;
> >+    vqs = g_new0(struct vhost_virtqueue, s->dev.nvqs);
> >+    s->dev.vqs = vqs;
> >+    s->dev.vq_index = 0;
> >+    s->dev.vq_index_end = s->dev.nvqs;
> >+    s->dev.backend_features = 0;
> >+    s->started = false;
> >+
> >+    ret = vhost_dev_init(&s->dev, &s->vdpa, VHOST_BACKEND_TYPE_VDPA, 0,
> NULL);
> >+    if (ret < 0) {
> >+        error_setg(errp, "vhost-vdpa-device: vhost initialization
> failed: %s",
> >+                   strerror(-ret));
> >+        goto free_vqs;
> >+    }
> >+
> >+    vdev_id = vhost_vdpa_device_get_u32(s->vdpa_dev_fd,
> >+                                        VHOST_VDPA_GET_DEVICE_ID, errp);
> >+    if (ret < 0) {
> >+        error_setg(errp, "vhost-vdpa-device: vhost get device id failed: %s",
> >+                   strerror(-ret));
> >+        goto vhost_cleanup;
> >+    }
> >+
> >+    s->config_size = vhost_vdpa_device_get_u32(s->vdpa_dev_fd,
> >+                                               VHOST_VDPA_GET_CONFIG_SIZE,
> errp);
> >+    if (*errp) {
> >+        goto vhost_cleanup;
> >+    }
> >+    s->config = g_malloc0(s->config_size);
> >+
> >+    ret = vhost_dev_get_config(&s->dev, s->config, s->config_size, NULL);
> >+    if (ret < 0) {
> >+        error_setg(errp, "vhost-vdpa-device: get config failed");
> >+        goto free_config;
> >+    }
> >+
> >+    virtio_init(vdev, "vhost-vdpa", vdev_id, s->config_size);
> >+
> >+    s->virtqs = g_new0(VirtQueue *, s->dev.nvqs);
> >+    for (i = 0; i < s->dev.nvqs; i++) {
> >+        s->virtqs[i] = virtio_add_queue(vdev, s->queue_size,
> >+
> vhost_vdpa_device_dummy_handle_output);
> >+    }
> >+
> >     return;
> >+
> >+free_config:
> >+    g_free(s->config);
> >+vhost_cleanup:
> >+    vhost_dev_cleanup(&s->dev);
> >+free_vqs:
> >+    g_free(vqs);
> >+out:
> >+    qemu_close(s->vdpa_dev_fd);
> >+    s->vdpa_dev_fd = -1;
> > }
> >
> > static void vhost_vdpa_device_unrealize(DeviceState *dev)
> >@@ -64,6 +164,7 @@ static void vhost_vdpa_device_set_status(VirtIODevice *vdev,
> uint8_t status)
> > static Property vhost_vdpa_device_properties[] = {
> >     DEFINE_PROP_STRING("vdpa-dev", VhostVdpaDevice, vdpa_dev),
> >     DEFINE_PROP_INT32("vdpa-dev-fd", VhostVdpaDevice, vdpa_dev_fd, -1),
> >+    DEFINE_PROP_UINT16("queue-size", VhostVdpaDevice, queue_size, 0),
> >     DEFINE_PROP_END_OF_LIST(),
> > };
> >
> >diff --git a/include/hw/virtio/vdpa-dev.h b/include/hw/virtio/vdpa-dev.h
> >index e7ad349113..e0482035cf 100644
> >--- a/include/hw/virtio/vdpa-dev.h
> >+++ b/include/hw/virtio/vdpa-dev.h
> >@@ -14,6 +14,14 @@ struct VhostVdpaDevice {
> >     char *vdpa_dev;
> >     int vdpa_dev_fd;
> >     int32_t bootindex;
> >+    struct vhost_dev dev;
> >+    struct vhost_vdpa vdpa;
> >+    VirtQueue **virtqs;
> >+    uint8_t *config;
> >+    int config_size;
> >+    uint32_t num_queues;
> >+    uint16_t queue_size;
> >+    bool started;
> > };
> >
> > uint32_t vhost_vdpa_device_get_u32(int fd, unsigned long int cmd, Error
> **errp);
> >--
> >2.23.0
> >
> >

On Sat, Mar 05, 2022 at 07:07:54AM +0000, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote:
>
>
>> -----Original Message-----
>> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
>> Sent: Wednesday, January 19, 2022 7:31 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 05/10] vdpa-dev: implement the realize interface
>>
>> On Mon, Jan 17, 2022 at 08:43:26PM +0800, Longpeng(Mike) via wrote:
>> >From: Longpeng <longpeng2@huawei.com>
>> >
>> >Implements the .realize interface.
>> >
>> >Signed-off-by: Longpeng <longpeng2@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.

Can't we set `class_code`, `trans_devid`, and `nvectors` after calling 
qdev_realize()?

Thanks,
Stefano

> -----Original Message-----
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Monday, March 7, 2022 4: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 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:sgarzare@redhat.com]
> >> Sent: Wednesday, January 19, 2022 7:31 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 05/10] vdpa-dev: implement the realize interface
> >>
> >> On Mon, Jan 17, 2022 at 08:43:26PM +0800, Longpeng(Mike) via wrote:
> >> >From: Longpeng <longpeng2@huawei.com>
> >> >
> >> >Implements the .realize interface.
> >> >
> >> >Signed-off-by: Longpeng <longpeng2@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()?

> Can't we set `class_code`, `trans_devid`, and `nvectors` after calling
> qdev_realize()?
> 
> Thanks,
> Stefano

On Mon, Mar 07, 2022 at 11:13:02AM +0000, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote:
>
>
>> -----Original Message-----
>> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
>> Sent: Monday, March 7, 2022 4: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 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:sgarzare@redhat.com]
>> >> Sent: Wednesday, January 19, 2022 7:31 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 05/10] vdpa-dev: implement the realize interface
>> >>
>> >> On Mon, Jan 17, 2022 at 08:43:26PM +0800, Longpeng(Mike) via wrote:
>> >> >From: Longpeng <longpeng2@huawei.com>
>> >> >
>> >> >Implements the .realize interface.
>> >> >
>> >> >Signed-off-by: Longpeng <longpeng2@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.

Stefano

> -----Original Message-----
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Monday, March 7, 2022 8:14 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 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:sgarzare@redhat.com]
> >> Sent: Monday, March 7, 2022 4: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 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:sgarzare@redhat.com]
> >> >> Sent: Wednesday, January 19, 2022 7:31 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 05/10] vdpa-dev: implement the realize interface
> >> >>
> >> >> On Mon, Jan 17, 2022 at 08:43:26PM +0800, Longpeng(Mike) via wrote:
> >> >> >From: Longpeng <longpeng2@huawei.com>
> >> >> >
> >> >> >Implements the .realize interface.
> >> >> >
> >> >> >Signed-off-by: Longpeng <longpeng2@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.
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);
    }
    ...
}

> Stefano

On Tue, Mar 08, 2022 at 03:19:55AM +0000, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote:
>
>
>> -----Original Message-----
>> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
>> Sent: Monday, March 7, 2022 8:14 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 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:sgarzare@redhat.com]
>> >> Sent: Monday, March 7, 2022 4: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 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:sgarzare@redhat.com]
>> >> >> Sent: Wednesday, January 19, 2022 7:31 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 05/10] vdpa-dev: implement the realize interface
>> >> >>
>> >> >> On Mon, Jan 17, 2022 at 08:43:26PM +0800, Longpeng(Mike) via wrote:
>> >> >> >From: Longpeng <longpeng2@huawei.com>
>> >> >> >
>> >> >> >Implements the .realize interface.
>> >> >> >
>> >> >> >Signed-off-by: Longpeng <longpeng2@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

> -----Original Message-----
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Tuesday, March 8, 2022 4:41 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 05/10] vdpa-dev: implement the realize interface
> 
> On Tue, Mar 08, 2022 at 03:19:55AM +0000, Longpeng (Mike, Cloud Infrastructure
> Service Product Dept.) wrote:
> >
> >
> >> -----Original Message-----
> >> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> >> Sent: Monday, March 7, 2022 8:14 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 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:sgarzare@redhat.com]
> >> >> Sent: Monday, March 7, 2022 4: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 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:sgarzare@redhat.com]
> >> >> >> Sent: Wednesday, January 19, 2022 7:31 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 05/10] vdpa-dev: implement the realize interface
> >> >> >>
> >> >> >> On Mon, Jan 17, 2022 at 08:43:26PM +0800, Longpeng(Mike) via wrote:
> >> >> >> >From: Longpeng <longpeng2@huawei.com>
> >> >> >> >
> >> >> >> >Implements the .realize interface.
> >> >> >> >
> >> >> >> >Signed-off-by: Longpeng <longpeng2@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;
>   }
> 

It may be too late to set these fields.

In fact, qdev_realize() calls vhost_vdpa_device_realize() first and then 
calls virtio_pci_device_plugged() which would use class_code, trans_devid 
and nvectors, so we need to make sure they're set before invoking 
virtio_pci_device_plugged().


>   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

On Tue, Mar 08, 2022 at 09:42:25AM +0000, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote:
>
>
>> -----Original Message-----
>> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
>> Sent: Tuesday, March 8, 2022 4:41 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 05/10] vdpa-dev: implement the realize interface
>>
>> On Tue, Mar 08, 2022 at 03:19:55AM +0000, Longpeng (Mike, Cloud Infrastructure
>> Service Product Dept.) wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
>> >> Sent: Monday, March 7, 2022 8:14 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 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:sgarzare@redhat.com]
>> >> >> Sent: Monday, March 7, 2022 4: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 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:sgarzare@redhat.com]
>> >> >> >> Sent: Wednesday, January 19, 2022 7:31 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 05/10] vdpa-dev: implement the realize interface
>> >> >> >>
>> >> >> >> On Mon, Jan 17, 2022 at 08:43:26PM +0800, Longpeng(Mike) via wrote:
>> >> >> >> >From: Longpeng <longpeng2@huawei.com>
>> >> >> >> >
>> >> >> >> >Implements the .realize interface.
>> >> >> >> >
>> >> >> >> >Signed-off-by: Longpeng <longpeng2@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;
>>   }
>>
>
>It may be too late to set these fields.
>
>In fact, qdev_realize() calls vhost_vdpa_device_realize() first and then
>calls virtio_pci_device_plugged() which would use class_code, trans_devid
>and nvectors, so we need to make sure they're set before invoking
>virtio_pci_device_plugged().

Right, so let's go with your solution unless someone has a better idea.

Thanks,
Stefano