[PATCH v2 00/10] add generic vDPA device support

[PATCH v2 00/10] add generic vDPA device support

[Longpeng(Mike) via]

From: Longpeng <longpeng2@huawei.com>

Hi guys,

This patchset tries to support the generic vDPA device, the previous
disscussion can be found here [1].

With the generic vDPA device, QEMU won't need to touch the device
types any more, such like vfio.

We can use the generic vDPA device as follow:
  -device vhost-vdpa-device-pci,vdpa-dev=/dev/vhost-vdpa-X

I've done some simple tests on Huawei's offloading card (net, 0.95)
and vdpa_sim_blk (1.0);

The kernel part:
  https://lkml.org/lkml/2022/1/17/239

Changes RFC -> v1
  Patch 1:
    - rename 'pdev_id' to 'trans_devid'  [Michael]
    - only use transitional device id for the devices
      listed in the spec  [Michael]
    - use macros to make the id_info table clearer  [Longpeng]
    - add some modern devices in the id_info table  [Longpeng]
  Patch 2:
    - remove the GET_VECTORS_NUM command  [Jason]
  Patch 4:
    - expose vdpa_dev_fd as a QOM preperty  [Stefan]
    - introduce vhost_vdpa_device_get_u32 as a common
      function to make the code clearer  [Stefan]
    - fix the misleading description of 'dc->desc'  [Stefano]
  Patch 5:
    - check returned number of virtqueues  [Stefan]
  Patch 6:
    - init s->num_queues  [Stefano]
    - free s->dev.vqs  [Stefano]

Longpeng (Mike) (10):
  virtio: get class_id and pci device id by the virtio id
  update linux headers
  vdpa: add the infrastructure of vdpa-dev
  vdpa-dev: implement the instance_init/class_init interface
  vdpa-dev: implement the realize interface
  vdpa-dev: implement the unrealize interface
  vdpa-dev: implement the get_config/set_config interface
  vdpa-dev: implement the get_features interface
  vdpa-dev: implement the set_status interface
  vdpa-dev: mark the device as unmigratable

 hw/virtio/Kconfig            |   5 +
 hw/virtio/meson.build        |   2 +
 hw/virtio/vdpa-dev-pci.c     |  99 ++++++++++
 hw/virtio/vdpa-dev.c         | 357 +++++++++++++++++++++++++++++++++++
 hw/virtio/virtio-pci.c       |  77 ++++++++
 hw/virtio/virtio-pci.h       |   5 +
 include/hw/virtio/vdpa-dev.h |  29 +++
 linux-headers/linux/vhost.h  |   7 +
 8 files changed, 581 insertions(+)
 create mode 100644 hw/virtio/vdpa-dev-pci.c
 create mode 100644 hw/virtio/vdpa-dev.c
 create mode 100644 include/hw/virtio/vdpa-dev.h

-- 
2.23.0

[PATCH v2 01/10] virtio: get class_id and pci device id by the virtio id

[Longpeng(Mike) via]

From: Longpeng <longpeng2@huawei.com>

Add helpers to get the "Transitional PCI Device ID" and "class_id"
of the device specified by the "Virtio Device ID".

These helpers will be used to build the generic vDPA device later.

Signed-off-by: Longpeng <longpeng2@huawei.com>
---
 hw/virtio/virtio-pci.c | 77 ++++++++++++++++++++++++++++++++++++++++++
 hw/virtio/virtio-pci.h |  5 +++
 2 files changed, 82 insertions(+)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 750aa47ec1..373e26d7c3 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -19,6 +19,7 @@
 
 #include "exec/memop.h"
 #include "standard-headers/linux/virtio_pci.h"
+#include "standard-headers/linux/virtio_ids.h"
 #include "hw/boards.h"
 #include "hw/virtio/virtio.h"
 #include "migration/qemu-file-types.h"
@@ -213,6 +214,79 @@ static int virtio_pci_load_queue(DeviceState *d, int n, QEMUFile *f)
     return 0;
 }
 
+typedef struct VirtIOPCIIDInfo {
+    /* virtio id */
+    uint16_t vdev_id;
+    /* pci device id for the transitional device */
+    uint16_t trans_devid;
+    uint16_t class_id;
+} VirtIOPCIIDInfo;
+
+#define VIRTIO_TRANS_DEV_ID_INFO(name, class)       \
+    {                                               \
+        .vdev_id = VIRTIO_ID_##name,                \
+        .trans_devid = PCI_DEVICE_ID_VIRTIO_##name, \
+        .class_id = class,                          \
+    }
+
+#define VIRTIO_MODERN_DEV_ID_NFO(name, class)       \
+    {                                               \
+        .vdev_id = VIRTIO_ID_##name,                \
+        .class_id = class,                          \
+    }
+
+static const VirtIOPCIIDInfo virtio_pci_id_info[] = {
+    /* Non-transitional devices */
+    VIRTIO_MODERN_DEV_ID_NFO(CRYPTO,    PCI_CLASS_OTHERS),
+    VIRTIO_MODERN_DEV_ID_NFO(FS,        PCI_CLASS_STORAGE_OTHER),
+    /* Transitional devices */
+    VIRTIO_TRANS_DEV_ID_INFO(NET,       PCI_CLASS_NETWORK_ETHERNET),
+    VIRTIO_TRANS_DEV_ID_INFO(BLOCK,     PCI_CLASS_STORAGE_SCSI),
+    VIRTIO_TRANS_DEV_ID_INFO(CONSOLE,   PCI_CLASS_COMMUNICATION_OTHER),
+    VIRTIO_TRANS_DEV_ID_INFO(SCSI,      PCI_CLASS_STORAGE_SCSI),
+    VIRTIO_TRANS_DEV_ID_INFO(9P,        PCI_BASE_CLASS_NETWORK),
+    VIRTIO_TRANS_DEV_ID_INFO(BALLOON,   PCI_CLASS_OTHERS),
+    VIRTIO_TRANS_DEV_ID_INFO(RNG,       PCI_CLASS_OTHERS),
+};
+
+static const VirtIOPCIIDInfo *virtio_pci_get_id_info(uint16_t vdev_id)
+{
+    const VirtIOPCIIDInfo *info = NULL;
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(virtio_pci_id_info); i++) {
+        if (virtio_pci_id_info[i].vdev_id == vdev_id) {
+            info = &virtio_pci_id_info[i];
+            break;
+        }
+    }
+
+    if (!info) {
+        /* The device id is invalid or not added to the id_info yet. */
+        error_report("Invalid virtio device(id %u)", vdev_id);
+        abort();
+    }
+
+    return info;
+}
+
+/*
+ * Get the Transitional Device ID for the specific device, return
+ * zero if the device is non-transitional.
+ */
+uint16_t virtio_pci_get_trans_devid(uint16_t device_id)
+{
+    return virtio_pci_get_id_info(device_id)->trans_devid;
+}
+
+/*
+ * Get the Class ID for the specific device.
+ */
+uint16_t virtio_pci_get_class_id(uint16_t device_id)
+{
+    return virtio_pci_get_id_info(device_id)->class_id;
+}
+
 static bool virtio_pci_ioeventfd_enabled(DeviceState *d)
 {
     VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
@@ -1674,6 +1748,9 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
          * is set to PCI_SUBVENDOR_ID_REDHAT_QUMRANET by default.
          */
         pci_set_word(config + PCI_SUBSYSTEM_ID, virtio_bus_get_vdev_id(bus));
+        if (proxy->trans_devid) {
+            pci_config_set_device_id(config, proxy->trans_devid);
+        }
     } else {
         /* pure virtio-1.0 */
         pci_set_word(config + PCI_VENDOR_ID,
diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
index 2446dcd9ae..f08665cd1b 100644
--- a/hw/virtio/virtio-pci.h
+++ b/hw/virtio/virtio-pci.h
@@ -146,6 +146,8 @@ struct VirtIOPCIProxy {
     bool disable_modern;
     bool ignore_backend_features;
     OnOffAuto disable_legacy;
+    /* Transitional device id */
+    uint16_t trans_devid;
     uint32_t class_code;
     uint32_t nvectors;
     uint32_t dfselect;
@@ -158,6 +160,9 @@ struct VirtIOPCIProxy {
     VirtioBusState bus;
 };
 
+uint16_t virtio_pci_get_trans_devid(uint16_t device_id);
+uint16_t virtio_pci_get_class_id(uint16_t device_id);
+
 static inline bool virtio_pci_modern(VirtIOPCIProxy *proxy)
 {
     return !proxy->disable_modern;
-- 
2.23.0

[PATCH v2 02/10] update linux headers

[Longpeng(Mike) via]

From: Longpeng <longpeng2@huawei.com>

Update linux headers to 5.xxx(kernel part is not merged yet)

To support generic vdpa deivce, we need add the following ioctls:
- VHOST_VDPA_GET_CONFIG_SIZE: get the configuration size.
- VHOST_VDPA_GET_VQS_NUM: get the count of supported virtqueues.

Signed-off-by: Longpeng <longpeng2@huawei.com>
---
 linux-headers/linux/vhost.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/linux-headers/linux/vhost.h b/linux-headers/linux/vhost.h
index c998860d7b..5d99e7c242 100644
--- a/linux-headers/linux/vhost.h
+++ b/linux-headers/linux/vhost.h
@@ -150,4 +150,11 @@
 /* Get the valid iova range */
 #define VHOST_VDPA_GET_IOVA_RANGE	_IOR(VHOST_VIRTIO, 0x78, \
 					     struct vhost_vdpa_iova_range)
+
+/* Get the config size */
+#define VHOST_VDPA_GET_CONFIG_SIZE	_IOR(VHOST_VIRTIO, 0x79, __u32)
+
+/* Get the count of all virtqueues */
+#define VHOST_VDPA_GET_VQS_COUNT	_IOR(VHOST_VIRTIO, 0x80, __u32)
+
 #endif
-- 
2.23.0

[PATCH v2 03/10] vdpa: add the infrastructure of vdpa-dev

[Longpeng(Mike) via]

From: Longpeng <longpeng2@huawei.com>

Add the infrastructure of vdpa-dev (the generic vDPA device), we
can add a generic vDPA device as follow:
  -device vhost-vdpa-device-pci,vdpa-dev=/dev/vhost-vdpa-X

Signed-off-by: Longpeng <longpeng2@huawei.com>
---
 hw/virtio/Kconfig            |  5 ++++
 hw/virtio/meson.build        |  2 ++
 hw/virtio/vdpa-dev-pci.c     | 51 ++++++++++++++++++++++++++++++++++++
 hw/virtio/vdpa-dev.c         | 41 +++++++++++++++++++++++++++++
 include/hw/virtio/vdpa-dev.h | 16 +++++++++++
 5 files changed, 115 insertions(+)
 create mode 100644 hw/virtio/vdpa-dev-pci.c
 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..2723283382 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 if VIRTIO_PCI
+    depends on VIRTIO && VHOST_VDPA && LINUX
diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index 521f7d64a8..8e8943e20b 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'))
@@ -49,6 +50,7 @@ virtio_pci_ss.add(when: 'CONFIG_VIRTIO_SERIAL', if_true: files('virtio-serial-pc
 virtio_pci_ss.add(when: 'CONFIG_VIRTIO_PMEM', if_true: files('virtio-pmem-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VIRTIO_IOMMU', if_true: files('virtio-iommu-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: files('virtio-mem-pci.c'))
+virtio_pci_ss.add(when: 'CONFIG_VHOST_VDPA_DEV', if_true: files('vdpa-dev-pci.c'))
 
 virtio_ss.add_all(when: 'CONFIG_VIRTIO_PCI', if_true: virtio_pci_ss)
 
diff --git a/hw/virtio/vdpa-dev-pci.c b/hw/virtio/vdpa-dev-pci.c
new file mode 100644
index 0000000000..a5a7b528a9
--- /dev/null
+++ b/hw/virtio/vdpa-dev-pci.c
@@ -0,0 +1,51 @@
+#include "qemu/osdep.h"
+#include <sys/ioctl.h>
+#include <linux/vhost.h>
+#include "hw/virtio/virtio.h"
+#include "hw/virtio/vdpa-dev.h"
+#include "hw/pci/pci.h"
+#include "hw/qdev-properties.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
+#include "qemu/module.h"
+#include "virtio-pci.h"
+#include "qom/object.h"
+
+
+typedef struct VhostVdpaDevicePCI VhostVdpaDevicePCI;
+
+#define TYPE_VHOST_VDPA_DEVICE_PCI "vhost-vdpa-device-pci-base"
+DECLARE_INSTANCE_CHECKER(VhostVdpaDevicePCI, VHOST_VDPA_DEVICE_PCI,
+                         TYPE_VHOST_VDPA_DEVICE_PCI)
+
+struct VhostVdpaDevicePCI {
+    VirtIOPCIProxy parent_obj;
+    VhostVdpaDevice vdev;
+};
+
+static void vhost_vdpa_device_pci_instance_init(Object *obj)
+{
+    return;
+}
+
+static void vhost_vdpa_device_pci_class_init(ObjectClass *klass, void *data)
+{
+    return;
+}
+
+static const VirtioPCIDeviceTypeInfo vhost_vdpa_device_pci_info = {
+    .base_name               = TYPE_VHOST_VDPA_DEVICE_PCI,
+    .generic_name            = "vhost-vdpa-device-pci",
+    .transitional_name       = "vhost-vdpa-device-pci-transitional",
+    .non_transitional_name   = "vhost-vdpa-device-pci-non-transitional",
+    .instance_size  = sizeof(VhostVdpaDevicePCI),
+    .instance_init  = vhost_vdpa_device_pci_instance_init,
+    .class_init     = vhost_vdpa_device_pci_class_init,
+};
+
+static void vhost_vdpa_device_pci_register(void)
+{
+    virtio_pci_types_register(&vhost_vdpa_device_pci_info);
+}
+
+type_init(vhost_vdpa_device_pci_register);
diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
new file mode 100644
index 0000000000..f4f92b90b0
--- /dev/null
+++ b/hw/virtio/vdpa-dev.c
@@ -0,0 +1,41 @@
+#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_class_init(ObjectClass *klass, void *data)
+{
+    return;
+}
+
+static void vhost_vdpa_device_instance_init(Object *obj)
+{
+    return;
+}
+
+static const TypeInfo vhost_vdpa_device_info = {
+    .name = TYPE_VHOST_VDPA_DEVICE,
+    .parent = TYPE_VIRTIO_DEVICE,
+    .instance_size = sizeof(VhostVdpaDevice),
+    .class_init = vhost_vdpa_device_class_init,
+    .instance_init = vhost_vdpa_device_instance_init,
+};
+
+static void register_vhost_vdpa_device_type(void)
+{
+    type_register_static(&vhost_vdpa_device_info);
+}
+
+type_init(register_vhost_vdpa_device_type);
diff --git a/include/hw/virtio/vdpa-dev.h b/include/hw/virtio/vdpa-dev.h
new file mode 100644
index 0000000000..dd94bd74a2
--- /dev/null
+++ b/include/hw/virtio/vdpa-dev.h
@@ -0,0 +1,16 @@
+#ifndef _VHOST_VDPA_DEVICE_H
+#define _VHOST_VDPA_DEVICE_H
+
+#include "hw/virtio/vhost.h"
+#include "hw/virtio/vhost-vdpa.h"
+#include "qom/object.h"
+
+
+#define TYPE_VHOST_VDPA_DEVICE "vhost-vdpa-device"
+OBJECT_DECLARE_SIMPLE_TYPE(VhostVdpaDevice, VHOST_VDPA_DEVICE)
+
+struct VhostVdpaDevice {
+    VirtIODevice parent_obj;
+};
+
+#endif
-- 
2.23.0

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

[Longpeng(Mike) via]

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);
+    if (*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;
+    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);
 }
 
 static void vhost_vdpa_device_pci_class_init(ObjectClass *klass, void *data)
 {
-    return;
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
+
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+    device_class_set_props(dc, vhost_vdpa_device_pci_properties);
+    k->realize = vhost_vdpa_device_pci_realize;
 }
 
 static const VirtioPCIDeviceTypeInfo vhost_vdpa_device_pci_info = {
diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
index f4f92b90b0..b103768f33 100644
--- a/hw/virtio/vdpa-dev.c
+++ b/hw/virtio/vdpa-dev.c
@@ -15,16 +15,93 @@
 #include "sysemu/sysemu.h"
 #include "sysemu/runstate.h"
 
-static void vhost_vdpa_device_class_init(ObjectClass *klass, void *data)
+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)
 {
     return;
 }
 
-static void vhost_vdpa_device_instance_init(Object *obj)
+static void vhost_vdpa_device_unrealize(DeviceState *dev)
+{
+    return;
+}
+
+static void
+vhost_vdpa_device_get_config(VirtIODevice *vdev, uint8_t *config)
+{
+    return;
+}
+
+static void
+vhost_vdpa_device_set_config(VirtIODevice *vdev, const uint8_t *config)
+{
+    return;
+}
+
+static uint64_t vhost_vdpa_device_get_features(VirtIODevice *vdev,
+                                               uint64_t features,
+                                               Error **errp)
+{
+    return (uint64_t)-1;
+}
+
+static void vhost_vdpa_device_set_status(VirtIODevice *vdev, uint8_t status)
 {
     return;
 }
 
+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_END_OF_LIST(),
+};
+
+static const VMStateDescription vmstate_vhost_vdpa_device = {
+    .name = "vhost-vdpa-device",
+    .minimum_version_id = 1,
+    .version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_VIRTIO_DEVICE,
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static void vhost_vdpa_device_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
+
+    device_class_set_props(dc, vhost_vdpa_device_properties);
+    dc->desc = "VDPA-based generic device assignment";
+    dc->vmsd = &vmstate_vhost_vdpa_device;
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+    vdc->realize = vhost_vdpa_device_realize;
+    vdc->unrealize = vhost_vdpa_device_unrealize;
+    vdc->get_config = vhost_vdpa_device_get_config;
+    vdc->set_config = vhost_vdpa_device_set_config;
+    vdc->get_features = vhost_vdpa_device_get_features;
+    vdc->set_status = vhost_vdpa_device_set_status;
+}
+
+static void vhost_vdpa_device_instance_init(Object *obj)
+{
+    VhostVdpaDevice *s = VHOST_VDPA_DEVICE(obj);
+
+    device_add_bootindex_property(obj, &s->bootindex, "bootindex",
+                                  NULL, DEVICE(obj));
+}
+
 static const TypeInfo vhost_vdpa_device_info = {
     .name = TYPE_VHOST_VDPA_DEVICE,
     .parent = TYPE_VIRTIO_DEVICE,
diff --git a/include/hw/virtio/vdpa-dev.h b/include/hw/virtio/vdpa-dev.h
index dd94bd74a2..e7ad349113 100644
--- a/include/hw/virtio/vdpa-dev.h
+++ b/include/hw/virtio/vdpa-dev.h
@@ -11,6 +11,11 @@ OBJECT_DECLARE_SIMPLE_TYPE(VhostVdpaDevice, VHOST_VDPA_DEVICE)
 
 struct VhostVdpaDevice {
     VirtIODevice parent_obj;
+    char *vdpa_dev;
+    int vdpa_dev_fd;
+    int32_t bootindex;
 };
 
+uint32_t vhost_vdpa_device_get_u32(int fd, unsigned long int cmd, Error **errp);
+
 #endif
-- 
2.23.0

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

[Longpeng(Mike) via]

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

[PATCH v2 06/10] vdpa-dev: implement the unrealize interface

[Longpeng(Mike) via]

From: Longpeng <longpeng2@huawei.com>

Implements the .unrealize interface.

Signed-off-by: Longpeng <longpeng2@huawei.com>
---
 hw/virtio/vdpa-dev.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
index bd28cf7a15..e5691d02bb 100644
--- a/hw/virtio/vdpa-dev.c
+++ b/hw/virtio/vdpa-dev.c
@@ -132,9 +132,31 @@ out:
     s->vdpa_dev_fd = -1;
 }
 
+static void vhost_vdpa_vdev_unrealize(VhostVdpaDevice *s)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+    int i;
+
+    for (i = 0; i < s->num_queues; i++) {
+        virtio_delete_queue(s->virtqs[i]);
+    }
+    g_free(s->virtqs);
+    virtio_cleanup(vdev);
+
+    g_free(s->config);
+}
+
 static void vhost_vdpa_device_unrealize(DeviceState *dev)
 {
-    return;
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VhostVdpaDevice *s = VHOST_VDPA_DEVICE(vdev);
+
+    virtio_set_status(vdev, 0);
+    vhost_vdpa_vdev_unrealize(s);
+    g_free(s->dev.vqs);
+    vhost_dev_cleanup(&s->dev);
+    qemu_close(s->vdpa_dev_fd);
+    s->vdpa_dev_fd = -1;
 }
 
 static void
-- 
2.23.0

[PATCH v2 07/10] vdpa-dev: implement the get_config/set_config interface

[Longpeng(Mike) via]

From: Longpeng <longpeng2@huawei.com>

Implements the .get_config and .set_config interface.

Signed-off-by: Longpeng <longpeng2@huawei.com>
---
 hw/virtio/vdpa-dev.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
index e5691d02bb..cef0a58012 100644
--- a/hw/virtio/vdpa-dev.c
+++ b/hw/virtio/vdpa-dev.c
@@ -162,13 +162,23 @@ static void vhost_vdpa_device_unrealize(DeviceState *dev)
 static void
 vhost_vdpa_device_get_config(VirtIODevice *vdev, uint8_t *config)
 {
-    return;
+    VhostVdpaDevice *s = VHOST_VDPA_DEVICE(vdev);
+
+    memcpy(config, s->config, s->config_size);
 }
 
 static void
 vhost_vdpa_device_set_config(VirtIODevice *vdev, const uint8_t *config)
 {
-    return;
+    VhostVdpaDevice *s = VHOST_VDPA_DEVICE(vdev);
+    int ret;
+
+    ret = vhost_dev_set_config(&s->dev, s->config, 0, s->config_size,
+                               VHOST_SET_CONFIG_TYPE_MASTER);
+    if (ret) {
+        error_report("set device config space failed");
+        return;
+    }
 }
 
 static uint64_t vhost_vdpa_device_get_features(VirtIODevice *vdev,
-- 
2.23.0

[PATCH v2 08/10] vdpa-dev: implement the get_features interface

[Longpeng(Mike) via]

From: Longpeng <longpeng2@huawei.com>

Implements the .get_features interface.

Signed-off-by: Longpeng <longpeng2@huawei.com>
---
 hw/virtio/vdpa-dev.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
index cef0a58012..7bf07fef9b 100644
--- a/hw/virtio/vdpa-dev.c
+++ b/hw/virtio/vdpa-dev.c
@@ -185,7 +185,14 @@ static uint64_t vhost_vdpa_device_get_features(VirtIODevice *vdev,
                                                uint64_t features,
                                                Error **errp)
 {
-    return (uint64_t)-1;
+    VhostVdpaDevice *s = VHOST_VDPA_DEVICE(vdev);
+    uint64_t backend_features = s->dev.features;
+
+    if (!virtio_has_feature(features, VIRTIO_F_IOMMU_PLATFORM)) {
+        virtio_clear_feature(&backend_features, VIRTIO_F_IOMMU_PLATFORM);
+    }
+
+    return backend_features;
 }
 
 static void vhost_vdpa_device_set_status(VirtIODevice *vdev, uint8_t status)
-- 
2.23.0

[PATCH v2 09/10] vdpa-dev: implement the set_status interface

[Longpeng(Mike) via]

From: Longpeng <longpeng2@huawei.com>

Implements the .set_status interface.

Signed-off-by: Longpeng <longpeng2@huawei.com>
---
 hw/virtio/vdpa-dev.c | 100 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 99 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
index 7bf07fef9b..99722c88a1 100644
--- a/hw/virtio/vdpa-dev.c
+++ b/hw/virtio/vdpa-dev.c
@@ -195,9 +195,107 @@ static uint64_t vhost_vdpa_device_get_features(VirtIODevice *vdev,
     return backend_features;
 }
 
+static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp)
+{
+    VhostVdpaDevice *s = VHOST_VDPA_DEVICE(vdev);
+    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    int i, ret;
+
+    if (!k->set_guest_notifiers) {
+        error_setg(errp, "binding does not support guest notifiers");
+        return -ENOSYS;
+    }
+
+    ret = vhost_dev_enable_notifiers(&s->dev, vdev);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Error enabling host notifiers");
+        return ret;
+    }
+
+    ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, true);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Error binding guest notifier");
+        goto err_host_notifiers;
+    }
+
+    s->dev.acked_features = vdev->guest_features;
+
+    ret = vhost_dev_start(&s->dev, vdev);
+    if (ret < 0) {
+        error_setg_errno(errp, -ret, "Error starting vhost");
+        goto err_guest_notifiers;
+    }
+    s->started = true;
+
+    /*
+     * guest_notifier_mask/pending not used yet, so just unmask
+     * everything here. virtio-pci will do the right thing by
+     * enabling/disabling irqfd.
+     */
+    for (i = 0; i < s->dev.nvqs; i++) {
+        vhost_virtqueue_mask(&s->dev, vdev, i, false);
+    }
+
+    return ret;
+
+err_guest_notifiers:
+    k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
+err_host_notifiers:
+    vhost_dev_disable_notifiers(&s->dev, vdev);
+    return ret;
+}
+
+static void vhost_vdpa_device_stop(VirtIODevice *vdev)
+{
+    VhostVdpaDevice *s = VHOST_VDPA_DEVICE(vdev);
+    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    int ret;
+
+    if (!s->started) {
+        return;
+    }
+    s->started = false;
+
+    if (!k->set_guest_notifiers) {
+        return;
+    }
+
+    vhost_dev_stop(&s->dev, vdev);
+
+    ret = k->set_guest_notifiers(qbus->parent, s->dev.nvqs, false);
+    if (ret < 0) {
+        error_report("vhost guest notifier cleanup failed: %d", ret);
+        return;
+    }
+
+    vhost_dev_disable_notifiers(&s->dev, vdev);
+}
+
 static void vhost_vdpa_device_set_status(VirtIODevice *vdev, uint8_t status)
 {
-    return;
+    VhostVdpaDevice *s = VHOST_VDPA_DEVICE(vdev);
+    bool should_start = virtio_device_started(vdev, status);
+    Error *local_err = NULL;
+    int ret;
+
+    if (!vdev->vm_running) {
+        should_start = false;
+    }
+
+    if (s->started == should_start) {
+        return;
+    }
+
+    if (should_start) {
+        ret = vhost_vdpa_device_start(vdev, &local_err);
+        if (ret < 0) {
+            error_reportf_err(local_err, "vhost-vdpa-device: start failed: ");
+        }
+    } else {
+        vhost_vdpa_device_stop(vdev);
+    }
 }
 
 static Property vhost_vdpa_device_properties[] = {
-- 
2.23.0

[PATCH v2 10/10] vdpa-dev: mark the device as unmigratable

[Longpeng(Mike) via]

From: Longpeng <longpeng2@huawei.com>

The generic vDPA device doesn't support migration currently, so
mark it as unmigratable temporarily.

Signed-off-by: Longpeng <longpeng2@huawei.com>
---
 hw/virtio/vdpa-dev.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
index 99722c88a1..65511243f9 100644
--- a/hw/virtio/vdpa-dev.c
+++ b/hw/virtio/vdpa-dev.c
@@ -307,6 +307,7 @@ static Property vhost_vdpa_device_properties[] = {
 
 static const VMStateDescription vmstate_vhost_vdpa_device = {
     .name = "vhost-vdpa-device",
+    .unmigratable = 1,
     .minimum_version_id = 1,
     .version_id = 1,
     .fields = (VMStateField[]) {
-- 
2.23.0

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

[Stefano Garzarella]

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.

>+    if (*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);
                                                  ^
The build fails here, I think this should be VHOST_VDPA_GET_VQS_COUNT

>+    if (*errp) {
>+        qemu_close(fd);
>+        return;
>+    }
>+
>+    dev->vdev.vdpa_dev_fd = fd;
>+    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);
> }
>
> static void vhost_vdpa_device_pci_class_init(ObjectClass *klass, void 
> *data)
> {
>-    return;
>+    DeviceClass *dc = DEVICE_CLASS(klass);
>+    VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
>+
>+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>+    device_class_set_props(dc, vhost_vdpa_device_pci_properties);
>+    k->realize = vhost_vdpa_device_pci_realize;
> }
>
> static const VirtioPCIDeviceTypeInfo vhost_vdpa_device_pci_info = {
>diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
>index f4f92b90b0..b103768f33 100644
>--- a/hw/virtio/vdpa-dev.c
>+++ b/hw/virtio/vdpa-dev.c
>@@ -15,16 +15,93 @@
> #include "sysemu/sysemu.h"
> #include "sysemu/runstate.h"
>
>-static void vhost_vdpa_device_class_init(ObjectClass *klass, void *data)
>+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)
> {
>     return;
> }
>
>-static void vhost_vdpa_device_instance_init(Object *obj)
>+static void vhost_vdpa_device_unrealize(DeviceState *dev)
>+{
>+    return;
>+}
>+
>+static void
>+vhost_vdpa_device_get_config(VirtIODevice *vdev, uint8_t *config)
>+{
>+    return;
>+}
>+
>+static void
>+vhost_vdpa_device_set_config(VirtIODevice *vdev, const uint8_t *config)
>+{
>+    return;
>+}
>+
>+static uint64_t vhost_vdpa_device_get_features(VirtIODevice *vdev,
>+                                               uint64_t features,
>+                                               Error **errp)
>+{
>+    return (uint64_t)-1;
>+}
>+
>+static void vhost_vdpa_device_set_status(VirtIODevice *vdev, uint8_t status)
> {
>     return;
> }
>
>+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_END_OF_LIST(),
>+};
>+
>+static const VMStateDescription vmstate_vhost_vdpa_device = {
>+    .name = "vhost-vdpa-device",
>+    .minimum_version_id = 1,
>+    .version_id = 1,
>+    .fields = (VMStateField[]) {
>+        VMSTATE_VIRTIO_DEVICE,
>+        VMSTATE_END_OF_LIST()
>+    },
>+};
>+
>+static void vhost_vdpa_device_class_init(ObjectClass *klass, void *data)
>+{
>+    DeviceClass *dc = DEVICE_CLASS(klass);
>+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
>+
>+    device_class_set_props(dc, vhost_vdpa_device_properties);
>+    dc->desc = "VDPA-based generic device assignment";
>+    dc->vmsd = &vmstate_vhost_vdpa_device;
>+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>+    vdc->realize = vhost_vdpa_device_realize;
>+    vdc->unrealize = vhost_vdpa_device_unrealize;
>+    vdc->get_config = vhost_vdpa_device_get_config;
>+    vdc->set_config = vhost_vdpa_device_set_config;
>+    vdc->get_features = vhost_vdpa_device_get_features;
>+    vdc->set_status = vhost_vdpa_device_set_status;
>+}
>+
>+static void vhost_vdpa_device_instance_init(Object *obj)
>+{
>+    VhostVdpaDevice *s = VHOST_VDPA_DEVICE(obj);
>+
>+    device_add_bootindex_property(obj, &s->bootindex, "bootindex",
>+                                  NULL, DEVICE(obj));
>+}
>+
> static const TypeInfo vhost_vdpa_device_info = {
>     .name = TYPE_VHOST_VDPA_DEVICE,
>     .parent = TYPE_VIRTIO_DEVICE,
>diff --git a/include/hw/virtio/vdpa-dev.h b/include/hw/virtio/vdpa-dev.h
>index dd94bd74a2..e7ad349113 100644
>--- a/include/hw/virtio/vdpa-dev.h
>+++ b/include/hw/virtio/vdpa-dev.h
>@@ -11,6 +11,11 @@ OBJECT_DECLARE_SIMPLE_TYPE(VhostVdpaDevice, VHOST_VDPA_DEVICE)
>
> struct VhostVdpaDevice {
>     VirtIODevice parent_obj;
>+    char *vdpa_dev;
>+    int vdpa_dev_fd;
>+    int32_t bootindex;
> };
>
>+uint32_t vhost_vdpa_device_get_u32(int fd, unsigned long int cmd, Error **errp);
>+
> #endif
>-- 
>2.23.0
>
>

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

[Stefano Garzarella]

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

Re: [PATCH v2 06/10] vdpa-dev: implement the unrealize interface

[Stefano Garzarella]

On Mon, Jan 17, 2022 at 08:43:27PM +0800, Longpeng(Mike) via wrote:
>From: Longpeng <longpeng2@huawei.com>
>
>Implements the .unrealize interface.
>
>Signed-off-by: Longpeng <longpeng2@huawei.com>
>---
> hw/virtio/vdpa-dev.c | 24 +++++++++++++++++++++++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
>diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
>index bd28cf7a15..e5691d02bb 100644
>--- a/hw/virtio/vdpa-dev.c
>+++ b/hw/virtio/vdpa-dev.c
>@@ -132,9 +132,31 @@ out:
>     s->vdpa_dev_fd = -1;
> }
>
>+static void vhost_vdpa_vdev_unrealize(VhostVdpaDevice *s)
>+{
>+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
>+    int i;
>+
>+    for (i = 0; i < s->num_queues; i++) {
>+        virtio_delete_queue(s->virtqs[i]);
>+    }
>+    g_free(s->virtqs);
>+    virtio_cleanup(vdev);
>+
>+    g_free(s->config);

Is there a particular reason for these steps in a separate function?

>+}
>+
> static void vhost_vdpa_device_unrealize(DeviceState *dev)
> {
>-    return;
>+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>+    VhostVdpaDevice *s = VHOST_VDPA_DEVICE(vdev);
>+
>+    virtio_set_status(vdev, 0);
>+    vhost_vdpa_vdev_unrealize(s);
>+    g_free(s->dev.vqs);
>+    vhost_dev_cleanup(&s->dev);
>+    qemu_close(s->vdpa_dev_fd);
>+    s->vdpa_dev_fd = -1;
> }

Maybe we can have all steps (in the reverse order of 
vhost_vdpa_device_realize) in vhost_vdpa_device_unrealize().

Stefano

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

[longpeng2--- via]

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

> >+    if (*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);
>                                                   ^
> The build fails here, I think this should be VHOST_VDPA_GET_VQS_COUNT
> 

Yes, I sent a wrong version, I'll send v3 later.

> >+    if (*errp) {
> >+        qemu_close(fd);
> >+        return;
> >+    }
> >+
> >+    dev->vdev.vdpa_dev_fd = fd;
> >+    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);
> > }
> >
> > static void vhost_vdpa_device_pci_class_init(ObjectClass *klass, void
> > *data)
> > {
> >-    return;
> >+    DeviceClass *dc = DEVICE_CLASS(klass);
> >+    VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
> >+
> >+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> >+    device_class_set_props(dc, vhost_vdpa_device_pci_properties);
> >+    k->realize = vhost_vdpa_device_pci_realize;
> > }
> >
> > static const VirtioPCIDeviceTypeInfo vhost_vdpa_device_pci_info = {
> >diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
> >index f4f92b90b0..b103768f33 100644
> >--- a/hw/virtio/vdpa-dev.c
> >+++ b/hw/virtio/vdpa-dev.c
> >@@ -15,16 +15,93 @@
> > #include "sysemu/sysemu.h"
> > #include "sysemu/runstate.h"
> >
> >-static void vhost_vdpa_device_class_init(ObjectClass *klass, void *data)
> >+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)
> > {
> >     return;
> > }
> >
> >-static void vhost_vdpa_device_instance_init(Object *obj)
> >+static void vhost_vdpa_device_unrealize(DeviceState *dev)
> >+{
> >+    return;
> >+}
> >+
> >+static void
> >+vhost_vdpa_device_get_config(VirtIODevice *vdev, uint8_t *config)
> >+{
> >+    return;
> >+}
> >+
> >+static void
> >+vhost_vdpa_device_set_config(VirtIODevice *vdev, const uint8_t *config)
> >+{
> >+    return;
> >+}
> >+
> >+static uint64_t vhost_vdpa_device_get_features(VirtIODevice *vdev,
> >+                                               uint64_t features,
> >+                                               Error **errp)
> >+{
> >+    return (uint64_t)-1;
> >+}
> >+
> >+static void vhost_vdpa_device_set_status(VirtIODevice *vdev, uint8_t status)
> > {
> >     return;
> > }
> >
> >+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_END_OF_LIST(),
> >+};
> >+
> >+static const VMStateDescription vmstate_vhost_vdpa_device = {
> >+    .name = "vhost-vdpa-device",
> >+    .minimum_version_id = 1,
> >+    .version_id = 1,
> >+    .fields = (VMStateField[]) {
> >+        VMSTATE_VIRTIO_DEVICE,
> >+        VMSTATE_END_OF_LIST()
> >+    },
> >+};
> >+
> >+static void vhost_vdpa_device_class_init(ObjectClass *klass, void *data)
> >+{
> >+    DeviceClass *dc = DEVICE_CLASS(klass);
> >+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
> >+
> >+    device_class_set_props(dc, vhost_vdpa_device_properties);
> >+    dc->desc = "VDPA-based generic device assignment";
> >+    dc->vmsd = &vmstate_vhost_vdpa_device;
> >+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> >+    vdc->realize = vhost_vdpa_device_realize;
> >+    vdc->unrealize = vhost_vdpa_device_unrealize;
> >+    vdc->get_config = vhost_vdpa_device_get_config;
> >+    vdc->set_config = vhost_vdpa_device_set_config;
> >+    vdc->get_features = vhost_vdpa_device_get_features;
> >+    vdc->set_status = vhost_vdpa_device_set_status;
> >+}
> >+
> >+static void vhost_vdpa_device_instance_init(Object *obj)
> >+{
> >+    VhostVdpaDevice *s = VHOST_VDPA_DEVICE(obj);
> >+
> >+    device_add_bootindex_property(obj, &s->bootindex, "bootindex",
> >+                                  NULL, DEVICE(obj));
> >+}
> >+
> > static const TypeInfo vhost_vdpa_device_info = {
> >     .name = TYPE_VHOST_VDPA_DEVICE,
> >     .parent = TYPE_VIRTIO_DEVICE,
> >diff --git a/include/hw/virtio/vdpa-dev.h b/include/hw/virtio/vdpa-dev.h
> >index dd94bd74a2..e7ad349113 100644
> >--- a/include/hw/virtio/vdpa-dev.h
> >+++ b/include/hw/virtio/vdpa-dev.h
> >@@ -11,6 +11,11 @@ OBJECT_DECLARE_SIMPLE_TYPE(VhostVdpaDevice,
> VHOST_VDPA_DEVICE)
> >
> > struct VhostVdpaDevice {
> >     VirtIODevice parent_obj;
> >+    char *vdpa_dev;
> >+    int vdpa_dev_fd;
> >+    int32_t bootindex;
> > };
> >
> >+uint32_t vhost_vdpa_device_get_u32(int fd, unsigned long int cmd, Error
> **errp);
> >+
> > #endif
> >--
> >2.23.0
> >
> >

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

[longpeng2--- via]

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

RE: [PATCH v2 06/10] vdpa-dev: implement the unrealize interface

[longpeng2--- via]

> -----Original Message-----
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, January 19, 2022 7:36 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 06/10] vdpa-dev: implement the unrealize interface
> 
> On Mon, Jan 17, 2022 at 08:43:27PM +0800, Longpeng(Mike) via wrote:
> >From: Longpeng <longpeng2@huawei.com>
> >
> >Implements the .unrealize interface.
> >
> >Signed-off-by: Longpeng <longpeng2@huawei.com>
> >---
> > hw/virtio/vdpa-dev.c | 24 +++++++++++++++++++++++-
> > 1 file changed, 23 insertions(+), 1 deletion(-)
> >
> >diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c
> >index bd28cf7a15..e5691d02bb 100644
> >--- a/hw/virtio/vdpa-dev.c
> >+++ b/hw/virtio/vdpa-dev.c
> >@@ -132,9 +132,31 @@ out:
> >     s->vdpa_dev_fd = -1;
> > }
> >
> >+static void vhost_vdpa_vdev_unrealize(VhostVdpaDevice *s)
> >+{
> >+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> >+    int i;
> >+
> >+    for (i = 0; i < s->num_queues; i++) {
> >+        virtio_delete_queue(s->virtqs[i]);
> >+    }
> >+    g_free(s->virtqs);
> >+    virtio_cleanup(vdev);
> >+
> >+    g_free(s->config);
> 
> Is there a particular reason for these steps in a separate function?
> 
> >+}
> >+
> > static void vhost_vdpa_device_unrealize(DeviceState *dev)
> > {
> >-    return;
> >+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> >+    VhostVdpaDevice *s = VHOST_VDPA_DEVICE(vdev);
> >+
> >+    virtio_set_status(vdev, 0);
> >+    vhost_vdpa_vdev_unrealize(s);
> >+    g_free(s->dev.vqs);
> >+    vhost_dev_cleanup(&s->dev);
> >+    qemu_close(s->vdpa_dev_fd);
> >+    s->vdpa_dev_fd = -1;
> > }
> 
> Maybe we can have all steps (in the reverse order of
> vhost_vdpa_device_realize) in vhost_vdpa_device_unrealize().
> 

Make sense. Will do.

> Stefano

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

[Stefano Garzarella]

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.

I think we should allow to specify an fd (as we already do for other 
vhost devices), because it's a common use case when qemu is launched 
from higher layers (e.g. libvirt).

>
>> >+    if (*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);
>>                                                   ^
>> The build fails here, I think this should be VHOST_VDPA_GET_VQS_COUNT
>>
>
>Yes, I sent a wrong version, I'll send v3 later.

Thanks,
Stefano

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

[Stefano Garzarella]

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

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

[longpeng2--- via]

> -----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 should allow to specify an fd (as we already do for other
> vhost devices), because it's a common use case when qemu is launched
> from higher layers (e.g. libvirt).
> 
> >
> >> >+    if (*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);
> >>                                                   ^
> >> The build fails here, I think this should be VHOST_VDPA_GET_VQS_COUNT
> >>
> >
> >Yes, I sent a wrong version, I'll send v3 later.
> 
> Thanks,
> Stefano

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

[Stefano Garzarella]

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

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

[longpeng2--- via]

> -----Original Message-----
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Monday, March 7, 2022 5:13 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, 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.
> 

Make sense, I'll try. Thanks.

> Thanks,
> Stefano

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

[longpeng2--- via]

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

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

[Stefano Garzarella]

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

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

[longpeng2--- via]

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

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

[Stefano Garzarella]

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

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

[longpeng2--- via]

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

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

[Stefano Garzarella]

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