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

[Duan, Zhenzhong]

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);

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.
>
>