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

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Nov 06, 2023 at 04:23:10PM +0530, Vasant Hegde wrote:
> 
> >> @@ -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.

I saw that, I still think it is a maze.

> >> @@ -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.

The iommu cache only needs to be flushed if the IOPTEs associated with
the cache tag change, and that doesn't happen during "detach".

Some of this is confusing because we've largely removed the word
'detach' from the iommu language, we now just have attach, eg replace.

When the driver has to replace an iommu_domain translation to a PCI
RID with a new translation it should run a sequence depending on how
it is using cache tags.

Cache tag shared in the domain - eg the cache tag is stored in the
iommu_domain struct:

 - Load the DTE/GCR3/etc with the new translation
 - Invalidate any RID -> Cache tag caches (at this point ATS requests
    see the new translation)
 - Invalidate the ATC
 
 The cache tag is flushed when the domain is freed and the tag
 returned to the allocator

Cache tag is unique to the device, replacing the translation continues
to use the existing tag:

 - Load the DTE/GCR3/etc with the new translation
 - Invalidate any RID -> DTE/Cache tag caches (at this point ATS requests
    see the new translation)
 - Invalidate the cache tag, since we changed the translation
 - Invalidate the ATC

 The cache tag is flushed when the GCR3 table is freed and the tag
 returned to the allocator

I agree it does not need to be in this series, but there is a logic to
how all this should flow that should be captured in the APIs. Bundling
all kinds of IOTLB and ATC invalidation into one call chain is not
consistent with where things should end up.

The attach/replace ops should implement the exact sequence of
invalidations they need for what they are doing in a clear sequence.

Jason