Messages in this thread | | | Date | Thu, 4 Jan 2018 23:47:31 +0100 | From | Peter Zijlstra <> | Subject | Re: [PATCH 4/7] x86/idle: Disable IBRS entering idle and enable it on wakeup |
| |
On Thu, Jan 04, 2018 at 09:56:45AM -0800, Tim Chen wrote: > @@ -100,15 +101,33 @@ static inline void __sti_mwait(unsigned long eax, unsigned long ecx) > static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx) > { > if (static_cpu_has_bug(X86_BUG_MONITOR) || !current_set_polling_and_test()) { > + bool can_toggle_ibrs = false; > if (static_cpu_has_bug(X86_BUG_CLFLUSH_MONITOR)) { > mb(); > clflush((void *)¤t_thread_info()->flags); > mb(); > } > > + if (irqs_disabled()) { > + /* > + * CPUs run faster with speculation protection > + * disabled. All CPU threads in a core must > + * disable speculation protection for it to be > + * disabled. Disable it while we are idle so the > + * other hyperthread can run fast. > + * > + * nmi uses the save_paranoid model which > + * always enables ibrs on exception entry > + * before any indirect jump can run. > + */ > + can_toggle_ibrs = true; > + unprotected_speculation_begin(); > + } > __monitor((void *)¤t_thread_info()->flags, 0, 0); > if (!need_resched()) > __mwait(eax, ecx); > + if (can_toggle_ibrs) > + unprotected_speculation_end(); > } > current_clr_polling(); > }
Argh.. no. Who is calling this with IRQs enabled? And why can't we frob the MSR with IRQs enabled? That comment doesn't seem to explain anything.
> diff --git a/arch/x86/include/asm/spec_ctrl.h b/arch/x86/include/asm/spec_ctrl.h > index 16fc4f58..28b0314 100644 > --- a/arch/x86/include/asm/spec_ctrl.h > +++ b/arch/x86/include/asm/spec_ctrl.h > @@ -76,5 +76,42 @@ > 10: > .endm > > +#else > +#include <asm/microcode.h> > + > +static inline void __disable_indirect_speculation(void) > +{ > + native_wrmsrl(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_ENABLE_IBRS); > +} > + > +static inline void __enable_indirect_speculation(void) > +{ > + native_wrmsrl(MSR_IA32_SPEC_CTRL, SPEC_CTRL_FEATURE_DISABLE_IBRS); > +} > + > +/* > + * Interrupts must be disabled to begin unprotected speculation. > + * Otherwise interrupts could be running in unprotected mode. > + */ > +static inline void unprotected_speculation_begin(void) > +{ > + WARN_ON_ONCE(!irqs_disabled());
lockdep_assert_irqs_disabled()
> + if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) > + __enable_indirect_speculation(); > +} > + > +static inline void unprotected_speculation_end(void) > +{ > + if (boot_cpu_has(X86_FEATURE_SPEC_CTRL)) > + __disable_indirect_speculation(); > + else > + /* > + * If we intended to disable indirect speculation > + * but come here due to mis-speculation, we need > + * to stop the mis-speculation with rmb. > + */ > + rmb();
Code is lacking {}, also the comment doesn't make sense. If we don't have the MSR, why are we doing an LFENCE?
And why are these boot_cpu_has() and not static_cpu_has() ?
| |