Re: [PATCH v2 03/10] backends/iommufd: Introduce abstract HIODIOMMUFD device

[Cédric Le Goater]

On 4/8/24 10:12, Zhenzhong Duan wrote:
> HIODIOMMUFD represents a host IOMMU device under iommufd backend.
>
> Currently it includes only public iommufd handle and device id.
> which could be used to get hw IOMMU information.
>
> When nested translation is supported in future, vIOMMU is going
> to have iommufd related operations like attaching/detaching hwpt,
> So IOMMUFDDevice interface will be further extended at that time.
>
> VFIO and VDPA device have different way of attaching/detaching hwpt.
> So HIODIOMMUFD is still an abstract class which will be inherited by
> VFIO and VDPA device.
>
> Introduce a helper hiod_iommufd_init() to initialize HIODIOMMUFD
> device.
>
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Originally-by: Yi Liu <yi.l.liu@intel.com>
> Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   include/sysemu/iommufd.h | 22 +++++++++++++++++++
>   backends/iommufd.c       | 47 ++++++++++++++++++++++++++--------------
>   2 files changed, 53 insertions(+), 16 deletions(-)
>
> diff --git a/include/sysemu/iommufd.h b/include/sysemu/iommufd.h
> index 9af27ebd6c..71c53cbb45 100644
> --- a/include/sysemu/iommufd.h
> +++ b/include/sysemu/iommufd.h
> @@ -4,6 +4,7 @@
>   #include "qom/object.h"
>   #include "exec/hwaddr.h"
>   #include "exec/cpu-common.h"
> +#include "sysemu/host_iommu_device.h"
>   
>   #define TYPE_IOMMUFD_BACKEND "iommufd"
>   OBJECT_DECLARE_TYPE(IOMMUFDBackend, IOMMUFDBackendClass, IOMMUFD_BACKEND)
> @@ -33,4 +34,25 @@ int iommufd_backend_map_dma(IOMMUFDBackend *be, uint32_t 
> ioas_id, hwaddr iova,
>                               ram_addr_t size, void *vaddr, bool readonly);
>   int iommufd_backend_unmap_dma(IOMMUFDBackend *be, uint32_t ioas_id,
>                                 hwaddr iova, ram_addr_t size);
> +
> +#define TYPE_HIOD_IOMMUFD TYPE_HOST_IOMMU_DEVICE "-iommufd"
Please keep TYPE_HOST_IOMMU_DEVICE

> +OBJECT_DECLARE_TYPE(HIODIOMMUFD, HIODIOMMUFDClass, HIOD_IOMMUFD)
> +
> +struct HIODIOMMUFD {
> +    /*< private >*/
> +    HostIOMMUDevice parent;
> +    void *opaque;
> +
> +    /*< public >*/
> +    IOMMUFDBackend *iommufd;
> +    uint32_t devid;
> +};
> +
> +struct HIODIOMMUFDClass {
> +    /*< private >*/
> +    HostIOMMUDeviceClass parent_class;
> +};
This new class doesn't seem useful. Do you have plans for handlers ?

> +
> +void hiod_iommufd_init(HIODIOMMUFD *idev, IOMMUFDBackend *iommufd,
> +                       uint32_t devid);
>   #endif
> diff --git a/backends/iommufd.c b/backends/iommufd.c
> index 62a79fa6b0..ef8b3a808b 100644
> --- a/backends/iommufd.c
> +++ b/backends/iommufd.c
> @@ -212,23 +212,38 @@ int iommufd_backend_unmap_dma(IOMMUFDBackend *be, 
> uint32_t ioas_id,
>       return ret;
>   }
>   
> -static const TypeInfo iommufd_backend_info = {
> -    .name = TYPE_IOMMUFD_BACKEND,
> -    .parent = TYPE_OBJECT,
> -    .instance_size = sizeof(IOMMUFDBackend),
> -    .instance_init = iommufd_backend_init,
> -    .instance_finalize = iommufd_backend_finalize,
> -    .class_size = sizeof(IOMMUFDBackendClass),
> -    .class_init = iommufd_backend_class_init,
> -    .interfaces = (InterfaceInfo[]) {
> -        { TYPE_USER_CREATABLE },
> -        { }
> -    }
> -};
> +void hiod_iommufd_init(HIODIOMMUFD *idev, IOMMUFDBackend *iommufd,
> +                       uint32_t devid)
> +{
> +    idev->iommufd = iommufd;
> +    idev->devid = devid;
> +}
This routine doesn't seem useful. I wonder if we shouldn't introduce
properties. I'm not sure this is useful either.


> -static void register_types(void)
> +static void hiod_iommufd_class_init(ObjectClass *oc, void *data)
>   {
> -    type_register_static(&iommufd_backend_info);
>   }
>   
> -type_init(register_types);
> +static const TypeInfo types[] = {
> +    {
> +        .name = TYPE_IOMMUFD_BACKEND,
> +        .parent = TYPE_OBJECT,
> +        .instance_size = sizeof(IOMMUFDBackend),
> +        .instance_init = iommufd_backend_init,
> +        .instance_finalize = iommufd_backend_finalize,
> +        .class_size = sizeof(IOMMUFDBackendClass),
> +        .class_init = iommufd_backend_class_init,
> +        .interfaces = (InterfaceInfo[]) {
> +            { TYPE_USER_CREATABLE },
> +            { }
> +        }
> +    }, {
> +        .name = TYPE_HIOD_IOMMUFD,
> +        .parent = TYPE_HOST_IOMMU_DEVICE,
> +        .instance_size = sizeof(HIODIOMMUFD),
> +        .class_size = sizeof(HIODIOMMUFDClass),
> +        .class_init = hiod_iommufd_class_init,
> +        .abstract = true,
> +    }
> +};
> +
> +DEFINE_TYPES(types)