Re: [PATCH v4 07/16] iommu/amd: Introduce per-device domain ID to workaround potential TLB aliasing issue

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

Joson,

[+ Baolu - adding as I have some question around PASID support for UNMANAGED domain]

On 1/6/2024 12:25 AM, Jason Gunthorpe wrote:
> On Tue, Dec 12, 2023 at 08:52:15AM +0000, Vasant Hegde wrote:
>> With v1 page table, the AMD IOMMU spec states that the hardware must use
>> the domain ID to tag its internal translation caches. I/O devices with
>> different v1 page tables must be given different domain IDs. I/O devices
>> that share the same v1 page table __may__ be given the same domain ID.
>> This domain ID management policy is currently implemented by the AMD
>> IOMMU driver. In this case, only the domain ID is needed when issuing the
>> INVALIDATE_IOMMU_PAGES command to invalidate the IOMMU translation cache
>> (TLB).
>>
>> With v2 page table, the hardware uses domain ID and PASID as parameters
>> to tag and issue the INVALIDATE_IOMMU_PAGES command. Since the GCR3 table
>> is setup per-device, and there is no guarantee for PASID to be unique
>> across multiple devices. The same PASID for different devices could
>> have different v2 page tables. In such case, if multiple devices share the
>> same domain ID, IOMMU translation cache for these devices would be polluted
>> due to TLB aliasing.
>>
>> Hence, avoid the TLB aliasing issue with v2 page table by allocating unique
>> domain ID for each device even when multiple devices are sharing the same v1
>> page table. Please note that this workaround would result in multiple
>> INVALIDATE_IOMMU_PAGES commands (one per domain id) when unmapping a
>> translation on the shared v1 page table.
> 
> Huh? This is a typo? "the same v2 page table"  "unmapping a
> translation on the shared v2 page table" ??

Its typo. Will fix it.

> 
> The code seems to be fine, domain_flush_pages_v1() looks optimal?
> 

I'd say its optimal for given state. I have a patch to move dev_iommu[] to
xarray. I am planning to fine tune and post those patches after SVA. With that
changes it will be better.


>> diff --git a/drivers/iommu/amd/amd_iommu_types.h b/drivers/iommu/amd/amd_iommu_types.h
>> index fead9033796f..51daf5dd4729 100644
>> --- a/drivers/iommu/amd/amd_iommu_types.h
>> +++ b/drivers/iommu/amd/amd_iommu_types.h
>> @@ -842,6 +842,8 @@ struct iommu_dev_data {
>>  	u8 ppr          :1;		  /* Enable device PPR support */
>>  	bool use_vapic;			  /* Enable device to use vapic mode */
>>  	bool defer_attach;
>> +	/* Per device domain ID. Used with V2 page table */
>> +	u16 domid;
> 
> This should really be put into the 'struct gcr3_tbl_info' - logically
> that is the struct the HW cache tag is linked to. ie if the gcr3 table
> is the same pointer then the cache tag can be re-used by the HW.
> 

The reason we put it in dev_data is because its per device ID, not specific to
GCR3 table.

> Then when you want to optimize for the no-pasid case then the right
> way to do it is putting a 'struct gcr3_tbl_info' inside the v2
> protection_domain.
> 
> The DTE will point at the v2 protection_domain's version of the gcr3
> if the PASID table is empty, otherwise the DTE will point at the
> struct iommu_dev_data version of the gcr3 table.

This makese sense if we are sure we will do per-device-domain-ID only with V2
page table. I still need to see how SVA support with vIOMMU works. For now I
will keep this in my list. Once I have better picture I will fine tune.

> 
> Naturally this will optimize the lifecycle of the domain_id.
> 
> Then put the domain_id alloc/dealloc inside the functions that
> alloc/free the memory under the struct gcr3_tbl_info - ie it is part
> of the gcr3 layer.

Domain ID is part of domain/device not specific to gcr3 layer. In V1 we don't
have GCR3 stuff.

> 
>> +/*
>> + * Allocate per device domain ID when using V2 page table
>> + */
>> +static inline bool domain_id_is_per_dev(struct protection_domain *pdom)
>> +{
>> +	return (pdom && pdom->pd_mode != PD_MODE_V1);
>> +}
> 
> Under that view this function probably would make more sense as:
> 
> domain_requires_gcr3(pdom)
> 
> But frankly I'd just stick with pdom_is_v2_pgtbl_mode().
> 
> Also pdom should never be null when this is called, right?

Right. Will fix it.

> 
>> -/*
>> - * TLB invalidation function which is called from the mapping functions.
>> - * It invalidates a single PTE if the range to flush is within a single
>> - * page. Otherwise it flushes the whole TLB of the IOMMU.
>> - */
>> -static void __domain_flush_pages(struct protection_domain *domain,
>> +static int domain_flush_pages_v2(struct protection_domain *pdom,
>>  				 u64 address, size_t size)
>>  {
>>  	struct iommu_dev_data *dev_data;
>>  	struct iommu_cmd cmd;
>> -	int ret = 0, i;
>> -	ioasid_t pasid = IOMMU_NO_PASID;
>> -	bool gn = false;
>> +	int ret = 0;
>>  
>> -	if (pdom_is_v2_pgtbl_mode(domain))
>> -		gn = true;
>> +	list_for_each_entry(dev_data, &pdom->dev_list, list) {
>> +		struct amd_iommu *iommu = get_amd_iommu_from_dev(dev_data->dev);
>> +
>> +		build_inv_iommu_pages(&cmd, address, size,
>> +				      dev_data->domid, IOMMU_NO_PASID, true);
>> +
>> +		ret |= iommu_queue_command(iommu, &cmd);
>> +	}
> 
> This is fine for where things are here, but what you want to get to
> long term is what I've been talking about of having the attachment
> list on the protection_domain.
> 
> Each attachment entry would hold:
>   struct device *dev;
>   unsigned int pasid;
> 
> Keep the list sorted by (iommu, gcr.domain_id).
> 
> Then optimized invalidation is simply this:
> 
> for_each:
>   if (get_amd_iommu_from_dev(entry->dev) == last_iommu &&
>       get_gcr(entry, pdom)->domain_id == last_domain_id)
>      continue;
>   build_inv_iommu_pages(..)
>   last_iommu = get_amd_iommu_from_dev(entry->dev)
>   domain_id = get_gcr(entry, pdom)->domain_id;


I have done a prototype something in this line. Basically we will eventually
have single API for invalidation :
  domain_flush_pages(pdom, addr, size)

.. and internally it will decide to flush host/guest page table.


[Slightly unrelated topic]

UNAMANGED Domain and PASID support:
  - I was considering this scenario as well. I don't think I understood the use
case of and how invalidation is suppose to work here.

   Can you explain (again?) the use case and how invalidation is suppose to work?

  - For PASID capable device we will have per-device-domain-ID
  - We will have default page table setup (PASID zero in our case) during domain
initialization.
  - We attach PASIDs to same domain. We can add this to list (protection domain
device List info - which will have dev_data/PASID). So set/remove PASIDs is fine.

  - iommu_ops->iotlb_sync_map/flush_iotlb_all will flush PASID zero.
    Looking into intel driver they seems to be invalidating all PASIDs in this
path. I didn't get why it has to flush all PASIDs here.

  - For other PASIDs do we have mmu notifier to invalidate as its attached to
some process?


> 
> And this algorithm matches what Intel and SMMU need and I'm strongly
> thinking about making a driver utility library to handle it, so
> we can revisit this later on.

If things are common across drivers then it makes sense. We can consider common
set of functions.


> 
>> +static int domain_flush_pages_v1(struct protection_domain *pdom,
>> +				 u64 address, size_t size)
>> +{
>> +	struct iommu_cmd cmd;
>> +	int ret = 0, i;
>>  
>> -	build_inv_iommu_pages(&cmd, address, size, domain->id, pasid, gn);
>> +	build_inv_iommu_pages(&cmd, address, size,
>> +			      pdom->id, IOMMU_NO_PASID, false);
>>  
>>  	for (i = 0; i < amd_iommu_get_num_iommus(); ++i) {
>> -		if (!domain->dev_iommu[i])
>> +		if (!pdom->dev_iommu[i])
>>  			continue;
> 
> Right, iterate over every iommu

As explained above I believe this will be better once I move this to xarray.

-Vasant