Re: [PATCH v4 08/12] vfio/iommufd: Probe and request hwpt dirty tracking capability

[Eric Auger]

On 7/17/24 14:38, Joao Martins wrote:
> On 17/07/2024 13:27, Eric Auger wrote:
>> Hi Joao,
>>
>> On 7/12/24 13:47, Joao Martins wrote:
>>> Probe hardware dirty tracking support by querying device hw capabilities via
>>> IOMMUFD_GET_HW_INFO.
>> this is not what the patch brings. GET_HW_INFO is always in place.
> Yes. This is my mistake in squashing things as there was some shuffling going
> around on how we do GET_HW_INFO. and didn't adjust the right hand of this 
> sentence.
>
> I'll rephrase it.
>
>>> In preparation to using the dirty tracking UAPI, request dirty tracking in 
>>> the
>>> HWPT flags when the IOMMU supports dirty tracking.
>> this is what the patch brings.
> Right.
>
>>> The auto domain logic allows different IOMMU domains to be created when DMA
>>> dirty tracking is not desired (and VF can provide it) while others doesn't 
>>> have
>> don't
> Right
>
>>> it and want the IOMMU capability. This is not used in this way here given 
>>> how
>>> VFIODevice migration capability checking takes place *after* the device
>>> attachment.
>> Id on't understand the above sentence
>>
> The whole paragraph is meant to emphasize that we don't know if VF dirty
> tracking is supported because VFIODevice migration state hasn't been probed
> *yet*. And so we can't pick VF dirty tracking vs IOMMU dirty tracking at this
> stage when using IOMMU_HWPT_ALLOC_DIRTY_TRACKING flag and hence we always use 
> it
> if IOMMU hw supports it even if later on VFIOMigration decides to use VF dirty
> tracking always instead.

that sounds a clearer explanation to me

Eric
>
>> Eric
>>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>>> ---
>>>  include/hw/vfio/vfio-common.h |  1 +
>>>  hw/vfio/iommufd.c             | 12 ++++++++++++
>>>  2 files changed, 13 insertions(+)
>>>
>>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>>> index 2dd468ce3c02..760f31d84ac8 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;
>>> diff --git a/hw/vfio/iommufd.c b/hw/vfio/iommufd.c
>>> index d34dc88231ec..edc8f97d8f3d 100644
>>> --- a/hw/vfio/iommufd.c
>>> +++ b/hw/vfio/iommufd.c
>>> @@ -246,6 +246,15 @@ static bool iommufd_cdev_autodomains_get(VFIODevice 
>>> *vbasedev,
>>>          }
>>>      }
>>>  
>>> +    /*
>>> +     * This is quite early and VFIODevice isn't yet fully initialized,
>> so what's the problem exactly with the above?
> I should really say 'VFIO Migration state' here (see previous comment)
>
>>> +     * thus rely on IOMMU hardware capabilities as to whether IOMMU dirty
>>> +     * tracking is going to be needed.
>>> +     */
>>> +    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 +264,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);
>>> @@ -267,6 +277,8 @@ static bool 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 true;
>>>  }
>>>