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

[Baolu Lu <baolu.lu@linux.intel.com>]

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.

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

>   	struct iommu_domain *(*domain_alloc_sva)(struct device *dev,
>   						 struct mm_struct *mm);
>   

Thanks,
baolu