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

-- 
Regards,
Yi Liu