RE: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@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 <clg@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 <clg@redhat.com>
>>>>> Sent: Tuesday, March 19, 2024 4:17 PM
>>>>> To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
>>>>> devel@nongnu.org
>>>>> Cc: alex.williamson@redhat.com; eric.auger@redhat.com;
>>>>> peterx@redhat.com; jasowang@redhat.com; mst@redhat.com;
>>>>> jgg@nvidia.com; nicolinc@nvidia.com; joao.m.martins@oracle.com;
>Tian,
>>>>> Kevin <kevin.tian@intel.com>; Liu, Yi L <yi.l.liu@intel.com>; Sun, Yi Y
>>>>> <yi.y.sun@intel.com>; Peng, Chao P <chao.p.peng@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.auger@redhat.com>
>>>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@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