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

[Duan, Zhenzhong]

>-----Original Message-----
>From: Cédric Le Goater <clg@redhat.com>
>Subject: Re: [PATCH v6 5/9] vfio/iommufd: Probe and request hwpt dirty
>tracking capability
>
>On 7/23/24 08:13, Joao Martins wrote:
>> 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
>
>
>Fixed on vfio-9.1.

Feel free to add my RB,

Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Thanks
Zhenzhong

>
>Thanks,
>
>C.
>
>
>
>>
>> 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;
>>           }
>>       }
>>