Re: [PATCH V1 18/26] vfio/iommufd: define iommufd_cdev_make_hwpt

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

On 1/29/25 15:43, Steve Sistare wrote:
> Refactor and define iommufd_cdev_make_hwpt, to be called by CPR in a
> a later patch.  No functional change.
> 
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>   hw/vfio/iommufd.c | 69 +++++++++++++++++++++++++++++++++----------------------
>   1 file changed, 41 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 3490a8f..42ba63f 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -275,6 +275,41 @@ static bool iommufd_cdev_detach_ioas_hwpt(VFIODevice *vbasedev, Error **errp)
>       return true;
>   }
>   
> +static void iommufd_cdev_set_hwpt(VFIODevice *vbasedev, VFIOIOASHwpt *hwpt)
> +{
> +    vbasedev->hwpt = hwpt;
> +    vbasedev->iommu_dirty_tracking = iommufd_hwpt_dirty_tracking(hwpt);
> +    QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next);
> +}
> +
> +static VFIOIOASHwpt *iommufd_cdev_make_hwpt(VFIODevice *vbasedev,
> +                                            VFIOIOMMUFDContainer *container,
> +                                            uint32_t hwpt_id)
> +{
> +    VFIOIOASHwpt *hwpt = g_malloc0(sizeof(*hwpt));
> +    uint32_t flags = 0;
> +
> +    /*
> +     * This is quite early and VFIO Migration state isn't yet fully
> +     * initialized, thus rely only on IOMMU hardware capabilities as to
> +     * whether IOMMU dirty tracking is going to be requested. Later
> +     * vfio_migration_realize() may decide to use VF dirty tracking
> +     * instead.
> +     */
> +    if (vbasedev->hiod->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) {
> +        flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
> +    }
> +
> +    hwpt->hwpt_id = hwpt_id;
> +    hwpt->hwpt_flags = flags;
> +    QLIST_INIT(&hwpt->device_list);
> +
> +    QLIST_INSERT_HEAD(&container->hwpt_list, hwpt, next);
> +    container->bcontainer.dirty_pages_supported |=
> +                                vbasedev->iommu_dirty_tracking;
> +    return hwpt;
> +}
> +
>   static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>                                            VFIOIOMMUFDContainer *container,
>                                            Error **errp)
> @@ -304,24 +339,11 @@ static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>   
>               return false;
>           } else {
> -            vbasedev->hwpt = hwpt;
> -            QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next);
> -            vbasedev->iommu_dirty_tracking = iommufd_hwpt_dirty_tracking(hwpt);
> +            iommufd_cdev_set_hwpt(vbasedev, hwpt);
>               return true;
>           }
>       }
>   
> -    /*
> -     * This is quite early and VFIO Migration state isn't yet fully
> -     * initialized, thus rely only on IOMMU hardware capabilities as to
> -     * whether IOMMU dirty tracking is going to be requested. Later
> -     * vfio_migration_realize() may decide to use VF dirty tracking
> -     * instead.
> -     */
> -    if (vbasedev->hiod->caps.hw_caps & IOMMU_HW_CAP_DIRTY_TRACKING) {
> -        flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
> -    }
>

AFAICT, iommufd_backend_alloc_hwpt() below needs the flag value.

Thanks,

C.



>       if (!iommufd_backend_alloc_hwpt(iommufd, vbasedev->devid,
>                                       container->ioas_id, flags,
>                                       IOMMU_HWPT_DATA_NONE, 0, NULL,
> @@ -329,24 +351,15 @@ static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>           return false;
>       }
>   
> -    hwpt = g_malloc0(sizeof(*hwpt));
> -    hwpt->hwpt_id = hwpt_id;
> -    hwpt->hwpt_flags = flags;
> -    QLIST_INIT(&hwpt->device_list);
> -
> -    ret = iommufd_cdev_attach_ioas_hwpt(vbasedev, hwpt->hwpt_id, errp);
> +    ret = iommufd_cdev_attach_ioas_hwpt(vbasedev, hwpt_id, errp);
>       if (ret) {
> -        iommufd_backend_free_id(container->be, hwpt->hwpt_id);
> -        g_free(hwpt);
> +        iommufd_backend_free_id(container->be, hwpt_id);
>           return false;
>       }
>   
> -    vbasedev->hwpt = hwpt;
> -    vbasedev->iommu_dirty_tracking = iommufd_hwpt_dirty_tracking(hwpt);
> -    QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next);
> -    QLIST_INSERT_HEAD(&container->hwpt_list, hwpt, next);
> -    container->bcontainer.dirty_pages_supported |=
> -                                vbasedev->iommu_dirty_tracking;
> +    hwpt = iommufd_cdev_make_hwpt(vbasedev, container, hwpt_id);
> +    iommufd_cdev_set_hwpt(vbasedev, hwpt);
> +
>       if (container->bcontainer.dirty_pages_supported &&
>           !vbasedev->iommu_dirty_tracking) {
>           warn_report("IOMMU instance for device %s doesn't support dirty tracking",