Re: [PATCH v2] x86/bugs: Explicitly clear speculative MSR bits

[Pawan Gupta]

On Mon, Nov 28, 2022 at 03:02:21PM -0800, Pawan Gupta wrote:
> On Mon, Nov 28, 2022 at 11:40:19PM +0100, Borislav Petkov wrote:
> > On Mon, Nov 28, 2022 at 02:03:58PM -0800, Pawan Gupta wrote:
> > > diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
> > > index 3e3230cccaa7..cfc2ed2661fc 100644
> > > --- a/arch/x86/kernel/cpu/bugs.c
> > > +++ b/arch/x86/kernel/cpu/bugs.c
> > > @@ -66,7 +66,7 @@ static DEFINE_MUTEX(spec_ctrl_mutex);
> > >  */
> > > void write_spec_ctrl_current(u64 val, bool force)
> > > {
> > > -	if (this_cpu_read(x86_spec_ctrl_current) == val)
> > > +	if (!force && this_cpu_read(x86_spec_ctrl_current) == val)
> > > 		return;
> > > 	this_cpu_write(x86_spec_ctrl_current, val);
> >
> > Still looks hacky to me.
> >
> > I think it would be a lot cleaner if MSR_IA32_SPEC_CTRL gets cleaned of
> > the speculation bits in init_speculation_control() which gets run on
> > *every* CPU.
> >
> > So by the time check_bugs() gets to setup stuff, the MSR will be ready
> > to go regardless.
> >
> > I.e., something like this (not supposed to work - just to show what I
> > mean):
> >
> > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> > index 73cc546e024d..367732c92942 100644
> > --- a/arch/x86/kernel/cpu/common.c
> > +++ b/arch/x86/kernel/cpu/common.c
> > @@ -993,9 +993,19 @@ static void init_speculation_control(struct cpuinfo_x86 *c)
> > 	 * Intel CPUs, for finer-grained selection of what's available.
> > 	 */
> > 	if (cpu_has(c, X86_FEATURE_SPEC_CTRL)) {
> > +		u64 msr;
> > +
> > 		set_cpu_cap(c, X86_FEATURE_IBRS);
> > 		set_cpu_cap(c, X86_FEATURE_IBPB);
> > 		set_cpu_cap(c, X86_FEATURE_MSR_SPEC_CTRL);
> > +
> > +		/*
> > +		 * Clear speculation control settings from a previous kernel
> > +		 * run, i.e., kexec.
> > +		 */
> > +		rdmsrl(MSR_IA32_SPEC_CTRL, msr);
> > +		if (msr & SPEC_CTRL_MASK)
> > +			wrmsr (MSR_IA32_SPEC_CTRL, msr & ~SPEC_CTRL_MASK);
>
> Yes thats a cleaner approach, except that the late microcode load will
> ruin the MSR:

Root of the original problem is x86_spec_ctrl_current is not the current
value of MSR at bootup.

How about we update x86_spec_ctrl_current before any writes to the MSR?:

diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 3e3230cccaa7..68ed52394fd9 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -137,8 +137,18 @@ 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, like kexec for example, may
+		 * have some controls turned ON.
+		 * Clear them and let the mitigations setup below set them
+		 * based on configuration.
+		 */
+		this_cpu_write(x86_spec_ctrl_current, x86_spec_ctrl_base);
+		x86_spec_ctrl_base &= ~SPEC_CTRL_MITIGATIONS_MASK;
+		write_spec_ctrl_current(x86_spec_ctrl_base, true);
+	}
 
 	/* Select the proper CPU mitigations before patching alternatives: */
 	spectre_v1_select_mitigation();
@@ -2047,8 +2057,13 @@ int arch_prctl_spec_ctrl_get(struct task_struct *task, unsigned long which)
 
 void x86_spec_ctrl_setup_ap(void)
 {
-	if (boot_cpu_has(X86_FEATURE_MSR_SPEC_CTRL))
+	if (boot_cpu_has(X86_FEATURE_MSR_SPEC_CTRL)) {
+		u64 msr;
+
+		rdmsrl(MSR_IA32_SPEC_CTRL, msr);
+		this_cpu_write(x86_spec_ctrl_current, msr);
 		write_spec_ctrl_current(x86_spec_ctrl_base, true);
+	}
 
 	if (ssb_mode == SPEC_STORE_BYPASS_DISABLE)
 		x86_amd_ssb_disable();