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

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

On 19/05/2023 12:40, Jason Gunthorpe wrote:
> On Thu, May 18, 2023 at 09:46:30PM +0100, 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)
>> +{
> 
> Definately no bus argument.
> 
> The supported_flags should be in the domain op not the bus op.
> 
> But I think this is sort of the wrong direction, the dirty tracking
> mode should be requested when the domain is created, not changed after
> the fact.

In practice it is done as soon after the domain is created but I understand what
you mean that both should be together; I have this implemented like that as my
first take as a domain_alloc passed flags, but I was a little undecided because
we are adding another domain_alloc() op for the user-managed pagetable and after
having another one we would end up with 3 ways of creating iommu domain -- but
maybe that's not an issue