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

Yi Liu
2024-12-17T13:23:31Z

On 2024/12/16 16:26, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> 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