On 15/02/2022 14:01, Jan Beulich wrote: > On 14.02.2022 13:50, Andrew Cooper wrote: >> --- a/xen/arch/x86/Kconfig >> +++ b/xen/arch/x86/Kconfig >> @@ -39,6 +39,11 @@ config HAS_AS_CET_SS >> # binutils >= 2.29 or LLVM >= 6 >> def_bool $(as-instr,wrssq %rax$(comma)0;setssbsy) >> >> +config HAS_CC_CET_IBT >> + # GCC >= 9 and binutils >= 2.29 >> + # Retpoline check to work around >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93654 >> + def_bool $(cc-option,-fcf-protection=branch -mmanual-endbr >> -mindirect-branch=thunk-extern) && $(as-instr,endbr64) > At the top of asm-defns.h we have a number of similarly operand-less > instructions expressed via .macro expanding to .byte. I don't see why > we couldn't do so here as well, eliminating the need for the > $(as-instr ...). In fact ... > >> --- a/xen/arch/x86/include/asm/asm-defns.h >> +++ b/xen/arch/x86/include/asm/asm-defns.h >> @@ -57,6 +57,12 @@ >> INDIRECT_BRANCH jmp \arg >> .endm >> >> +#ifdef CONFIG_XEN_IBT >> +# define ENDBR64 endbr64 >> +#else >> +# define ENDBR64 >> +#endif > ... it could also be this macro which ends up conditionally empty, > but would then want expressing as an assembler macro. Albeit no, the > lower case form would probably still be needed to deal with compiler > emitted insns, as the compiler doesn't appear to make recognition of > the command line option dependent on the underlying assembler's > capabilities.
$(as-instr) isn't only for endbr64. It also for the notrack prefix, which GCC does emit for any function pointer call laundered through void * even when everything was otherwise cf_check. It's another area where treating the cf_check-ness as type-checking falls down, and created some very weird build failures until I figured out that Juergen's "Don't use the hypercall table for calling compat hypercalls" really did need to be a prerequisite. CET-IBT toolchain support is 3 years old already, and I don't think there is any value attempting to support a developer mixing a new GCC and ancient binutils. >> --- a/xen/arch/x86/include/asm/cpufeatures.h >> +++ b/xen/arch/x86/include/asm/cpufeatures.h >> @@ -39,6 +39,7 @@ XEN_CPUFEATURE(SC_VERW_PV, X86_SYNTH(23)) /* VERW >> used by Xen for PV */ >> XEN_CPUFEATURE(SC_VERW_HVM, X86_SYNTH(24)) /* VERW used by Xen for >> HVM */ >> XEN_CPUFEATURE(SC_VERW_IDLE, X86_SYNTH(25)) /* VERW used by Xen for >> idle */ >> XEN_CPUFEATURE(XEN_SHSTK, X86_SYNTH(26)) /* Xen uses CET Shadow >> Stacks */ >> +XEN_CPUFEATURE(XEN_IBT, X86_SYNTH(27)) /* Xen uses CET Indirect >> Branch Tracking */ > Is a feature flag actually warranted here, rather than a single > global boolean? You don't key any alternatives patching to this > bit, unlike was the case for XEN_SHSTK. And the only consumer is > cpu_has_xen_ibt, expanding to the boot CPU's instance of the bit. These are just bits. They long predate alternatives finding a convenient use for the form, and are 8 times more compact than a global boolean, with better locality of reference too. ~Andrew
