All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] x86/bugs: Explicitly clear IBRS MSR bit
@ 2022-11-18 18:21 Breno Leitao
  2022-11-18 19:46 ` Pawan Gupta
  0 siblings, 1 reply; 5+ messages in thread
From: Breno Leitao @ 2022-11-18 18:21 UTC (permalink / raw)
  To: tglx, mingo, bp, dave.hansen, hpa, jpoimboe, peterz
  Cc: x86, cascardo, leit, kexec

Currently x86_spec_ctrl_base is read at boot time, and SPEC_CTRL_IBRS
bit is set if CONFIG_CPU_IBRS_ENTRY is enabled. There is no change in
the bit if CONFIG_CPU_IBRS_ENTRY is not set.

This is a problem when kexec-ing a kernel that has the mitigation
disabled, from a kernel that has the mitigation enabled. In this case,
the MSR bit is carried forward and not cleared at the boot of the new
kernel. This might have some performance degradation that is hard to
find.

This problem does not happen if the machine is (hard) rebooted, because
the bit will be cleared by default.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 arch/x86/kernel/cpu/bugs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 3e3230cccaa7..5b59e850de6e 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -1451,6 +1451,9 @@ static void __init spectre_v2_select_mitigation(void)
 	if (spectre_v2_in_ibrs_mode(mode)) {
 		x86_spec_ctrl_base |= SPEC_CTRL_IBRS;
 		write_spec_ctrl_current(x86_spec_ctrl_base, true);
+	} else {
+		x86_spec_ctrl_base = x86_spec_ctrl_base & (~SPEC_CTRL_IBRS);
+		write_spec_ctrl_current(x86_spec_ctrl_base, true);
 	}
 
 	switch (mode) {
-- 
2.30.2


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [RFC PATCH] x86/bugs: Explicitly clear IBRS MSR bit
  2022-11-18 18:21 [RFC PATCH] x86/bugs: Explicitly clear IBRS MSR bit Breno Leitao
@ 2022-11-18 19:46 ` Pawan Gupta
  2022-11-20 12:02   ` [PATCH] x86/bugs: Explicitly clear speculative MSR bits Breno Leitao
  0 siblings, 1 reply; 5+ messages in thread
From: Pawan Gupta @ 2022-11-18 19:46 UTC (permalink / raw)
  To: Breno Leitao
  Cc: tglx, mingo, bp, dave.hansen, hpa, jpoimboe, peterz, x86,
	cascardo, leit, kexec

On Fri, Nov 18, 2022 at 10:21:10AM -0800, Breno Leitao wrote:
>Currently x86_spec_ctrl_base is read at boot time, and SPEC_CTRL_IBRS
>bit is set if CONFIG_CPU_IBRS_ENTRY is enabled. There is no change in
>the bit if CONFIG_CPU_IBRS_ENTRY is not set.
>
>This is a problem when kexec-ing a kernel that has the mitigation
>disabled, from a kernel that has the mitigation enabled. In this case,
>the MSR bit is carried forward and not cleared at the boot of the new
>kernel. This might have some performance degradation that is hard to
>find.
>
>This problem does not happen if the machine is (hard) rebooted, because
>the bit will be cleared by default.
>
>Signed-off-by: Breno Leitao <leitao@debian.org>
>---
> arch/x86/kernel/cpu/bugs.c | 3 +++
> 1 file changed, 3 insertions(+)
>
>diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
>index 3e3230cccaa7..5b59e850de6e 100644
>--- a/arch/x86/kernel/cpu/bugs.c
>+++ b/arch/x86/kernel/cpu/bugs.c
>@@ -1451,6 +1451,9 @@ static void __init spectre_v2_select_mitigation(void)
> 	if (spectre_v2_in_ibrs_mode(mode)) {
> 		x86_spec_ctrl_base |= SPEC_CTRL_IBRS;
> 		write_spec_ctrl_current(x86_spec_ctrl_base, true);
>+	} else {
>+		x86_spec_ctrl_base = x86_spec_ctrl_base & (~SPEC_CTRL_IBRS);
>+		write_spec_ctrl_current(x86_spec_ctrl_base, true);

Can we solve this problem in a more generic way by clearing all the
known bits before any mitigation selection is done:

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 5b59e850de6e..26c612792150 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -137,8 +137,15 @@ void __init check_bugs(void)
  	 * have unknown values. AMD64_LS_CFG MSR is cached in the early AMD
  	 * init code as it is not enumerated and depends on the family.
  	 */
-	if (boot_cpu_has(X86_FEATURE_MSR_SPEC_CTRL))
+	if (boot_cpu_has(X86_FEATURE_MSR_SPEC_CTRL)) {
  		rdmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
+		/*
+		 * Previously running software may have some controls turned ON.
+		 * Clear them and let kernel decide which controls to use.
+		 */
+		x86_spec_ctrl_base &= ~(SPEC_CTRL_IBRS | SPEC_CTRL_STIBP | SPEC_CTRL_SSBD | SPEC_CTRL_RRSBA_DIS_S);
+		wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
+	}
  
  	/* Select the proper CPU mitigations before patching alternatives: */
  	spectre_v1_select_mitigation();

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH] x86/bugs: Explicitly clear speculative MSR bits
  2022-11-18 19:46 ` Pawan Gupta
@ 2022-11-20 12:02   ` Breno Leitao
  2022-11-21 22:20     ` Pawan Gupta
  0 siblings, 1 reply; 5+ messages in thread
From: Breno Leitao @ 2022-11-20 12:02 UTC (permalink / raw)
  To: tglx, mingo, bp, dave.hansen, pawan.kumar.gupta
  Cc: leit, cascardo, hpa, jpoimboe, peterz, x86, kexec

Currently x86_spec_ctrl_base is read at boot time, and speculative bits
are set if configs are enable, such as MSR[SPEC_CTRL_IBRS] is enabled if
CONFIG_CPU_IBRS_ENTRY is configured. These MSR bits are not cleared if
the CONFIGs are not set.

This is a problem when kexec-ing a kernel that has the mitigation
disabled, from a kernel that has the mitigation enabled. In this case,
the MSR bits are carried forward and not cleared at the boot of the new
kernel. This might have some performance degradation that is hard to
find.

This problem does not happen if the machine is (hard) rebooted, because
the bit will be cleared by default.

This patch also defines a SPEC_CTRL_MASK macro, so, we can easily track
and clear if eventually some new mitigation show up.

Suggested-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 arch/x86/include/asm/msr-index.h |  3 +++
 arch/x86/kernel/cpu/bugs.c       | 10 +++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 10ac52705892..672de926281e 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -54,6 +54,9 @@
 #define SPEC_CTRL_RRSBA_DIS_S_SHIFT	6	   /* Disable RRSBA behavior */
 #define SPEC_CTRL_RRSBA_DIS_S		BIT(SPEC_CTRL_RRSBA_DIS_S_SHIFT)
 
+#define SPEC_CTRL_MASK			(SPEC_CTRL_IBRS | SPEC_CTRL_STIBP | SPEC_CTRL_SSBD \
+							| SPEC_CTRL_RRSBA_DIS_S)
+
 #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/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 3e3230cccaa7..970b277d02a6 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -137,8 +137,16 @@ void __init check_bugs(void)
 	 * have unknown values. AMD64_LS_CFG MSR is cached in the early AMD
 	 * init code as it is not enumerated and depends on the family.
 	 */
-	if (boot_cpu_has(X86_FEATURE_MSR_SPEC_CTRL))
+	if (boot_cpu_has(X86_FEATURE_MSR_SPEC_CTRL)) {
 		rdmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
+		/*
+		 * Previously running software may have some controls turned ON.
+		 * Clear them and let kernel decide which controls to use.
+		 */
+		x86_spec_ctrl_base &= ~SPEC_CTRL_MASK;
+		wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
+	}
+
 
 	/* Select the proper CPU mitigations before patching alternatives: */
 	spectre_v1_select_mitigation();
-- 
2.38.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] x86/bugs: Explicitly clear speculative MSR bits
  2022-11-20 12:02   ` [PATCH] x86/bugs: Explicitly clear speculative MSR bits Breno Leitao
@ 2022-11-21 22:20     ` Pawan Gupta
  2022-11-22 15:55       ` Breno Leitao
  0 siblings, 1 reply; 5+ messages in thread
From: Pawan Gupta @ 2022-11-21 22:20 UTC (permalink / raw)
  To: Breno Leitao
  Cc: tglx, mingo, bp, dave.hansen, leit, cascardo, hpa, jpoimboe,
	peterz, x86, kexec

On Sun, Nov 20, 2022 at 12:02:55PM +0000, Breno Leitao wrote:
>Currently x86_spec_ctrl_base is read at boot time, and speculative bits
>are set if configs are enable, such as MSR[SPEC_CTRL_IBRS] is enabled if
>CONFIG_CPU_IBRS_ENTRY is configured. These MSR bits are not cleared if
>the CONFIGs are not set.

Also when the CONFIGs are set but the mitigations are disabled at
runtime e.g. using mitigations=off parameter.

>This is a problem when kexec-ing a kernel that has the mitigation
>disabled, from a kernel that has the mitigation enabled. In this case,
>the MSR bits are carried forward and not cleared at the boot of the new
>kernel. This might have some performance degradation that is hard to
>find.
>
>This problem does not happen if the machine is (hard) rebooted, because
>the bit will be cleared by default.
>
>This patch also defines a SPEC_CTRL_MASK macro, so, we can easily track
>and clear if eventually some new mitigation show up.
>
>Suggested-by: Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
>Signed-off-by: Breno Leitao <leitao@debian.org>
>---
> arch/x86/include/asm/msr-index.h |  3 +++
> arch/x86/kernel/cpu/bugs.c       | 10 +++++++++-
> 2 files changed, 12 insertions(+), 1 deletion(-)
>
>diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
>index 10ac52705892..672de926281e 100644
>--- a/arch/x86/include/asm/msr-index.h
>+++ b/arch/x86/include/asm/msr-index.h
>@@ -54,6 +54,9 @@
> #define SPEC_CTRL_RRSBA_DIS_S_SHIFT	6	   /* Disable RRSBA behavior */
> #define SPEC_CTRL_RRSBA_DIS_S		BIT(SPEC_CTRL_RRSBA_DIS_S_SHIFT)
>
>+#define SPEC_CTRL_MASK			(SPEC_CTRL_IBRS | SPEC_CTRL_STIBP | SPEC_CTRL_SSBD \
>+							| SPEC_CTRL_RRSBA_DIS_S)
>+
> #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/bugs.c b/arch/x86/kernel/cpu/bugs.c
>index 3e3230cccaa7..970b277d02a6 100644
>--- a/arch/x86/kernel/cpu/bugs.c
>+++ b/arch/x86/kernel/cpu/bugs.c
>@@ -137,8 +137,16 @@ void __init check_bugs(void)
> 	 * have unknown values. AMD64_LS_CFG MSR is cached in the early AMD
> 	 * init code as it is not enumerated and depends on the family.
> 	 */
>-	if (boot_cpu_has(X86_FEATURE_MSR_SPEC_CTRL))
>+	if (boot_cpu_has(X86_FEATURE_MSR_SPEC_CTRL)) {
> 		rdmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
>+		/*
>+		 * Previously running software may have some controls turned ON.
>+		 * Clear them and let kernel decide which controls to use.
>+		 */
>+		x86_spec_ctrl_base &= ~SPEC_CTRL_MASK;
>+		wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
>+	}
>+

Nit, extra newline.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] x86/bugs: Explicitly clear speculative MSR bits
  2022-11-21 22:20     ` Pawan Gupta
@ 2022-11-22 15:55       ` Breno Leitao
  0 siblings, 0 replies; 5+ messages in thread
From: Breno Leitao @ 2022-11-22 15:55 UTC (permalink / raw)
  To: Pawan Gupta
  Cc: tglx, mingo, bp, dave.hansen, leit, cascardo, hpa, jpoimboe,
	peterz, x86, kexec

On Mon, Nov 21, 2022 at 02:20:31PM -0800, Pawan Gupta wrote:
> On Sun, Nov 20, 2022 at 12:02:55PM +0000, Breno Leitao wrote:
...
> > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> > index 3e3230cccaa7..970b277d02a6 100644
> > --- a/arch/x86/kernel/cpu/bugs.c
> > +++ b/arch/x86/kernel/cpu/bugs.c
> > @@ -137,8 +137,16 @@ void __init check_bugs(void)
> > 	 * have unknown values. AMD64_LS_CFG MSR is cached in the early AMD
> > 	 * init code as it is not enumerated and depends on the family.
> > 	 */
> > -	if (boot_cpu_has(X86_FEATURE_MSR_SPEC_CTRL))
> > +	if (boot_cpu_has(X86_FEATURE_MSR_SPEC_CTRL)) {
> > 		rdmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
> > +		/*
> > +		 * Previously running software may have some controls turned ON.
> > +		 * Clear them and let kernel decide which controls to use.
> > +		 */
> > +		x86_spec_ctrl_base &= ~SPEC_CTRL_MASK;
> > +		wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
> > +	}
> > +
> 
> Nit, extra newline.

Thanks. If no other comment about the diff come until tomorrow, I will
send a v2 without this extra line.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-11-22 17:00 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-18 18:21 [RFC PATCH] x86/bugs: Explicitly clear IBRS MSR bit Breno Leitao
2022-11-18 19:46 ` Pawan Gupta
2022-11-20 12:02   ` [PATCH] x86/bugs: Explicitly clear speculative MSR bits Breno Leitao
2022-11-21 22:20     ` Pawan Gupta
2022-11-22 15:55       ` Breno Leitao

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.