Hello Zhenzhong,

On 3/28/24 04:06, Duan, Zhenzhong wrote:
Hi Cédric,

-----Original Message-----
From: Cédric Le Goater <c...@redhat.com>
Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct

Hello Zhenzhong,

On 3/19/24 12:58, Duan, Zhenzhong wrote:
Hi Cédric,

-----Original Message-----
From: Cédric Le Goater <c...@redhat.com>
Sent: Tuesday, March 19, 2024 4:17 PM
To: Duan, Zhenzhong <zhenzhong.d...@intel.com>; qemu-
Cc: alex.william...@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>; Sun, Yi Y
<yi.y....@intel.com>; Peng, Chao P <chao.p.p...@intel.com>
Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct

Hello Zhenzhong,

On 2/28/24 04:58, Zhenzhong Duan wrote:
HostIOMMUDevice will be inherited by two sub classes,
legacy and iommufd currently.

Introduce a helper function host_iommu_base_device_init to initialize it.

Suggested-by: Eric Auger <eric.au...@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.d...@intel.com>
    include/sysemu/host_iommu_device.h | 22
    1 file changed, 22 insertions(+)
    create mode 100644 include/sysemu/host_iommu_device.h

diff --git a/include/sysemu/host_iommu_device.h
new file mode 100644
index 0000000000..fe80ab25fb
--- /dev/null
+++ b/include/sysemu/host_iommu_device.h
@@ -0,0 +1,22 @@
+typedef enum HostIOMMUDevice_Type {
+    HID_MAX,
+} HostIOMMUDevice_Type;
+typedef struct HostIOMMUDevice {
+    HostIOMMUDevice_Type type;

A type field is not a good sign and that's where QOM is useful.

Yes, agree.
I didn't choose QOM because in iommufd-cdev series, VFIOContainer
chooses not using QOM model.
See the discussion:
I thought HostIOMMUDevice need to follow same rule.

But after further digging into this, I think it may be ok to use QOM model
as long as we don't expose
HostIOMMUDevice in qapi/qom.json and not use USER_CREATABLE
interface. Your thoughts?

yes. Can we change a bit this series to use QOM ? something like :

     typedef struct HostIOMMUDevice {
         Object parent;
     } HostIOMMUDevice;

     #define TYPE_HOST_IOMMU "host.iommu"

     struct HostIOMMUClass {
         ObjectClass parent_class;

         int (*get_type)(HostIOMMUDevice *hiod, uint64_t *type, Error **errp);
         int (*get_cap)(HostIOMMUDevice *hiod, uint64_t *cap, Error **errp);

Inherited objects would be TYPE_HOST_IOMMU_IOMMUFD and
Each class implementing the handlers or not (legacy mode).

Understood, thanks for your guide.

The class handlers are introduced for the intel-iommu helper
in order to avoid using iommufd routines directly. HostIOMMUDevice is
to abstract the Host IOMMU device, so we need to abstract also all the
interfaces to this object.

I'd like to have a minimal adjustment to class handers. Just let me know if you 
have strong

Cap/ecap is intel_iommu specific, I'd like to make it a bit generic also for 
arm smmu usage,
and merge get_type and get_cap into one function as they both calls 
something like:
get_info(HostIOMMUDevice *hiod, enum iommu_hw_info_type *type, void **data, 
void **len,  Error **errp);

OK. Let's see how it goes. Having more users of this new object Host
IOMMU device is important to get a better feeling of the interface.
As of today, it doesn't have not much value. The iommufd object could
be QOM linked to the vIOMMU when available and we could get the bind
devid in some other ways I suppose. Anyhow, please keep it simple and
let's explore.



and let iommu emulater to extract content of *data. For intel_iommu, it's:

struct iommu_hw_info_vtd {
         __u32 flags;
         __u32 __reserved;
         __aligned_u64 cap_reg;
         __aligned_u64 ecap_reg;

The .host_iommu_device_create() handler could be merged
in .attach_device()
possibly. Anyhow, please use now object_new() and object_unref() instead.
host_iommu_base_device_init() is useless IMHO.

Good idea, will do.

Is vtd_check_hdev() the only use of this field ?

Currently yes. virtio-iommu may have similar usage.

If so, can we simplify with a QOM interface in any way ?

QOM interface is a set of callbacks, guess you mean QOM class,
saying HostIOMMUDevice class, IOMMULegacyDevice class and
IOMMUFDDevice class?

See above proposal. it should work fine.

Also, I think it is better to use a IOMMUFDBackend* parameter for
iommufd_device_get_info() to be consistent with the other routines.

Sure, then I'd like to also rename it to iommufd_backend_get_device_info().


Then It would interesting to see how this applies to Eric's series.



Reply via email to