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

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Nov 06, 2023 at 04:42:17PM +0530, Vasant Hegde wrote:
> 
> 
> On 11/5/2023 11:25 PM, Jason Gunthorpe wrote:
> > On Fri, Oct 06, 2023 at 10:16:21AM +0000, Vasant Hegde wrote:
> > 
> >> @@ -1557,7 +1583,9 @@ static void domain_flush_pages(struct protection_domain *domain,
> >>  void amd_iommu_domain_flush_pages(struct protection_domain *pdom,
> >>  				  u64 address, size_t size)
> >>  {
> >> -	return domain_flush_pages(pdom, address, size);
> >> +	ioasid_t pasid = pdom_get_default_pasid(pdom);
> >> +
> >> +	return domain_flush_pages(pdom, pasid, address, size);
> >>  }
> > 
> > There should be no such thing as a pasid for a domain. There should
> 
> We do have default PASID (zero) when we boot in v2 page table mode.

That is the PASID for a RID, not the PASID or a domain.

> > not be an api called pdom_get_default_pasid(), it is nonsensical.
> 
> But we have single command to deal with all IOMMU TLB flush.

This is probably a mistake..

> I have a choice of doing all checks in low level function (what this patch does)
>  -OR- add checks in all API path (like flush_iotlb_all, iotlb_sync, etc). If
> that's what preferred I can do that.

Well, you should not have concepts like the 'pasid of a domain' and
you should not encode the v1/v2 type into the pasid. I didn't look
closely how to sort that out to say exactly.

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.

So if you have:

   invalidate_iotlb_range/all_v1(domain)
   invalidate_iotlb_range/all_v2(device, pasid)
   invalidate_iotlb_full_v2(device)  // Clean every pasid
   invalidate_atc_range/all(domain)
     invalidate_atc_device_range_all(device, pasid)
   invalidate_atc_full(device) // Clean every pasid

As working functions then the ops should be writable to know exactly
which of those call based on the op.

For instance, if you have a v1 page table then you should have a v1
specific flush op that does:

   invalidate_iotlb_range/all_v1(domain)
   if (domain->ats in use)
     invalidate_atc_range/all(domain)

No maze.

Jason