Re: [PATCH v2 06/16] iommu/amd: Introduce helper functions for managing GCR3 table

[Jason Gunthorpe <jgg@ziepe.ca>]

On Fri, Jul 28, 2023 at 05:35:59AM +0000, Vasant Hegde wrote:
> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> 
> Refactor domain_enable_v2() into helper functions for managing GCR3 table
> (i.e. setup_gcr3_table() and get_gcr3_levels()), which will be used in
> subsequent patches. Also re-arrange code and remove forward declaration.

It makes alot of sense to change the name like this
 
> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Co-developed-by: Vasant Hegde <vasant.hegde@amd.com>
> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
> ---
>  drivers/iommu/amd/iommu.c | 63 ++++++++++++++++++++++-----------------
>  1 file changed, 36 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> index 09749ad4445c..3c14c49d6d72 100644
> --- a/drivers/iommu/amd/iommu.c
> +++ b/drivers/iommu/amd/iommu.c
> @@ -77,7 +77,6 @@ struct iommu_cmd {
>  struct kmem_cache *amd_iommu_irq_cache;
>  
>  static void detach_device(struct device *dev);
> -static int domain_enable_v2(struct protection_domain *domain, int pasids);
>  
>  /****************************************************************************
>   *
> @@ -1575,6 +1574,40 @@ static void free_gcr3_table(struct protection_domain *domain)
>  	free_page((unsigned long)domain->gcr3_tbl);
>  }
>  
> +static int get_gcr3_levels(int pasids)
> +{
> +	int levels = 0;
> +
> +	if (pasids == -1)
> +		return amd_iommu_max_glx_val;
> +
> +	/* Number of GCR3 table levels required */
> +	for ( ; (pasids != 0) && ((pasids - 1) & ~0x1ff); pasids >>= 9)
> +		levels += 1;

This can surely be a closed expression, something like:

DIV_ROUND_UP(get_count_order(pasids), 9)

?

> +
> +	return 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)
> +{
> +	int levels = get_gcr3_levels(pasids);
> +
> +	if (levels > amd_iommu_max_glx_val)
> +		return -EINVAL;
> +
> +	domain->gcr3_tbl = (void *)get_zeroed_page(GFP_ATOMIC);

I gather it is recommended to just call kzalloc(PAGE_SIZE) now. And
really this shouldn't be PAGE_SIZE but some constant reflecting the
size of the HW's gcr3 levels.

Jason