Re: [PATCH v1 09/13] iommu/amd: Refactor domain flush global function

[Vasant Hegde <vasant.hegde@amd.com>]

On 11/5/2023 11:22 PM, Jason Gunthorpe wrote:
> On Fri, Oct 06, 2023 at 10:16:20AM +0000, Vasant Hegde wrote:
> 
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 1f695dd50fec..c30b08e2a939 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -1553,10 +1553,18 @@ static void domain_flush_pages(struct protection_domain *domain,
>>  	amd_iommu_domain_flush_complete(domain);
>>  }
>>  
>> +/* Flush range of IO/TLB for a given protection domain */
>> +void amd_iommu_domain_flush_pages(struct protection_domain *pdom,
>> +				  u64 address, size_t size)
>> +{
>> +	return domain_flush_pages(pdom, address, size);
>> +}
> 
> What is the point of having this maze of functions? Just rename
> domain_flush_pages?

Later patch adds PASID check to this function.

> 
>> @@ -1859,7 +1867,7 @@ static void do_detach(struct iommu_dev_data *dev_data)
>>  	device_flush_dte(dev_data);
>>  
>>  	/* Flush IOTLB and wait for the flushes to finish */
>> -	amd_iommu_domain_flush_tlb_pde(domain);
>> +	amd_iommu_domain_flush_all(domain);
> 
> This is weird.. The cache tag for a domain shouldn't necessarily be
> flushed from the iotlb when a domain is removed from a dte, should it?


In this path we need to flush IOMMU cache along with device IOTLB. That's why
original code called domain flush. May be we can split this into separate piece
where DTE update path will flush the IOMMU TLB and then in do_detach() path we
can just flush IOTLB. I think this needs more investigation to make sure we
don't break things.

Given the amount of changes we are doing in this space I'd prefer not to touch
this now. I will add it into my TODO list. Will revisit along with other
invalidation improvement that I planned to do later (like re-introducing PDE
bit, etc.).


-Vasant

> 
> ATC and IOTLB flushing should be seperate. The IOTLB should be flushed
> immediately before some event that causes the cache tag to become
> invalid, eg returning the tag to an allocator.
> 
> It is a minor point, but it does make things more understandable if
> stuff is done in the right places.
> 
> Jason