Re: [PATCH v5 3/4] vdpa: add vdpa-dev support

[Stefano Garzarella <sgarzare@redhat.com>]

On Fri, May 13, 2022 at 10:17:41AM +0800, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote:
>
>
>在 2022/5/12 22:36, Stefano Garzarella 写道:
>>On Thu, May 12, 2022 at 02:21:02PM +0800, Longpeng(Mike) wrote:
>>>From: Longpeng <longpeng2@huawei.com>
>>>
>>>Supports vdpa-dev, we can use the deivce directly:
>>>
>>>-M microvm -m 512m -smp 2 -kernel ... -initrd ... -device \
>>>vhost-vdpa-device,vhostdev=/dev/vhost-vdpa-x
>>>
>>>Signed-off-by: Longpeng <longpeng2@huawei.com>
>>>---
>>>hw/virtio/Kconfig            |   5 +
>>>hw/virtio/meson.build        |   1 +
>>>hw/virtio/vdpa-dev.c         | 377 +++++++++++++++++++++++++++++++++++
>>>include/hw/virtio/vdpa-dev.h |  43 ++++
>>>4 files changed, 426 insertions(+)
>>>create mode 100644 hw/virtio/vdpa-dev.c
>>>create mode 100644 include/hw/virtio/vdpa-dev.h
>>>
>>>diff --git a/hw/virtio/Kconfig b/hw/virtio/Kconfig
>>>index c144d42f9b..724eb58a32 100644
>>>--- a/hw/virtio/Kconfig
>>>+++ b/hw/virtio/Kconfig
>>>@@ -68,3 +68,8 @@ config VHOST_USER_RNG
>>>    bool
>>>    default y
>>>    depends on VIRTIO && VHOST_USER
>>>+
>>>+config VHOST_VDPA_DEV
>>>+    bool
>>>+    default y
>>>+    depends on VIRTIO && VHOST_VDPA && LINUX
>>>diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
>>>index 67dc77e00f..8f6f86db71 100644
>>>--- a/hw/virtio/meson.build
>>>+++ b/hw/virtio/meson.build
>>>@@ -29,6 +29,7 @@ virtio_ss.add(when: 'CONFIG_VHOST_USER_I2C', 
>>>if_true: files('vhost-user-i2c.c'))
>>>virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 
>>>'CONFIG_VHOST_USER_I2C'], if_true: files('vhost-user-i2c-pci.c'))
>>>virtio_ss.add(when: 'CONFIG_VHOST_USER_RNG', if_true: 
>>>files('vhost-user-rng.c'))
>>>virtio_ss.add(when: ['CONFIG_VHOST_USER_RNG', 
>>>'CONFIG_VIRTIO_PCI'], if_true: files('vhost-user-rng-pci.c'))
>>>+virtio_ss.add(when: 'CONFIG_VHOST_VDPA_DEV', if_true: 
>>>files('vdpa-dev.c'))
>>>
>>>virtio_pci_ss = ss.source_set()
>>>virtio_pci_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true: 
>>>files('vhost-vsock-pci.c'))
>>>diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
>>>new file mode 100644
>>>index 0000000000..56597c881a
>>>--- /dev/null
>>>+++ b/hw/virtio/vdpa-dev.c
>>>@@ -0,0 +1,377 @@
>>>+/*
>>>+ * Vhost Vdpa Device
>>>+ *
>>>+ * Copyright (c) Huawei Technologies Co., Ltd. 2022. All Rights 
>>>Reserved.
>>>+ *
>>>+ * Authors:
>>>+ *   Longpeng <longpeng2@huawei.com>
>>>+ *
>>>+ * Largely based on the "vhost-user-blk-pci.c" and 
>>>"vhost-user-blk.c" implemented by:
>>>+ *   Changpeng Liu <changpeng.liu@intel.com>
>>>+ *
>>>+ * This work is licensed under the terms of the GNU LGPL, version 
>>>2 or later.
>>>+ * See the COPYING.LIB file in the top-level directory.
>>>+ */
>>>+#include "qemu/osdep.h"
>>>+#include <sys/ioctl.h>
>>>+#include <linux/vhost.h>
>>>+#include "qapi/error.h"
>>>+#include "qemu/error-report.h"
>>>+#include "qemu/cutils.h"
>>>+#include "hw/qdev-core.h"
>>>+#include "hw/qdev-properties.h"
>>>+#include "hw/qdev-properties-system.h"
>>>+#include "hw/virtio/vhost.h"
>>>+#include "hw/virtio/virtio.h"
>>>+#include "hw/virtio/virtio-bus.h"
>>>+#include "hw/virtio/virtio-access.h"
>>>+#include "hw/virtio/vdpa-dev.h"
>>>+#include "sysemu/sysemu.h"
>>>+#include "sysemu/runstate.h"
>>>+
>>>+static void
>>>+vhost_vdpa_device_dummy_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>>>+{
>>>+    /* Nothing to do */
>>>+}
>>>+
>>>+static uint32_t
>>>+vhost_vdpa_device_get_u32(int fd, unsigned long int cmd, Error **errp)
>>>+{
>>>+    uint32_t val = (uint32_t)-1;
>>>+
>>>+    if (ioctl(fd, cmd, &val) < 0) {
>>>+        error_setg(errp, "vhost-vdpa-device: cmd 0x%lx failed: %s",
>>>+                   cmd, strerror(errno));
>>>+    }
>>>+
>>>+    return val;
>>>+}
>>>+
>>>+static void vhost_vdpa_device_realize(DeviceState *dev, Error **errp)
>>>+{
>>>+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>>+    VhostVdpaDevice *v = VHOST_VDPA_DEVICE(vdev);
>>>+    uint16_t max_queue_size;
>>>+    struct vhost_virtqueue *vqs;
>>>+    int i, ret;
>>>+
>>>+    if (!v->vhostdev) {
>>>+        error_setg(errp, "vhost-vdpa-device: vhostdev are missing");
>>>+        return;
>>>+    }
>>>+
>>>+    v->vhostfd = qemu_open(v->vhostdev, O_RDWR, errp);
>>>+    if (*errp) {
>>>+        return;
>>>+    }
>>>+    v->vdpa.device_fd = v->vhostfd;
>>>+
>>>+    v->vdev_id = vhost_vdpa_device_get_u32(v->vhostfd,
>>>+                                           
>>>VHOST_VDPA_GET_DEVICE_ID, errp);
>>>+    if (*errp) {
>>>+        goto out;
>>>+    }
>>>+
>>>+    max_queue_size = vhost_vdpa_device_get_u32(v->vhostfd,
>>>+                                               
>>>VHOST_VDPA_GET_VRING_NUM, errp);
>>>+    if (*errp) {
>>>+        goto out;
>>>+    }
>>>+
>>>+    if (v->queue_size > max_queue_size) {
>>>+        error_setg(errp, "vhost-vdpa-device: invalid queue_size: 
>>>%u (max:%u)",
>>>+                   v->queue_size, max_queue_size);
>>>+        goto out;
>>>+    } else if (!v->queue_size) {
>>>+        v->queue_size = max_queue_size;
>>>+    }
>>>+
>>>+    v->num_queues = vhost_vdpa_device_get_u32(v->vhostfd,
>>>+                                              
>>>VHOST_VDPA_GET_VQS_COUNT, errp);
>>>+    if (*errp) {
>>>+        goto out;
>>>+    }
>>>+
>>>+    if (!v->num_queues || v->num_queues > VIRTIO_QUEUE_MAX) {
>>>+        error_setg(errp, "invalid number of virtqueues: %u (max:%u)",
>>>+                   v->num_queues, VIRTIO_QUEUE_MAX);
>>>+        goto out;
>>>+    }
>>>+
>>>+    v->dev.nvqs = v->num_queues;
>>>+    vqs = g_new0(struct vhost_virtqueue, v->dev.nvqs);
>>>+    v->dev.vqs = vqs;
>>>+    v->dev.vq_index = 0;
>>>+    v->dev.vq_index_end = v->dev.nvqs;
>>>+    v->dev.backend_features = 0;
>>>+    v->started = false;
>>>+
>>>+    ret = vhost_dev_init(&v->dev, &v->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;
>>>+    }
>>>+
>>>+    v->config_size = vhost_vdpa_device_get_u32(v->vhostfd,
>>>+                                               
>>>VHOST_VDPA_GET_CONFIG_SIZE, errp);
>>>+    if (*errp) {
>>>+        goto vhost_cleanup;
>>>+    }
>>>+
>>>+    if (v->post_init && v->post_init(v, errp) < 0) {
>>>+        goto free_virtio;
>>
>>This goto seems wrong.
>>
>>Either we use the `vhost_cleanup` label or we move this block just 
>>before return as it seems to me was in v3.
>>
>
>Good catch. We should use 'vhost_cleanup' label here.

Yep, so in that case we can remove the `free_virtio` label and the 
related block.

>
>>Out of curiosity, is there a reason for this change from v3 or was a 
>>simple mistake as a consequence of patch reorganization?
>>
>
>We should invoke .post_init() before calling virtio_init() which would 
>use the fields of VirtIOPCIProxy.

Okay, so maybe a comment there would be helpful.

Thanks,
Stefano