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 10:43, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Thursday, December 12, 2024 3:13 PM
>>
>> On 2024/12/12 13:51, Tian, Kevin wrote:
>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>> Sent: Thursday, December 12, 2024 11:15 AM
>>>>
>>>> So maybe we should not stick with the initial purpose of ALLOC_PASID flag.
>>>> It actually means selecting V2 page table. But the definition of it allows
>>>> us to consider the nested domains to be pasid-compat as Intel allows it.
>>>> And, a sane userspace running on ARM/AMD will never attach nested
>>>> domain
>>>> to PASIDs. Even it does, the ARM SMMU and AMD iommu driver can fail
>> such
>>>> attempts. In this way, we can enforce the ALLOC_PASID flag for any
>> domains
>>>> used by PASID-capable devices in iommufd. This suits the existing
>>>> ALLOC_PASID definition as well.
>>>
>>> Isn't it what I was suggesting? IOMMUFD just enforces that flag must
>>> be set if a domain will be attached to PASID, and drivers will do
>>> additional restrictions e.g. AMD/ARM allows the flag only on paging
>>> domain while VT-d allows it for any type.
>>
>> A slight difference. :) I think we also need to enforce it for the
>> non-PASID path. If not, the PASID path cannot work according to the
>> ALLOC_PASID definition. But we are on the same page about the additional
>> restrictions in ARM/AMD drivers about the nested domain used on PASIDs.
>> This is supposed to be done in attach phase instead of domain allocation
>> time.
>>
> 
> Here is my full picture:
> 
> At domain allocation the driver should decide whether the setting of
> ALLOC_PASID is compatible to the given domain type.
> 
> If paging and iommu supports pasid then ALLOC_PASID is allowed. This
> applies to all drivers. AMD driver will further select V1 vs. V2 according
> to the flag bit.
> 
> If nesting, AMR/ARM drivers will reject the bit as a CD/PASID table
> cannot be attached to a PASID. Intel driver allows it if pasid is supported
> by iommu.

Following your opinion, I think the enforcement is something like this,
it only checks pasid_compat for the PASID path.

+	if (idev->dev->iommu->max_pasids && pasid != IOMMU_NO_PASID && 
!hwpt->pasid_compat)
+		return -EINVAL;

This means the RID path is not surely be attached to pasid-comapt domain
or not. either iommufd or iommu driver should do across check between the
RID and PASID path. It is failing attaching non-pasid compat domain to RID
if PASID has been attached, and vice versa, attaching PASIDs should be
failed if RID has been attached to non pasid comapt domain. I doubt if this
can be done easily as there is no lock between RID and PASID paths. Maybe
it can still be done by enforcing pasid-comapt for pasid-capable device.
But this may be done in iommu drivers? If still done in iommufd. It would
be like:

+	if (idev->dev->iommu->max_pasids && !hwpt->pasid_compat)
+		return -EINVAL;

If so, ARM/AMD drivers need to allow allocating nested domain with
ALLOC_PASID flag. If not, attaching nested domain to RID would be failed.
That's why I intend to allow it, while let ARM/AMD drivers fail the
attempt of attaching nested domain to PASIDs.

> At attach phase, a domain with ALLOC_PASID can be attached to RID
> of any device no matter the device supports pasid or not. But a domain
> must have ALLOC_PASID set for attaching to a PASID (if the device has
> non-zero max_pasids), enforced by iommufd.


Regards,
Yi Liu