Re: [Xen-devel] [PATCH v2 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_HARDEN_BRANCH and disable it

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.10.2019 16:32, Andrew Cooper wrote:
> The code generation for barrier_nospec_true() is not correct; the lfence
> instructions are generally too early in the instruction stream, resulting in a
> performance hit but no additional speculative safety.
> 
> This is caused by inline assembly trying to fight the compiler optimiser, and
> the optimiser winning.  There is no clear way to achieve safety, so turn the
> perf hit off for now.

For one (following the v1 thread which was still in progress when you
sent this) and important but not (explicitly) mentioned aspect here is
that in case affected inline functions to not get inlined, the LFENCE
would end up in the function body rather than in the caller. I think
this wants making explicit.

As to "no clear way" - is the "convert all involved inline functions
to always_inline" not a sufficiently promising approach, until aid by
compilers is available? (For gcc 9 the asm inline() approach could also
be chosen, and shouldn't be overly difficult to carry out.)

Finally ...

> --- a/xen/include/asm-x86/nospec.h
> +++ b/xen/include/asm-x86/nospec.h
> @@ -9,13 +9,13 @@
>  /* Allow to insert a read memory barrier into conditionals */
>  static always_inline bool barrier_nospec_true(void)
>  {
> -#ifdef CONFIG_HVM
> -    alternative("", "lfence", X86_FEATURE_SC_L1TF_VULN);
> +#ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH
> +    asm volatile ( "lfence" ::: "memory" );
>  #endif
>      return true;
>  }

... doesn't this change alone (assuming the config option could be set
to Y) already take care of the issue? By there no longer being the
(misleading to the compiler) complexity of alternative(), there should
be far less (if any) instances of this (and its inline users) not
getting inlined. In fact I wonder whether then the always_inline here
couldn't also be converted back to just inline (except perhaps for
clang, as per the other patch of yours).

Then again if the config option could be set to Y, we'd not want the
LFENCE unconditionally anyway aiui: Hardware affected by neither L1TF
nor the MDS variations (i.e. in particular all of AMD hardware)
shouldn't get penalized. So perhaps it was a bad request of mine to
switch from alternative() to asm(); instead I should have asked that
your use of X86_FEATURE_ALWAYS in v1 be replaced by something that
would actually trigger a build error (or work correctly) if the config
option became settable to Y.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel