From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from userp2130.oracle.com ([156.151.31.86]) by Galois.linutronix.de with esmtps (TLS1.2:RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1fImv1-0003NT-JN for speck@linutronix.de; Wed, 16 May 2018 05:16:00 +0200 Received: from pps.filterd (userp2130.oracle.com [127.0.0.1]) by userp2130.oracle.com (8.16.0.22/8.16.0.22) with SMTP id w4G3BjmQ122192 for ; Wed, 16 May 2018 03:15:52 GMT Received: from aserv0022.oracle.com (aserv0022.oracle.com [141.146.126.234]) by userp2130.oracle.com with ESMTP id 2hx29wav3r-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Wed, 16 May 2018 03:15:52 +0000 Received: from userv0121.oracle.com (userv0121.oracle.com [156.151.31.72]) by aserv0022.oracle.com (8.14.4/8.14.4) with ESMTP id w4G3Fp5d006784 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Wed, 16 May 2018 03:15:51 GMT Received: from abhmp0016.oracle.com (abhmp0016.oracle.com [141.146.116.22]) by userv0121.oracle.com (8.14.4/8.13.8) with ESMTP id w4G3FprB014840 for ; Wed, 16 May 2018 03:15:51 GMT Date: Tue, 15 May 2018 23:15:50 -0400 From: Konrad Rzeszutek Wilk Subject: [MODERATED] Re: [patch 06/15] Hidden 6 Message-ID: <20180516031550.GF18660@char.us.oracle.com> References: <20180513140048.543641807@linutronix.de> <20180513140538.715249208@linutronix.de> MIME-Version: 1.0 In-Reply-To: <20180513140538.715249208@linutronix.de> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: On Sun, May 13, 2018 at 04:00:54PM +0200, speck for Thomas Gleixner wrote: > Subject: [patch 06/15] x86/speculation: Handle HT correctly on AMD > From: Thomas Gleixner > > The AMD64_LS_CFG MSR is a per core MSR on Family 17H CPUs. That means when > hyperthreading is enabled the SSBD bit toggle needs to take both cores into > account. Otherwise the following situation can happen: > > CPU0 CPU1 > > disable SSB > disable SSB > enable SSB <- Enables it for the Core, i.e. for CPU0 as well > > So after the SSB enable on CPU1 the task on CPU0 runs with SSB enabled > again. > > On Intel the SSBD control is per core as well, but the synchronization > logic is implemented behind the per thread SPEC_CTRL MSR. I am missing something here. You speak of hardware synchronization which would mean you would get the same exact behavior as AMD? That is whacking the MSR would synchronize the state on both siblings? That is if you enable memory disambiguation on one sibling it would enable it on the other? has sibling level granularity? So if one sibling is running with SSBD ON and the other with OFF it has the brains to figure this out? Or would the brains be to keep it OFF for both siblings? In which case why the 'On Intel the SSBD control is per core as well' ? Or.. oh, you are saying it keeps the state latched - so if one has memory disambiguation disabled then _both_ siblings have it so - even if the other tries to enable it back on. Gosh, I hope this is spelled out in the SDM when that comes out. Perhaps then: "s/synchronization/synchronization (keep it disabled even if another sibling enables - only enable it if both siblings set this)/" ? > > Add the necessary synchronization logic for AMD family 17H. Unfortunately > that requires a spinlock to serialize the access to the MSR, but the locks > are only shared between siblings. > > Signed-off-by: Thomas Gleixner > --- > arch/x86/include/asm/spec-ctrl.h | 6 ++ > arch/x86/kernel/process.c | 108 ++++++++++++++++++++++++++++++++++++--- > arch/x86/kernel/smpboot.c | 5 + > 3 files changed, 113 insertions(+), 6 deletions(-) > > --- a/arch/x86/include/asm/spec-ctrl.h > +++ b/arch/x86/include/asm/spec-ctrl.h > @@ -33,6 +33,12 @@ static inline u64 ssbd_tif_to_amd_ls_cfg > return (tifn & _TIF_SSBD) ? x86_amd_ls_cfg_ssbd_mask : 0ULL; > } > > +#ifdef CONFIG_SMP > +extern void speculative_store_bypass_ht_init(void); > +#else > +static inline void speculative_store_bypass_ht_init(void) { } > +#endif > + > extern void speculative_store_bypass_update(void); > > #endif > --- a/arch/x86/kernel/process.c > +++ b/arch/x86/kernel/process.c > @@ -279,22 +279,118 @@ static inline void switch_to_bitmap(stru > } > } > > -static __always_inline void __speculative_store_bypass_update(unsigned long tifn) > +#ifdef CONFIG_SMP > + > +struct ssb_state { > + struct ssb_state *shared_state; > + raw_spinlock_t lock; > + unsigned int disable_state; > + unsigned long local_state; > +}; > + > +#define LSTATE_SSB 0 > + > +static DEFINE_PER_CPU(struct ssb_state, ssb_state); > + > +void speculative_store_bypass_ht_init(void) > { > - u64 msr; > + struct ssb_state *st = this_cpu_ptr(&ssb_state); > + unsigned int this_cpu = smp_processor_id(); > + unsigned int cpu; > + > + st->local_state = 0; > + if (st->shared_state) > + return; > + > + raw_spin_lock_init(&st->lock); Should we also hold this lock in the CPU hotplug code? That is when you power off an CPU? > + > + /* Go over HT siblings: */ > + for_each_cpu(cpu, topology_sibling_cpumask(this_cpu)) { .. As could you (on a bad of course), access the sibling here - right when the sibling is powered-off? > + if (cpu == this_cpu) > + continue; > > - if (static_cpu_has(X86_FEATURE_LS_CFG_SSBD)) { > - msr = x86_amd_ls_cfg_base | ssbd_tif_to_amd_ls_cfg(tifn); > + if (!per_cpu(ssb_state, cpu).shared_state) > + continue; > + > + /* Link it to the state of the sibling: */ > + st->shared_state = per_cpu(ssb_state, cpu).shared_state; And then this would refer to a dead per-cpu area. Do we clear the per-cpu area when offlining? Aka is the per_cpu(.., cpu) where CPU is offline end up with a NULL pointer? > + return; > + } > + /* Link shared state of the first HT sibling to itself. */ > + st->shared_state = st; > +} > + > +/* > + * Logic is: first HT sibling enables SSBD for both siblings in the core and > + * last sibling to disable it, disables it for the whole core. Would it make sense to say this follows how the Intel CPU has it implemented? > + */