Re: [PATCH 08/14] iommu/amd: Store the nid in io_pgtable_cfg instead of the domain

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Aug 28, 2024 at 12:09:01PM +0530, Vasant Hegde wrote:
> > @@ -145,7 +145,7 @@ static bool increase_address_space(struct protection_domain *domain,
> >  	bool ret = true;
> >  	u64 *pte;
> >  
> > -	pte = iommu_alloc_page_node(domain->nid, gfp);
> > +	pte = iommu_alloc_page_node(domain->iop.pgtbl.cfg.amd.nid, gfp);
> 
> Too many level and used in multiple places. IMO its worth having helper function
> to set/get nid.

There would need to be four functions - set/get domain/pgtbl which
seems like overkill.. How about we just put some variables in:

diff --git a/drivers/iommu/amd/io_pgtable.c b/drivers/iommu/amd/io_pgtable.c
index d56be842c0b71e..f71140416fcf6c 100644
--- a/drivers/iommu/amd/io_pgtable.c
+++ b/drivers/iommu/amd/io_pgtable.c
@@ -141,11 +141,12 @@ static bool increase_address_space(struct protection_domain *domain,
 				   unsigned long address,
 				   gfp_t gfp)
 {
+	struct io_pgtable_cfg *cfg = &domain->iop.pgtbl.cfg;
 	unsigned long flags;
 	bool ret = true;
 	u64 *pte;
 
-	pte = iommu_alloc_page_node(domain->iop.pgtbl.cfg.amd.nid, gfp);
+	pte = iommu_alloc_page_node(cfg->amd.nid, gfp);
 	if (!pte)
 		return false;
 
@@ -182,6 +183,7 @@ static u64 *alloc_pte(struct protection_domain *domain,
 		      gfp_t gfp,
 		      bool *updated)
 {
+	struct io_pgtable_cfg *cfg = &domain->iop.pgtbl.cfg;
 	int level, end_lvl;
 	u64 *pte, *page;
 
@@ -233,8 +235,7 @@ static u64 *alloc_pte(struct protection_domain *domain,
 
 		if (!IOMMU_PTE_PRESENT(__pte) ||
 		    pte_level == PAGE_MODE_NONE) {
-			page = iommu_alloc_page_node(
-				domain->iop.pgtbl.cfg.amd.nid, gfp);
+			page = iommu_alloc_page_node(cfg->amd.nid, gfp);
 
 			if (!page)
 				return NULL;
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 1cdd1675a01af2..806156632bf70a 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -2030,6 +2030,7 @@ static int do_attach(struct iommu_dev_data *dev_data,
 		     struct protection_domain *domain)
 {
 	struct amd_iommu *iommu = get_amd_iommu_from_dev_data(dev_data);
+	struct io_pgtable_cfg *cfg = &domain->iop.pgtbl.cfg;
 	int ret = 0;
 
 	/* Update data structures */
@@ -2037,8 +2038,8 @@ static int do_attach(struct iommu_dev_data *dev_data,
 	list_add(&dev_data->list, &domain->dev_list);
 
 	/* Update NUMA Node ID */
-	if (domain->iop.pgtbl.cfg.amd.nid == NUMA_NO_NODE)
-		domain->iop.pgtbl.cfg.amd.nid = dev_to_node(dev_data->dev);
+	if (cfg->amd.nid == NUMA_NO_NODE)
+		cfg->amd.nid = dev_to_node(dev_data->dev);
 
 	/* Do reference counting */
 	domain->dev_iommu[iommu->index] += 1;


Thanks,
Jason