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

[Jason Gunthorpe <jgg@nvidia.com>]

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?

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

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