Re: [PATCH v1 10/13] iommu/amd: Consolidate domain flush logic

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Nov 09, 2023 at 07:22:09PM +0530, Vasant Hegde wrote:
> 
> 
> On 11/7/2023 6:39 PM, Jason Gunthorpe wrote:
> > On Tue, Nov 07, 2023 at 10:14:17AM +0530, Vasant Hegde wrote:
> > 
> >>> I can say that in SMMUv3 structuring it so that the attach op at the
> >>> top of the call chain knows what to do, and called a bunch of specific
> >>> helpers was reasonable. We know at the op level if we are going to do
> >>> a V1 or V2 invalidation because the OP itself already knows if it is
> >>> working on a V1/V2 iommu domain due to how it has to load the domain
> >>> into the DTE/GCR3.
> >>
> >> AMD IOMMU invalidation is based on domain and we have single command which takes
> >> PASID as well. Current code does have separate functions but that made code
> >> duplicate. So I try to consolidate all low level functions.
> > 
> > It is really not different from SMMU, invalidation commands for S1 or
> > S2 tables are constructed differently.
> 
> I don't understand ARM completely. So I don't think I understood the entire code.

It is really similar, of the three AMD and ARM are almost twins. VT-D
is completely different.

> Looking into https://github.com/jgunthorpe/linux/commits/smmuv3_newapi tree,
> 
> As I understand you have S1/S2. I guess that's something similar to V1 and V2
> page table. 

Yes

> arm_smmu_tlb_inv_range_domain() function basically checks for S1 or S2.

Yes, I haven't touched that yet.

The function is always called from an OP callback:

 iommu_flush_ops -> arm_smmu_tlb_inv_walk -> arm_smmu_tlb_inv_range_domain
 iotlb_sync -> arm_smmu_tlb_inv_range_domain

And in both cases we can arrange things so that there are S1 and S2
specific ops that already know exactly what they should be doing.

> domain_flush_pages_v1()
> {
> 	for (i = 0; i < amd_iommu_get_num_iommus(); ++i)
> 		iommu_queue_command()
> }
> 
> domain_flush_pages_v2()
> {
> 	list_for_each_entry(dev_data, &domain->dev_list, list)
> 		iommu_queue_command()
> }
> 
> /* Invalidation based on domain */
> amd_iommu_domain_flush_pages(pdom, addr, size)
> {
> 	pasid = INVALID;
> 	gn = false;
> 
> 	if (pdom_is_v2_pgtbl_mode(pdom)) {
> 		pasid = 0; gn = true
> 		domain_flush_pages_v2()
> 	} else
> 		domain_flush_pages_v1()
> 
> 	list_for_each_entry(dev_data, &domain->dev_list, list)
> 		__device_flush_iotlb(dev_data, addr, size, pasid, gn)
> 		
> }

Looks logical

> /* Invalidation based on device */
>
> amd_iommu_dev_flush_pasid_pages()
> {
> 	iommu_flush_tlb_range(iommu, domid, pasid, addr, size)
> 
> 	device_flush_iotlb_pasid(dev_data, pasid, addr, size)
> }

When does this happen? There were only two places in ARM land where we
needed a non-domain invalidation:
 - Flushing a PCI device ATC in an attach op callback due to a
   translation change or ATS enable/disable
 - Clearing an entire cache tag before returning it to the tag
   allocator

So I would not expect to see a range here, or to have the ATC in the
same function..

Jason