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.
> Eric
>>
>> Signed-off-by: Joao Martins <joao.m.mart...@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;
>> }
>>
>
