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

From: "Cédric Le Goater" <clg@redhat.com>
To: Zhenzhong Duan <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,
	kevin.tian@intel.com, yi.l.liu@intel.com, yi.y.sun@intel.com,
	chao.p.peng@intel.com
Subject: Re: [PATCH v1 01/11] Introduce a common abstract struct HostIOMMUDevice
Date: Tue, 19 Mar 2024 09:16:39 +0100	[thread overview]
Message-ID: <e6af68f4-f38d-413f-a63c-d1ae7254d802@redhat.com> (raw)
In-Reply-To: <20240228035900.1085727-2-zhenzhong.duan@intel.com>

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.

Is vtd_check_hdev() the only use of this field ? If so, can we simplify
with a QOM interface in any way ?

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