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

[Joao Martins]

On 23/07/2024 08:50, Eric Auger wrote:
> Hi Joao,
> 
> On 7/22/24 23:13, Joao Martins wrote:
>> 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);
>>      QLIST_INSERT_HEAD(&hwpt->device_list, vbasedev, hwpt_next);
>>      QLIST_INSERT_HEAD(&container->hwpt_list, hwpt, next);
>> +    container->bcontainer.dirty_pages_supported |=
>> +                                vbasedev->iommu_dirty_tracking;
> Is it possible to have several devices with different
> 
> iommu_dirty_tracking value in the same container? In other words would they 
> be attached to different container/ioas?
> 

In theory, yes, they can be in the same container/ioas. But I guess with IOMMUFD
it's possible that we can allocate different containers for different devices
given that we can manipulate/pass a different IOMMUFD object.

In pratice I don't know if such HW platforms even exist where different IOMMU
instances present different value of dirty tracking, given that this is a IOMMU
feature, rather than endpoint dependent. In x86 it's homogeneous, and likely on
smmuv3 server too. There are indeed endpoint related features which may be
different in IOMMU instances, but those only reflect on logic that the device
needs to implement (e.g. PCIe PRS).

Having said that I can only think of mdevs, where the realize() will block
migration because the vbasedev->iommu_dirty_tracking is 0 should the mdev not
support dma-logging vfio (but it doesn't go via this codepath above anyhow).