[PATCH] iommu/vt-d: Set SRE bit when hardware has SRS cap

[PATCH] iommu/vt-d: Set SRE bit when hardware has SRS cap

[Tina Zhang]

SRS cap is the hardware cap telling if the hardware IOMMU can support
requests seeking supervisor privilege or not. SRE bit in scalable-mode
PASID table entry is treated as Reserved(0) for implementation not
supporting SRS cap.

Checking SRS cap before setting SRE bit can avoid the non-recoverable
fault of "Non-zero reserved field set in PASID Table Entry" caused by
setting SRE bit while there is no SRS cap support.

Currently, both intel_pasid_setup_first_level() and
intel_pasid_setup_second_level() implicitly take care about of it. The
only missing SRS cap checking is in intel_pasid_setup_pass_through().

Fixes: 6f7db75e1c46 ("iommu/vt-d: Add second level page table interface")
Signed-off-by: Tina Zhang <tina.zhang@intel.com>
---
 drivers/iommu/intel/pasid.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index c30ddac40ee5..4a59035a5181 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -685,7 +685,8 @@ int intel_pasid_setup_pass_through(struct intel_iommu *iommu,
 	 * We should set SRE bit as well since the addresses are expected
 	 * to be GPAs.
 	 */
-	pasid_set_sre(pte);
+	if (ecap_srs(iommu->ecap))
+		pasid_set_sre(pte);
 	pasid_set_present(pte);
 	spin_unlock(&iommu->lock);
 
-- 
2.25.1

Re: [PATCH] iommu/vt-d: Set SRE bit when hardware has SRS cap

[Baolu Lu]

On 2022/11/14 16:55, Tina Zhang wrote:
> SRS cap is the hardware cap telling if the hardware IOMMU can support
> requests seeking supervisor privilege or not. SRE bit in scalable-mode
> PASID table entry is treated as Reserved(0) for implementation not
> supporting SRS cap.
> 
> Checking SRS cap before setting SRE bit can avoid the non-recoverable
> fault of "Non-zero reserved field set in PASID Table Entry" caused by
> setting SRE bit while there is no SRS cap support.
> 
> Currently, both intel_pasid_setup_first_level() and
> intel_pasid_setup_second_level() implicitly take care about of it. The
> only missing SRS cap checking is in intel_pasid_setup_pass_through().
> 
> Fixes: 6f7db75e1c46 ("iommu/vt-d: Add second level page table interface")
> Signed-off-by: Tina Zhang<tina.zhang@intel.com>

Nice catch! Tina.

Can you please double check  intel_pasid_setup_second_level()? Its seems
that we missed such check there as well.

Best regards,
baolu

Re: [PATCH] iommu/vt-d: Set SRE bit when hardware has SRS cap

[tina.zhang]

On 11/14/22 20:00, Baolu Lu wrote:
> On 2022/11/14 16:55, Tina Zhang wrote:
>> SRS cap is the hardware cap telling if the hardware IOMMU can support
>> requests seeking supervisor privilege or not. SRE bit in scalable-mode
>> PASID table entry is treated as Reserved(0) for implementation not
>> supporting SRS cap.
>>
>> Checking SRS cap before setting SRE bit can avoid the non-recoverable
>> fault of "Non-zero reserved field set in PASID Table Entry" caused by
>> setting SRE bit while there is no SRS cap support.
>>
>> Currently, both intel_pasid_setup_first_level() and
>> intel_pasid_setup_second_level() implicitly take care about of it. The
>> only missing SRS cap checking is in intel_pasid_setup_pass_through().
>>
>> Fixes: 6f7db75e1c46 ("iommu/vt-d: Add second level page table interface")
>> Signed-off-by: Tina Zhang<tina.zhang@intel.com>
> 
> Nice catch! Tina.
> 
> Can you please double check  intel_pasid_setup_second_level()? Its seems
> that we missed such check there as well.
Hi Baolu,

Yes, I thought about it.

In current kernel code, the intel_pasid_setup_second_level() is called 
only in one place and value of the pasid parameter is PASID_RID2PASID. 
In the body of intel_pasid_setup_second_level, the pasid_set_sre() won't 
be called if pasid == PASID_RID2PASID, see:

intel_pasid_setup_second_level(..., pasid) {
	...
	if (pasid != PASID_RID2PASID)
		pasid_set_sre(pte);
	...

}

IIUC, in the current kernel code we don't have the case which can test 
pasid_set_sre(pte) for second_level PASID_ENTRY_PGTT_SL_ONLY. But in 
feature, if we add more points to invoke 
intel_pasid_setup_second_level(), we may need to checkout this SRS cap.

So, do you think we should include the checking in 
intel_pasid_setup_second_level() as well in this patch?

Regards,
-Tina

> 
> Best regards,
> baolu

Re: [PATCH] iommu/vt-d: Set SRE bit when hardware has SRS cap

[Baolu Lu]

On 11/15/22 7:55 AM, tina.zhang wrote:
> 
> On 11/14/22 20:00, Baolu Lu wrote:
>> On 2022/11/14 16:55, Tina Zhang wrote:
>>> SRS cap is the hardware cap telling if the hardware IOMMU can support
>>> requests seeking supervisor privilege or not. SRE bit in scalable-mode
>>> PASID table entry is treated as Reserved(0) for implementation not
>>> supporting SRS cap.
>>>
>>> Checking SRS cap before setting SRE bit can avoid the non-recoverable
>>> fault of "Non-zero reserved field set in PASID Table Entry" caused by
>>> setting SRE bit while there is no SRS cap support.
>>>
>>> Currently, both intel_pasid_setup_first_level() and
>>> intel_pasid_setup_second_level() implicitly take care about of it. The
>>> only missing SRS cap checking is in intel_pasid_setup_pass_through().
>>>
>>> Fixes: 6f7db75e1c46 ("iommu/vt-d: Add second level page table 
>>> interface")
>>> Signed-off-by: Tina Zhang<tina.zhang@intel.com>
>>
>> Nice catch! Tina.
>>
>> Can you please double check  intel_pasid_setup_second_level()? Its seems
>> that we missed such check there as well.
> Hi Baolu,
> 
> Yes, I thought about it.
> 
> In current kernel code, the intel_pasid_setup_second_level() is called 
> only in one place and value of the pasid parameter is PASID_RID2PASID. 
> In the body of intel_pasid_setup_second_level, the pasid_set_sre() won't 
> be called if pasid == PASID_RID2PASID, see:
> 
> intel_pasid_setup_second_level(..., pasid) {
>      ...
>      if (pasid != PASID_RID2PASID)
>          pasid_set_sre(pte);
>      ...
> 
> }
> 
> IIUC, in the current kernel code we don't have the case which can test 
> pasid_set_sre(pte) for second_level PASID_ENTRY_PGTT_SL_ONLY. But in 
> feature, if we add more points to invoke 
> intel_pasid_setup_second_level(), we may need to checkout this SRS cap.
> 
> So, do you think we should include the checking in 
> intel_pasid_setup_second_level() as well in this patch?

I guess the purpose of this patch is to make sure that software never
sets the pasid entry fields that are treated by the hardware as
Reserved(0). If so, let's make it explicit in all places. :-)

Best regards,
baolu

Re: [PATCH] iommu/vt-d: Set SRE bit when hardware has SRS cap

[tina.zhang]

On 11/15/22 09:40, Baolu Lu wrote:
> On 11/15/22 7:55 AM, tina.zhang wrote:
>>
>> On 11/14/22 20:00, Baolu Lu wrote:
>>> On 2022/11/14 16:55, Tina Zhang wrote:
>>>> SRS cap is the hardware cap telling if the hardware IOMMU can support
>>>> requests seeking supervisor privilege or not. SRE bit in scalable-mode
>>>> PASID table entry is treated as Reserved(0) for implementation not
>>>> supporting SRS cap.
>>>>
>>>> Checking SRS cap before setting SRE bit can avoid the non-recoverable
>>>> fault of "Non-zero reserved field set in PASID Table Entry" caused by
>>>> setting SRE bit while there is no SRS cap support.
>>>>
>>>> Currently, both intel_pasid_setup_first_level() and
>>>> intel_pasid_setup_second_level() implicitly take care about of it. The
>>>> only missing SRS cap checking is in intel_pasid_setup_pass_through().
>>>>
>>>> Fixes: 6f7db75e1c46 ("iommu/vt-d: Add second level page table 
>>>> interface")
>>>> Signed-off-by: Tina Zhang<tina.zhang@intel.com>
>>>
>>> Nice catch! Tina.
>>>
>>> Can you please double check  intel_pasid_setup_second_level()? Its seems
>>> that we missed such check there as well.
>> Hi Baolu,
>>
>> Yes, I thought about it.
>>
>> In current kernel code, the intel_pasid_setup_second_level() is called 
>> only in one place and value of the pasid parameter is PASID_RID2PASID. 
>> In the body of intel_pasid_setup_second_level, the pasid_set_sre() 
>> won't be called if pasid == PASID_RID2PASID, see:
>>
>> intel_pasid_setup_second_level(..., pasid) {
>>      ...
>>      if (pasid != PASID_RID2PASID)
>>          pasid_set_sre(pte);
>>      ...
>>
>> }
>>
>> IIUC, in the current kernel code we don't have the case which can test 
>> pasid_set_sre(pte) for second_level PASID_ENTRY_PGTT_SL_ONLY. But in 
>> feature, if we add more points to invoke 
>> intel_pasid_setup_second_level(), we may need to checkout this SRS cap.
>>
>> So, do you think we should include the checking in 
>> intel_pasid_setup_second_level() as well in this patch?
> 
> I guess the purpose of this patch is to make sure that software never
> sets the pasid entry fields that are treated by the hardware as
> Reserved(0). If so, let's make it explicit in all places. :-)
Sure.

Regards,
-Tina
> 
> Best regards,
> baolu