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

Message

Roger Pau Monné Feb. 16, 2022, 4:21 p.m. UTC

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(-)

Comments

Andrew Cooper Feb. 18, 2022, 1:58 p.m. UTC | #1

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

Roger Pau Monné Feb. 18, 2022, 2:17 p.m. UTC | #2

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.