[PATCH v2 1/1] iommu/vt-d: Fix RID2PASID setup failure

[PATCH v2 1/1] iommu/vt-d: Fix RID2PASID setup failure

[Lu Baolu]

The IOMMU driver shares the pasid table for PCI alias devices. When the
RID2PASID entry of the shared pasid table has been filled by the first
device, the subsequent devices will encounter the "DMAR: Setup RID2PASID
failed" failure as the pasid entry has already been marked as present. As
the result, the IOMMU probing process will be aborted.

This fixes it by skipping RID2PASID setting if the pasid entry has been
populated. This works because the IOMMU core ensures that only the same
IOMMU domain can be attached to all PCI alias devices at the same time.
Therefore the subsequent devices just try to setup the RID2PASID entry
with the same domain, which is negligible. This also adds domain validity
checks for more confidence anyway.

Fixes: ef848b7e5a6a0 ("iommu/vt-d: Setup pasid entry for RID2PASID support")
Reported-by: Chenyi Qiang <chenyi.qiang@intel.com>
Cc: stable@vger.kernel.org
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/pasid.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

Change log:
v2:
 - Add domain validity check in RID2PASID entry setup.

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index cb4c1d0cf25c..4f3525f3346f 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -575,6 +575,19 @@ static inline int pasid_enable_wpe(struct pasid_entry *pte)
 	return 0;
 };
 
+/*
+ * Return true if @pasid is RID2PASID and the domain @did has already
+ * been setup to the @pte. Otherwise, return false. PCI alias devices
+ * probably share the single RID2PASID pasid entry in the shared pasid
+ * table. It's reasonable that those devices try to set a share domain
+ * in their probe paths.
+ */
+static inline bool
+rid2pasid_domain_valid(struct pasid_entry *pte, u32 pasid, u16 did)
+{
+	return pasid == PASID_RID2PASID && pasid_get_domain_id(pte) == did;
+}
+
 /*
  * Set up the scalable mode pasid table entry for first only
  * translation type.
@@ -595,9 +608,8 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
 	if (WARN_ON(!pte))
 		return -EINVAL;
 
-	/* Caller must ensure PASID entry is not in use. */
 	if (pasid_pte_is_present(pte))
-		return -EBUSY;
+		return rid2pasid_domain_valid(pte, pasid, did) ? 0 : -EBUSY;
 
 	pasid_clear_entry(pte);
 
@@ -698,9 +710,8 @@ int intel_pasid_setup_second_level(struct intel_iommu *iommu,
 		return -ENODEV;
 	}
 
-	/* Caller must ensure PASID entry is not in use. */
 	if (pasid_pte_is_present(pte))
-		return -EBUSY;
+		return rid2pasid_domain_valid(pte, pasid, did) ? 0 : -EBUSY;
 
 	pasid_clear_entry(pte);
 	pasid_set_domain_id(pte, did);
@@ -738,9 +749,8 @@ int intel_pasid_setup_pass_through(struct intel_iommu *iommu,
 		return -ENODEV;
 	}
 
-	/* Caller must ensure PASID entry is not in use. */
 	if (pasid_pte_is_present(pte))
-		return -EBUSY;
+		return rid2pasid_domain_valid(pte, pasid, did) ? 0 : -EBUSY;
 
 	pasid_clear_entry(pte);
 	pasid_set_domain_id(pte, did);
-- 
2.25.1

RE: [PATCH v2 1/1] iommu/vt-d: Fix RID2PASID setup failure

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Wednesday, June 22, 2022 12:41 PM
> 
> The IOMMU driver shares the pasid table for PCI alias devices. When the
> RID2PASID entry of the shared pasid table has been filled by the first
> device, the subsequent devices will encounter the "DMAR: Setup RID2PASID
> failed" failure as the pasid entry has already been marked as present. As
> the result, the IOMMU probing process will be aborted.
> 
> This fixes it by skipping RID2PASID setting if the pasid entry has been
> populated. This works because the IOMMU core ensures that only the same
> IOMMU domain can be attached to all PCI alias devices at the same time.
> Therefore the subsequent devices just try to setup the RID2PASID entry
> with the same domain, which is negligible. This also adds domain validity
> checks for more confidence anyway.
> 
> Fixes: ef848b7e5a6a0 ("iommu/vt-d: Setup pasid entry for RID2PASID
> support")
> Reported-by: Chenyi Qiang <chenyi.qiang@intel.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH v2 1/1] iommu/vt-d: Fix RID2PASID setup failure

[Ethan Zhao]

Hi,

在 2022/6/22 12:41, Lu Baolu 写道:
> The IOMMU driver shares the pasid table for PCI alias devices. When the
> RID2PASID entry of the shared pasid table has been filled by the first
> device, the subsequent devices will encounter the "DMAR: Setup RID2PASID
> failed" failure as the pasid entry has already been marked as present. As
> the result, the IOMMU probing process will be aborted.
>
> This fixes it by skipping RID2PASID setting if the pasid entry has been
> populated. This works because the IOMMU core ensures that only the same
> IOMMU domain can be attached to all PCI alias devices at the same time.
> Therefore the subsequent devices just try to setup the RID2PASID entry
> with the same domain, which is negligible. This also adds domain validity
> checks for more confidence anyway.
>
> Fixes: ef848b7e5a6a0 ("iommu/vt-d: Setup pasid entry for RID2PASID support")
> Reported-by: Chenyi Qiang <chenyi.qiang@intel.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>   drivers/iommu/intel/pasid.c | 22 ++++++++++++++++------
>   1 file changed, 16 insertions(+), 6 deletions(-)
>
> Change log:
> v2:
>   - Add domain validity check in RID2PASID entry setup.
>
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index cb4c1d0cf25c..4f3525f3346f 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -575,6 +575,19 @@ static inline int pasid_enable_wpe(struct pasid_entry *pte)
>   	return 0;
>   };
>   
> +/*
> + * Return true if @pasid is RID2PASID and the domain @did has already
> + * been setup to the @pte. Otherwise, return false. PCI alias devices
> + * probably share the single RID2PASID pasid entry in the shared pasid
> + * table. It's reasonable that those devices try to set a share domain
> + * in their probe paths.
> + */

I am thinking about the counter-part, the intel_pasid_tear_down_entry(),

Multi devices share the same PASID entry, then one was detached from the 
domain,

so the entry doesn't exist anymore, while another devices don't know 
about the change,

and they are using the mapping, is it possible case ？shared thing, no 
refer-counter,

am I missing something ?


Thanks，

Ethan


> +static inline bool
> +rid2pasid_domain_valid(struct pasid_entry *pte, u32 pasid, u16 did)
> +{
> +	return pasid == PASID_RID2PASID && pasid_get_domain_id(pte) == did;
> +}
> +
>   /*
>    * Set up the scalable mode pasid table entry for first only
>    * translation type.
> @@ -595,9 +608,8 @@ int intel_pasid_setup_first_level(struct intel_iommu *iommu,
>   	if (WARN_ON(!pte))
>   		return -EINVAL;
>   
> -	/* Caller must ensure PASID entry is not in use. */
>   	if (pasid_pte_is_present(pte))
> -		return -EBUSY;
> +		return rid2pasid_domain_valid(pte, pasid, did) ? 0 : -EBUSY;
>   
>   	pasid_clear_entry(pte);
>   
> @@ -698,9 +710,8 @@ int intel_pasid_setup_second_level(struct intel_iommu *iommu,
>   		return -ENODEV;
>   	}
>   
> -	/* Caller must ensure PASID entry is not in use. */
>   	if (pasid_pte_is_present(pte))
> -		return -EBUSY;
> +		return rid2pasid_domain_valid(pte, pasid, did) ? 0 : -EBUSY;
>   
>   	pasid_clear_entry(pte);
>   	pasid_set_domain_id(pte, did);
> @@ -738,9 +749,8 @@ int intel_pasid_setup_pass_through(struct intel_iommu *iommu,
>   		return -ENODEV;
>   	}
>   
> -	/* Caller must ensure PASID entry is not in use. */
>   	if (pasid_pte_is_present(pte))
> -		return -EBUSY;
> +		return rid2pasid_domain_valid(pte, pasid, did) ? 0 : -EBUSY;
>   
>   	pasid_clear_entry(pte);
>   	pasid_set_domain_id(pte, did);

-- 
AFAIK = As Far As I Know
AKA = Also Known As
ASAP = As Soon As Possible

Re: [PATCH v2 1/1] iommu/vt-d: Fix RID2PASID setup failure

[Baolu Lu]

On 2022/6/22 17:09, Ethan Zhao wrote:
> 
> 在 2022/6/22 12:41, Lu Baolu 写道:
>> The IOMMU driver shares the pasid table for PCI alias devices. When the
>> RID2PASID entry of the shared pasid table has been filled by the first
>> device, the subsequent devices will encounter the "DMAR: Setup RID2PASID
>> failed" failure as the pasid entry has already been marked as present. As
>> the result, the IOMMU probing process will be aborted.
>>
>> This fixes it by skipping RID2PASID setting if the pasid entry has been
>> populated. This works because the IOMMU core ensures that only the same
>> IOMMU domain can be attached to all PCI alias devices at the same time.
>> Therefore the subsequent devices just try to setup the RID2PASID entry
>> with the same domain, which is negligible. This also adds domain validity
>> checks for more confidence anyway.
>>
>> Fixes: ef848b7e5a6a0 ("iommu/vt-d: Setup pasid entry for RID2PASID 
>> support")
>> Reported-by: Chenyi Qiang <chenyi.qiang@intel.com>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/intel/pasid.c | 22 ++++++++++++++++------
>>   1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> Change log:
>> v2:
>>   - Add domain validity check in RID2PASID entry setup.
>>
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>> index cb4c1d0cf25c..4f3525f3346f 100644
>> --- a/drivers/iommu/intel/pasid.c
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -575,6 +575,19 @@ static inline int pasid_enable_wpe(struct 
>> pasid_entry *pte)
>>       return 0;
>>   };
>> +/*
>> + * Return true if @pasid is RID2PASID and the domain @did has already
>> + * been setup to the @pte. Otherwise, return false. PCI alias devices
>> + * probably share the single RID2PASID pasid entry in the shared pasid
>> + * table. It's reasonable that those devices try to set a share domain
>> + * in their probe paths.
>> + */
> 
> I am thinking about the counter-part, the intel_pasid_tear_down_entry(),
> 
> Multi devices share the same PASID entry, then one was detached from the 
> domain,
> 
> so the entry doesn't exist anymore, while another devices don't know 
> about the change,
> 
> and they are using the mapping, is it possible case ？shared thing, no 
> refer-counter,
> 
> am I missing something ?

No. You are right. When any alias device is hot-removed from the system,
the shared RID2PASID will be cleared without any notification to other
devices. Hence any DMAs from those devices are blocked.

We still have a lot to do for sharing pasid table among alias devices.
Before we arrive there, let's remove it for now.

Best regards,
baolu