Re: [PATCH] x86/msr-index: make SPEC_CTRL_IBRS assembler-portable

[Nick Desaulniers <ndesaulniers@google.com>]

On Fri, Nov 4, 2022 at 2:55 AM H. Peter Anvin <hpa@zytor.com> wrote:
>
> On November 3, 2022 2:07:48 PM PDT, Nick Desaulniers <ndesaulniers@google.com> wrote:
> >GNU binutils' assembler (GAS) didn't support L suffixes on immediates
> >until binutils 2.28 release. Building arch/x86/entry/entry_64.S with GAS
> >v2.27 will produce the following assembler errors:
> >
> >  arch/x86/entry/entry_64.S: Assembler messages:
> >  arch/x86/entry/entry_64.S:308: Error: found 'L', expected: ')'
> >  arch/x86/entry/entry_64.S:308: Error: found 'L', expected: ')'
> >  arch/x86/entry/entry_64.S:308: Error: junk `L<<(0)))' after expression
> >  arch/x86/entry/entry_64.S:596: Error: found 'L', expected: ')'
> >  arch/x86/entry/entry_64.S:596: Error: found 'L', expected: ')'
> >  arch/x86/entry/entry_64.S:596: Error: junk `L<<(0)))' after expression
> >
> >These come from the use of the preprocessor defined SPEC_CTRL_IBRS in
> >the IBRS_ENTER and IBRS_EXIT assembler macros. SPEC_CTRL_IBRS was using
> >the BIT macros from include/linux/bits.h which are only portable between
> >C and assembler for assemblers such as GAS v2.28 (or newer) or clang
> >because they use the L suffixes for immediate operands, which older GAS
> >releases cannot parse. The kernel still supports GAS v2.23 and newer
> >(and older for branches of stable). Let's expand the value of
> >SPEC_CTRL_IBRS in place so that assemblers don't have issues parsing the
> >value.
> >
> >Fixes: 2dbb887e875b ("x86/entry: Add kernel IBRS implementation")
> >Reported-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> >---
> >Some other ideas considered:
> >* Use U64_C from include/asm-generic/int-ll64.h rather than BIT for the
> >  value of SPEC_CTRL_IBRS.
> >  * Do so for the entirety of arch/x86/include/asm/msr-index.h or just
> >    SPEC_CTRL_IBRS? include/asm-generic/int-ll64.h doesn't define a UL
> >    suffix; add one?
> >* Make include/linux/bits.h assembler-portable (for older assemblers)
> >  via the use of include/asm-generic/int-ll64.h.
> >
> > arch/x86/include/asm/msr-index.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> >index 10ac52705892..0192d853136c 100644
> >--- a/arch/x86/include/asm/msr-index.h
> >+++ b/arch/x86/include/asm/msr-index.h
> >@@ -46,7 +46,7 @@
> > #define MSR_TEST_CTRL_SPLIT_LOCK_DETECT               BIT(MSR_TEST_CTRL_SPLIT_LOCK_DETECT_BIT)
> >
> > #define MSR_IA32_SPEC_CTRL            0x00000048 /* Speculation Control */
> >-#define SPEC_CTRL_IBRS                        BIT(0)     /* Indirect Branch Restricted Speculation */
> >+#define SPEC_CTRL_IBRS                        1          /* Indirect Branch Restricted Speculation */
> > #define SPEC_CTRL_STIBP_SHIFT         1          /* Single Thread Indirect Branch Predictor (STIBP) bit */
> > #define SPEC_CTRL_STIBP                       BIT(SPEC_CTRL_STIBP_SHIFT)      /* STIBP mask */
> > #define SPEC_CTRL_SSBD_SHIFT          2          /* Speculative Store Bypass Disable bit */
>
> Let's fix the macro instead.

Ah, I need to do more homework; this issue is specific to stable
backports of the retbleed mitigations.
commit 95b980d62d52 ("linux/bits.h: make BIT(), GENMASK(), and friends
available in assembly")
fixed this exact problem already.  Ok, I'll work with stable to get
that backported as part of the series.  Sorry for the noise.
-- 
Thanks,
~Nick Desaulniers