| Message ID | a3ce1558b68a64f52ea56000f2bbdfd6e7799258.1743617897.git.jpoimboe@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/bugs: RSB mitigation fixes and documentation | expand |
On Wed, Apr 02, 2025 at 11:19:18AM -0700, Josh Poimboeuf wrote: > There's nothing entry-specific about entry_ibpb(). In preparation for Not anymore - it was done on entry back then AFAIR. > calling it from elsewhere, rename it to __write_ibpb(). > > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> > --- > arch/x86/entry/entry.S | 7 ++++--- > arch/x86/include/asm/nospec-branch.h | 6 +++--- > arch/x86/kernel/cpu/bugs.c | 6 +++--- > 3 files changed, 10 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/entry/entry.S b/arch/x86/entry/entry.S > index d3caa31240ed..3a53319988b9 100644 > --- a/arch/x86/entry/entry.S > +++ b/arch/x86/entry/entry.S > @@ -17,7 +17,8 @@ > > .pushsection .noinstr.text, "ax" > > -SYM_FUNC_START(entry_ibpb) > +// Clobbers AX, CX, DX Why the ugly comment style if the rest of the file is already using the multiline one... > +SYM_FUNC_START(__write_ibpb) ... and why the __ ? > ANNOTATE_NOENDBR > movl $MSR_IA32_PRED_CMD, %ecx > movl $PRED_CMD_IBPB, %eax
On Wed, Apr 02, 2025 at 08:29:28PM +0200, Borislav Petkov wrote: > On Wed, Apr 02, 2025 at 11:19:18AM -0700, Josh Poimboeuf wrote: > > There's nothing entry-specific about entry_ibpb(). In preparation for > > Not anymore - it was done on entry back then AFAIR. > > > calling it from elsewhere, rename it to __write_ibpb(). > > > > Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> > > --- > > arch/x86/entry/entry.S | 7 ++++--- > > arch/x86/include/asm/nospec-branch.h | 6 +++--- > > arch/x86/kernel/cpu/bugs.c | 6 +++--- > > 3 files changed, 10 insertions(+), 9 deletions(-) > > > > diff --git a/arch/x86/entry/entry.S b/arch/x86/entry/entry.S > > index d3caa31240ed..3a53319988b9 100644 > > --- a/arch/x86/entry/entry.S > > +++ b/arch/x86/entry/entry.S > > @@ -17,7 +17,8 @@ > > > > .pushsection .noinstr.text, "ax" > > > > -SYM_FUNC_START(entry_ibpb) > > +// Clobbers AX, CX, DX > > Why the ugly comment style if the rest of the file is already using the > multiline one... It helps it stand out more? :-) > > +SYM_FUNC_START(__write_ibpb) > > ... and why the __ ? I was thinking the calling interface is a bit nonstandard. But actually it's fine to call from C as those registers are already caller-saved anyway. So yeah, let's drop the '__'.
On Wed, Apr 02, 2025 at 11:44:15AM -0700, Josh Poimboeuf wrote: > It helps it stand out more? :-) Please don't. Someone thought that it is a good idea to start using that // ugliness all of a sudden. So we decided we should limit it in tip: Documentation/process/maintainer-tip.rst The paragrapn that starts with "Use C++ style, tail comments when documenting ..." > I was thinking the calling interface is a bit nonstandard. But actually > it's fine to call from C as those registers are already caller-saved > anyway. So yeah, let's drop the '__'. Thx.
* Josh Poimboeuf <jpoimboe@kernel.org> wrote: > There's nothing entry-specific about entry_ibpb(). In preparation for > calling it from elsewhere, rename it to __write_ibpb(). Minor Git-log hygienic request, could you please mention in such patches what the *new* name is? This title is annoyingly passive-aggressive: x86/bugs: Rename entry_ibpb() Let's make it: x86/bugs: Rename entry_ibpb() to write_ibpb() ... or so. Because the new name is infinitely more important going forward than the old name is ever going to be. :-) Thanks, Ingo
On Wed, Apr 02, 2025 at 09:37:07PM +0200, Ingo Molnar wrote: > > * Josh Poimboeuf <jpoimboe@kernel.org> wrote: > > > There's nothing entry-specific about entry_ibpb(). In preparation for > > calling it from elsewhere, rename it to __write_ibpb(). > > Minor Git-log hygienic request, could you please mention in such > patches what the *new* name is? > > This title is annoyingly passive-aggressive: LOL > x86/bugs: Rename entry_ibpb() > > Let's make it: > > x86/bugs: Rename entry_ibpb() to write_ibpb() > > ... or so. Because the new name is infinitely more important going > forward than the old name is ever going to be. :-) Indeed, thanks.
diff --git a/arch/x86/entry/entry.S b/arch/x86/entry/entry.S index d3caa31240ed..3a53319988b9 100644 --- a/arch/x86/entry/entry.S +++ b/arch/x86/entry/entry.S @@ -17,7 +17,8 @@ .pushsection .noinstr.text, "ax" -SYM_FUNC_START(entry_ibpb) +// Clobbers AX, CX, DX +SYM_FUNC_START(__write_ibpb) ANNOTATE_NOENDBR movl $MSR_IA32_PRED_CMD, %ecx movl $PRED_CMD_IBPB, %eax @@ -27,9 +28,9 @@ SYM_FUNC_START(entry_ibpb) /* Make sure IBPB clears return stack preductions too. */ FILL_RETURN_BUFFER %rax, RSB_CLEAR_LOOPS, X86_BUG_IBPB_NO_RET RET -SYM_FUNC_END(entry_ibpb) +SYM_FUNC_END(__write_ibpb) /* For KVM */ -EXPORT_SYMBOL_GPL(entry_ibpb); +EXPORT_SYMBOL_GPL(__write_ibpb); .popsection diff --git a/arch/x86/include/asm/nospec-branch.h b/arch/x86/include/asm/nospec-branch.h index 8a5cc8e70439..bbac79cad04c 100644 --- a/arch/x86/include/asm/nospec-branch.h +++ b/arch/x86/include/asm/nospec-branch.h @@ -269,7 +269,7 @@ * typically has NO_MELTDOWN). * * While retbleed_untrain_ret() doesn't clobber anything but requires stack, - * entry_ibpb() will clobber AX, CX, DX. + * __write_ibpb() will clobber AX, CX, DX. * * As such, this must be placed after every *SWITCH_TO_KERNEL_CR3 at a point * where we have a stack but before any RET instruction. @@ -279,7 +279,7 @@ VALIDATE_UNRET_END CALL_UNTRAIN_RET ALTERNATIVE_2 "", \ - "call entry_ibpb", \ibpb_feature, \ + "call __write_ibpb", \ibpb_feature, \ __stringify(\call_depth_insns), X86_FEATURE_CALL_DEPTH #endif .endm @@ -368,7 +368,7 @@ extern void srso_return_thunk(void); extern void srso_alias_return_thunk(void); extern void entry_untrain_ret(void); -extern void entry_ibpb(void); +extern void __write_ibpb(void); #ifdef CONFIG_X86_64 extern void clear_bhb_loop(void); diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c index 4386aa6c69e1..310cb3f7139c 100644 --- a/arch/x86/kernel/cpu/bugs.c +++ b/arch/x86/kernel/cpu/bugs.c @@ -1142,7 +1142,7 @@ static void __init retbleed_select_mitigation(void) setup_clear_cpu_cap(X86_FEATURE_RETHUNK); /* - * There is no need for RSB filling: entry_ibpb() ensures + * There is no need for RSB filling: __write_ibpb() ensures * all predictions, including the RSB, are invalidated, * regardless of IBPB implementation. */ @@ -2676,7 +2676,7 @@ static void __init srso_select_mitigation(void) setup_clear_cpu_cap(X86_FEATURE_RETHUNK); /* - * There is no need for RSB filling: entry_ibpb() ensures + * There is no need for RSB filling: __write_ibpb() ensures * all predictions, including the RSB, are invalidated, * regardless of IBPB implementation. */ @@ -2701,7 +2701,7 @@ static void __init srso_select_mitigation(void) srso_mitigation = SRSO_MITIGATION_IBPB_ON_VMEXIT; /* - * There is no need for RSB filling: entry_ibpb() ensures + * There is no need for RSB filling: __write_ibpb() ensures * all predictions, including the RSB, are invalidated, * regardless of IBPB implementation. */
There's nothing entry-specific about entry_ibpb(). In preparation for calling it from elsewhere, rename it to __write_ibpb(). Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org> --- arch/x86/entry/entry.S | 7 ++++--- arch/x86/include/asm/nospec-branch.h | 6 +++--- arch/x86/kernel/cpu/bugs.c | 6 +++--- 3 files changed, 10 insertions(+), 9 deletions(-)