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

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

Jason,


On 1/20/2024 1:29 AM, Jason Gunthorpe wrote:
> 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()

We do domain allocation in device attach path only. But GCR3 table
allocation/setup can happen while enabling SVA (like passthrough -> SVA path).
Hence I have kept it in attach() path itself.


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

Fine I can do that.

> 
> It is missing some error handling too

Where?

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

We need to handle passthrough as well. It doesn't make sense to allocate and
keep GCR3 table when we are not going to use it.

-Vasant


> 
>> @@ -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);
>  }
>  
>  /*