| Message ID | 20220216162142.15384-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | retpoline: add clang support + Kconfig selectable | expand |
On 16.02.2022 17:21, Roger Pau Monne wrote: > Current retpoline checks apply to GCC only, so make it obvious by > prefixing the Kconfig option with GCC. Keep the previous option as a > way to signal generic retpoline support regardless of the underlying > compiler. > > No functional change intended. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> > --- > Changes since v1: > - Put def_bool before depend on. Just for the record: A slightly shorter alternative would have been ... > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -32,9 +32,13 @@ config ARCH_DEFCONFIG > string > default "arch/x86/configs/x86_64_defconfig" > > -config INDIRECT_THUNK > +config GCC_INDIRECT_THUNK > def_bool $(cc-option,-mindirect-branch-register) > > +config INDIRECT_THUNK > + def_bool y > + depends on GCC_INDIRECT_THUNK config INDIRECT_THUNK bool config GCC_INDIRECT_THUNK def_bool $(cc-option,-mindirect-branch-register) select INDIRECT_THUNK A more appropriate thing to use for "depends on" might have been CC_IS_GCC. With the next patch in mind this would then avoid potential confusion if e.g. Clang folks decided to (also) support the gcc style command line options. Jan
On 17.02.2022 09:59, Jan Beulich wrote: > On 16.02.2022 17:21, Roger Pau Monne wrote: >> Current retpoline checks apply to GCC only, so make it obvious by >> prefixing the Kconfig option with GCC. Keep the previous option as a >> way to signal generic retpoline support regardless of the underlying >> compiler. >> >> No functional change intended. >> >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> --- >> Changes since v1: >> - Put def_bool before depend on. > > Just for the record: A slightly shorter alternative would have been ... > >> --- a/xen/arch/x86/Kconfig >> +++ b/xen/arch/x86/Kconfig >> @@ -32,9 +32,13 @@ config ARCH_DEFCONFIG >> string >> default "arch/x86/configs/x86_64_defconfig" >> >> -config INDIRECT_THUNK >> +config GCC_INDIRECT_THUNK >> def_bool $(cc-option,-mindirect-branch-register) >> >> +config INDIRECT_THUNK >> + def_bool y >> + depends on GCC_INDIRECT_THUNK > > config INDIRECT_THUNK > bool > > config GCC_INDIRECT_THUNK > def_bool $(cc-option,-mindirect-branch-register) > select INDIRECT_THUNK Oh, looking at patch 3 again (which I should have still had in mind) this would of course not help. Yet .. > A more appropriate thing to use for "depends on" might have been > CC_IS_GCC. With the next patch in mind this would then avoid > potential confusion if e.g. Clang folks decided to (also) support > the gcc style command line options. ... adding this dependency (and a respective one in patch 2) might still be a good thing. Jan
On Thu, Feb 17, 2022 at 10:04:01AM +0100, Jan Beulich wrote: > On 17.02.2022 09:59, Jan Beulich wrote: > > On 16.02.2022 17:21, Roger Pau Monne wrote: > >> Current retpoline checks apply to GCC only, so make it obvious by > >> prefixing the Kconfig option with GCC. Keep the previous option as a > >> way to signal generic retpoline support regardless of the underlying > >> compiler. > >> > >> No functional change intended. > >> > >> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > >> Reviewed-by: Jan Beulich <jbeulich@suse.com> > >> --- > >> Changes since v1: > >> - Put def_bool before depend on. > > > > Just for the record: A slightly shorter alternative would have been ... > > > >> --- a/xen/arch/x86/Kconfig > >> +++ b/xen/arch/x86/Kconfig > >> @@ -32,9 +32,13 @@ config ARCH_DEFCONFIG > >> string > >> default "arch/x86/configs/x86_64_defconfig" > >> > >> -config INDIRECT_THUNK > >> +config GCC_INDIRECT_THUNK > >> def_bool $(cc-option,-mindirect-branch-register) > >> > >> +config INDIRECT_THUNK > >> + def_bool y > >> + depends on GCC_INDIRECT_THUNK > > > > config INDIRECT_THUNK > > bool > > > > config GCC_INDIRECT_THUNK > > def_bool $(cc-option,-mindirect-branch-register) > > select INDIRECT_THUNK > > Oh, looking at patch 3 again (which I should have still had in mind) > this would of course not help. Yet .. > > > A more appropriate thing to use for "depends on" might have been > > CC_IS_GCC. With the next patch in mind this would then avoid > > potential confusion if e.g. Clang folks decided to (also) support > > the gcc style command line options. > > ... adding this dependency (and a respective one in patch 2) might > still be a good thing. So you would like to make GCC_INDIRECT_THUNK depend on CC_IS_GCC? That would be fine, but I think it's extremely unlikely for CLANG to adopt the GCC options - I've already mentioned at implementation time and they refused. Thanks, Roger.
On 17.02.2022 11:37, Roger Pau Monné wrote: > On Thu, Feb 17, 2022 at 10:04:01AM +0100, Jan Beulich wrote: >> On 17.02.2022 09:59, Jan Beulich wrote: >>> On 16.02.2022 17:21, Roger Pau Monne wrote: >>>> Current retpoline checks apply to GCC only, so make it obvious by >>>> prefixing the Kconfig option with GCC. Keep the previous option as a >>>> way to signal generic retpoline support regardless of the underlying >>>> compiler. >>>> >>>> No functional change intended. >>>> >>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> >>>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>>> --- >>>> Changes since v1: >>>> - Put def_bool before depend on. >>> >>> Just for the record: A slightly shorter alternative would have been ... >>> >>>> --- a/xen/arch/x86/Kconfig >>>> +++ b/xen/arch/x86/Kconfig >>>> @@ -32,9 +32,13 @@ config ARCH_DEFCONFIG >>>> string >>>> default "arch/x86/configs/x86_64_defconfig" >>>> >>>> -config INDIRECT_THUNK >>>> +config GCC_INDIRECT_THUNK >>>> def_bool $(cc-option,-mindirect-branch-register) >>>> >>>> +config INDIRECT_THUNK >>>> + def_bool y >>>> + depends on GCC_INDIRECT_THUNK >>> >>> config INDIRECT_THUNK >>> bool >>> >>> config GCC_INDIRECT_THUNK >>> def_bool $(cc-option,-mindirect-branch-register) >>> select INDIRECT_THUNK >> >> Oh, looking at patch 3 again (which I should have still had in mind) >> this would of course not help. Yet .. >> >>> A more appropriate thing to use for "depends on" might have been >>> CC_IS_GCC. With the next patch in mind this would then avoid >>> potential confusion if e.g. Clang folks decided to (also) support >>> the gcc style command line options. >> >> ... adding this dependency (and a respective one in patch 2) might >> still be a good thing. > > So you would like to make GCC_INDIRECT_THUNK depend on CC_IS_GCC? It would seem more clean to me, but ... > That would be fine, but I think it's extremely unlikely for CLANG to > adopt the GCC options - I've already mentioned at implementation time > and they refused. ... I'm not going to insist. Jan
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index b4abfca46f..219ef9791d 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -32,9 +32,13 @@ config ARCH_DEFCONFIG string default "arch/x86/configs/x86_64_defconfig" -config INDIRECT_THUNK +config GCC_INDIRECT_THUNK def_bool $(cc-option,-mindirect-branch-register) +config INDIRECT_THUNK + def_bool y + depends on GCC_INDIRECT_THUNK + config HAS_AS_CET_SS # binutils >= 2.29 or LLVM >= 6 def_bool $(as-instr,wrssq %rax$(comma)0;setssbsy) diff --git a/xen/arch/x86/arch.mk b/xen/arch/x86/arch.mk index fa7cf38443..2da4bdb1ed 100644 --- a/xen/arch/x86/arch.mk +++ b/xen/arch/x86/arch.mk @@ -42,10 +42,10 @@ CFLAGS += -mno-red-zone -fpic # SSE setup for variadic function calls. CFLAGS += -mno-sse $(call cc-option,$(CC),-mskip-rax-setup) -# Compile with thunk-extern, indirect-branch-register if avaiable. -CFLAGS-$(CONFIG_INDIRECT_THUNK) += -mindirect-branch=thunk-extern -CFLAGS-$(CONFIG_INDIRECT_THUNK) += -mindirect-branch-register -CFLAGS-$(CONFIG_INDIRECT_THUNK) += -fno-jump-tables +# Compile with gcc thunk-extern, indirect-branch-register if available. +CFLAGS-$(CONFIG_GCC_INDIRECT_THUNK) += -mindirect-branch=thunk-extern +CFLAGS-$(CONFIG_GCC_INDIRECT_THUNK) += -mindirect-branch-register +CFLAGS-$(CONFIG_GCC_INDIRECT_THUNK) += -fno-jump-tables # If supported by the compiler, reduce stack alignment to 8 bytes. But allow # this to be overridden elsewhere.