On 16.02.2022 22:54, Andrew Cooper wrote: > 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.
Oh, I see. I can certainly accept this as a reason, but half a sentence mentioning this would be nice in the description. >>> --- 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. Well, I disagree (and we were here before, so I think you could have predicted such a comment coming back). We should never have cloned this directly from Linux. It's only bits, but with enough CPUs it sums up. We shouldn't duplicate data when we need only a single instance (and when no other infrastructure, like alternative patching, depends on it). Last time you put me in a situation like this one, I told myself to not ack such changes anymore, but here I am again - in the interest of not being blamed for blocking this series: Acked-by: Jan Beulich <jbeul...@suse.com> Jan
