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

[Robin Murphy <robin.murphy@arm.com>]

On 28/06/2024 1:23 pm, Baolu Lu wrote:
> On 2024/6/28 14:43, Vasant Hegde wrote:
>> 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")
>>
>>
>>
>> PROBLEM :
>> =========
>> With iommu_ops->domain_alloc_paging(dev) API AMD driver will chose 
>> best page
>> table based on device capabilities (V2 for PASID capable device and V1 
>> page
>> table for rest of the devices).
>>
>> But we would like to continue enforcing V1 page table for UNMANAGED 
>> domain. As
>> in terms of IOMMU caching, it performs better than V2 page table.
>>
>> Also while adding SVA in AMD driver Jason mentioned that we should 
>> support PASID
>> with UNMANAGED domain. As I understand currently we don't have this 
>> feature in
>> upstream but we would like to support it in future. Keeping this use 
>> case also
>> in mind, we came up with below two options :
>>
>>    1 - Pass domain type : domain_alloc_paging(dev, type)
>>     When we add PASID with UNMANAGED domain we need a way to 
>> differentiate the
>> domain 'type' (See below attach sample code)
> 
> The domain type is not enough for the iommu driver to differentiate
> between device or PASID. For example, a domain of type UNMANAGED could
> be attached to either a device or a PASID. Furthermore, SVA domain is
> not a paging domain.
> 
>>    2 - Introduce new flag for domain_alloc_paging()
>>      Something like :
>>      #define DOMAIN_FLAG_UNMANAGED    0x01
>>      #define DOMAIN_FLAG_UNMANAGED_SVA    0x02
>>
>>      domain_alloc_paging(dev, flag)
>>      For now we will have 'DOMAIN_PAGING_UNMANAGED'.
> 
> Add an allocation flag seems to work. It doesn't indicate any type of
> domain. Instead, it indicates the required iommu feature.
> 
> Something like
> 
> #define DOMAIN_ALLOC_FLAG_PASID        0x1
> 
> means the allocated domain requires PASID to support on both device and
> IOMMU. If the hardware lacks this support, it should return failure.

Yeah, we could do with something sufficiently general, as we'll also 
have a need for passing pagetable-format options through to drivers to 
be able to replace iommu_set_pgtable_quirks().

Thanks,
Robin.