Re: [PATCH v2 06/10] iommu/amd: Refactor helper function for setting / clearing GCR3

[Jason Gunthorpe <jgg@ziepe.ca>]

On Wed, Aug 16, 2023 at 05:40:27PM +0000, Vasant Hegde wrote:
> @@ -2651,61 +2656,71 @@ static u64 *__get_gcr3_pte(u64 *root, int level, u32 pasid, bool alloc)
>  	return pte;
>  }
>  
> -static int __set_gcr3(struct protection_domain *domain, u32 pasid,
> -		      unsigned long cr3)
> +static int __set_gcr3(struct iommu_dev_data *dev_data,
> +		      u32 pasid, unsigned long gcr3)
>  {
> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
>  	u64 *pte;
>  
> -	if (domain->iop.mode != PAGE_MODE_NONE)
> -		return -EINVAL;
> +	lockdep_assert_held(&dev_data->lock);
>  
> -	pte = __get_gcr3_pte(domain->gcr3_tbl, domain->glx, pasid, true);
> +	pte = __get_gcr3_pte(gcr3_info->gcr3_tbl,
> +			     gcr3_info->glx, pasid, true);

It would be a nice touch to change these kinds of functions to take in
the struct gcr3_tbl_info instead of two parameters

>  	if (pte == NULL)
>  		return -ENOMEM;
>  
> -	*pte = (cr3 & PAGE_MASK) | GCR3_VALID;
> +	*pte = (gcr3 & PAGE_MASK) | GCR3_VALID;
> +	__amd_iommu_flush_tlb(dev_data->domain, pasid);

This flushing doesn't seem to make sense to me, it shouldn't take in a
domain parameter.

At this point we changed the gcr3 table for a single iommu_dev_data
and so this change has exactly one iommu, device, and cache tag that
needs flushing.

I'm reading the spec here so I may get this wrong but.. It looks like
the IOTLB cache is tagged by (DTE.domain_id, PASID)?

So in v1 mode PASID is always zero and DTE.domain_id should refer to
a per-domain ID.

In v2 mode PASID can vary and many DTEs in the system can have
aliasing PASIDs. ie DTE A and B may have different translations for
PASID 0.

Understand that the core iommu code does not provide a guarentee that
PASID is non-aliasing. It is perfectly allowed that the same PASID can
point to different translations on different devices.

So this looks broken to me. The RID domain should not provide
DTE.domain_id in v2 mode. DTE.domain_id needs to reflect a global set
of non-aliasing PASIDs.

The naive way to make this work is to have DTE.domain_id be per-GCR3
table (eg per-device) when in v2 mode. This will obviously make the
cache tag work correctly.

This HW has a similar, though IMHO worse, cache tag misdesign as
Intel's does. The choice of cache tags makes very little sense for the
SW model we have. ARM got this correct where (in AMD language) there
is a per-GCR3 entry cache tag (ASID) and a per DTE entry cache tag
(VMID) for the v1 host translation. The IOTLB never uses PASID as a
tag.

Jason