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); 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(). Thanks Zhenzhong > >Then It would interesting to see how this applies to Eric's series. > >Thanks, > >C. > >