Re: [PATCH 2/5] vfio: Move realize() after attach_device()

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

On 4/11/25 12:17, Zhenzhong Duan wrote:
> Previously device attaching depends on realize() getting host iommu
> capabilities to check dirty tracking support.
> 
> Now we save a caps copy in VFIODevice and check that copy for dirty
> tracking support, there is no dependency any more, move realize()
> call after attach_device() call in vfio_device_attach().
> 
> Drop vfio_device_hiod_realize() which looks redundant now.
> 
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Suggested-by: Donald Dutile <ddutile@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>   include/hw/vfio/vfio-device.h |  1 -
>   hw/vfio/container.c           |  4 ----
>   hw/vfio/device.c              | 28 +++++++++++-----------------
>   hw/vfio/iommufd.c             |  4 ----
>   4 files changed, 11 insertions(+), 26 deletions(-)
> 
> diff --git a/include/hw/vfio/vfio-device.h b/include/hw/vfio/vfio-device.h
> index 09a7af891a..14559733c6 100644
> --- a/include/hw/vfio/vfio-device.h
> +++ b/include/hw/vfio/vfio-device.h
> @@ -124,7 +124,6 @@ bool vfio_device_irq_set_signaling(VFIODevice *vbasedev, int index, int subindex
>   
>   void vfio_device_reset_handler(void *opaque);
>   bool vfio_device_is_mdev(VFIODevice *vbasedev);
> -bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp);
>   bool vfio_device_attach(char *name, VFIODevice *vbasedev,
>                           AddressSpace *as, Error **errp);
>   void vfio_device_detach(VFIODevice *vbasedev);
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index 23a3373470..676e88cef4 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -883,10 +883,6 @@ static bool vfio_legacy_attach_device(const char *name, VFIODevice *vbasedev,
>   
>       trace_vfio_device_attach(vbasedev->name, groupid);
>   
> -    if (!vfio_device_hiod_realize(vbasedev, errp)) {
> -        return false;
> -    }
> -
>       group = vfio_group_get(groupid, as, errp);
>       if (!group) {
>           return false;
> diff --git a/hw/vfio/device.c b/hw/vfio/device.c
> index 4de6948cf4..6154d3f443 100644
> --- a/hw/vfio/device.c
> +++ b/hw/vfio/device.c
> @@ -347,17 +347,6 @@ bool vfio_device_is_mdev(VFIODevice *vbasedev)
>       return subsys && (strcmp(subsys, "/sys/bus/mdev") == 0);
>   }
>   
> -bool vfio_device_hiod_realize(VFIODevice *vbasedev, Error **errp)
> -{
> -    HostIOMMUDevice *hiod = vbasedev->hiod;
> -
> -    if (!hiod) {
> -        return true;
> -    }
> -
> -    return HOST_IOMMU_DEVICE_GET_CLASS(hiod)->realize(hiod, vbasedev, errp);
> -}
> -
>   VFIODevice *vfio_get_vfio_device(Object *obj)
>   {
>       if (object_dynamic_cast(obj, TYPE_VFIO_PCI)) {
> @@ -372,6 +361,7 @@ bool vfio_device_attach(char *name, VFIODevice *vbasedev,
>   {
>       const VFIOIOMMUClass *ops =
>           VFIO_IOMMU_CLASS(object_class_by_name(TYPE_VFIO_IOMMU_LEGACY));
> +    HostIOMMUDeviceClass *hiodc;
>       HostIOMMUDevice *hiod = NULL;
>   
>       if (vbasedev->iommufd) {
> @@ -380,16 +370,20 @@ bool vfio_device_attach(char *name, VFIODevice *vbasedev,
>   
>       assert(ops);
>   
> +    if (!ops->attach_device(name, vbasedev, as, errp)) {
> +        return false;
> +    }
>   
>       if (!vbasedev->mdev) {
>           hiod = HOST_IOMMU_DEVICE(object_new(ops->hiod_typename));
> -        vbasedev->hiod = hiod;
> -    }
> +        hiodc = HOST_IOMMU_DEVICE_GET_CLASS(hiod);
>   
> -    if (!ops->attach_device(name, vbasedev, as, errp)) {
> -        object_unref(hiod);
> -        vbasedev->hiod = NULL;
> -        return false;
> +        if (!hiodc->realize(hiod, vbasedev, errp)) {
> +            object_unref(hiod);
> +            ops->detach_device(vbasedev);
> +            return false;
> +        }
> +        vbasedev->hiod = hiod;

This is not what I meant. I was not clear enough. Sorry about that.

hiodc->realize can be called under each container backend: legacy
and iommufd. I don't see much much value to make it common and
it would remove the unref/detach sequence to handle errors.

Thanks,

C.



>       }
>   
>       return true;
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 530cde6740..e05b472e35 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -502,10 +502,6 @@ static bool iommufd_cdev_attach(const char *name, VFIODevice *vbasedev,
>        * FD to be connected and having a devid to be able to successfully call
>        * iommufd_backend_get_device_info().
>        */
> -    if (!vfio_device_hiod_realize(vbasedev, errp)) {
> -        goto err_alloc_ioas;
> -    }
> -
>       if (!iommufd_backend_get_device_info(vbasedev->iommufd, vbasedev->devid,
>                                            &caps->type, &caps->vendor_caps,
>                                            sizeof(VendorCaps), &caps->hw_caps,