Re: [PATCH v3 02/12] iommu: Introduce iommu_paging_domain_alloc_flags()

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

Yi,


On 10/21/2024 1:17 PM, Yi Liu wrote:
> On 2024/10/17 22:01, Vasant Hegde wrote:
>> From: Jason Gunthorpe <jgg@ziepe.ca>
>>
>> Currently drivers calls iommu_paging_domain_alloc(dev) to get an
>> UNMANAGED domain. This is not sufficient to support PASID with
>> UNMANAGED domain as some HW like AMD requires certain page table type
>> to support PASIDs.
>>
>> Also domain_alloc_paging() passes device as param for domain
> 
> The below may look clearer to reader. :)
> "Also the domain_alloc_paging op only passes device as param for domain"

Better. Fixed.

> 
>> allocation. This is not sufficient for AMD driver to decide the right
>> page table.
>> > Hence add iommu_paging_domain_alloc_flags() API which takes flags as
>> parameter. Driver can pass additional parameter to indicate type of
>> domain required, etc. iommu_paging_domain_alloc_flags() internally calls
>> appropriate callback function to allocate a domain.
> 
> how about mentioning that the domain_alloc_user op accepts flags from
> user which meets the AMD driver's requirement? Hence, it's clear why
> we are not extending the domain_alloc_paging op to accept flags. :)

I have added below sentence instead.

Instead of extending ops->domain_alloc_paging() it was decided to
enhance ops->domain_alloc_user() so that caller can pass various
additional flags.

> 
>>
>> Signed-off-by: Jason Gunthorpe <jgg@ziepe.ca>
>> [Added description - Vasant]
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>> ---
>>   drivers/iommu/iommu.c | 30 ++++++++++++++++++++++++++----
>>   include/linux/iommu.h | 14 +++++++++++---
>>   2 files changed, 37 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 811748bec4b0..e13d64ffd14f 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -2027,20 +2027,42 @@ struct iommu_domain *iommu_domain_alloc(const struct
>> bus_type *bus)
>>   EXPORT_SYMBOL_GPL(iommu_domain_alloc);
>>     /**
>> - * iommu_paging_domain_alloc() - Allocate a paging domain
>> + * iommu_paging_domain_alloc_flags() - Allocate a paging domain
>>    * @dev: device for which the domain is allocated
>> + * @flags: Bitmap of iommufd_hwpt_alloc_flags
> 
> maybe "enum iommufd_hwpt_alloc_flags" would be more accurate.

Ack.

> 
>>    *
>>    * Allocate a paging domain which will be managed by a kernel driver. Return
>>    * allocated domain if successful, or a ERR pointer for failure.
> 
> nit: s/a/an/

Fixed.

-Vasant