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

[Eric Auger]

On 7/23/24 10:00, Joao Martins wrote:
> 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.
Yes I would have suspected they would end up in different
containers/ioas but I am not sure.
>
> 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
on ARM you may have several SMMU instances. I do agree that the
likelyhood of those instances having heterogeneous dirty page tracking
support is low but well I don't know. Maybe we should add a wanrning at
least, later on if this case arises.

Eric
> 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).
>