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