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

[Avihai Horon <avihaih@nvidia.com>]

On 20/02/2024 12:51, Joao Martins wrote:
> External email: Use caution opening links or attachments
>
>
> On 19/02/2024 09:03, Avihai Horon wrote:
>> 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.
>>
> The check was there to prevent a null-deref in case the device didn't support
> migration.

I meant that "vbasedev->migration->mig_flags & VFIO_MIGRATION_STOP_COPY" 
check is redundant because if vbasedev->migration != NULL then 
VFIO_MIGRATION_STOP_COPY is supported (it's already checked in 
vfio_migration_init()).

But never mind, given what you wrote below.

>
>>> +}
>>> +
>>> +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?
> Definitely not.
>
>> Otherwise, errp argument of iommufd_dirty_pages_supported() is redundant and we
>> can handle iommufd_device_get_hw_capabilities() error locally.
>>
> I'll handle 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.
> I should replace with its own vfio device probing but the next point invalidates
> this
>
>> Why do we need to check this? Can't we simply request IOMMUFD DPT if it's
>> supported?
>>
> There's no point in force requesting dpt in the domain if the device doesn't do
> migration that was my thinking here; but otoh as past hotplug bug fixes have
> shown it needs to proof against a new device getting add up that supports
> migration while and the unsupported one be removed. So I guess we might not have
> another option but to always ask for it if 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
>>>