Re: [PATCH v6 5/9] vfio/iommufd: Probe and request hwpt dirty tracking capability

[Joao Martins]

On 23/07/2024 06:11, Duan, Zhenzhong wrote:
> 
> 
>> -----Original Message-----
>> From: Joao Martins <joao.m.martins@oracle.com>
>> Subject: [PATCH v6 5/9] vfio/iommufd: Probe and request hwpt dirty
>> tracking capability
>>
>> In preparation to using the dirty tracking UAPI, probe whether the IOMMU
>> supports dirty tracking. This is done via the data stored in
>> hiod::caps::hw_caps initialized from GET_HW_INFO.
>>
>> Qemu doesn't know if VF dirty tracking is supported when allocating
>> hardware pagetable in iommufd_cdev_autodomains_get(). This is because
>> VFIODevice migration state hasn't been initialized *yet* hence it can't pick
>> between VF dirty tracking vs IOMMU dirty tracking. So, if IOMMU supports
>> dirty tracking it always creates HWPTs with
>> IOMMU_HWPT_ALLOC_DIRTY_TRACKING
>> even if later on VFIOMigration decides to use VF dirty tracking instead.
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>> include/hw/vfio/vfio-common.h |  2 ++
>> hw/vfio/iommufd.c             | 20 ++++++++++++++++++++
>> 2 files changed, 22 insertions(+)
>>
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-
>> common.h
>> index 4e44b26d3c45..1e02c98b09ba 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;
>> @@ -139,6 +140,7 @@ typedef struct VFIODevice {
>>     OnOffAuto pre_copy_dirty_page_tracking;
>>     bool dirty_pages_supported;
>>     bool dirty_tracking;
>> +    bool iommu_dirty_tracking;
>>     HostIOMMUDevice *hiod;
>>     int devid;
>>     IOMMUFDBackend *iommufd;
>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>> index 2324bf892c56..7afea0b041ed 100644
>> --- a/hw/vfio/iommufd.c
>> +++ b/hw/vfio/iommufd.c
>> @@ -110,6 +110,11 @@ static void
>> iommufd_cdev_unbind_and_disconnect(VFIODevice *vbasedev)
>>     iommufd_backend_disconnect(vbasedev->iommufd);
>> }
>>
>> +static bool iommufd_hwpt_dirty_tracking(VFIOIOASHwpt *hwpt)
>> +{
>> +    return hwpt && hwpt->hwpt_flags &
>> IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
>> +}
>> +
>> static int iommufd_cdev_getfd(const char *sysfs_path, Error **errp)
>> {
>>     ERRP_GUARD();
>> @@ -246,6 +251,17 @@ static bool
>> iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>>         }
>>     }
>>
>> +    /*
>> +     * 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;
>> +    }
>> +
>>     if (!iommufd_backend_alloc_hwpt(iommufd, vbasedev->devid,
>>                                     container->ioas_id, flags,
>>                                     IOMMU_HWPT_DATA_NONE, 0, NULL,
>> @@ -255,6 +271,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);
>> @@ -265,8 +282,11 @@ static bool
>> iommufd_cdev_autodomains_get(VFIODevice *vbasedev,
>>     }
>>
>>     vbasedev->hwpt = hwpt;
>> +    vbasedev->iommu_dirty_tracking =
>> iommufd_hwpt_dirty_tracking(hwpt);
> 
> Don't we need to do same if attach to existing hwpt?
> 

Nice catch!

Yes, we do need it e.g. we will need this fix up fo this patch

diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
index 92b976464283..833a7400486c 100644
--- a/hw/vfio/iommufd.c
+++ b/hw/vfio/iommufd.c
@@ -305,6 +305,7 @@ static bool iommufd_cdev_autodomains_get(VFIODevice 
*vbasedev,
         } else {
             vbasedev->hwpt = hwpt;
             QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next);
+            vbasedev->iommu_dirty_tracking = iommufd_hwpt_dirty_tracking(hwpt);
             return true;
         }
     }