Re: [PATCH v5 14/17] iommu/amd: Refactor GCR3 table helper functions

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Jan 16, 2024 at 04:53:32PM +0000, Vasant Hegde wrote:

> @@ -1738,22 +1744,22 @@ static int get_gcr3_levels(int pasids)
>  	return levels ? (DIV_ROUND_UP(levels, 9) - 1) : levels;
>  }
>  
> -/* Note: This function expects iommu_domain->lock to be held prior calling the function. */
> -static int setup_gcr3_table(struct protection_domain *domain, int pasids)
> +static int setup_gcr3_table(struct gcr3_tbl_info *gcr3_info,
> +			    int nid, int pasids)
>  {
>  	int levels = get_gcr3_levels(pasids);
>  
>  	if (levels > amd_iommu_max_glx_val)
>  		return -EINVAL;
>  
> -	domain->gcr3_tbl = alloc_pgtable_page(domain->nid, GFP_ATOMIC);
> -	if (domain->gcr3_tbl == NULL)
> -		return -ENOMEM;
> +	if (gcr3_info->gcr3_tbl)
> +		return -EBUSY;
>  
> -	domain->glx      = levels;
> -	domain->flags   |= PD_IOMMUV2_MASK;
> +	gcr3_info->gcr3_tbl = alloc_pgtable_page(nid, GFP_KERNEL);
> +	if (gcr3_info->gcr3_tbl == NULL)
> +		return -ENOMEM;
>  
> -	amd_iommu_domain_update(domain);
> +	gcr3_info->glx = levels;

I think this patch should also move the domain_id
allocation/deallocation into setup_gcr3_table()/free_gcr3_table()

But it would really be better to move patch 7 to after the gcr3 layer
is cleaned and seperated from protection_domain so you don't have to
move it in the first place..

It is missing some error handling too

And domain_id_is_per_dev() is pretty redundant once you do that. See below

> @@ -1974,14 +1982,15 @@ static int do_attach(struct iommu_dev_data *dev_data,
>  	/* Init GCR3 table and update device table */
>  	if (domain->pd_mode == PD_MODE_V2) {
>  		/* By default, setup GCR3 table to support single PASID */
> -		ret = setup_gcr3_table(dev_data->domain, 1);
> +		ret = setup_gcr3_table(&dev_data->gcr3_info,
> +				       dev_to_node(dev_data->dev), 1);

IMO it is better to pass the struct amd_iommu * to setup_gcr3_table()
and then it can get the NUMA locality more sensibly via

 dev_to_node(&iommu->dev->dev)

The iommu is the thing that will walk the table and needs the
locality, not the end point device. Locality should flow from
the affiliated amd_iommu always.

Jason

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 256b5507272ff9..fb94ffa269717b 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1769,6 +1769,8 @@ static void free_gcr3_table(struct gcr3_tbl_info *gcr3_info)
 
 	free_page((unsigned long)gcr3_info->gcr3_tbl);
 	gcr3_info->gcr3_tbl = NULL;
+
+	domain_id_free(gcr3_info.domid);
 }
 
 /*
@@ -1788,7 +1790,7 @@ static int get_gcr3_levels(int pasids)
 }
 
 static int setup_gcr3_table(struct gcr3_tbl_info *gcr3_info,
-			    int nid, int pasids)
+			    struct amd_iommu *iommu, int pasids)
 {
 	int levels = get_gcr3_levels(pasids);
 
@@ -1798,12 +1800,19 @@ static int setup_gcr3_table(struct gcr3_tbl_info *gcr3_info,
 	if (gcr3_info->gcr3_tbl)
 		return -EBUSY;
 
-	gcr3_info->gcr3_tbl = alloc_pgtable_page(nid, GFP_KERNEL);
+	gcr3_info->gcr3_tbl = alloc_pgtable_page(
+		iommu ? dev_to_node(&iommu->dev->dev) : 1, GFP_KERNEL);
 	if (gcr3_info->gcr3_tbl == NULL)
 		return -ENOMEM;
 
 	gcr3_info->glx = levels;
 
+	gcr3_info.domid = domain_id_alloc();
+	if (!gcr3_info.domid) {
+		free_page((unsigned long)gcr3_info->gcr3_tbl);
+		return -ENOMEM;
+	}
+
 	return 0;
 }
 
@@ -1902,7 +1911,7 @@ static void set_dte_entry(struct amd_iommu *iommu,
 	struct dev_table_entry *dev_table = get_dev_table(iommu);
 	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
 
-	if (domain_id_is_per_dev(domain))
+	if (gcr3_info && gcr3_info->gcr3_tbl) {
 		domid = dev_data->gcr3_info.domid;
 	else
 		domid = domain->id;
@@ -2018,15 +2027,10 @@ static int do_attach(struct iommu_dev_data *dev_data,
 	domain->dev_iommu[iommu->index] += 1;
 	domain->dev_cnt                 += 1;
 
-	/* Allocate per device domain ID */
-	if (domain_id_is_per_dev(domain))
-		dev_data->gcr3_info.domid = domain_id_alloc();
-
 	/* Init GCR3 table and update device table */
 	if (domain->pd_mode == PD_MODE_V2) {
 		/* By default, setup GCR3 table to support single PASID */
-		ret = setup_gcr3_table(&dev_data->gcr3_info,
-				       dev_to_node(dev_data->dev), 1);
+		ret = setup_gcr3_table(&dev_data->gcr3_info, iommu, 1);
 		if (ret)
 			return ret;
 
@@ -2073,10 +2077,6 @@ static void do_detach(struct iommu_dev_data *dev_data)
 	/* decrease reference counters - needs to happen after the flushes */
 	domain->dev_iommu[iommu->index] -= 1;
 	domain->dev_cnt                 -= 1;
-
-	/* Free per device domain ID */
-	if (domain_id_is_per_dev(domain))
-		domain_id_free(dev_data->gcr3_info.domid);
 }
 
 /*