[v2,1/1] x86/cpufeatures: Implement Predictive Store Forwarding control.
diff mbox series

Message ID 20210422171013.50207-2-rsaripal@amd.com
State New, archived
Headers show
Series
  • Introduce support for PSF control.
Related show

Commit Message

Saripalli, RK April 22, 2021, 5:10 p.m. UTC
From: Ramakrishna Saripalli <rk.saripalli@amd.com>

Certain AMD processors feature a new technology called Predictive Store
Forwarding (PSF).

PSF is a micro-architectural optimization designed to improve the
performance of code execution by predicting dependencies between
loads and stores.

Incorrect PSF predictions can occur due to two reasons.

- It is possible that the load/store pair may have had dependency for
  a while but the dependency has stopped because the address in the
  load/store pair has changed.

- Second source of incorrect PSF prediction can occur because of an alias
  in the PSF predictor structure stored in the microarchitectural state.
  PSF predictor tracks load/store pair based on portions of instruction
  pointer. It is possible that a load/store pair which does have a
  dependency may be aliased by another load/store pair which does not have
  the same dependency. This can result in incorrect speculation.

  Software may be able to detect this aliasing and perform side-channel
  attacks.

All CPUs that implement PSF provide one bit to disable this feature.
If the bit to disable this feature is available, it means that the CPU
implements PSF feature and is therefore vulnerable to PSF risks.

The bits that are introduced

X86_FEATURE_PSFD: CPUID_Fn80000008_EBX[28] ("PSF disable")
	If this bit is 1, CPU implements PSF and PSF control
	via SPEC_CTRL_MSR is supported in the CPU.

All AMD processors that support PSF implement a bit in
SPEC_CTRL MSR (0x48) to disable or enable Predictive Store
Forwarding.

PSF control introduces a new kernel parameter called
	predict_store_fwd.

Kernel parameter predict_store_fwd has the following values

- off. This value is used to disable PSF on all CPUs.

- on. This value is used to enable PSF on all CPUs.
        This is also the default setting.

ChangeLogs:
    V1->V2:
	- Smashed multiple commits into one commit.
	- Rename PSF to a control instead of mitigation.

    V1:
	- Initial patchset.
	- Kernel parameter controls enable and disable of PSF.
====================
Signed-off-by: Ramakrishna Saripalli<rk.saripalli@amd.com>
---
 .../admin-guide/kernel-parameters.txt         |  5 +++++
 arch/x86/include/asm/cpufeatures.h            |  1 +
 arch/x86/include/asm/msr-index.h              |  2 ++
 arch/x86/kernel/cpu/amd.c                     | 19 +++++++++++++++++++
 4 files changed, 27 insertions(+)

Comments

Randy Dunlap April 22, 2021, 5:49 p.m. UTC | #1
On 4/22/21 10:10 AM, Ramakrishna Saripalli wrote:
> From: Ramakrishna Saripalli <rk.saripalli@amd.com>
> 
> ====================
> Signed-off-by: Ramakrishna Saripalli<rk.saripalli@amd.com>
> ---
>  .../admin-guide/kernel-parameters.txt         |  5 +++++
>  arch/x86/include/asm/cpufeatures.h            |  1 +
>  arch/x86/include/asm/msr-index.h              |  2 ++
>  arch/x86/kernel/cpu/amd.c                     | 19 +++++++++++++++++++
>  4 files changed, 27 insertions(+)

as from v1:

> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 04545725f187..58f6bd02385b 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3940,6 +3940,11 @@
>  			Format: {"off"}
>  			Disable Hardware Transactional Memory
>  
> +	predict_store_fwd	[X86] This option controls PSF mitigation

	predict_store_fwd=	...

> +			off - Turns on PSF mitigation.
> +			on  - Turns off PSF mitigation.
> +			default : on.

			default: on.

> +
>  	preempt=	[KNL]
>  			Select preemption mode if you have CONFIG_PREEMPT_DYNAMIC
>  			none - Limited to cond_resched() calls
thanks.
Saripalli, RK April 22, 2021, 7:32 p.m. UTC | #2
On 4/22/2021 12:49 PM, Randy Dunlap wrote:
> On 4/22/21 10:10 AM, Ramakrishna Saripalli wrote:
>> From: Ramakrishna Saripalli <rk.saripalli@amd.com>
>>
>> ====================
>> Signed-off-by: Ramakrishna Saripalli<rk.saripalli@amd.com>
>> ---
>>  .../admin-guide/kernel-parameters.txt         |  5 +++++
>>  arch/x86/include/asm/cpufeatures.h            |  1 +
>>  arch/x86/include/asm/msr-index.h              |  2 ++
>>  arch/x86/kernel/cpu/amd.c                     | 19 +++++++++++++++++++
>>  4 files changed, 27 insertions(+)
> 
> as from v1:

Randy, could you clarify your comments please?. Is there something here I need to change/clarify/fix?

> 
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 04545725f187..58f6bd02385b 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -3940,6 +3940,11 @@
>>  			Format: {"off"}
>>  			Disable Hardware Transactional Memory
>>  
>> +	predict_store_fwd	[X86] This option controls PSF mitigation
> 
> 	predict_store_fwd=	...
> 

OK

>> +			off - Turns on PSF mitigation.
>> +			on  - Turns off PSF mitigation.
>> +			default : on.
> 
> 			default: on.
> 

OK

>> +
>>  	preempt=	[KNL]
>>  			Select preemption mode if you have CONFIG_PREEMPT_DYNAMIC
>>  			none - Limited to cond_resched() calls
> thanks.
>
Randy Dunlap April 22, 2021, 8:46 p.m. UTC | #3
On 4/22/21 12:32 PM, Saripalli, RK wrote:
> 
> 
> On 4/22/2021 12:49 PM, Randy Dunlap wrote:
>> On 4/22/21 10:10 AM, Ramakrishna Saripalli wrote:
>>> From: Ramakrishna Saripalli <rk.saripalli@amd.com>
>>>
>>> ====================
>>> Signed-off-by: Ramakrishna Saripalli<rk.saripalli@amd.com>
>>> ---
>>>  .../admin-guide/kernel-parameters.txt         |  5 +++++
>>>  arch/x86/include/asm/cpufeatures.h            |  1 +
>>>  arch/x86/include/asm/msr-index.h              |  2 ++
>>>  arch/x86/kernel/cpu/amd.c                     | 19 +++++++++++++++++++
>>>  4 files changed, 27 insertions(+)
>>
>> as from v1:
> 
> Randy, could you clarify your comments please?. Is there something here I need to change/clarify/fix?
> 

Only that I had made these same comments (below)
in v1 of the patch.

https://lore.kernel.org/lkml/4c688fc7-67df-3187-54b2-bf20e510fb39@infradead.org/

>>
>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>>> index 04545725f187..58f6bd02385b 100644
>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>> @@ -3940,6 +3940,11 @@
>>>  			Format: {"off"}
>>>  			Disable Hardware Transactional Memory
>>>  
>>> +	predict_store_fwd	[X86] This option controls PSF mitigation
>>
>> 	predict_store_fwd=	...
>>
> 
> OK
> 
>>> +			off - Turns on PSF mitigation.
>>> +			on  - Turns off PSF mitigation.
>>> +			default : on.
>>
>> 			default: on.
>>
> 
> OK
> 
>>> +
>>>  	preempt=	[KNL]
>>>  			Select preemption mode if you have CONFIG_PREEMPT_DYNAMIC
>>>  			none - Limited to cond_resched() calls
Saripalli, RK April 22, 2021, 9:23 p.m. UTC | #4
On 4/22/2021 3:46 PM, Randy Dunlap wrote:
> On 4/22/21 12:32 PM, Saripalli, RK wrote:
>>
>>
>> On 4/22/2021 12:49 PM, Randy Dunlap wrote:
>>> On 4/22/21 10:10 AM, Ramakrishna Saripalli wrote:
>>>> From: Ramakrishna Saripalli <rk.saripalli@amd.com>
>>>>
>>>> ====================
>>>> Signed-off-by: Ramakrishna Saripalli<rk.saripalli@amd.com>
>>>> ---
>>>>  .../admin-guide/kernel-parameters.txt         |  5 +++++
>>>>  arch/x86/include/asm/cpufeatures.h            |  1 +
>>>>  arch/x86/include/asm/msr-index.h              |  2 ++
>>>>  arch/x86/kernel/cpu/amd.c                     | 19 +++++++++++++++++++
>>>>  4 files changed, 27 insertions(+)
>>>
>>> as from v1:
>>
>> Randy, could you clarify your comments please?. Is there something here I need to change/clarify/fix?
>>
> 
> Only that I had made these same comments (below)
> in v1 of the patch.

Yes I see them. I missed them. I will include them in the next patch release (batch them with other changes)
> 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F4c688fc7-67df-3187-54b2-bf20e510fb39%40infradead.org%2F&amp;data=04%7C01%7Crk.saripalli%40amd.com%7Cfd08fbf7545d4c27c4cd08d905cfde95%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637547212629713144%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=hlGLgbZobkqXf3aBdMhSFC6Lg8nBLg1ssjHJDoyzI9s%3D&amp;reserved=0
> 
>>>
>>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>>>> index 04545725f187..58f6bd02385b 100644
>>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>>> @@ -3940,6 +3940,11 @@
>>>>  			Format: {"off"}
>>>>  			Disable Hardware Transactional Memory
>>>>  
>>>> +	predict_store_fwd	[X86] This option controls PSF mitigation
>>>
>>> 	predict_store_fwd=	...
>>>
>>
>> OK
>>
>>>> +			off - Turns on PSF mitigation.
>>>> +			on  - Turns off PSF mitigation.
>>>> +			default : on.
>>>
>>> 			default: on.
>>>
>>
>> OK
>>
>>>> +
>>>>  	preempt=	[KNL]
>>>>  			Select preemption mode if you have CONFIG_PREEMPT_DYNAMIC
>>>>  			none - Limited to cond_resched() calls
> 

thanks,
RK
Reiji Watanabe April 26, 2021, 4:28 p.m. UTC | #5
> --- a/arch/x86/kernel/cpu/amd.c
> +++ b/arch/x86/kernel/cpu/amd.c
> @@ -1170,3 +1170,22 @@ void set_dr_addr_mask(unsigned long mask, int dr)
>                 break;
>         }
>  }
> +
> +static int __init psf_cmdline(char *str)
> +{
> +       if (!boot_cpu_has(X86_FEATURE_PSFD))
> +               return 0;
> +
> +       if (!str)
> +               return -EINVAL;
> +
> +       if (!strcmp(str, "off")) {
> +               x86_spec_ctrl_base |= SPEC_CTRL_PSFD;
> +               wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
> +               setup_clear_cpu_cap(X86_FEATURE_PSFD);

Shouldn't X86_FEATURE_MSR_SPEC_CTRL be set if the CPU supports PSF ?
x86_spec_ctrl_setup_ap(), which is called on non-boot CPUs, doesn't
update MSR_IA32_SPEC_CTRL with x86_spec_ctrl_base not having
X86_FEATURE_MSR_SPEC_CTRL (i.e. if a CPU supports PSF but no other
existing feature that makes the kernel set X86_FEATURE_MSR_SPEC_CTRL).

Also, since check_bugs() reads the SPEC_CTRL MSR to account for reserved
bits which may have unknown bits to set x86_spec_ctrl_base
(if X86_FEATURE_MSR_SPEC_CTRL is set),
I'm wondering if psf_cmdline(), which is called earlier
than check_bugs(), should do the same instead of overwriting
it with x86_spec_ctrl_base | SPEC_CTRL_PSFD.


Thanks,
Reiji
Saripalli, RK April 26, 2021, 4:40 p.m. UTC | #6
On 4/26/2021 11:28 AM, Reiji Watanabe wrote:
>> --- a/arch/x86/kernel/cpu/amd.c
>> +++ b/arch/x86/kernel/cpu/amd.c
>> @@ -1170,3 +1170,22 @@ void set_dr_addr_mask(unsigned long mask, int dr)
>>                 break;
>>         }
>>  }
>> +
>> +static int __init psf_cmdline(char *str)
>> +{
>> +       if (!boot_cpu_has(X86_FEATURE_PSFD))
>> +               return 0;
>> +
>> +       if (!str)
>> +               return -EINVAL;
>> +
>> +       if (!strcmp(str, "off")) {
>> +               x86_spec_ctrl_base |= SPEC_CTRL_PSFD;
>> +               wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
>> +               setup_clear_cpu_cap(X86_FEATURE_PSFD);
> 
> Shouldn't X86_FEATURE_MSR_SPEC_CTRL be set if the CPU supports PSF ?
> x86_spec_ctrl_setup_ap(), which is called on non-boot CPUs, doesn't
> update MSR_IA32_SPEC_CTRL with x86_spec_ctrl_base not having
> X86_FEATURE_MSR_SPEC_CTRL (i.e. if a CPU supports PSF but no other
> existing feature that makes the kernel set X86_FEATURE_MSR_SPEC_CTRL).

Reiji, that is a good catch. 
This patch unconditionally sets to bit 7 even on machines where bit 7 is undefined thereby risking a kernel oops.
I will fix this.

I did test this code on a Milan machine and verified that with the setting to off, the SPEC_CTRL MSR was showing bit 7 set to 1 and with on setting SPEC_CTRL MSR bit 7 was cleared.
I tested this with both spec_ctrl_bypass_disable kernel parameter and without. 
I verified with {sudo modprobe msr; rdmsr -a 72 which dumps on all CPUs.
But I may not have tested this patch on a non-Milan machine with both settings.
I will make sure to test it on non-Milan machines before sending the next version out.

> 
> Also, since check_bugs() reads the SPEC_CTRL MSR to account for reserved
> bits which may have unknown bits to set x86_spec_ctrl_base
> (if X86_FEATURE_MSR_SPEC_CTRL is set),
> I'm wondering if psf_cmdline(), which is called earlier
> than check_bugs(), should do the same instead of overwriting
> it with x86_spec_ctrl_base | SPEC_CTRL_PSFD.
> 
Ok. I did not think of that path. I will go through that code and fix up the patch.
> 
> Thanks,
> Reiji
> 

Thanks,
RK

Patch
diff mbox series

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 04545725f187..58f6bd02385b 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3940,6 +3940,11 @@ 
 			Format: {"off"}
 			Disable Hardware Transactional Memory
 
+	predict_store_fwd	[X86] This option controls PSF mitigation
+			off - Turns on PSF mitigation.
+			on  - Turns off PSF mitigation.
+			default : on.
+
 	preempt=	[KNL]
 			Select preemption mode if you have CONFIG_PREEMPT_DYNAMIC
 			none - Limited to cond_resched() calls
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index cc96e26d69f7..078f46022293 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -309,6 +309,7 @@ 
 #define X86_FEATURE_AMD_SSBD		(13*32+24) /* "" Speculative Store Bypass Disable */
 #define X86_FEATURE_VIRT_SSBD		(13*32+25) /* Virtualized Speculative Store Bypass Disable */
 #define X86_FEATURE_AMD_SSB_NO		(13*32+26) /* "" Speculative Store Bypass is fixed in hardware. */
+#define X86_FEATURE_PSFD		(13*32+28) /* Predictive Store Forward Disable */
 
 /* Thermal and Power Management Leaf, CPUID level 0x00000006 (EAX), word 14 */
 #define X86_FEATURE_DTHERM		(14*32+ 0) /* Digital Thermal Sensor */
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 546d6ecf0a35..f569918c8754 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -51,6 +51,8 @@ 
 #define SPEC_CTRL_STIBP			BIT(SPEC_CTRL_STIBP_SHIFT)	/* STIBP mask */
 #define SPEC_CTRL_SSBD_SHIFT		2	   /* Speculative Store Bypass Disable bit */
 #define SPEC_CTRL_SSBD			BIT(SPEC_CTRL_SSBD_SHIFT)	/* Speculative Store Bypass Disable */
+#define SPEC_CTRL_PSFD_SHIFT		7
+#define SPEC_CTRL_PSFD			BIT(SPEC_CTRL_PSFD_SHIFT)	/* Predictive Store Forwarding Disable */
 
 #define MSR_IA32_PRED_CMD		0x00000049 /* Prediction Command */
 #define PRED_CMD_IBPB			BIT(0)	   /* Indirect Branch Prediction Barrier */
diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c
index 347a956f71ca..88aac52eeb1b 100644
--- a/arch/x86/kernel/cpu/amd.c
+++ b/arch/x86/kernel/cpu/amd.c
@@ -1170,3 +1170,22 @@  void set_dr_addr_mask(unsigned long mask, int dr)
 		break;
 	}
 }
+
+static int __init psf_cmdline(char *str)
+{
+	if (!boot_cpu_has(X86_FEATURE_PSFD))
+		return 0;
+
+	if (!str)
+		return -EINVAL;
+
+	if (!strcmp(str, "off")) {
+		x86_spec_ctrl_base |= SPEC_CTRL_PSFD;
+		wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
+		setup_clear_cpu_cap(X86_FEATURE_PSFD);
+	}
+
+	return 0;
+}
+
+early_param("predict_store_fwd", psf_cmdline);