1. Patchew
  2. Xen
  3. [PATCH v2 0/3] retpoline: add clang support + Kconfig selectable
  4. View patch

[PATCH v2 1/3] x86/retpoline: rename retpoline Kconfig check to include GCC prefix

Roger Pau Monne posted 3 patches 3 years, 11 months ago
There is a newer version of this series
[PATCH v2 1/3] x86/retpoline: rename retpoline Kconfig check to include GCC prefix
Posted by Roger Pau Monne 3 years, 11 months ago 
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.
---
 xen/arch/x86/Kconfig | 6 +++++-
 xen/arch/x86/arch.mk | 8 ++++----
 2 files changed, 9 insertions(+), 5 deletions(-)

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.
-- 
2.34.1
Re: [PATCH v2 1/3] x86/retpoline: rename retpoline Kconfig check to include GCC prefix
Posted by Jan Beulich 3 years, 11 months ago 
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
Re: [PATCH v2 1/3] x86/retpoline: rename retpoline Kconfig check to include GCC prefix
Posted by Jan Beulich 3 years, 11 months ago 
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
Re: [PATCH v2 1/3] x86/retpoline: rename retpoline Kconfig check to include GCC prefix
Posted by Roger Pau Monné 3 years, 11 months ago 
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.
Re: [PATCH v2 1/3] x86/retpoline: rename retpoline Kconfig check to include GCC prefix
Posted by Jan Beulich 3 years, 11 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.