Re: [PATCH RFCv2 04/24] iommu: Add iommu_domain ops for dirty tracking

[Joao Martins <joao.m.martins@oracle.com>]

On 19/05/2023 09:42, Baolu Lu wrote:
> On 2023/5/19 4:46, Joao Martins wrote:
>> Add to iommu domain operations a set of callbacks to perform dirty
>> tracking, particulary to start and stop tracking and finally to read and
>> clear the dirty data.
>>
>> Drivers are generally expected to dynamically change its translation
>> structures to toggle the tracking and flush some form of control state
>> structure that stands in the IOVA translation path. Though it's not
>> mandatory, as drivers will be enable dirty tracking at boot, and just flush
>> the IO pagetables when setting dirty tracking.  For each of the newly added
>> IOMMU core APIs:
>>
>> .supported_flags[IOMMU_DOMAIN_F_ENFORCE_DIRTY]: Introduce a set of flags
>> that enforce certain restrictions in the iommu_domain object. For dirty
>> tracking this means that when IOMMU_DOMAIN_F_ENFORCE_DIRTY is set via its
>> helper iommu_domain_set_flags(...) devices attached via attach_dev will
>> fail on devices that do*not*  have dirty tracking supported. IOMMU drivers
>> that support dirty tracking should advertise this flag, while enforcing
>> that dirty tracking is supported by the device in its .attach_dev iommu op.
>>
>> iommu_cap::IOMMU_CAP_DIRTY: new device iommu_capable value when probing for
>> capabilities of the device.
>>
>> .set_dirty_tracking(): an iommu driver is expected to change its
>> translation structures and enable dirty tracking for the devices in the
>> iommu_domain. For drivers making dirty tracking always-enabled, it should
>> just return 0.
>>
>> .read_and_clear_dirty(): an iommu driver is expected to walk the iova range
>> passed in and use iommu_dirty_bitmap_record() to record dirty info per
>> IOVA. When detecting a given IOVA is dirty it should also clear its dirty
>> state from the PTE,*unless*  the flag IOMMU_DIRTY_NO_CLEAR is passed in --
>> flushing is steered from the caller of the domain_op via iotlb_gather. The
>> iommu core APIs use the same data structure in use for dirty tracking for
>> VFIO device dirty (struct iova_bitmap) abstracted by
>> iommu_dirty_bitmap_record() helper function.
>>
>> Signed-off-by: Joao Martins<joao.m.martins@oracle.com>
>> ---
>>   drivers/iommu/iommu.c      | 11 +++++++
>>   include/linux/io-pgtable.h |  4 +++
>>   include/linux/iommu.h      | 67 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 82 insertions(+)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 2088caae5074..95acc543e8fb 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -2013,6 +2013,17 @@ struct iommu_domain *iommu_domain_alloc(const struct
>> bus_type *bus)
>>   }
>>   EXPORT_SYMBOL_GPL(iommu_domain_alloc);
>>   +int iommu_domain_set_flags(struct iommu_domain *domain,
>> +               const struct bus_type *bus, unsigned long val)
>> +{
>> +    if (!(val & bus->iommu_ops->supported_flags))
>> +        return -EINVAL;
>> +
>> +    domain->flags |= val;
>> +    return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_domain_set_flags);
> 
> This seems to be a return to an old question. IOMMU domains are
> allocated through buses, but can a domain be attached to devices on
> different buses that happen to have different IOMMU ops? In reality, we
> may not have such heterogeneous configurations yet, but it is better to
> avoid such confusion as much as possible.
> 
> How about adding a domain op like .enforce_dirty_page_tracking. The
> individual iommu driver which implements this callback will iterate all
> devices that the domain has been attached and return success only if all
> attached devices support dirty page tracking.
> 

Hmm, but isn't the point is to actually prevent this from happening? Meaning to
ensure that only devices that support dirty tracking are attached in the domain?
The flag is meant to be advertise that individual domain knows about dirty
tracking enforcement, and it will validate it at .attach_dev when asked.

> Then, in the domain's attach_dev or set_dev_pasid callbacks, if the

I certainly didn't handle the ::set_dev_pasid callback, might have to fix next
iteration

> domain has been enforced dirty page tracking while the device to be
> attached doesn't support it, -EINVAL will be returned which could be
> intercepted by the caller as domain is incompatible.

This part is already done; I am just a little stuck on a
::enforce_dirty_tracking domain op being done /after/ devices are already
present in the domain. Note that today this is done right after we create the
hwpt (i.e. the iommu_domain) without devices being attached to it yet. I had a
separate version where I create a domain object with (bus, flags) as an
alternate incantation of this. In the variants alternatives I implemented,
ultimately picked this one as it could other similar things to sit on (e.g.
enforce_cache_coherency?)

I can switch to enforce_dirty_tracking instead of a flag, but I think it looks
more correct to ensure the property remains imutable at-domain-creation rather
than post dev attachment, unless I am missing something here.