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

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

Thanks
Zhenzhong

>
>Thanks,
>
>C.
>
>
>
>
>> +    size_t size;
>> +} HostIOMMUDevice;
>> +
>> +static inline void host_iommu_base_device_init(HostIOMMUDevice *dev,
>> +                                               HostIOMMUDevice_Type type,
>> +                                               size_t size)
>> +{
>> +    dev->type = type;
>> +    dev->size = size;
>> +}
>> +#endif