RE: [PATCH v2 05/10] vdpa-dev: implement the realize interface

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

> -----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
> >
> >