On Thu, Jan 04, 2018 at 03:26:52PM -0800, Tim Chen wrote: > On 01/04/2018 02:54 PM, Peter Zijlstra wrote: > > On Thu, Jan 04, 2018 at 09:56:47AM -0800, Tim Chen wrote: > >> .macro ENABLE_IBRS > >> - ALTERNATIVE "jmp 10f", "", X86_FEATURE_SPEC_CTRL > >> + testl $SPEC_CTRL_IBRS_INUSE, spec_ctrl_ibrs > >> + jz .Lskip_\@ > >> + > >> PUSH_MSR_REGS > >> WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS > >> POP_MSR_REGS > >> -10: > >> + > >> + jmp .Ldone_\@ > >> +.Lskip_\@: > >> + /* > >> + * prevent speculation beyond here as we could want to > >> + * stop speculation by enabling IBRS > >> + */ > >> + lfence > >> +.Ldone_\@: > >> .endm > > > > > > Yeah no. We have jump labels for this stuff. There is no reason what so > > ever to do dynamic tests for a variable that _never_ changes. > > > > Admin can change spec_ctrl_ibrs value at run time, or when > we scan new microcode. So it doesn't often change, but it could. > > There may be time when the admin wants to run the system > in a more secure mode, and time when it is okay to leave out > IBRS.
I think he meant to use STATIC_JUMP_IF_TRUE instead of testl spec_ctrl_ibrs to avoid the conditional jump but still allow to enable/disable the branch. I suggested using static key earlier too. In older kernels there's not even the boilerplate to check the static key in asm, so I used a pcp bitfield in a already guaranteed hot cacheline so in practice it's as fast as static key but static key is always theoretically preferable.
