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

[Vasant Hegde <vasant.hegde@amd.com>]

Jason,


On 9/13/2023 8:42 PM, Jason Gunthorpe wrote:
> 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

Agree. But that is kind of not relevant to this patch. Hence I didn't make that
change. I intend to re-arrange the code within this file little bit after this
series. I will address this along with re-arranging code.


> 
>>  	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.
>
> 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)?
> 

Yeah. It works, but name is confusing.
I don't wanted to touch invalidation code in this series. But on second thought
I am thinking of getting invalidation series on top of Part2 and then rebase
this series on top of that. I will add function to something like

amd_iommu_device_flush_pasid_all(dev_data, pasid)
{
	// Flush domain
	// Flush single device ATS
}


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

PASID 0 GCR3 is shared by all devices in the domain. So it should be fine.

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

Sure.

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

In our case domain ID is per device, not per PASID. Also when we enable PASID we
will have single PASID capable device in the group (ACS requirement). So it
works fine.


> 
> 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 will impact the performance as have to do unnecessary invalidations.

-Vasant