>-----Original Message----- >From: Cédric Le Goater <c...@redhat.com> >Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct >HostIOMMUDevice > >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 >>> HostIOMMUDevice >>> >>> 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- >>>>> de...@nongnu.org >>>>> 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 >>>>> HostIOMMUDevice >>>>> >>>>> 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 >>>>> b/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 @@ >>>>>> +#ifndef HOST_IOMMU_DEVICE_H >>>>>> +#define HOST_IOMMU_DEVICE_H >>>>>> + >>>>>> +typedef enum HostIOMMUDevice_Type { >>>>>> + HID_LEGACY, >>>>>> + HID_IOMMUFD, >>>>>> + 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: >>> https://lore.kernel.org/all/YmuFv2s5TPuw7K%2Fu@yekko/ >>>> 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" >>> OBJECT_DECLARE_TYPE(HostIOMMUDevice, HostIOMMUClass, >>> 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 >>> TYPE_HOST_IOMMU_LEGACY. >>> Each class implementing the handlers or not (legacy mode). >> >> Understood, thanks for your guide. >> >>> >>> The class handlers are introduced for the intel-iommu helper >>> vtd_check_hdev() >>> in order to avoid using iommufd routines directly. HostIOMMUDevice is >>> supposed >>> 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 >> preference. >> >> 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 >ioctl(IOMMU_GET_HW_INFO), >> 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.
Got it, thanks Cédric! BRs. Zhenzhong