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

[Jason Gunthorpe <jgg@ziepe.ca>]

On Fri, Jan 12, 2024 at 06:15:07PM +0530, Vasant Hegde wrote:

> Right. It makes sense when using V2 page table. But when we are in V1 page table
> we don't have GCR3. We will revisit once we finalize some of the vIOMMU stuff.

But in v1 mode the domain_id comes from the iommu_domain struct, there
is no case where it logically is part of the device.

> > 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.
> 
> What kind of page table will be attached with non zero PASID? like KVM page table?

Probably at copy of the KVM page table in some for, certainly today
starting with an UNAMANGED domain as is normal. I think people will
want to do different things here..

> >>   - 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.
> 
> IIUC with UNMANAGED domain with PASID, we will have device/pasid list with
> different PASIDs pointing to different page tables.
>   ex: PASID 0 with DMA-API mode , PASID1 pointing to some other page table.

The *device* has a list of PASID's that point to iommu_domains. This
is stored in an xarray inside the iommu_group.

The *iommu_domain* has a list of *devices & PASIDs* that can use this
domain for translation (ie that it was attached to)

The RID attach is just PASID 0.

> This is where the confusion is. Current iommu ops doesn't take pasid as
> parameter. So if we go over entire dev/pasid list and flush it becomes overkill
> right?

If I have a v2 paging iommu domain (unmanaged) and I change the IOPTE
to effect a certain IOVA range then I have to invalidate it at every
place that is caching it.

For ATC that means I need to issue an invalidation to every
(iommu, RID, PASID) combination that has it in cache.

For V2 AMD IOTLB I need to issue an invalidation for every
(iommu, device->gcr3->domain_id, PASID) that has it in the cache.

For V1 AMD IOTLB I need to invalidate every
(iommu, domain->domain_id) combination.

It is a data structure problem to store a minimal list of those
things off of the iommu_domain.

No new op parameters are needed.

ATC is a list of every attached struct device and PASID, populated by
ops->attach_dev (PASID=0) or ops->set_dev_pasid.

V2 IOTLB is that same list with duplicate device->gcr3->domain_id's
removed

IOMMU is that same list with duplicate device->amd_iommu's removed,
which is the list V1 IOTLB needs.

So, store one linked list for ATC in the iommu_domain.

Sort it by (device->amd_iommu, device->gcr3->domain_id, device->id)

Skip consecutive runs of same domain_id or same iommu when generating
the invalidation sequences.

Use RCU to lock the list so the invalidation fast path is
lockless. (this is tricky)

Jason