[PATCH v2 0/3] retpoline: add clang support + Kconfig selectable

[PATCH v2 0/3] retpoline: add clang support + Kconfig selectable

[Roger Pau Monne]

Hello,

The following series adds retpoline support for clang builds, and also
allows the user to select whether to enable retpoline support at build
time via a new Kconfig option.

I've tried adding a suitable description to the Kconfig option, but I'm
sure there's room for improvement.

Thanks, Roger.

Roger Pau Monne (3):
  x86/retpoline: rename retpoline Kconfig check to include GCC prefix
  x86/clang: add retpoline support
  x86/Kconfig: introduce option to select retpoline usage

 xen/arch/x86/Kconfig |  5 ++++-
 xen/arch/x86/arch.mk | 13 +++++++++----
 xen/common/Kconfig   | 16 ++++++++++++++++
 3 files changed, 29 insertions(+), 5 deletions(-)

-- 
2.34.1

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

[Roger Pau Monne]

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

[PATCH v2 2/3] x86/clang: add retpoline support

[Roger Pau Monne]

Detect whether the compiler supports clang retpoline option and enable
by default if available, just like it's done for gcc.

Note clang already disables jump tables when retpoline is enabled, so
there's no need to also pass the fno-jump-tables parameter. Also clang
already passes the return address on a register always on amd64, so
there's no need for any equivalent mindirect-branch-register
parameter.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/Kconfig | 5 ++++-
 xen/arch/x86/arch.mk | 3 +++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 219ef9791d..2fa456292b 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -35,9 +35,12 @@ config ARCH_DEFCONFIG
 config GCC_INDIRECT_THUNK
 	def_bool $(cc-option,-mindirect-branch-register)
 
+config CLANG_INDIRECT_THUNK
+	def_bool $(cc-option,-mretpoline-external-thunk)
+
 config INDIRECT_THUNK
 	def_bool y
-	depends on GCC_INDIRECT_THUNK
+	depends on GCC_INDIRECT_THUNK || CLANG_INDIRECT_THUNK
 
 config HAS_AS_CET_SS
 	# binutils >= 2.29 or LLVM >= 6
diff --git a/xen/arch/x86/arch.mk b/xen/arch/x86/arch.mk
index 2da4bdb1ed..f2aa2a515f 100644
--- a/xen/arch/x86/arch.mk
+++ b/xen/arch/x86/arch.mk
@@ -47,6 +47,9 @@ 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
 
+# Enable clang retpoline support if available.
+CFLAGS-$(CONFIG_CLANG_INDIRECT_THUNK) += -mretpoline-external-thunk
+
 # If supported by the compiler, reduce stack alignment to 8 bytes. But allow
 # this to be overridden elsewhere.
 $(call cc-option-add,CFLAGS_stack_boundary,CC,-mpreferred-stack-boundary=3)
-- 
2.34.1

[PATCH v2 3/3] x86/Kconfig: introduce option to select retpoline usage

[Roger Pau Monne]

Add a new Kconfig option under the "Speculative hardening" section
that allows selecting whether to enable retpoline. This depends on the
underlying compiler having retpoline support.

Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Fix description of option to use indirect branches instead of
   indirect calls.
---
 xen/arch/x86/Kconfig |  4 ----
 xen/arch/x86/arch.mk |  2 ++
 xen/common/Kconfig   | 16 ++++++++++++++++
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 2fa456292b..7c73802adc 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -38,10 +38,6 @@ config GCC_INDIRECT_THUNK
 config CLANG_INDIRECT_THUNK
 	def_bool $(cc-option,-mretpoline-external-thunk)
 
-config INDIRECT_THUNK
-	def_bool y
-	depends on GCC_INDIRECT_THUNK || CLANG_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 f2aa2a515f..0597e714f9 100644
--- a/xen/arch/x86/arch.mk
+++ b/xen/arch/x86/arch.mk
@@ -42,6 +42,7 @@ CFLAGS += -mno-red-zone -fpic
 # SSE setup for variadic function calls.
 CFLAGS += -mno-sse $(call cc-option,$(CC),-mskip-rax-setup)
 
+ifeq ($(CONFIG_INDIRECT_THUNK),y)
 # 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
@@ -49,6 +50,7 @@ CFLAGS-$(CONFIG_GCC_INDIRECT_THUNK) += -fno-jump-tables
 
 # Enable clang retpoline support if available.
 CFLAGS-$(CONFIG_CLANG_INDIRECT_THUNK) += -mretpoline-external-thunk
+endif
 
 # If supported by the compiler, reduce stack alignment to 8 bytes. But allow
 # this to be overridden elsewhere.
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index db687b1785..e688e45513 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -146,6 +146,22 @@ config SPECULATIVE_HARDEN_GUEST_ACCESS
 
 	  If unsure, say Y.
 
+config INDIRECT_THUNK
+	bool "Speculative Branch Target Injection Protection"
+	depends on X86 && (GCC_INDIRECT_THUNK || CLANG_INDIRECT_THUNK)
+	default y
+	help
+	  Contemporary processors may use speculative execution as a
+	  performance optimisation, but this can potentially be abused by an
+	  attacker to leak data via speculative sidechannels.
+
+	  One source of data leakage is via branch target injection.
+
+	  When enabled, indirect branches are implemented using a new construct
+	  called "retpoline" that prevents speculation.
+
+	  If unsure, say Y.
+
 endmenu
 
 config HYPFS
-- 
2.34.1

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

[Jan Beulich]

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

[Jan Beulich]

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 3/3] x86/Kconfig: introduce option to select retpoline usage

[Jan Beulich]

On 16.02.2022 17:21, Roger Pau Monne wrote:
> Add a new Kconfig option under the "Speculative hardening" section
> that allows selecting whether to enable retpoline. This depends on the
> underlying compiler having retpoline support.
> 
> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

There's one aspect though which I would like to see Arm maintainer
input on:

> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -38,10 +38,6 @@ config GCC_INDIRECT_THUNK
>  config CLANG_INDIRECT_THUNK
>  	def_bool $(cc-option,-mretpoline-external-thunk)
>  
> -config INDIRECT_THUNK
> -	def_bool y
> -	depends on GCC_INDIRECT_THUNK || CLANG_INDIRECT_THUNK

Moving this ...

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -146,6 +146,22 @@ config SPECULATIVE_HARDEN_GUEST_ACCESS
>  
>  	  If unsure, say Y.
>  
> +config INDIRECT_THUNK
> +	bool "Speculative Branch Target Injection Protection"
> +	depends on X86 && (GCC_INDIRECT_THUNK || CLANG_INDIRECT_THUNK)

... here despite being explicitly marked x86-specific looks a
little odd. Since the dependencies are x86-specific, dropping
X86 from here would make my slight concern go away.

Jan

Re: [PATCH v2 3/3] x86/Kconfig: introduce option to select retpoline usage

[Roger Pau Monné]

On Thu, Feb 17, 2022 at 10:07:32AM +0100, Jan Beulich wrote:
> On 16.02.2022 17:21, Roger Pau Monne wrote:
> > Add a new Kconfig option under the "Speculative hardening" section
> > that allows selecting whether to enable retpoline. This depends on the
> > underlying compiler having retpoline support.
> > 
> > Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> There's one aspect though which I would like to see Arm maintainer
> input on:
> 
> > --- a/xen/arch/x86/Kconfig
> > +++ b/xen/arch/x86/Kconfig
> > @@ -38,10 +38,6 @@ config GCC_INDIRECT_THUNK
> >  config CLANG_INDIRECT_THUNK
> >  	def_bool $(cc-option,-mretpoline-external-thunk)
> >  
> > -config INDIRECT_THUNK
> > -	def_bool y
> > -	depends on GCC_INDIRECT_THUNK || CLANG_INDIRECT_THUNK
> 
> Moving this ...
> 
> > --- a/xen/common/Kconfig
> > +++ b/xen/common/Kconfig
> > @@ -146,6 +146,22 @@ config SPECULATIVE_HARDEN_GUEST_ACCESS
> >  
> >  	  If unsure, say Y.
> >  
> > +config INDIRECT_THUNK
> > +	bool "Speculative Branch Target Injection Protection"
> > +	depends on X86 && (GCC_INDIRECT_THUNK || CLANG_INDIRECT_THUNK)
> 
> ... here despite being explicitly marked x86-specific looks a
> little odd. Since the dependencies are x86-specific, dropping
> X86 from here would make my slight concern go away.

Right - I've added the X86 because I was concerned about GCC or CLANG
also exposing the repoline options on Arm, but that's not an issue
because the compiler tests are only done for x86 anyway.

Feel free to drop the 'X86 &&' and the parentheses if you wish.
Otherwise I can resend if you prefer.

Thanks, Roger.

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

[Roger Pau Monné]

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

[Jan Beulich]

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

Re: [PATCH v2 3/3] x86/Kconfig: introduce option to select retpoline usage

[Jan Beulich]

On 17.02.2022 11:34, Roger Pau Monné wrote:
> On Thu, Feb 17, 2022 at 10:07:32AM +0100, Jan Beulich wrote:
>> On 16.02.2022 17:21, Roger Pau Monne wrote:
>>> Add a new Kconfig option under the "Speculative hardening" section
>>> that allows selecting whether to enable retpoline. This depends on the
>>> underlying compiler having retpoline support.
>>>
>>> Requested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> There's one aspect though which I would like to see Arm maintainer
>> input on:
>>
>>> --- a/xen/arch/x86/Kconfig
>>> +++ b/xen/arch/x86/Kconfig
>>> @@ -38,10 +38,6 @@ config GCC_INDIRECT_THUNK
>>>  config CLANG_INDIRECT_THUNK
>>>  	def_bool $(cc-option,-mretpoline-external-thunk)
>>>  
>>> -config INDIRECT_THUNK
>>> -	def_bool y
>>> -	depends on GCC_INDIRECT_THUNK || CLANG_INDIRECT_THUNK
>>
>> Moving this ...
>>
>>> --- a/xen/common/Kconfig
>>> +++ b/xen/common/Kconfig
>>> @@ -146,6 +146,22 @@ config SPECULATIVE_HARDEN_GUEST_ACCESS
>>>  
>>>  	  If unsure, say Y.
>>>  
>>> +config INDIRECT_THUNK
>>> +	bool "Speculative Branch Target Injection Protection"
>>> +	depends on X86 && (GCC_INDIRECT_THUNK || CLANG_INDIRECT_THUNK)
>>
>> ... here despite being explicitly marked x86-specific looks a
>> little odd. Since the dependencies are x86-specific, dropping
>> X86 from here would make my slight concern go away.
> 
> Right - I've added the X86 because I was concerned about GCC or CLANG
> also exposing the repoline options on Arm, but that's not an issue
> because the compiler tests are only done for x86 anyway.
> 
> Feel free to drop the 'X86 &&' and the parentheses if you wish.
> Otherwise I can resend if you prefer.

No need to resend just for this.

Jan

Re: [PATCH v2 0/3] retpoline: add clang support + Kconfig selectable

[Andrew Cooper]

On 16/02/2022 16:21, Roger Pau Monne wrote:
> Hello,
>
> The following series adds retpoline support for clang builds, and also
> allows the user to select whether to enable retpoline support at build
> time via a new Kconfig option.
>
> I've tried adding a suitable description to the Kconfig option, but I'm
> sure there's room for improvement.
>
> Thanks, Roger.
>
> Roger Pau Monne (3):
>   x86/retpoline: rename retpoline Kconfig check to include GCC prefix
>   x86/clang: add retpoline support
>   x86/Kconfig: introduce option to select retpoline usage

I don't particularly want to nitpick, but IMO this would be a lot easier
to follow if we ended up with

config CC_HAS_RETPOLINE
    def_bool $(cc-option,-mindirect-branch-register) ||
$(cc-option,-mretpoline-external-thunk)

config INDIRECT_THUNK
    depends on CC_HAS_RETPOLINE

and

ifeq ($(CONFIG_INDIRECT_THUNK),y)
CFLAGS-$(CONFIG_CC_IS_GCC) += ...
CFLAGS-$(CONFIG_CC_IS_CLANG) += ...
endif

because this reduces the number of CONFIG_* options involved.

Thoughts?

On substantially more minor points, INDIRECT_THUNK wants to be first in
the speculative hardening list, seeing as it is by far and away the most
important one, and I think we should stop writing things like "If
unsure, ..." in the help because it's just parroting the default which
is also rendered to people reading this message.  Our audience here are
developers, and I think we can depend on them to intuit the stated default.

~Andrew

Re: [PATCH v2 0/3] retpoline: add clang support + Kconfig selectable

[Roger Pau Monné]

On Fri, Feb 18, 2022 at 01:58:31PM +0000, Andrew Cooper wrote:
> On 16/02/2022 16:21, Roger Pau Monne wrote:
> > Hello,
> >
> > The following series adds retpoline support for clang builds, and also
> > allows the user to select whether to enable retpoline support at build
> > time via a new Kconfig option.
> >
> > I've tried adding a suitable description to the Kconfig option, but I'm
> > sure there's room for improvement.
> >
> > Thanks, Roger.
> >
> > Roger Pau Monne (3):
> >   x86/retpoline: rename retpoline Kconfig check to include GCC prefix
> >   x86/clang: add retpoline support
> >   x86/Kconfig: introduce option to select retpoline usage
> 
> I don't particularly want to nitpick, but IMO this would be a lot easier
> to follow if we ended up with
> 
> config CC_HAS_RETPOLINE
>     def_bool $(cc-option,-mindirect-branch-register) ||
> $(cc-option,-mretpoline-external-thunk)
> 
> config INDIRECT_THUNK
>     depends on CC_HAS_RETPOLINE
> 
> and
> 
> ifeq ($(CONFIG_INDIRECT_THUNK),y)
> CFLAGS-$(CONFIG_CC_IS_GCC) += ...
> CFLAGS-$(CONFIG_CC_IS_CLANG) += ...
> endif
> 
> because this reduces the number of CONFIG_* options involved.
> 
> Thoughts?

That would reduce one hidden Kconfig option. I don't mind
implementing it that way.

> On substantially more minor points, INDIRECT_THUNK wants to be first in
> the speculative hardening list, seeing as it is by far and away the most
> important one, and I think we should stop writing things like "If
> unsure, ..." in the help because it's just parroting the default which
> is also rendered to people reading this message.  Our audience here are
> developers, and I think we can depend on them to intuit the stated default.

OK, so let me put that one first on the list then, and drop the "If
unsure, "

Thanks, Roger.