Re: [PATCH v5 08/12] iommufd: Enforce pasid compatible domain for PASID-capable device

[Yi Liu <yi.l.liu@intel.com>]

On 2024/12/13 20:40, Jason Gunthorpe wrote:
> On Fri, Dec 13, 2024 at 07:52:40AM +0000, Tian, Kevin wrote:
> 
>> I'm not sure where that requirement comes from. Does AMD require RID
>> and PASID to use the same format when nesting is disabled? If yes, that's
>> still a driver burden to handle, not iommufd's...
> 
> Yes, ARM and AMD require this too
> 
> The point of the iommufd enforcement of ALLOC_PAGING is to try to
> discourage bad apps - ie apps that only work on Intel. We can check
> the rid attach too if it is easy to do

I have an easy way to enforce RID attach. It is:

If the device is device capable, I would enforce all domains for this
device (either RID or PASID) be flagged. The device capable info is static,
so no need to add extra lock across the RID and PASID attach paths for the
page table format alignment. This has only one drawback. If userspace is
not going to use PASID, it still needs to allocated domain with this flag.
I think AMD may need to confirm if it is acceptable.

@Kevin, I'd like to echo the prior suggestion for nested domain. It looks
hard to apply the pasid enforcement on it. So I'd like to limit the
ALLOC_PASID flag to paging domains. Also, I doubt if the uapi needs to
mandate the RID part to use this flag. It appears to me it can be done
iommu drivers. If so, no need to mandate it in uapi. So I'm considering
to do below changes to IOMMU_HWPT_ALLOC_PASID. The new definition does not
mandate the RID part of devices, and leaves it to vendors. Hence, the
iommufd only needs to ensure the paging domains used by PASID should be
flagged. e.g. Intel won't fail PASID attach even its RID is using a domain
that is not flagged (e.g. nested domain, under the new definition, nested
domain does not use this flag). While, AMD would fail it if the RID domain
is not using this flag. This has one more benefit, it leaves the
flexibility of using pasid or not to user.

diff --git a/include/uapi/linux/iommufd.h b/include/uapi/linux/iommufd.h
index 0e27557fb86b..a1a11041d941 100644
--- a/include/uapi/linux/iommufd.h
+++ b/include/uapi/linux/iommufd.h
@@ -387,19 +387,20 @@ 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: 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 flagged, otherwise attaching a
- *                          PASID will blocked.
- *                          If IOMMU does not support PASID it will return
- *                          error (-EOPNOTSUPP).
+ * @IOMMU_HWPT_ALLOC_PAGING_PASID: Requests a paging domain that can be used
+ *                                 with PASID. The domain can be attached to
+ *                                 any PASID on the device. Vendors may 
require
+ *                                 the non-PASID part of the device use this
+ *                                 flag as well. If yes, attaching a PASID 
will
+ *                                 blocked if non-PASID part is not using it.
+ *                                 If IOMMU does not support PASID it will
+ *                                 return error (-EOPNOTSUPP).
   */
  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,
-	IOMMU_HWPT_ALLOC_PASID = 1 << 3,
+	IOMMU_HWPT_ALLOC_PAGING_PASID = 1 << 3,
  };

  /**
-- 
Regards,
Yi Liu