by Pavel Machek

[permalink] [raw]

Subject: Re: [PATCH 4/6] s390: add options to change branch prediction behaviour for the kernel

On Wed 2018-02-07 08:00:09, Martin Schwidefsky wrote:
> Add the PPA instruction to the system entry and exit path to switch
> the kernel to a different branch prediction behaviour. The instructions
> are added via CPU alternatives and can be disabled with the "nospec"
> or the "nobp=0" kernel parameter. If the default behaviour selected
> with CONFIG_KERNEL_NOBP is set to "n" then the "nobp=1" parameter can be
> used to enable the changed kernel branch prediction.
>
> Acked-by: Cornelia Huck <[email protected]>
> Signed-off-by: Martin Schwidefsky <[email protected]>
> ---
> arch/s390/Kconfig | 17 ++++++++++++++
> arch/s390/include/asm/processor.h | 1 +
> arch/s390/kernel/alternative.c | 23 +++++++++++++++++++
> arch/s390/kernel/early.c | 2 ++
> arch/s390/kernel/entry.S | 48 +++++++++++++++++++++++++++++++++++++++
> arch/s390/kernel/ipl.c | 1 +
> arch/s390/kernel/smp.c | 2 ++
> 7 files changed, 94 insertions(+)
>
> diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig
> index 0105ce2..d514e25 100644
> --- a/arch/s390/Kconfig
> +++ b/arch/s390/Kconfig
> @@ -540,6 +540,23 @@ config ARCH_RANDOM
>
> If unsure, say Y.
>
> +config KERNEL_NOBP
> + def_bool n
> + prompt "Enable modified branch prediction for the kernel by default"
> + help
> + If this option is selected the kernel will switch to a modified
> + branch prediction mode if the firmware interface is available.
> + The modified branch prediction mode improves the behaviour in
> + regard to speculative execution.
> +
> + With the option enabled the kernel parameter "nobp=0" or "nospec"
> + can be used to run the kernel in the normal branch prediction mode.
> +
> + With the option disabled the modified branch prediction mode is
> + enabled with the "nobp=1" kernel parameter.
> +
> + If unsure, say N.
> +

This could use some improvement.

Afaict the config option only changes the default behaviour? Do we
need the option in such case? (CONFIG_CMDLINE_APPEND can be useful to
avoid some options).

Plus, it is not clear what "modified branch prediction mode is" and if
"improves behaviour" means "faster" or "safer".

Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Attachments:

(No filename) (2.35 kB)
signature.asc (188.00 B)
Digital signature Download all attachments

2018-02-07 10:08:40

On Wed, 2018-02-07 at 11:07 +0100, Pavel Machek wrote:
> This is really unfortunate naming of kernel option.
>
> spectre_v2=off sounds like we are turning the "bug" off, but i somehow
> suspect you are turning the bug _workaround_ off.

That's consistent with what we have on x86.

> > +ifdef CONFIG_EXPOLINE
> > +  ifeq ($(call cc-option-yn,$(CC_FLAGS_MARCH) -mindirect-branch=thunk),y)
> > +    CC_FLAGS_EXPOLINE := -mindirect-branch=thunk
> > +    CC_FLAGS_EXPOLINE += -mfunction-return=thunk
> > +    CC_FLAGS_EXPOLINE += -mindirect-branch-table
> > +    export CC_FLAGS_EXPOLINE
> > +    cflags-y += $(CC_FLAGS_EXPOLINE)
> > +  else
> > +    $(warning "Your gcc lacks the -mindirect-branch= option")
> > +  endif
> > +endif

That isn't, though. Linus asked us to drop the $(warning) part.

... and then spent a week building with a non-retpoline compiler and
not noticing, so he might have changed his mind ;)

Attachments:

smime.p7s (5.09 kB)

2018-02-07 12:18:57

by Martin Schwidefsky

[permalink] [raw]

Subject: Re: [PATCH 6/6] s390: introduce execute-trampolines for branches

On Wed, 07 Feb 2018 12:07:55 +0000
David Woodhouse <[email protected]> wrote:

> On Wed, 2018-02-07 at 11:07 +0100, Pavel Machek wrote:
> > This is really unfortunate naming of kernel option.
> >
> > spectre_v2=off sounds like we are turning the "bug" off, but i somehow
> > suspect you are turning the bug _workaround_ off.
>
> That's consistent with what we have on x86.
>
> > > +ifdef CONFIG_EXPOLINE
> > > +  ifeq ($(call cc-option-yn,$(CC_FLAGS_MARCH) -mindirect-branch=thunk),y)
> > > +    CC_FLAGS_EXPOLINE := -mindirect-branch=thunk
> > > +    CC_FLAGS_EXPOLINE += -mfunction-return=thunk
> > > +    CC_FLAGS_EXPOLINE += -mindirect-branch-table
> > > +    export CC_FLAGS_EXPOLINE
> > > +    cflags-y += $(CC_FLAGS_EXPOLINE)
> > > +  else
> > > +    $(warning "Your gcc lacks the -mindirect-branch= option")
> > > +  endif
> > > +endif
>
> That isn't, though. Linus asked us to drop the $(warning) part.
>
> ... and then spent a week building with a non-retpoline compiler and
> not noticing, so he might have changed his mind ;)

I found the warning to have some value, it helps for the case where my
fingers are faster than my brain and I type "make" instead of "smake"
which uses the alternative compiler with the required support.

@Linus: do you want a warning or prefer not to have one ?

--
blue skies,
Martin.

"Reality continues to ruin my life." - Calvin.

2018-02-07 12:21:06

2018-02-08 9:57 GMT+09:00 Kees Cook <[email protected]>:
> On Thu, Feb 8, 2018 at 10:44 AM, Masahiro Yamada
> <[email protected]> wrote:
>> 2018-02-08 2:55 GMT+09:00 Linus Torvalds <[email protected]>:
>>> What I would really want - and this is entirely unrelated to this
>>> particular case - is to have those damn compiler option tests as part
>>> of the config phase in general. We now have about a million of these
>>> crazy things, where we have config options that simply depend on which
>>> compiler we have, and we have no sane way to show them at
>>> configuration time.
>>>
>>> Though Andrew's tree I got yet another ugly hack
>>> (CONFIG_CC_STACKPROTECTOR_AUTO) that handles just _one_ special case
>>> by turning it into a special magic Kconfig entry in the main Makefile.
>>> See commit 44c6dc940b19 ("Makefile: introduce
>>> CONFIG_CC_STACKPROTECTOR_AUTO"). I wasn't sure if I really wanted it,
>>> and honestly, I'm still thinking of just reverting it, because it's
>>> _so_ ugly and _so_ wrong.
>>>
>>> What we need is an extension to the Kconfig language itself so that we can do
>>>
>>> config CC_HAS_RETPOLINE
>>> cc_option "-mindirect-branch=thunk -mindirect-branch-table"
>>>
>>> or something. And then we can make sane _conditional_ dependencies at
>>> Kconfig time, and our makefiles would be much cleaner too when you
>>> could just do
>>>
>>> cflags-$(USE_RETPOLINE) += -mfunction-return=thunk -mindirect-branch-table
>>>
>>> because the validity of the C compiler flag has been tested when configuring.
>>>
>>> And then we could add that warning at configure time (or just disable
>>> the option there thanks to "depends on CC_HAS_xyz" logic).
>>>
>>> All our compiler option handling right now is just nasty nasty nasty crud.
>>>
>>> Adding more people in the hopes that somebody gets motivated.. I've
>>> talked about this before, so far we haven't made any progress.
>>
>>
>> Sorry for slow progress.
>>
>> I agreed this before, and still motivated.
>> (because I also motivated to remove kbuild cache.
>> This turned out not so clever as I first thought)
>>
>> I was trying to do this, but in this development cycle
>> I spent most of my time to flush out lots of piled up Kconfig patches.
>> Sorry.
>>
>> Unless somebody is working, I will.

I sent the first draft.

For people interested, I pushed the series in the following too.

git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git
kconfig-opt-shell-rfc

--
Best Regards
Masahiro Yamada