by Linus Torvalds

[permalink] [raw]

Subject: Re: [PATCH RFC rebase 3/9] powerpc/64: Use barrier_nospec in syscall entry

On Fri, Mar 16, 2018 at 2:15 AM, Michal Suchánek <[email protected]> wrote:
>
> As far as I understand barriers they separate code before the barrier
> and code after the barrier.

Almost certainly not. Even if you were to do an expensive
serialization before the branch, the branch will still predict after
the serialization.

The thing is, it doesn't make sense to insert a barrier before a
conditional branch for Spectre mitigation.

The problem is not that the data isn't ready for the branch - the
problem is that the branch is predicted _regardless_ of the data.

Sure, some micro-architecture might not predict branches at all if
they have a stable conditional, so a barrier _can_ make sense.

But fundamentally, good branch prediction - in order to be optimal -
has to happen before instructions have even been parsed, much less
things like "stable conditional register state" having been decided
on. You'll want to do I$ prefetching etc.

So the problem is that even if the data is ready, the branch will be
predicted according to some unrelated historical data, and a barrier
to make the branch conditional be stable is pointless.

A barrier *after* the branch, making sure that you don't actually
start executing instructions past it (even if you might have predicted
and fetched stuff past it) *if* you have mis-predicted the previous
branch, is what a sane architecture would specify.

Of course, on x86, we mostly tried to avoid branch prediction being
the critical problem and having to have barriers by just making it an
address generation dependency instead. That should presumably work on
powerpc too, since address generation is part of the memory ordering
definition. But obviously a microarchitecture *could* end up
speculating and just redoing even for memory ordering, and maybe the
ppc architects prefer the barrier since they are already used to crazy
and not very well architected barriers elsewhere.

Linus

2018-03-23 16:01:52

by Diana Madalina Craciun

[permalink] [raw]

Subject: Re: [PATCH RFC rebase 9/9] powerpc/64: barrier_nospec: Add commandline trigger

On 3/15/2018 9:34 PM, Michal Suchanek wrote:
> Add commandline options spectre_v2 and nospectre_v2
>
> These are named same as similar x86 options regardless of actual effect
> to not require platform-specific configuration.
>
> Supported options:
> nospectre_v2 or spectre_v2=off - speculation barrier not used
> spectre_v2=on or spectre_v2=auto - speculation barrier used

Why the barrier is enabled only for spectre variant 2 mitigations? It
can be used as well for variant 1 mitigations. In fact I am not sure
that the places where it is used fall under spectre 2 at all.

>
> Changing the settings after boot is not supported and VM migration may
> change requirements so auto is same as on.
>
> Based on s390 implementation
>
> Signed-off-by: Michal Suchanek <[email protected]>
> ---
> arch/powerpc/kernel/setup_64.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)
>
> diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
> index f6678a7b6114..c74e656265df 100644
> --- a/arch/powerpc/kernel/setup_64.c
> +++ b/arch/powerpc/kernel/setup_64.c
> @@ -840,6 +840,28 @@ static int __init handle_no_pti(char *p)
> }
> early_param("nopti", handle_no_pti);
>
> +static int __init nospectre_v2_setup_early(char *str)
> +{
> + no_nospec = true;
> + return 0;
> +}
> +early_param("nospectre_v2", nospectre_v2_setup_early);
> +
> +static int __init spectre_v2_setup_early(char *str)
> +{
> + if (str && !strncmp(str, "on", 2))
> + no_nospec = false;
> +
> + if (str && !strncmp(str, "off", 3))
> + no_nospec = true;
> +
> + if (str && !strncmp(str, "auto", 4))
> + no_nospec = false;
> +
> + return 0;
> +}
> +early_param("spectre_v2", spectre_v2_setup_early);
> +
> static void do_nothing(void *unused)
> {
> /*

Thanks,

Diana