Re: [PATCH v2 7/8] iommu/amd: Add iommu_ops->domain_alloc_paging support

[Jason Gunthorpe <jgg@ziepe.ca>]

On Wed, Sep 11, 2024 at 10:19:10AM +0000, Vasant Hegde wrote:
> Add support to allocate paging domain. Note that core will call
> domain_alloc_user() to allocate PASID capable domain. Hence its not
> checking device capability and allocates page table based on
> amd_iommu_pgtable.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 1c9e539f15a7..e6b4460c485d 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2395,6 +2395,16 @@ static struct iommu_domain *amd_iommu_domain_alloc(unsigned int type)
>  	return domain;
>  }
>  
> +/*
> + * Allocate domain with default (amd_iommu_pgtable) page table type. Core will
> + * call domain_alloc_user() interface to allocate PASID capable domain.
> + */
> +static struct iommu_domain *amd_iommu_domain_alloc_paging(struct device *dev)
> +{
> +	return do_iommu_domain_alloc(IOMMU_DOMAIN_DMA,
> +				     dev, 0, amd_iommu_pgtable);
> +}

I think you should avoid this. The _user one (aka extended) should
handle everthing. It doesn't really make sense anyhow, as the legacy
VFIO path is going to take this branch as it will have 0 flags.

Can we just get rid of amd_iommu_pgtable ? The logic to do feature
negotiation with HW support needs to be embedded inside
amd_iommu_domain_alloc_user() anyhow

And the two kernel command line options seem totally useless now that
we select the page table format correctly automatically. As you've
explained the v1 format is always better.

Maybe the kernel command line options should be revised to force ONLY
v2 format and ONLY v1 format, but that still wouldn't need this
function. But also maybe they should just be removed.

Jason