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

[Jason Gunthorpe <jgg@ziepe.ca>]

On Thu, Jan 11, 2024 at 04:48:01PM +0530, Vasant Hegde wrote:

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

I'm not keen on an xarray, that isn't fully addressing the problem and
when Intel did it they messed up the locking..

I would like to make a shared helper to solve this problem. A rcu
backed linked list, with a helper API to iterate once per-iommu.
The RCU is a bit tricky.

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

But from a HW perspective it is actually linked to the GCR3 table as
that is the data pointer that is being cached. Two devices that share
a GCR3 pointer can share a domain ID.

If you want to optimize the domain ID tagging the logical way to do it
is to add code to share the GCR3 tables across devices. If the GCR3
table is the same then the domain ID can be the same.

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

SVA support is ugly on AMD - you need to flow through the virtual
domain ID to a consistent physical domain ID even though the guest may
have a DTEs tagged with the same domain ID but different GCR3
pointers.

Most likely the implementation will have the physical domain ID be
part of the nesting domain to achieve this.

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

Invalidation is no different than any other V2 domain use case. The
domain ops trigger invalidation. In AMD HW you need to record that an
unmanaged domain is connected to a domain ID & PASID and push the
right invalidate. The driver can't just assume the PASID is 0 for V2
unmanaged domains.

So the implementation is to track the attacked devices, iommus and
PASIDs in a linked list and use that linked list to generate
invalidations. Intel and SMMU (after my patches) both have
implementations of this. I would like to unify them to a helper
because they are both kind of bad.

There are many use cases for PASID mappings without SVA, including
SIOV-like devices and virtualization modes with non-SVA PRI.

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

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.

Right

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

You iterate ove the list above and flush every PASID in the list. I
don't know what Intel is doing, fush all PASID on domain invalidation
sounds like overkill.
 
>   - For other PASIDs do we have mmu notifier to invalidate as its attached to
> some process?

mmu notifier has nothing to do with PASID.

The driver should have a general scheme to keep track of a
iommu_domain's cache tags. This is the per-domain linked list of
pasid/device/iommu.

When the page table changes (however that page table is stored) it
walks that linked list and pushes invalidation commands.

For an unmanaged domain this common code is called by the
iommu_domain ops unmap/flush/etc.

For a SVA domain this common code is called by the MMU notifier
arch_invalidate_range

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

I have been meaning to try to make something, lets see next week

Jason