Re: [PATCH 1/5] iommu: Enhance domain allocation code to take additional flags

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

Jason,


On 8/21/2024 10:01 PM, Jason Gunthorpe wrote:
> On Wed, Aug 21, 2024 at 01:35:50PM +0000, Vasant Hegde wrote:
>> From: Jason Gunthorpe <jgg@ziepe.ca>
>>
>> Currently drivers calls iommu_paging_domain_alloc(dev) to get an
>> UNAMANAGED domain. This is not sufficient to support PASID with
>> UNAMANAGED domain as some HW like AMD requires certain page table type
>> to support PASIDs.
>>
>> Also domain_alloc_paging() passes device as param for domain
>> 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. Also update default domain allocation path to use this new
>> API whenever device is PASID capable. HW driver should implement
>> domain_alloc_user() to allocate PASID capable domain.
>>
>> Finally introduce new flag (IOMMUFD_HWPT_ALLOC_PASID) to
>> domain_alloc_users() ops. If both IOMMU and device supports PASID it
>> will allocate domain. Otherwise return error.
>>
>> Signed-off-by: Jason Gunthorpe <jgg@ziepe.ca>
>> [Added __iommu_paging_domain_alloc_flags() and description - Vasant]
>> Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>
>> ---
>> @Jason,
>>   Notice that I have added __iommu_paging_domain_alloc_flags() so that
>>   it can call iommu_domain_init() with appropriate domain type.
> 
> It is okay, but also the type could be fixed in
> __iommu_group_alloc_default_domain() using something like:
> 
> 		ret->type |= req_type;

Sorry. I didn't get it. You mean domain->type? If yes, its set in
iommu_domain_init(). (That's why I had to introduce
__iommu_group_alloc_default_domain()).


> 
>>   I think instead of having separate function it may be better to
>>   enhance __iommu_domain_alloc() such that:
>>     - Keep below changes from this patch
>>       - iommu_domain_init()
>>       - iommu_get_dma_cookie call inside iommu_setup_default_domain()
>>     - modify __iommu_domain_alloc() to additional param (flags)
>>     - iommu_paging_domain_alloc_flags() will call __iommu_domain_alloc()
> 
> My expectation was to basically remove iommu_domain_alloc() entirely
> once Lu's work is merged.
> 
> Instead we'd have these direct APIs:
>   iommu_domain_alloc_paging_flags()
>   iommu_group_alloc_blocking_domain()
>   iommu_group_alloc_identity_domain()

Sure (For AMD driver, we may have to tweak a bit to allocate_identity domain,
but doable).


> 
> And then iommu_group_alloc_default_domain() would just call them in
> the right order maybe like this:
> 
> 	if (req_type & __IOMMU_DOMAIN_PAGING)
> 		return iommu_domain_alloc_paging_flags();
> 	if (req_type && req_type != IOMMU_DOMAIN_IDENTITY)
> 		return ERR_PTR(-EOPNOTSUPP);
> 
> 	if (req_type || iommu_def_domain_type == IOMMU_DOMAIN_IDENTITY) {
> 		dom = iommu_group_alloc_identity_domain();
> 		if (req_type || !IS_ERR(dom))
> 			return dom;
> 		/*
> 		 * if iommu_def_domain_type == IDENTITY fails then fall through
> 		 * to PAGING
> 		 */
> 	}
> 
> 	/* iommu_def_domain_type is PAGING */
> 	dom = iommu_domain_alloc_paging_flags();
> 	if (IS_ERR(dom))
> 		return dom;
> 
> 	pr_warn("Failed to allocate default IOMMU domain of type %u for group %s - Falling back to IOMMU_DOMAIN_DMA",
> 		iommu_def_domain_type, group->name);
> 	return dom;
> 
> Which is the only place we need to make a decision based on a type
> input.
> 
> Hoping Lu's final few patches make it this cycle
> 
>> +static void iommu_domain_init(struct iommu_domain *domain, unsigned int type,
>> +			      const struct iommu_ops *ops)
>> +{
>> +	domain->type = type;
>> +	domain->owner = ops;
>> +	if (!domain->ops)
>> +		domain->ops = ops->default_domain_ops;
>> +
>> +	/*
>> +	 * If not already set, assume all sizes by default; the driver
>> +	 * may override this later
>> +	 */
>> +	if (!domain->pgsize_bitmap)
>> +		domain->pgsize_bitmap = ops->pgsize_bitmap;
>> +}
> 
> Pedantically the pgsize_bitmap is only needed for paging domains, but
> it is OK like this too.

Right. Lets keep it for now. I can do a cleanup patch later.


> 
>> @@ -359,11 +359,17 @@ struct iommu_vfio_ioas {
>>   *                                   enforced on device attachment
>>   * @IOMMU_HWPT_FAULT_ID_VALID: The fault_id field of hwpt allocation data is
>>   *                             valid.
>> + * @IOMMUFD_HWPT_ALLOC_PASID: When the domain is used on a device, with no
>> + *                            PASID, the device will support later attaching
>> + *                            a PASID as well. Some HW requires a specific
>> + *                            domain format on the device to allow PASID to
>> + *                            work.
> 
> Maybe:
> 
>  Requests a domain that can be used with PASID. The domain can be
>  attached to any PASID on the device. Any domain attached to the
>  non-PASID part of the device must also be flaged, otherwise attaching
>  a PASID will blocked.

Ok.

> 
> Yi will need to add a check that IOMMUFD_HWPT_ALLOC_PASID was
> specified on the RID domain while processing attach on the PASID
> domain.
> 
>>  enum iommufd_hwpt_alloc_flags {
>>  	IOMMU_HWPT_ALLOC_NEST_PARENT = 1 << 0,
>>  	IOMMU_HWPT_ALLOC_DIRTY_TRACKING = 1 << 1,
>>  	IOMMU_HWPT_FAULT_ID_VALID = 1 << 2,
>> +	IOMMUFD_HWPT_ALLOC_PASID = 1 << 3,
> 
> Let's keep the consistent spelling IOMMU_HWPT_ALLOC_PASID

Sorry. Its my fault. Will fix it.

> 
> I think the series looks OK. The naming of domain_alloc_user() is now
> a little bit weird, it has now turned into
> domain_alloc_paging_extended().

Ack. Do you want me to do it now -OR- May be wait until all changes settles down
(above described group() related changes, this series, Baolu's series, etc)?


-Vasant