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

[longpeng2--- via <qemu-devel@nongnu.org>]

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