Re: [PATCH RFCv2 3/8] vfio/iommufd: Probe and request hwpt dirty tracking capability

[Avihai Horon <avihaih@nvidia.com>]

Hi Joao,

On 12/02/2024 15:56, Joao Martins wrote:
> External email: Use caution opening links or attachments
>
>
> 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 device doesn't support dirty page tracking or has
> it disabled; or when support when the VF backing IOMMU supports dirty
> tracking. The latter is in the possibility of a device being attached
> that doesn't have a dirty tracker.
>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>   hw/vfio/common.c              | 18 ++++++++++++++++++
>   hw/vfio/iommufd.c             | 25 ++++++++++++++++++++++++-
>   include/hw/vfio/vfio-common.h |  2 ++
>   3 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index f7f85160be88..d8fc7077f839 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -216,6 +216,24 @@ bool vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer)
>       return true;
>   }
>
> +bool vfio_device_migration_supported(VFIODevice *vbasedev)
> +{
> +    if (!vbasedev->migration) {
> +        return false;
> +    }
> +
> +    return vbasedev->migration->mig_flags & VFIO_MIGRATION_STOP_COPY;

I think this is redundant, as (vbasedev->migration != NULL) implies 
(vbasedev->migration->mig_flags & VFIO_MIGRATION_STOP_COPY) == true.

> +}
> +
> +bool vfio_device_dirty_pages_supported(VFIODevice *vbasedev)
> +{
> +    if (vbasedev->pre_copy_dirty_page_tracking == ON_OFF_AUTO_OFF) {
> +        return false;
> +    }
> +
> +    return !vbasedev->dirty_pages_supported;
> +}
> +
>   /*
>    * Check if all VFIO devices are running and migration is active, which is
>    * essentially equivalent to the migration being in pre-copy phase.
> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
> index ca7ec45e725c..edacb6d72748 100644
> --- a/hw/vfio/iommufd.c
> +++ b/hw/vfio/iommufd.c
> @@ -219,11 +219,26 @@ static int iommufd_cdev_detach_ioas_hwpt(VFIODevice *vbasedev, Error **errp)
>       return ret;
>   }
>
> +static bool iommufd_dirty_pages_supported(IOMMUFDDevice *iommufd_dev,
> +                                          Error **errp)
> +{
> +    uint64_t caps;
> +    int r;
> +
> +    r = iommufd_device_get_hw_capabilities(iommufd_dev, &caps, errp);
> +    if (r) {
> +        return false;
> +    }
> +
> +    return caps & IOMMU_HW_CAP_DIRTY_TRACKING;

The false return value of this function is overloaded, it can indicate 
both error and lack of DPT support.
Should we fail iommufd_cdev_autodomains_get() if 
iommufd_dirty_pages_supported() fails?
Otherwise, errp argument of iommufd_dirty_pages_supported() is redundant 
and we can handle iommufd_device_get_hw_capabilities() error locally.

> +}
> +
>   static int iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>                                           VFIOIOMMUFDContainer *container,
>                                           Error **errp)
>   {
>       int iommufd = vbasedev->iommufd_dev.iommufd->fd;
> +    uint32_t flags = 0;
>       VFIOIOASHwpt *hwpt;
>       Error *err = NULL;
>       int ret = -EINVAL;
> @@ -245,9 +260,15 @@ static int iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>           }
>       }
>
> +    if ((vfio_device_migration_supported(vbasedev) &&
> +         !vfio_device_dirty_pages_supported(vbasedev)) ||
> +        iommufd_dirty_pages_supported(&vbasedev->iommufd_dev, &err)) {

I think it's too early to check vfio_device_migration_supported() and 
vfio_device_dirty_pages_supported() here, as vfio_migration_init() 
hasn't been called yet so vbasedev->migration and 
vbasedev->dirty_pages_supported are not initialized.
Why do we need to check this? Can't we simply request IOMMUFD DPT if 
it's supported?

Thanks.

> +        flags = IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
> +    }
> +
>       ret = iommufd_backend_alloc_hwpt(iommufd,
>                                        vbasedev->iommufd_dev.devid,
> -                                     container->ioas_id, 0, 0, 0,
> +                                     container->ioas_id, flags, 0, 0,
>                                        NULL, &hwpt_id);
>       if (ret) {
>           error_append_hint(&err,
> @@ -271,6 +292,8 @@ static int 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 0;
>   }
>
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 7f7d823221e2..a3e691c126c6 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -271,6 +271,8 @@ bool
>   vfio_devices_all_running_and_mig_active(const VFIOContainerBase *bcontainer);
>   bool
>   vfio_devices_all_device_dirty_tracking(const VFIOContainerBase *bcontainer);
> +bool vfio_device_migration_supported(VFIODevice *vbasedev);
> +bool vfio_device_dirty_pages_supported(VFIODevice *vbasedev);
>   int vfio_devices_query_dirty_bitmap(const VFIOContainerBase *bcontainer,
>                                       VFIOBitmap *vbmap, hwaddr iova,
>                                       hwaddr size);
> --
> 2.39.3
>