Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device

[Cédric Le Goater]

Hello,

On 4/16/24 05:41, Duan, Zhenzhong wrote:
Hi Cédric,
-----Original Message-----
From: Cédric Le Goater <c...@redhat.com>
Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device

On 4/8/24 10:12, Zhenzhong Duan wrote:
HIODLegacyVFIO represents a host IOMMU device under VFIO legacy
container backend.

It includes a link to VFIODevice.

Suggested-by: Eric Auger <eric.au...@redhat.com>
Suggested-by: Cédric Le Goater <c...@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
---
   include/hw/vfio/vfio-common.h | 11 +++++++++++
   hw/vfio/container.c           | 11 ++++++++++-
   2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
common.h
index b9da6c08ef..f30772f534 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -31,6 +31,7 @@
   #endif
   #include "sysemu/sysemu.h"
   #include "hw/vfio/vfio-container-base.h"
+#include "sysemu/host_iommu_device.h"

   #define VFIO_MSG_PREFIX "vfio %s: "

@@ -147,6 +148,16 @@ typedef struct VFIOGroup {
       bool ram_block_discard_allowed;
   } VFIOGroup;

+#define TYPE_HIOD_LEGACY_VFIO TYPE_HOST_IOMMU_DEVICE "-legacy-
vfio"

I would prefer to keep the prefix TYPE_HOST_IOMMU_DEVICE.

Will do.


+OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO)
+
+/* Abstraction of VFIO legacy host IOMMU device */
+struct HIODLegacyVFIO {

same here

Should I do the same for all the HostIOMMUDevice and HostIOMMUDeviceClass 
sub-structures?

I would for type names. The main reason is for naming consistency, which is
useful for grep and code analysis.


The reason I used 'HIOD' abbreviation is some function names become extremely 
long
and exceed 80 characters. E.g.:

@@ -1148,9 +1148,9 @@ static void vfio_iommu_legacy_class_init(ObjectClass 
*klass, void *data)
      vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
  };

-static int hiod_legacy_vfio_get_host_iommu_info(HostIOMMUDevice *hiod,
-                                                void *data, uint32_t len,
-                                                Error **errp)
+static int host_iommu_device_legacy_vfio_get_host_iommu_info(HostIOMMUDevice 
*hiod,
+                                                             void *data, 
uint32_t len,
+                                                             Error **errp)
  {
      VFIODevice *vbasedev = HIOD_LEGACY_VFIO(hiod)->vdev;
      /* iova_ranges is a sorted list */
@@ -1173,7 +1173,7 @@ static void hiod_legacy_vfio_class_init(ObjectClass *oc, 
void *data)
  {
      HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);

-    hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info;
+    hioc->get_host_iommu_info = 
host_iommu_device_legacy_vfio_get_host_iommu_info;
  };

I didn't find other way to make it meet the 80 chars limitation. Any 
suggestions on this?

Try :

@@ -1177,7 +1177,8 @@ static void hiod_legacy_vfio_class_init(
 {
     HostIOMMUDeviceClass *hioc = HOST_IOMMU_DEVICE_CLASS(oc);
 
-    hioc->get_host_iommu_info = hiod_legacy_vfio_get_host_iommu_info;
+    hioc->get_host_iommu_info =
+        host_iommu_device_legacy_vfio_get_host_iommu_info;
 };
 
 static const TypeInfo types[] = {

That said, I agree that 'host_iommu_device_legacy_vfio' routine prefix
could be shortened to 'hiod_legacy_vfio'.


Thanks,

C.





+    /*< private >*/
+    HostIOMMUDevice parent;
+    VFIODevice *vdev;

It seems to me that the back pointer should be on the container instead.
Looks more correct conceptually.

Yes, that makes sense for legacy VFIO, as iova_ranges, pgsizes etc are all 
saved in bcontainer.



+};
+
   typedef struct VFIODMABuf {
       QemuDmaBuf buf;
       uint32_t pos_x, pos_y, pos_updates;
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index 77bdec276e..44018ef085 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -1143,12 +1143,21 @@ static void
vfio_iommu_legacy_class_init(ObjectClass *klass, void *data)
       vioc->pci_hot_reset = vfio_legacy_pci_hot_reset;
   };

+static void hiod_legacy_vfio_class_init(ObjectClass *oc, void *data)
+{
+};

Is it preferable to introduce routines when they are actually useful.
Please drop the .class_init definition.

Sure.

Thanks
Zhenzhong


Thanks,

C.


+
   static const TypeInfo types[] = {
       {
           .name = TYPE_VFIO_IOMMU_LEGACY,
           .parent = TYPE_VFIO_IOMMU,
           .class_init = vfio_iommu_legacy_class_init,
-    },
+    }, {
+        .name = TYPE_HIOD_LEGACY_VFIO,
+        .parent = TYPE_HOST_IOMMU_DEVICE,
+        .instance_size = sizeof(HIODLegacyVFIO),
+        .class_init = hiod_legacy_vfio_class_init,
+    }
   };

   DEFINE_TYPES(types)