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

[Philippe Mathieu-Daudé]

On 15/4/24 12:10, Duan, Zhenzhong wrote:
Hi Philippe,
-----Original Message-----
From: Philippe Mathieu-Daudé <phi...@linaro.org>
Sent: Monday, April 15, 2024 5:20 PM
To: Duan, Zhenzhong <zhenzhong.d...@intel.com>; qemu-
de...@nongnu.org
Cc: alex.william...@redhat.com; c...@redhat.com; eric.au...@redhat.com;
pet...@redhat.com; jasow...@redhat.com; m...@redhat.com;
j...@nvidia.com; nicol...@nvidia.com; joao.m.mart...@oracle.com; Tian,
Kevin <kevin.t...@intel.com>; Liu, Yi L <yi.l....@intel.com>; Peng, Chao P
<chao.p.p...@intel.com>
Subject: Re: [PATCH v2 02/10] vfio: Introduce HIODLegacyVFIO device

On 8/4/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"
+OBJECT_DECLARE_SIMPLE_TYPE(HIODLegacyVFIO, HIOD_LEGACY_VFIO)
+
+/* Abstraction of VFIO legacy host IOMMU device */
+struct HIODLegacyVFIO {
+    /*< private >*/

Please drop this comment.

Will do. But may I ask the rules when to use that comment and when not?

Sure, see 
https://www.qemu.org/docs/master/devel/style.html#qemu-object-model-declarations

I see some QOM use that comment to mark private vs. public, for example:

struct AccelState {
     /*< private >*/
     Object parent_obj;

This is old style which might be cleaned some day...

};

typedef struct AccelClass {
     /*< private >*/
     ObjectClass parent_class;
     /*< public >*/


+    HostIOMMUDevice parent;

Please name 'parent_obj'.

Will do.

Thanks,

Phil.


Thanks
Zhenzhong


+    VFIODevice *vdev;
+};