Re: [PATCH v3 05/10] vfio/iommufd: Probe and request hwpt dirty tracking capability

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

On 7/8/24 4:34 PM, Joao Martins wrote:
> Probe hardware dirty tracking support by querying device hw capabilities via
> IOMMUFD_GET_HW_INFO.
> 
> In preparation to using the dirty tracking UAPI, request dirty tracking in the
> HWPT flags when the IOMMU supports dirty tracking.
> 
> The auto domain logic allows different IOMMU domains to be created when DMA
> dirty tracking is not desired (and VF can provide it) while others doesn't have
> it and want the IOMMU capability. This is not used in this way here given how
> VFIODevice migration capability checking takes place *after* the device
> attachment. But such granularity is a nice property that can be implemented
> later on.
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>   include/hw/vfio/vfio-common.h |  1 +
>   hw/vfio/iommufd.c             | 26 ++++++++++++++++++++++++++
>   2 files changed, 27 insertions(+)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 82c5a4aaa61e..7ce925cfab19 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -97,6 +97,7 @@ typedef struct IOMMUFDBackend IOMMUFDBackend;
>   
>   typedef struct VFIOIOASHwpt {
>       uint32_t hwpt_id;
> +    uint32_t hwpt_flags;
>       QLIST_HEAD(, VFIODevice) device_list;
>       QLIST_ENTRY(VFIOIOASHwpt) next;
>   } VFIOIOASHwpt;
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index 2ca9a32cc7b6..1b5b46d28ed6 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -212,6 +212,20 @@ static bool iommufd_cdev_detach_ioas_hwpt(VFIODevice *vbasedev, Error **errp)
>       return true;
>   }
>   
> +static bool iommufd_device_dirty_tracking(IOMMUFDBackend *iommufd,
> +                                          VFIODevice *vbasedev)
> +{
> +    enum iommu_hw_info_type type;
> +    uint64_t caps;
> +
> +    if (!iommufd_backend_get_device_info(iommufd, vbasedev->devid, &type,
> +                                         NULL, 0, &caps, NULL)) {

I think we should report the error and not ignore it.

That said, since we are probing the hw features of the host IOMMU device,
could we use the data cached in the HostIOMMUDevice struct instead ?
This means would need to move the ->realize() call doing the probing
before attaching the device in vfio_attach_device(). That way we would
catch probing errors in one place. Does this make sense ?

Thanks,

C.




> +        return false;
> +    }
> +
> +    return caps & IOMMU_HW_CAP_DIRTY_TRACKING;
> +}
> +
>   static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>                                            VFIOIOMMUFDContainer *container,
>                                            Error **errp)
> @@ -239,6 +253,15 @@ static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>           }
>       }
>   
> +    /*
> +     * This is quite early and VFIODevice isn't yet fully initialized,
> +     * thus rely on IOMMU hardware capabilities as to whether IOMMU dirty
> +     * tracking is going to be needed.
> +     */
> +    if (iommufd_device_dirty_tracking(iommufd, vbasedev)) {
> +        flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
> +    }
> +
>       ret = iommufd_backend_alloc_hwpt(iommufd,
>                                        vbasedev->devid,
>                                        container->ioas_id, flags,
> @@ -255,6 +278,7 @@ static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>   
>       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);
> @@ -267,6 +291,8 @@ static bool iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>       vbasedev->hwpt = hwpt;
>       QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next);
>       QLIST_INSERT_HEAD(&container->hwpt_list, hwpt, next);
> +    container->bcontainer.dirty_pages_supported |=
> +                              (flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING);
>       return true;
>   }
>