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

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

Joson,


On 6/28/2024 11:34 PM, Jason Gunthorpe wrote:
> 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.

Right. If we solve the domain allocation issue, then we can fix our attach
device path to solve this (Yes. It needs code change. But its doable).

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

Thanks.

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

All we need a explicit indication in domain_alloc_paging() saying allocate
*PASID* capable domain.

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

Thanks.

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

Let me see if I can put together here.


- Basically we want domain_alloc_paging() to explicitly tell domain requirement
(ex: allocate PASID capable UNMANAGED domain)
  If we do that then we will fix ordering issue I described earlier.
- Fix attach device path so that for 'PASID capable UNMANAGED domain' :
  non-PASID capable device will use AMD V2 page table with GCR3[0]
  PASID capable device will setup its GCR3 table as well and it should work fine.

Net we need a enhancement to domain_alloc_paging(). Otherwise I don't see a way
to reliable implement things.

Regarding performance impact: Thanks for pointing out. I will start discussion
in that thread. Lets see if we can come up with something.

But these two can go independently?

-Vasant

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