Re: [PATCH v8 07/12] iommufd: Enforce PASID-compatible domain for RID

[Baolu Lu <baolu.lu@linux.intel.com>]

On 2/26/25 19:40, Yi Liu wrote:
> Per the definition of IOMMU_HWPT_ALLOC_PASID, iommufd needs to enforce
> the RID to use PASID-compatible domain if PASID has been attached, and
> vice versa. The PASID path has already enforced it. This adds the
> enforcement in the RID path.
> 
> This enforcement requires a lock across the RID and PASID attach path,
> the idev->igroup->lock is used as both the RID and the PASID path holds
> it.
> 
> Signed-off-by: Yi Liu<yi.l.liu@intel.com>
> ---
>   drivers/iommu/iommufd/device.c | 28 ++++++++++++++++++++++------
>   1 file changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index 30dd2f79491a..e0f097b04467 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -357,6 +357,22 @@ iommufd_device_attach_reserved_iova(struct iommufd_device *idev,
>   
>   /* The device attach/detach/replace helpers for attach_handle */
>   
> +static int iommufd_hwpt_pasid_compat(struct iommufd_hw_pagetable *hwpt,
> +				     struct iommufd_device *idev,
> +				     ioasid_t pasid)
> +{
> +	lockdep_assert_held(&idev->igroup->lock);
> +
> +	if (pasid == IOMMU_NO_PASID &&
> +	    !xa_empty(&idev->pasid_hwpts) && !hwpt->pasid_compat)
> +		return -EINVAL;
> +
> +	if (pasid != IOMMU_NO_PASID &&
> +	    (!idev->igroup->hwpt->pasid_compat || !hwpt->pasid_compat))
> +		return -EINVAL;
> +	return 0;
> +}

I'm not following this. Patch 05/12, which introduced the concept of
PASID-compatible domain, states, "AMD IOMMU requires attaching PASID-
compatible domains to PASID-capable devices." My understanding was that
if PASID is enabled on a device, the RID or PASID of that device should
only be configured with PASID-compatible domains.

However, the logic here differs. It seems that even if a device is
PASID-capable, it's allowed to attach a non-PASID-compatible domain to
the RID, if no domain is currently attached to the device's PASID. This
doesn't seem like a generic property.

Thanks,
baolu