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

[Jason Gunthorpe <jgg@ziepe.ca>]

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;

>   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()

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.

> @@ -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.

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

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().

Thanks,
Jason