Re: [PATCH RFCv2] iommu: Add domain type and flag to domain_alloc_paging()

[Jason Gunthorpe <jgg@ziepe.ca>]

On Wed, Aug 07, 2024 at 03:00:50PM +0530, Vasant Hegde wrote:
> 
> commit 312c2a99897937834f6a679b6f9956288e8c93de
> Author: Vasant Hegde <vasant.hegde@amd.com>
> Date:   Wed Aug 7 14:42:24 2024 +0530
> 
>     vfio: User iommu_ops->domain_alloc_user() to allocate IOMMU domain
> 
>     - Include uapi/linux/iommufd.h in vfio_iommu_type1.c so that we can use
>       HWPT flags.
>     - Move dev_iommu_ops() from iommu-priv.h to linux/iommu.h
> 
>     Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>

Yeah, this seems like a good start

> @@ -2134,8 +2135,14 @@ static void vfio_iommu_iova_insert_copy(struct vfio_iommu
> *iommu,
>  static int vfio_iommu_domain_alloc(struct device *dev, void *data)
>  {
>  	struct iommu_domain **domain = data;
> +	const struct iommu_ops *ops = dev_iommu_ops(dev);
> +	u32 flags = 0;
> +
> +	if (ops->domain_alloc_user)
> +		*domain = ops->domain_alloc_user(dev, flags, NULL, NULL);
> +	else
> +		*domain = iommu_paging_domain_alloc(dev);

I suggest to wrapper this in a EXPORT_SYMBOL rather than expose
dev_iommu_ops ?

> @@ -359,11 +359,14 @@ struct iommu_vfio_ioas {
>   *                                   enforced on device attachment
>   * @IOMMU_HWPT_FAULT_ID_VALID: The fault_id field of hwpt allocation data is
>   *                             valid.
> + * @IOMMU_HWPT_ALLOC_PASID : If set, allocate a HWPT that support PASID
> + *
>   */

Needs more words, maybe even two flags?

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

 @IOMMU_HWPT_ALLOC_PASID: The domain can be used as a PASID. The
 domain attached to the device must have also been allocated with
 IOMMU_HWPT_ALLOC_DEV_PASID.

And then we'd want to enforce this rule in iommufd to force userspace
to use the flags properly. Otherwise AMD will be permanently broken :\

I'm guessing that userspace will never want to share a PASID domain
and a device domain, and current iommu HW will treat both flags as the
same functionality (use the AMD v2 domain)

Yi will need to accomodate these flags in his series..

SMMUv3 support would look like this:

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 9bc50bded5af72..c2e46a352d6b09 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3052,7 +3052,9 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
                           const struct iommu_user_data *user_data)
 {
        struct arm_smmu_master *master = dev_iommu_priv_get(dev);
-       const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
+       const u32 PAGING_FLAGS = IOMMU_HWPT_ALLOC_DIRTY_TRACKING |
+                                IOMMU_HWPT_ALLOC_DEV_PASID |
+                                IOMMU_HWPT_ALLOC_PASID;
        struct arm_smmu_domain *smmu_domain;
        int ret;
 
@@ -3065,6 +3067,14 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
        if (!smmu_domain)
                return ERR_PTR(-ENOMEM);
 
+       if (flags & (IOMMU_HWPT_ALLOC_DEV_PASID | IOMMU_HWPT_ALLOC_PASID)) {
+               if (!(master->smmu->features & ARM_SMMU_FEAT_TRANS_S1)) {
+                       ret = -EOPNOTSUPP;
+                       goto err_free;
+               }
+               smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
+       }
+
        smmu_domain->domain.type = IOMMU_DOMAIN_UNMANAGED;
        smmu_domain->domain.ops = arm_smmu_ops.default_domain_ops;
        ret = arm_smmu_domain_finalise(smmu_domain, master->smmu, flags);

(though this will clash with the nesting series I just sent)

Jason