Re: [RFC] iommu_ops->domain_alloc_paging() enhancement to support AMD IOMMU driver

[Jason Gunthorpe <jgg@ziepe.ca>]

On Fri, Jun 28, 2024 at 11:19:23PM +0530, Vasant Hegde wrote:
> Hi Jason,
> 
> On 6/28/2024 6:33 PM, Jason Gunthorpe wrote:
> > On Fri, Jun 28, 2024 at 12:13:43PM +0530, Vasant Hegde wrote:
> >> Hi All,
> >>
> >> We are working on adding domain_alloc_paging() support in AMD driver and came
> >> across below issue.
> >>
> >> BACKGROUND:
> >> ============
> >> - AMD IOMMU HW has two different page tables : V1 (host page table) and V2
> >> (guest page table). Only V2 page table supports PASID and PRI features.
> >>
> >> - With V2 page table we have an aliasing issue. Hence we added
> >> per-device-domain-id when domain is configured with v2 page table. See upstream
> >> commit 87a6f1f22c97 ("iommu/amd: Introduce per-device domain ID to fix potential
> >> TLB aliasing issue")
> > 
> > Yes, but IIRC this is a shortcut to developing a proper packing
> > algorithm to optimize the IOTLB. HW like this that has aliasing issues
> > needs some more complex SW support to get optimal usage.
> > 
> > ie you can share DIDs if devices have a logically equivilant GCR3
> > table. Optimizing this is a SW problem inside the driver and should
> > not leak out to API.
> 
> Its not just SW issue. With V1 page table HW supports variable page size. (not
> just fixed 4k, 2M and 1G). So in terms of HW cache management, V1 page table is
> better.

Okay, that does make alot of sense.

> Another issue is with VFIO device passthrough and mixed device passthrough (few
> w/ PASID and few w/o PASID), type of domain we endup allocating is depends on
> the order in which VFIO requested for domain allocation. So its not deterministic.

Yes, VFIO is not good about optimizing disjoint domain types to
minimize domain requirements. This probably does need some more
work. You have the other problem too where if you attach the non-pasid
device first then pasid will be blocked and this is not expected
either.

> By the way, can you point me to your series please?

https://lore.kernel.org/linux-iommu/0-v9-5cd718286059+79186-smmuv3_newapi_p2b_jgg@nvidia.com/
 
> > 
> >> Please let us know which one is preferred -OR- is there any other better way to
> >> handle this.
> > 
> > Neither is really going to work. VFIO will have to assume the user
> > will want to use the PASID API and will always request a PASID capable
> > domain anyhow.
> 
> Even to make PASID support with VFIO, some way it should communicate the driver
> saying like "I need PASID capable domain". So that driver can allocate with
> right page table type.

Well, that is already implicit in the fact it asked for the domain from a
PASID capable device.

What we perhaps need is for VFIO to have some way to evaluate a lot of
devices together and decide on the best domain configuration for the
full set. It prpbably makes little sense to have a v1 page table and
then a copy with a v2 page table, for the PASID device.

> > We already have a path where the VFIO userspace can request a v1
> > domain by using the NESTING_PARENT flags during user domain
> > allocation.
> 
> That's with domain_alloc_user() API right? As I understand that should work fine
> for AMD driver.

Yes I expect so.
 
> > But I don't see any option here that doesn't involve userspace itself
> > making a request and indicating it wants a degrated VFIO
> > functionality.
> 
> That's along an option. We can explore it later.

But there is no later.

Your said your desire is to get a v1 domain even if the device
supports PASID, and VFIO will get PASID support likely before you post
your patches for this. Yi's work looks almost done to me.

Then VFIO will just request the V2 domain anyhow and you are right
back to the starting problem again.

Given your remarks it may make sense that VIFO disable PASID support
by default so that by default AMD does not have a performance
regression. You should start discussing this with Alex in Yi's series
to come to a decision. It makes sense to me at least.

If that is the choice then the idea of adding some flags/etc would
work well.

Jason