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

Yi Liu
2024-12-13T08:08:32Z

On 2024/12/13 16:11, Yi Liu wrote:
> On 2024/12/13 15:52, Tian, Kevin wrote:
>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>> Sent: Friday, December 13, 2024 3:20 PM
>>>
>>> On 2024/12/13 10:43, Tian, Kevin wrote:
>>>>
>>>> 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;
>>
>> shouldn't it be:
>>
>>     if (!idev->dev->iommu->max_pasids ||
>>          pasid == IOMMU_NO_PASID ||
>>          !hwpt->pasid_compat)
>>         return -EINVAL;
>>
>> ?
>
> no, this check is added in a common place shared by RID and PASID path. If
> it is added in place only for the PASID, it should be something like you
> wrote.
>
>>>
>>> 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.
>>
>> 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, I've asked this question [1]. AMD requires the RID and PASID use the
> same format. I agree it's a driver's burden but now it's defined in the
> ALLOC_PASID. So, I doubt if it becomes a common requirement to all the
> iommu drivers. Otherwise the ALLOC_PASID definition is broken. e.g. Intel
> may have no need to enforce it, but it would be like Intel is breaking
> it.

aha, forgot the link.

[1] https://lore.kernel.org/linux-iommu/20240822124433.GD3468552@ziepe.ca/

--
Regards,
Yi Liu