Re: [PATCH v8 04/28] vfio: add vfio_attach_device_by_iommu_type()

["Cédric Le Goater" <clg@redhat.com>]

On 2/19/25 15:48, John Levon wrote:
> Allow attachment by explicitly passing a TYPE_VFIO_IOMMU_* string;
> vfio-user will use this later.
> 
> Signed-off-by: John Levon <john.levon@nutanix.com>
> ---
>   hw/vfio/common.c              | 30 +++++++++++++++++++-----------
>   include/hw/vfio/vfio-common.h |  3 +++
>   2 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index b49aafc40c..eefd735bc6 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1569,25 +1569,20 @@ retry:
>       return info;
>   }
>   
> -bool vfio_attach_device(char *name, VFIODevice *vbasedev,
> -                        AddressSpace *as, Error **errp)
> +bool vfio_attach_device_by_iommu_type(const char *iommu_type, char *name,
> +                                      VFIODevice *vbasedev, AddressSpace *as,
> +                                      Error **errp)
>   {
> -    const VFIOIOMMUClass *ops =
> -        VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY));
>       HostIOMMUDevice *hiod = NULL;
> -
> -    if (vbasedev->iommufd) {
> -        ops = VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_IOMMUFD));
> -    }
> -
> -    assert(ops);
> -
> +    const VFIOIOMMUClass *ops =
> +        VFIO_IOMMU_CLASS(object_class_by_name(iommu_type));
>   
>       if (!vbasedev->mdev) {
>           hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
>           vbasedev->hiod = hiod;
>       }
>   
> +

please drop the extra line.

>       if (!ops->attach_device(name, vbasedev, as, errp)) {
>           object_unref(hiod);
>           vbasedev->hiod = NULL;
> @@ -1597,6 +1592,19 @@ bool vfio_attach_device(char *name, VFIODevice *vbasedev,
>       return true;
>   }
>   
> +bool vfio_attach_device(char *name, VFIODevice *vbasedev,
> +                       AddressSpace *as, Error **errp)
> +{
> +    const char *iommu_type = TYPE_VFIO_IOMMU_LEGACY;
> +
> +    if (vbasedev->iommufd) {
> +        iommu_type = TYPE_VFIO_IOMMU_IOMMUFD;
> +    }

May be use a ternary operator.

> +    return vfio_attach_device_by_iommu_type(iommu_type, name, vbasedev,
> +                                            as, errp);
> +}
> +
>   void vfio_detach_device(VFIODevice *vbasedev)
>   {
>       if (!vbasedev->bcontainer) {
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index f4f08eb8a6..c40f8de6bc 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -252,6 +252,9 @@ bool vfio_device_is_mdev(VFIODevice *vbasedev);
>   bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp);
>   bool vfio_attach_device(char *name, VFIODevice *vbasedev,
>                           AddressSpace *as, Error **errp);
> +bool vfio_attach_device_by_iommu_type(const char *iommu_type, char *name,
> +                                      VFIODevice *vbasedev, AddressSpace *as,
> +                                      Error **errp);
>   void vfio_detach_device(VFIODevice *vbasedev);
>   VFIODevice *vfio_get_vfio_device(Object *obj);
>   


Looks OK. It will need a refresh because of the spring cleanup.


Thanks,

C.