Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

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

Hi Baolu,


On 8/2/2024 6:14 AM, Baolu Lu wrote:
> On 2024/8/1 22:45, Vasant Hegde wrote:
>> Currently domain_alloc_paging() passes device as param for domain
>> allocation. While this is sufficient for some HW vendor, its not
>> sufficent for others.
>>
>> AMD IOMMU has two different page tables (v1 and v2). For DMA API mode it
>> wants to allocate page table based on device capability. V2 for PASID
>> capable device and v1 for rest of the devices. For UNMANAGED domain, it
>> wants to continue to enforce v1 page table as its cache efficient. Hence
>> include 'domain type' as parameter to domain_alloc_paging().
>>
>> While at it also add 'flag' as additional parameter. So that any page
>> table specific quirks (like IO_PGTABLE_QUIRK_*) can be passed to vendor
>> driver. Once we have this we can remove ops->set_pgtable_quirks()
>> interface.
>>
>> Note:
>>    Intent of this patch is to discuss/finalize the domain_alloc_paging()
>>    ops. Once we agree on interfaces I will fix other drivers and send proper
>>    patch series. That means with vendor driver config this doesn't
>>    compile.
>>
>> @Robin,
>>    Once we have this patch and Baolu's series [1], we can enhance
>>    iommu_paging_domain_alloc() to include page table quirks and then we can
>>    remove ops->set_pgtable_quirks(). I hope this works for ARM driver
>>    (arm/arm-smmu/arm-smmu.c).
>>
>> RFC v1 :
>> https://lore.kernel.org/linux-iommu/7e249bc6-c578-40f0-aca7-835149a0ad39@amd.com/
>>
>> Thanks everyone for looking into RFC patch and giving valuable suggestions.
>>
>> [1]
>> https://lore.kernel.org/linux-iommu/20240610085555.88197-2-baolu.lu@linux.intel.com/
> 
> That patch has been merged for v6.11-rc1.

Thanks!

> 
>>
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>>   drivers/iommu/amd/iommu.c | 26 ++++++++++++++++++++++++++
>>   drivers/iommu/iommu.c     |  2 +-
>>   include/linux/iommu.h     |  3 ++-
>>   3 files changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index b19e8c0f48fa..240cca8bed21 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -2429,6 +2429,31 @@ static struct iommu_domain
>> *amd_iommu_domain_alloc(unsigned int type)
>>       return domain;
>>   }
>>   +static struct iommu_domain *amd_iommu_domain_alloc_paging(struct device
>> *dev, u32 type, u32 flags)
>> +{
>> +    struct iommu_dev_data *dev_data;
>> +    int pgtable = amd_iommu_pgtable;
>> +
>> +    if (dev)
>> +        dev_data = dev_iommu_priv_get(dev);
>> +
>> +    /*
>> +     * - Force V1 page table for UNMANAGED domain.
>> +     * - Use V2 page table for PASID capable device except when :
>> +     *   - SNP is enabled, because it prohibits DTE[Mode]=0
>> +     *   - amd_iommu=pgtbl_v[1/2] kernel command line is passed
>> +     */
>> +    if (type == IOMMU_DOMAIN_UNMANAGED) {
>> +        pgtable = AMD_IOMMU_V1;
>> +    } else if (dev && dev_is_pci(dev) && pdev_pasid_supported(dev_data) &&
>> +           !amd_iommu_force_isolation && !amd_iommu_snp_en) {
>> +        pgtable = AMD_IOMMU_V2;
>> +    }
>> +
>> +    /* TODO: Pass pgtable as param */
>> +    return do_iommu_domain_alloc(IOMMU_DOMAIN_DMA, dev, 0);
>> +}
>> +
>>   static struct iommu_domain *
>>   amd_iommu_domain_alloc_user(struct device *dev, u32 flags,
>>                   struct iommu_domain *parent,
>> @@ -2860,6 +2885,7 @@ static int amd_iommu_dev_disable_feature(struct device
>> *dev,
>>   const struct iommu_ops amd_iommu_ops = {
>>       .capable = amd_iommu_capable,
>>       .domain_alloc = amd_iommu_domain_alloc,
>> +    .domain_alloc_paging = amd_iommu_domain_alloc_paging,
>>       .domain_alloc_user = amd_iommu_domain_alloc_user,
>>       .domain_alloc_sva = amd_iommu_domain_alloc_sva,
>>       .probe_device = amd_iommu_probe_device,
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index ed6c5cb60c5a..d8a67b39a4cb 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -1946,7 +1946,7 @@ static struct iommu_domain *__iommu_domain_alloc(const
>> struct iommu_ops *ops,
>>       else if (alloc_type == IOMMU_DOMAIN_BLOCKED && ops->blocked_domain)
>>           return ops->blocked_domain;
>>       else if (type & __IOMMU_DOMAIN_PAGING && ops->domain_alloc_paging)
>> -        domain = ops->domain_alloc_paging(dev);
>> +        domain = ops->domain_alloc_paging(dev, type, 0);
>>       else if (ops->domain_alloc)
>>           domain = ops->domain_alloc(alloc_type);
>>       else
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 4d47f2c33311..72383f6bdd9f 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -565,7 +565,8 @@ struct iommu_ops {
>>       struct iommu_domain *(*domain_alloc_user)(
>>           struct device *dev, u32 flags, struct iommu_domain *parent,
>>           const struct iommu_user_data *user_data);
>> -    struct iommu_domain *(*domain_alloc_paging)(struct device *dev);
>> +    struct iommu_domain *(*domain_alloc_paging)(struct device *dev,
>> +                            u32 iommu_domain_type, u32 flags);
> 
> I still can't see a value to pass the domain type in this callback.
> Different domain could have different domain allocation callback, hence
> the domain type has already been implied.
> 
> For the paging domain, there should be no difference between DMA and
> UNMNANAGED from iommu driver's point of view.

That's true. Its all paging domain. But we need a way to indicate the desired
capability like PASID.

I thoughts we can use `type` for allocating domain and then `flag` to pass the
quirks. Otherwise we have to club everything in `flags` itself.

Something like below works ?

  - DMA-API domain : flag - DOMAIN_ALLOC_FLAG_PASID
    If both device and IOMMU supports PASID it will allocate PASID capable
    domain (Like AMD case domain with V2 page table). Else it will alloate
    non-pasid capable domain (In AMD case it will be domain with v1 page table).

  - UNMANAGED domain : Do not pass *_PASID support flag
    Since PASID flag is *not* passed, driver will decide best suitable page
    table (in AMD case, we will allocate V1 page table)


-Vasant