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

[Vasant Hegde <vasant.hegde@amd.com>]

Jason,


On 8/28/2024 11:43 PM, Jason Gunthorpe wrote:
> 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:

This is better. Thanks!


-Vasant

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