On 2024/12/16 16:26, Tian, Kevin wrote:
From: Liu, Yi L <yi.l.liu@xxxxxxxxx>
Sent: Saturday, December 14, 2024 5:04 PM
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.
It's simple but break applications which are not aware of PASID. Ideally
if the app is not interested in PASID it has no business to query the PASID
cap and then set the flag accordingly.
yes.
@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,
};
I'm afraid that doing so adds more confusion as one could easily
ask why such enforcement is only applied to the paging domain.
Please note the end result of nesting domain can still meet the
restriction.
For ARM/AMD the nesting domain attached to RID cannot set
ALLOC_PASID so it cannot be attached to PASID later.
For Intel a nesting domain attached to RID can have the flag
set or cleared. If the domain is intended to be attached to
a PASID later, then it must have the ALLOC_PASID set.
So I don't see a need of exempting the nesting domain here.
btw what about requiring to acquire &idev->igroup->lock
in the pasid path? It's not a performance critical path, and
by holding that lock in both RID/PASID attach, we can check
idev->pasid_hwpts to decide whether a domain attached to
RID must have the flag set and vice versa when doing pasid
attach whether idev->igroup->hwpt already has the flag set.
seems workable although igroup->lock is supposed to protect the attach
of multi-device groups. Here we need to extend it to protect the
idev->pasid_hwpts as well. With this, I think we can apply the enforcement
to nested domains.
--
Regards,
Yi Liu
