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

[Jason Gunthorpe <jgg@ziepe.ca>]

On Fri, Aug 04, 2023 at 10:21:41PM +0530, Vasant Hegde wrote:
> Hi Jason,
> 
> 
> On 8/4/2023 6:47 PM, Jason Gunthorpe wrote:
> > On Fri, Aug 04, 2023 at 06:42:06AM +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.
> >>
> >> 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 | 66 +++++++++++++++++++++++----------------
> >>  1 file changed, 39 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
> >> index d5569eec0fe9..135fa087ce47 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,43 @@ 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. Level must be 4-Kbyte
> >> +	 * page and can contain upto 512 entries.
> >> +	 */
> >> +	for ( ; get_count_order(pasids) >= 9; pasids >>= 9)
> >> +		levels += 1;
> >> +
> >> +	return levels;
> > 
> > You missed to use the DIV_ROUND_UP
> 
> Sorry. I wanted to add comment to few patches separately. Somehow it got missed.
> 
> In our case,
>   00b=GCR3 table is single-level - PASID size 9 bits
>   01b=GCR3 table is two-level - PASID size 18 bits
>   10b=GCR3 table is three-level - - PASID size 20 bits.
> 
> Hence I didn't include DIV_ROUND_UP().

You literally coded a for loop to open code division :(

   levels = DIV_ROUND_UP(get_counter_order(pasids), 9) - 1;

Jason