Re: [PATCH 2/5] iommu/amd: Separate page table setup from domain allocation

[Jason Gunthorpe <jgg@ziepe.ca>]

On Wed, Aug 21, 2024 at 01:35:51PM +0000, Vasant Hegde wrote:
> Currently protection_domain_alloc() allocates domain and also sets up
> page table. Page table setup is required for PAGING domain only. Domain
> type like SVA doesn't need page table. Hence move page table setup code
> to separate function.
> 
> Also SVA domain allocation path does not call pdomain_setup_pgtable().
> Hence removed IOMMU_DOMAIN_SVA type check.
> 
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 46 ++++++++++++++++++++++++---------------
>  1 file changed, 28 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index b19e8c0f48fa..33930e08bd4c 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -2302,29 +2302,38 @@ static int protection_domain_init_v2(struct protection_domain *pdom)
>  
>  struct protection_domain *protection_domain_alloc(unsigned int type)
>  {
> -	struct io_pgtable_ops *pgtbl_ops;
>  	struct protection_domain *domain;
> -	int pgtable;
> -	int ret;
>  
>  	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
>  	if (!domain)
>  		return NULL;
>  
>  	domain->id = domain_id_alloc();
> -	if (!domain->id)
> -		goto out_err;
> +	if (!domain->id) {
> +		kfree(domain);
> +		return NULL;
> +	}
>  
>  	spin_lock_init(&domain->lock);
>  	INIT_LIST_HEAD(&domain->dev_list);
>  	INIT_LIST_HEAD(&domain->dev_data_list);
>  	domain->nid = NUMA_NO_NODE;
>  
> +	domain->domain.type = type;

In the end drivers won't be touching type at all because none of the
ops provide type. When the static identity domain is added this should
be removed.

It looks not too bad to do a basic static identity domain, it would be
good to put it as patch 2 and then remove type here and in later
patches.

> @@ -2402,12 +2407,17 @@ static struct iommu_domain *do_iommu_domain_alloc(unsigned int type,
>  	if (!domain)
>  		return ERR_PTR(-ENOMEM);
>  
> +	ret = pdom_setup_pgtable(domain, type);
> +	if (ret) {
> +		protection_domain_free(domain);
> +		return ERR_PTR(ret);
> +	}
> +
>  	domain->domain.geometry.aperture_start = 0;
>  	domain->domain.geometry.aperture_end   = dma_max_address();
>  	domain->domain.geometry.force_aperture = true;
>  
>  	if (iommu) {
> -		domain->domain.type = type;
>  		domain->domain.pgsize_bitmap = iommu->iommu.ops->pgsize_bitmap;
>  		domain->domain.ops = iommu->iommu.ops->default_domain_ops;

This existing code is wrong anyhow, type and ops are set by the core,
and pgsize_bitmap is being taken from the wrong value.. I am about to
send a series fixing some of that.

Jason