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

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

Jason,


On 8/7/2024 7:29 PM, Jason Gunthorpe wrote:
> 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 ?

Sure. We can do that.

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

I will add this.

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

Why do we need this flag? Previous one is already allocated PASID capable
domain. Is this similar to SVA domain?

> 
> 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:

Thanks! I will put it as part of this series.


@Baolu, Do we need to make any changes to intel driver?

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

If your series goes first then I can rebase it. Lets see.

-Vasant