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

[PATCH 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 marke 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.

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/iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 44016594831d..b9966c01a2a2 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct dmar_domain *domain, struct device *dev)
 			ret = intel_pasid_setup_second_level(iommu, domain,
 					dev, PASID_RID2PASID);
 		spin_unlock_irqrestore(&iommu->lock, flags);
-		if (ret) {
+		if (ret && ret != -EBUSY) {
 			dev_err(dev, "Setup RID2PASID failed\n");
 			dmar_remove_one_dev_info(dev);
 			return ret;
-- 
2.25.1

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

[Yi Liu]

Hi Baolu,

On 2022/6/20 16:17, Lu Baolu wrote:
> 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 marke as present. As

s/marke/marked/

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

nit. this sentence is a little bit to interpret. :-) I guess what you want
to describe is the PCI alias devices should be attached to the same domain
instead of different domain. right?

also, does it apply to all domain types? e.g. the SVA domains introduced in 
"iommu: SVA and IOPF refactoring"

> Therefore the subsequent devices just try to setup the RID2PASID entry
> with the same domain, which is negligible.
> 
> 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/iommu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 44016594831d..b9966c01a2a2 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct dmar_domain *domain, struct device *dev)
>   			ret = intel_pasid_setup_second_level(iommu, domain,
>   					dev, PASID_RID2PASID);
>   		spin_unlock_irqrestore(&iommu->lock, flags);
> -		if (ret) {
> +		if (ret && ret != -EBUSY) {
>   			dev_err(dev, "Setup RID2PASID failed\n");
>   			dmar_remove_one_dev_info(dev);
>   			return ret;

-- 
Regards,
Yi Liu

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

[Baolu Lu]

On 2022/6/20 16:31, Yi Liu wrote:
> Hi Baolu,
> 
> On 2022/6/20 16:17, Lu Baolu wrote:
>> 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 marke as present. As
> 
> s/marke/marked/

Updated. Thanks!

>> 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.
> 
> nit. this sentence is a little bit to interpret. :-) I guess what you want
> to describe is the PCI alias devices should be attached to the same domain
> instead of different domain. right?

Yes.

> 
> also, does it apply to all domain types? e.g. the SVA domains introduced 
> in "iommu: SVA and IOPF refactoring"

No. Only about the RID2PASID.

> 
>> Therefore the subsequent devices just try to setup the RID2PASID entry
>> with the same domain, which is negligible.
>>
>> 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/iommu.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index 44016594831d..b9966c01a2a2 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct 
>> dmar_domain *domain, struct device *dev)
>>               ret = intel_pasid_setup_second_level(iommu, domain,
>>                       dev, PASID_RID2PASID);
>>           spin_unlock_irqrestore(&iommu->lock, flags);
>> -        if (ret) {
>> +        if (ret && ret != -EBUSY) {
>>               dev_err(dev, "Setup RID2PASID failed\n");
>>               dmar_remove_one_dev_info(dev);
>>               return ret;
> 

--
Best regards,
baolu

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

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Monday, June 20, 2022 4:17 PM
> @@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct
> dmar_domain *domain, struct device *dev)
>  			ret = intel_pasid_setup_second_level(iommu,
> domain,
>  					dev, PASID_RID2PASID);
>  		spin_unlock_irqrestore(&iommu->lock, flags);
> -		if (ret) {
> +		if (ret && ret != -EBUSY) {
>  			dev_err(dev, "Setup RID2PASID failed\n");
>  			dmar_remove_one_dev_info(dev);
>  			return ret;
> --
> 2.25.1

It's cleaner to avoid this error at the first place, i.e. only do the
setup when the first device is attached to the pasid table.

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

[Baolu Lu]

On 2022/6/21 10:54, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Monday, June 20, 2022 4:17 PM
>> @@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct
>> dmar_domain *domain, struct device *dev)
>>   			ret = intel_pasid_setup_second_level(iommu,
>> domain,
>>   					dev, PASID_RID2PASID);
>>   		spin_unlock_irqrestore(&iommu->lock, flags);
>> -		if (ret) {
>> +		if (ret && ret != -EBUSY) {
>>   			dev_err(dev, "Setup RID2PASID failed\n");
>>   			dmar_remove_one_dev_info(dev);
>>   			return ret;
>> --
>> 2.25.1
> 
> It's cleaner to avoid this error at the first place, i.e. only do the
> setup when the first device is attached to the pasid table.

The logic that identifies the first device might introduce additional
unnecessary complexity. Devices that share a pasid table are rare. I
even prefer to give up sharing tables so that the code can be
simpler.:-)

--
Best regards,
baolu

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

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Tuesday, June 21, 2022 11:39 AM
> 
> On 2022/6/21 10:54, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@linux.intel.com>
> >> Sent: Monday, June 20, 2022 4:17 PM
> >> @@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct
> >> dmar_domain *domain, struct device *dev)
> >>   			ret = intel_pasid_setup_second_level(iommu,
> >> domain,
> >>   					dev, PASID_RID2PASID);
> >>   		spin_unlock_irqrestore(&iommu->lock, flags);
> >> -		if (ret) {
> >> +		if (ret && ret != -EBUSY) {
> >>   			dev_err(dev, "Setup RID2PASID failed\n");
> >>   			dmar_remove_one_dev_info(dev);
> >>   			return ret;
> >> --
> >> 2.25.1
> >
> > It's cleaner to avoid this error at the first place, i.e. only do the
> > setup when the first device is attached to the pasid table.
> 
> The logic that identifies the first device might introduce additional
> unnecessary complexity. Devices that share a pasid table are rare. I
> even prefer to give up sharing tables so that the code can be
> simpler.:-)
> 

It's not that complex if you simply move device_attach_pasid_table()
out of intel_pasid_alloc_table(). Then do the setup if
list_empty(&pasid_table->dev) and then attach device to the
pasid table in domain_add_dev_info().

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

[Baolu Lu]

On 2022/6/21 11:46, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Tuesday, June 21, 2022 11:39 AM
>>
>> On 2022/6/21 10:54, Tian, Kevin wrote:
>>>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>>> Sent: Monday, June 20, 2022 4:17 PM
>>>> @@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct
>>>> dmar_domain *domain, struct device *dev)
>>>>    			ret = intel_pasid_setup_second_level(iommu,
>>>> domain,
>>>>    					dev, PASID_RID2PASID);
>>>>    		spin_unlock_irqrestore(&iommu->lock, flags);
>>>> -		if (ret) {
>>>> +		if (ret && ret != -EBUSY) {
>>>>    			dev_err(dev, "Setup RID2PASID failed\n");
>>>>    			dmar_remove_one_dev_info(dev);
>>>>    			return ret;
>>>> --
>>>> 2.25.1
>>>
>>> It's cleaner to avoid this error at the first place, i.e. only do the
>>> setup when the first device is attached to the pasid table.
>>
>> The logic that identifies the first device might introduce additional
>> unnecessary complexity. Devices that share a pasid table are rare. I
>> even prefer to give up sharing tables so that the code can be
>> simpler.:-)
>>
> 
> It's not that complex if you simply move device_attach_pasid_table()
> out of intel_pasid_alloc_table(). Then do the setup if
> list_empty(&pasid_table->dev) and then attach device to the
> pasid table in domain_add_dev_info().

The pasid table is part of the device, hence a better place to
allocate/free the pasid table is in the device probe/release paths.
Things will become more complicated if we change relationship between
device and it's pasid table when attaching/detaching a domain. That's
the reason why I thought it was additional complexity.

--
Best regards,
baolu

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

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Tuesday, June 21, 2022 12:28 PM
> 
> On 2022/6/21 11:46, Tian, Kevin wrote:
> >> From: Baolu Lu <baolu.lu@linux.intel.com>
> >> Sent: Tuesday, June 21, 2022 11:39 AM
> >>
> >> On 2022/6/21 10:54, Tian, Kevin wrote:
> >>>> From: Lu Baolu <baolu.lu@linux.intel.com>
> >>>> Sent: Monday, June 20, 2022 4:17 PM
> >>>> @@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct
> >>>> dmar_domain *domain, struct device *dev)
> >>>>    			ret = intel_pasid_setup_second_level(iommu,
> >>>> domain,
> >>>>    					dev, PASID_RID2PASID);
> >>>>    		spin_unlock_irqrestore(&iommu->lock, flags);
> >>>> -		if (ret) {
> >>>> +		if (ret && ret != -EBUSY) {
> >>>>    			dev_err(dev, "Setup RID2PASID failed\n");
> >>>>    			dmar_remove_one_dev_info(dev);
> >>>>    			return ret;
> >>>> --
> >>>> 2.25.1
> >>>
> >>> It's cleaner to avoid this error at the first place, i.e. only do the
> >>> setup when the first device is attached to the pasid table.
> >>
> >> The logic that identifies the first device might introduce additional
> >> unnecessary complexity. Devices that share a pasid table are rare. I
> >> even prefer to give up sharing tables so that the code can be
> >> simpler.:-)
> >>
> >
> > It's not that complex if you simply move device_attach_pasid_table()
> > out of intel_pasid_alloc_table(). Then do the setup if
> > list_empty(&pasid_table->dev) and then attach device to the
> > pasid table in domain_add_dev_info().
> 
> The pasid table is part of the device, hence a better place to
> allocate/free the pasid table is in the device probe/release paths.
> Things will become more complicated if we change relationship between
> device and it's pasid table when attaching/detaching a domain. That's
> the reason why I thought it was additional complexity.
> 

If you do want to follow current route it’s still cleaner to check
whether the pasid entry has pointed to the domain in the individual
setup function instead of blindly returning -EBUSY and then ignoring
it even if a real busy condition occurs. The setup functions can
just return zero for this benign alias case.

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

[Baolu Lu]

On 2022/6/21 13:48, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Tuesday, June 21, 2022 12:28 PM
>>
>> On 2022/6/21 11:46, Tian, Kevin wrote:
>>>> From: Baolu Lu <baolu.lu@linux.intel.com>
>>>> Sent: Tuesday, June 21, 2022 11:39 AM
>>>>
>>>> On 2022/6/21 10:54, Tian, Kevin wrote:
>>>>>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>>>>> Sent: Monday, June 20, 2022 4:17 PM
>>>>>> @@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct
>>>>>> dmar_domain *domain, struct device *dev)
>>>>>>     			ret = intel_pasid_setup_second_level(iommu,
>>>>>> domain,
>>>>>>     					dev, PASID_RID2PASID);
>>>>>>     		spin_unlock_irqrestore(&iommu->lock, flags);
>>>>>> -		if (ret) {
>>>>>> +		if (ret && ret != -EBUSY) {
>>>>>>     			dev_err(dev, "Setup RID2PASID failed\n");
>>>>>>     			dmar_remove_one_dev_info(dev);
>>>>>>     			return ret;
>>>>>> --
>>>>>> 2.25.1
>>>>>
>>>>> It's cleaner to avoid this error at the first place, i.e. only do the
>>>>> setup when the first device is attached to the pasid table.
>>>>
>>>> The logic that identifies the first device might introduce additional
>>>> unnecessary complexity. Devices that share a pasid table are rare. I
>>>> even prefer to give up sharing tables so that the code can be
>>>> simpler.:-)
>>>>
>>>
>>> It's not that complex if you simply move device_attach_pasid_table()
>>> out of intel_pasid_alloc_table(). Then do the setup if
>>> list_empty(&pasid_table->dev) and then attach device to the
>>> pasid table in domain_add_dev_info().
>>
>> The pasid table is part of the device, hence a better place to
>> allocate/free the pasid table is in the device probe/release paths.
>> Things will become more complicated if we change relationship between
>> device and it's pasid table when attaching/detaching a domain. That's
>> the reason why I thought it was additional complexity.
>>
> 
> If you do want to follow current route it’s still cleaner to check
> whether the pasid entry has pointed to the domain in the individual
> setup function instead of blindly returning -EBUSY and then ignoring
> it even if a real busy condition occurs. The setup functions can
> just return zero for this benign alias case.

Fair enough. Let me improve it.

--
Best regards,
baolu

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

[Baolu Lu]

On 2022/6/21 13:48, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Tuesday, June 21, 2022 12:28 PM
>>
>> On 2022/6/21 11:46, Tian, Kevin wrote:
>>>> From: Baolu Lu <baolu.lu@linux.intel.com>
>>>> Sent: Tuesday, June 21, 2022 11:39 AM
>>>>
>>>> On 2022/6/21 10:54, Tian, Kevin wrote:
>>>>>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>>>>> Sent: Monday, June 20, 2022 4:17 PM
>>>>>> @@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct
>>>>>> dmar_domain *domain, struct device *dev)
>>>>>>     			ret = intel_pasid_setup_second_level(iommu,
>>>>>> domain,
>>>>>>     					dev, PASID_RID2PASID);
>>>>>>     		spin_unlock_irqrestore(&iommu->lock, flags);
>>>>>> -		if (ret) {
>>>>>> +		if (ret && ret != -EBUSY) {
>>>>>>     			dev_err(dev, "Setup RID2PASID failed\n");
>>>>>>     			dmar_remove_one_dev_info(dev);
>>>>>>     			return ret;
>>>>>> --
>>>>>> 2.25.1
>>>>>
>>>>> It's cleaner to avoid this error at the first place, i.e. only do the
>>>>> setup when the first device is attached to the pasid table.
>>>>
>>>> The logic that identifies the first device might introduce additional
>>>> unnecessary complexity. Devices that share a pasid table are rare. I
>>>> even prefer to give up sharing tables so that the code can be
>>>> simpler.:-)
>>>>
>>>
>>> It's not that complex if you simply move device_attach_pasid_table()
>>> out of intel_pasid_alloc_table(). Then do the setup if
>>> list_empty(&pasid_table->dev) and then attach device to the
>>> pasid table in domain_add_dev_info().
>>
>> The pasid table is part of the device, hence a better place to
>> allocate/free the pasid table is in the device probe/release paths.
>> Things will become more complicated if we change relationship between
>> device and it's pasid table when attaching/detaching a domain. That's
>> the reason why I thought it was additional complexity.
>>
> 
> If you do want to follow current route it’s still cleaner to check
> whether the pasid entry has pointed to the domain in the individual
> setup function instead of blindly returning -EBUSY and then ignoring
> it even if a real busy condition occurs. The setup functions can
> just return zero for this benign alias case.

Kevin, how do you like this one?

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index cb4c1d0cf25c..ecffd0129b2b 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -575,6 +575,16 @@ 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.
+ */
+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 +605,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 +707,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 +746,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);

--
Best regards,
baolu

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

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Tuesday, June 21, 2022 5:04 PM
> 
> On 2022/6/21 13:48, Tian, Kevin wrote:
> >> From: Baolu Lu <baolu.lu@linux.intel.com>
> >> Sent: Tuesday, June 21, 2022 12:28 PM
> >>
> >> On 2022/6/21 11:46, Tian, Kevin wrote:
> >>>> From: Baolu Lu <baolu.lu@linux.intel.com>
> >>>> Sent: Tuesday, June 21, 2022 11:39 AM
> >>>>
> >>>> On 2022/6/21 10:54, Tian, Kevin wrote:
> >>>>>> From: Lu Baolu <baolu.lu@linux.intel.com>
> >>>>>> Sent: Monday, June 20, 2022 4:17 PM
> >>>>>> @@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct
> >>>>>> dmar_domain *domain, struct device *dev)
> >>>>>>     			ret = intel_pasid_setup_second_level(iommu,
> >>>>>> domain,
> >>>>>>     					dev, PASID_RID2PASID);
> >>>>>>     		spin_unlock_irqrestore(&iommu->lock, flags);
> >>>>>> -		if (ret) {
> >>>>>> +		if (ret && ret != -EBUSY) {
> >>>>>>     			dev_err(dev, "Setup RID2PASID failed\n");
> >>>>>>     			dmar_remove_one_dev_info(dev);
> >>>>>>     			return ret;
> >>>>>> --
> >>>>>> 2.25.1
> >>>>>
> >>>>> It's cleaner to avoid this error at the first place, i.e. only do the
> >>>>> setup when the first device is attached to the pasid table.
> >>>>
> >>>> The logic that identifies the first device might introduce additional
> >>>> unnecessary complexity. Devices that share a pasid table are rare. I
> >>>> even prefer to give up sharing tables so that the code can be
> >>>> simpler.:-)
> >>>>
> >>>
> >>> It's not that complex if you simply move device_attach_pasid_table()
> >>> out of intel_pasid_alloc_table(). Then do the setup if
> >>> list_empty(&pasid_table->dev) and then attach device to the
> >>> pasid table in domain_add_dev_info().
> >>
> >> The pasid table is part of the device, hence a better place to
> >> allocate/free the pasid table is in the device probe/release paths.
> >> Things will become more complicated if we change relationship between
> >> device and it's pasid table when attaching/detaching a domain. That's
> >> the reason why I thought it was additional complexity.
> >>
> >
> > If you do want to follow current route it’s still cleaner to check
> > whether the pasid entry has pointed to the domain in the individual
> > setup function instead of blindly returning -EBUSY and then ignoring
> > it even if a real busy condition occurs. The setup functions can
> > just return zero for this benign alias case.
> 
> Kevin, how do you like this one?
> 
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index cb4c1d0cf25c..ecffd0129b2b 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -575,6 +575,16 @@ 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.
> + */
> +static inline bool
> +rid2pasid_domain_valid(struct pasid_entry *pte, u32 pasid, u16 did)
> +{
> +	return pasid == PASID_RID2PASID && pasid_get_domain_id(pte) ==
> did;
> +}

better this is not restricted to RID2PASID only, e.g. pasid_pte_match_domain()
and then read pasid from the pte to compare with the pasid argument.

> +
>   /*
>    * Set up the scalable mode pasid table entry for first only
>    * translation type.
> @@ -595,9 +605,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 +707,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 +746,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);
> 
> --
> Best regards,
> baolu

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

[Baolu Lu]

On 2022/6/22 11:06, Tian, Kevin wrote:
>> From: Baolu Lu<baolu.lu@linux.intel.com>
>> Sent: Tuesday, June 21, 2022 5:04 PM
>>
>> On 2022/6/21 13:48, Tian, Kevin wrote:
>>>> From: Baolu Lu<baolu.lu@linux.intel.com>
>>>> Sent: Tuesday, June 21, 2022 12:28 PM
>>>>
>>>> On 2022/6/21 11:46, Tian, Kevin wrote:
>>>>>> From: Baolu Lu<baolu.lu@linux.intel.com>
>>>>>> Sent: Tuesday, June 21, 2022 11:39 AM
>>>>>>
>>>>>> On 2022/6/21 10:54, Tian, Kevin wrote:
>>>>>>>> From: Lu Baolu<baolu.lu@linux.intel.com>
>>>>>>>> Sent: Monday, June 20, 2022 4:17 PM
>>>>>>>> @@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct
>>>>>>>> dmar_domain *domain, struct device *dev)
>>>>>>>>      			ret = intel_pasid_setup_second_level(iommu,
>>>>>>>> domain,
>>>>>>>>      					dev, PASID_RID2PASID);
>>>>>>>>      		spin_unlock_irqrestore(&iommu->lock, flags);
>>>>>>>> -		if (ret) {
>>>>>>>> +		if (ret && ret != -EBUSY) {
>>>>>>>>      			dev_err(dev, "Setup RID2PASID failed\n");
>>>>>>>>      			dmar_remove_one_dev_info(dev);
>>>>>>>>      			return ret;
>>>>>>>> --
>>>>>>>> 2.25.1
>>>>>>> It's cleaner to avoid this error at the first place, i.e. only do the
>>>>>>> setup when the first device is attached to the pasid table.
>>>>>> The logic that identifies the first device might introduce additional
>>>>>> unnecessary complexity. Devices that share a pasid table are rare. I
>>>>>> even prefer to give up sharing tables so that the code can be
>>>>>> simpler.:-)
>>>>>>
>>>>> It's not that complex if you simply move device_attach_pasid_table()
>>>>> out of intel_pasid_alloc_table(). Then do the setup if
>>>>> list_empty(&pasid_table->dev) and then attach device to the
>>>>> pasid table in domain_add_dev_info().
>>>> The pasid table is part of the device, hence a better place to
>>>> allocate/free the pasid table is in the device probe/release paths.
>>>> Things will become more complicated if we change relationship between
>>>> device and it's pasid table when attaching/detaching a domain. That's
>>>> the reason why I thought it was additional complexity.
>>>>
>>> If you do want to follow current route it’s still cleaner to check
>>> whether the pasid entry has pointed to the domain in the individual
>>> setup function instead of blindly returning -EBUSY and then ignoring
>>> it even if a real busy condition occurs. The setup functions can
>>> just return zero for this benign alias case.
>> Kevin, how do you like this one?
>>
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>> index cb4c1d0cf25c..ecffd0129b2b 100644
>> --- a/drivers/iommu/intel/pasid.c
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -575,6 +575,16 @@ 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.
>> + */
>> +static inline bool
>> +rid2pasid_domain_valid(struct pasid_entry *pte, u32 pasid, u16 did)
>> +{
>> +	return pasid == PASID_RID2PASID && pasid_get_domain_id(pte) ==
>> did;
>> +}
> better this is not restricted to RID2PASID only, e.g. pasid_pte_match_domain()
> and then read pasid from the pte to compare with the pasid argument.
> 

The pasid value is not encoded in the pasid table entry. This validity
check is only for RID2PASID as alias devices share the single RID2PASID
entry. For other cases, we should always return -EBUSY as what the code
is doing now.

Best regards,
baolu

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

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, June 22, 2022 11:28 AM
> 
> On 2022/6/22 11:06, Tian, Kevin wrote:
> >> From: Baolu Lu<baolu.lu@linux.intel.com>
> >> Sent: Tuesday, June 21, 2022 5:04 PM
> >>
> >> On 2022/6/21 13:48, Tian, Kevin wrote:
> >>>> From: Baolu Lu<baolu.lu@linux.intel.com>
> >>>> Sent: Tuesday, June 21, 2022 12:28 PM
> >>>>
> >>>> On 2022/6/21 11:46, Tian, Kevin wrote:
> >>>>>> From: Baolu Lu<baolu.lu@linux.intel.com>
> >>>>>> Sent: Tuesday, June 21, 2022 11:39 AM
> >>>>>>
> >>>>>> On 2022/6/21 10:54, Tian, Kevin wrote:
> >>>>>>>> From: Lu Baolu<baolu.lu@linux.intel.com>
> >>>>>>>> Sent: Monday, June 20, 2022 4:17 PM
> >>>>>>>> @@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct
> >>>>>>>> dmar_domain *domain, struct device *dev)
> >>>>>>>>      			ret = intel_pasid_setup_second_level(iommu,
> >>>>>>>> domain,
> >>>>>>>>      					dev, PASID_RID2PASID);
> >>>>>>>>      		spin_unlock_irqrestore(&iommu->lock, flags);
> >>>>>>>> -		if (ret) {
> >>>>>>>> +		if (ret && ret != -EBUSY) {
> >>>>>>>>      			dev_err(dev, "Setup RID2PASID failed\n");
> >>>>>>>>      			dmar_remove_one_dev_info(dev);
> >>>>>>>>      			return ret;
> >>>>>>>> --
> >>>>>>>> 2.25.1
> >>>>>>> It's cleaner to avoid this error at the first place, i.e. only do the
> >>>>>>> setup when the first device is attached to the pasid table.
> >>>>>> The logic that identifies the first device might introduce additional
> >>>>>> unnecessary complexity. Devices that share a pasid table are rare. I
> >>>>>> even prefer to give up sharing tables so that the code can be
> >>>>>> simpler.:-)
> >>>>>>
> >>>>> It's not that complex if you simply move device_attach_pasid_table()
> >>>>> out of intel_pasid_alloc_table(). Then do the setup if
> >>>>> list_empty(&pasid_table->dev) and then attach device to the
> >>>>> pasid table in domain_add_dev_info().
> >>>> The pasid table is part of the device, hence a better place to
> >>>> allocate/free the pasid table is in the device probe/release paths.
> >>>> Things will become more complicated if we change relationship
> between
> >>>> device and it's pasid table when attaching/detaching a domain. That's
> >>>> the reason why I thought it was additional complexity.
> >>>>
> >>> If you do want to follow current route it’s still cleaner to check
> >>> whether the pasid entry has pointed to the domain in the individual
> >>> setup function instead of blindly returning -EBUSY and then ignoring
> >>> it even if a real busy condition occurs. The setup functions can
> >>> just return zero for this benign alias case.
> >> Kevin, how do you like this one?
> >>
> >> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> >> index cb4c1d0cf25c..ecffd0129b2b 100644
> >> --- a/drivers/iommu/intel/pasid.c
> >> +++ b/drivers/iommu/intel/pasid.c
> >> @@ -575,6 +575,16 @@ 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.
> >> + */
> >> +static inline bool
> >> +rid2pasid_domain_valid(struct pasid_entry *pte, u32 pasid, u16 did)
> >> +{
> >> +	return pasid == PASID_RID2PASID && pasid_get_domain_id(pte) ==
> >> did;
> >> +}
> > better this is not restricted to RID2PASID only, e.g.
> pasid_pte_match_domain()
> > and then read pasid from the pte to compare with the pasid argument.
> >
> 
> The pasid value is not encoded in the pasid table entry. This validity
> check is only for RID2PASID as alias devices share the single RID2PASID
> entry. For other cases, we should always return -EBUSY as what the code
> is doing now.
> 

You are right.

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

[Baolu Lu]

On 2022/6/22 11:31, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Wednesday, June 22, 2022 11:28 AM
>>
>> On 2022/6/22 11:06, Tian, Kevin wrote:
>>>> From: Baolu Lu<baolu.lu@linux.intel.com>
>>>> Sent: Tuesday, June 21, 2022 5:04 PM
>>>>
>>>> On 2022/6/21 13:48, Tian, Kevin wrote:
>>>>>> From: Baolu Lu<baolu.lu@linux.intel.com>
>>>>>> Sent: Tuesday, June 21, 2022 12:28 PM
>>>>>>
>>>>>> On 2022/6/21 11:46, Tian, Kevin wrote:
>>>>>>>> From: Baolu Lu<baolu.lu@linux.intel.com>
>>>>>>>> Sent: Tuesday, June 21, 2022 11:39 AM
>>>>>>>>
>>>>>>>> On 2022/6/21 10:54, Tian, Kevin wrote:
>>>>>>>>>> From: Lu Baolu<baolu.lu@linux.intel.com>
>>>>>>>>>> Sent: Monday, June 20, 2022 4:17 PM
>>>>>>>>>> @@ -2564,7 +2564,7 @@ static int domain_add_dev_info(struct
>>>>>>>>>> dmar_domain *domain, struct device *dev)
>>>>>>>>>>       			ret = intel_pasid_setup_second_level(iommu,
>>>>>>>>>> domain,
>>>>>>>>>>       					dev, PASID_RID2PASID);
>>>>>>>>>>       		spin_unlock_irqrestore(&iommu->lock, flags);
>>>>>>>>>> -		if (ret) {
>>>>>>>>>> +		if (ret && ret != -EBUSY) {
>>>>>>>>>>       			dev_err(dev, "Setup RID2PASID failed\n");
>>>>>>>>>>       			dmar_remove_one_dev_info(dev);
>>>>>>>>>>       			return ret;
>>>>>>>>>> --
>>>>>>>>>> 2.25.1
>>>>>>>>> It's cleaner to avoid this error at the first place, i.e. only do the
>>>>>>>>> setup when the first device is attached to the pasid table.
>>>>>>>> The logic that identifies the first device might introduce additional
>>>>>>>> unnecessary complexity. Devices that share a pasid table are rare. I
>>>>>>>> even prefer to give up sharing tables so that the code can be
>>>>>>>> simpler.:-)
>>>>>>>>
>>>>>>> It's not that complex if you simply move device_attach_pasid_table()
>>>>>>> out of intel_pasid_alloc_table(). Then do the setup if
>>>>>>> list_empty(&pasid_table->dev) and then attach device to the
>>>>>>> pasid table in domain_add_dev_info().
>>>>>> The pasid table is part of the device, hence a better place to
>>>>>> allocate/free the pasid table is in the device probe/release paths.
>>>>>> Things will become more complicated if we change relationship
>> between
>>>>>> device and it's pasid table when attaching/detaching a domain. That's
>>>>>> the reason why I thought it was additional complexity.
>>>>>>
>>>>> If you do want to follow current route it’s still cleaner to check
>>>>> whether the pasid entry has pointed to the domain in the individual
>>>>> setup function instead of blindly returning -EBUSY and then ignoring
>>>>> it even if a real busy condition occurs. The setup functions can
>>>>> just return zero for this benign alias case.
>>>> Kevin, how do you like this one?
>>>>
>>>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>>>> index cb4c1d0cf25c..ecffd0129b2b 100644
>>>> --- a/drivers/iommu/intel/pasid.c
>>>> +++ b/drivers/iommu/intel/pasid.c
>>>> @@ -575,6 +575,16 @@ 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.
>>>> + */
>>>> +static inline bool
>>>> +rid2pasid_domain_valid(struct pasid_entry *pte, u32 pasid, u16 did)
>>>> +{
>>>> +	return pasid == PASID_RID2PASID && pasid_get_domain_id(pte) ==
>>>> did;
>>>> +}
>>> better this is not restricted to RID2PASID only, e.g.
>> pasid_pte_match_domain()
>>> and then read pasid from the pte to compare with the pasid argument.
>>>
>>
>> The pasid value is not encoded in the pasid table entry. This validity
>> check is only for RID2PASID as alias devices share the single RID2PASID
>> entry. For other cases, we should always return -EBUSY as what the code
>> is doing now.
>>
> 
> You are right.

Very appreciated for your input. I will update it with a v2.

Best regards,
baolu

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

[Baolu Lu]

On 2022/6/22 10:56, Ethan Zhao wrote:
> 在 2022/6/20 16:17, 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 marke 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.
>      We have two customers reported the issue "DMAR: Setup RID2PASID 
> failed",
> 
> Two ASPEED devices locate behind one PCIe-PCI bridge and iommu SM, PT 
> mode is enabled.  Most
> 
> Interesting thing is the second device is only used by BIOS, and BIOS 
> left it to OS without shutting down,
> 
> and it is useless for OS.

This sounds odd. Isn't this a bug?


> Is there practical case multi devices behind 
> PCIe-PCI bridge share the same
> 
> PASID entry without any security concern ? these two customer's case is 
> not.

The devices underneath the PCIe-PCI bridge are alias devices of the
bridge. PCI alias devices always sit in the same group (the minimal unit
that IOMMU guarantees isolation) and can only be attached with a same
domain (managed I/O address space). Hence, there's no security concern
if they further share the pasid table.

Best regards,
baolu