From mboxrd@z Thu Jan  1 00:00:00 1970
Return-path: <tglx@linutronix.de>
Received: from p5492e61e.dip0.t-ipconnect.de ([84.146.230.30]
 helo=nanos.glx-home)	by Galois.linutronix.de with esmtpsa
 (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256)	(Exim 4.80)	(envelope-from
 <tglx@linutronix.de>)	id 1fDQBA-0001s7-3w	for speck@linutronix.de; Tue, 01
 May 2018 09:58:28 +0200
Date: Tue, 1 May 2018 09:58:27 +0200 (CEST)
From: Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [patch V8 09/15] SSB 9
In-Reply-To: <5ae99b98-2837-edc1-90e3-9e553765d1d4@linux.intel.com>
Message-ID: <alpine.DEB.2.21.1805010954280.1596@nanos.tec.linutronix.de>
References: <20180430150423.180110059@linutronix.de>
 <20180430151232.528270696@linutronix.de>
 <5ae99b98-2837-edc1-90e3-9e553765d1d4@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
To: speck@linutronix.de
List-ID: <speck.linutronix.de>

On Mon, 30 Apr 2018, speck for Tim Chen wrote:
> On 04/30/2018 08:04 AM, speck for Thomas Gleixner wrote:
> 
> > 
> > void x86_set_spec_ctrl(u64 val)
> > {
> >-	if (val & ~(SPEC_CTRL_IBRS | SPEC_CTRL_RDS))
> >+	if (val & x86_spec_ctrl_mask)
> > 		WARN_ONCE(1, "SPEC_CTRL MSR value 0x%16llx is unknown.\n", val);
> > 	else
> > 		wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base | val);
> >@@ -459,6 +465,7 @@ static enum ssb_mitigation_cmd __init __
> > 		switch (boot_cpu_data.x86_vendor) {
> > 		case X86_VENDOR_INTEL:
> > 			x86_spec_ctrl_base |= SPEC_CTRL_RDS;
> >+			x86_spec_ctrl_mask &= ~(SPEC_CTRL_RDS);
> > 			x86_set_spec_ctrl(SPEC_CTRL_RDS);
> 
> We only have cpu 0's RDS set when we ask for SSB to be turned off for whole system
> in current v8 patchset.
> 
> I noticed that the x86_set_spec_ctrl call in
> init_intel got dropped in current patchset.  It was in Konrad's original patchset.

Yes, because it was just not the right thing to do it per vendor.

void identify_secondary_cpu(struct cpuinfo_x86 *c)
{
        BUG_ON(c == &boot_cpu_data);
        identify_cpu(c);
#ifdef CONFIG_X86_32
        enable_sep_cpu();
#endif
        mtrr_ap_init();
        validate_apic_and_package_id(c);
        x86_setup_ap_spec_ctrl();	<----------- This should take care
}


> Now only cpu 0 got RDS set in the code above and not the other CPUs when we are
> asking for RDS to be turned on for whole system.
> 
> root@otc-cfl-s-02:~# cat /sys/devices/system/cpu/vulnerabilities/spec_store_bypass 
> Mitigation: Speculative Store Bypass disabled
> 
> root@otc-cfl-s-02:~/md_test# ./md_dump
> msr 0x48 cpu 0  val: 4
> msr 0x48 cpu 1  val: 0
> We should get val: 4 for all CPUs.
> 
> With the proposed fix below, I get the expected value of 4 for all CPU.

> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index ef3f9c0..c799e48 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -770,6 +770,9 @@ static void init_intel(struct cpuinfo_x86 *c)
>  	init_intel_energy_perf(c);
>  
>  	init_intel_misc_features(c);
> +
> +	if (cpu_has(c, X86_FEATURE_SPEC_STORE_BYPASS_DISABLE))
> +		x86_set_spec_ctrl(SPEC_CTRL_RDS);

Groan no. x86_setup_ap_spec_ctrl() needs to be investigated not random crap
added to some random place again.

x86_setup_ap_spec_ctrl()
{
       if (boot_cpu_has(X86_FEATURE_IBRS))
                x86_set_spec_ctrl(x86_spec_ctrl_base & ~x86_spec_ctrl_mask);

So that's the issue.

Thanks,

	tglx