Re: [PATCH v4 13/16] iommu/amd: Refactor GCR3 table helper functions

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

Jason,


On 1/6/2024 12:51 AM, Jason Gunthorpe wrote:
> On Tue, Dec 12, 2023 at 08:52:21AM +0000, Vasant Hegde wrote:
>> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>>
>> To use the new per-device struct gcr3_tbl_info. Use GFP_KERNEL flag
>> instead of GFP_ATOMIC for GCR3 table allocation. Also modify
>> set_dte_entry() to use new per device GCR3 table.
>>
>> 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 | 55 +++++++++++++++++++++++----------------
>>  1 file changed, 33 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
>> index 584608b98f26..7e5f32817931 100644
>> --- a/drivers/iommu/amd/iommu.c
>> +++ b/drivers/iommu/amd/iommu.c
>> @@ -79,6 +79,9 @@ struct kmem_cache *amd_iommu_irq_cache;
>>  
>>  static void detach_device(struct device *dev);
>>  
>> +static void set_dte_entry(struct amd_iommu *iommu,
>> +			  struct iommu_dev_data *dev_data);
>> +
>>  /****************************************************************************
>>   *
>>   * Helper functions
>> @@ -1716,16 +1719,21 @@ static void free_gcr3_tbl_level2(u64 *tbl)
>>  	}
>>  }
>>  
>> -static void free_gcr3_table(struct protection_domain *domain)
>> +static void free_gcr3_table(struct iommu_dev_data *dev_data)
>>  {
> 
> Pass in struct gcr3_tbl_info not dev_data

Fixed.

> 
>> -/* 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 iommu_dev_data *dev_data, int pasids)
>>  {
>> +	struct gcr3_tbl_info *gcr3_info = &dev_data->gcr3_info;
>>  	int levels = get_gcr3_levels(pasids);
> 
> Pass in struct gcr3_tbl_info not dev_data

Fixed.

-Vasant

> 
> I don't think it is worth re-doing at this point - but I probably
> would have tried to structure this series as creating the struct
> gcr3_tbl_info and migrating the protection_domain to use it, then
> streamlined the APIs to have a clean gcr3_tbl_info layer, finally
> adding a gcr3_tbl_info to the iommu_dev_data and using it when
> appropriate.
> 
> Jason