Re: [PATCH] x86/nospec: Fix evaluate_nospec() code generation under Clang

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Apr 25, 2022 at 06:56:03PM +0100, Andrew Cooper wrote:
> It turns out that evaluate_nospec() code generation is not safe under Clang.
> Given:
> 
>   void eval_nospec_test(int x)
>   {
>       if ( evaluate_nospec(x) )
>           asm volatile ("nop #true" ::: "memory");
>       else
>           asm volatile ("nop #false" ::: "memory");
>   }
> 
> Clang emits:
> 
>   <eval_nospec_test>:
>          0f ae e8                lfence
>          85 ff                   test   %edi,%edi
>          74 02                   je     <eval_nospec_test+0x9>
>          90                      nop
>          c3                      ret
>          90                      nop
>          c3                      ret
> 
> which is not safe because the lfence has been hoisted above the conditional
> jump.  Clang concludes that both barrier_nospec_true()'s have identical side
> effects and can safely be merged.
> 
> Clang can be persuaded that the side effects are different if there are
> different comments in the asm blocks.  This is fragile, but no more fragile
> that other aspects of this construct.
> 
> Introduce barrier_nospec_false() with a separate internal comment to prevent
> Clang merging it with barrier_nospec_true() despite the otherwise-identical
> content.  The generated code now becomes:
> 
>   <eval_nospec_test>:
>          85 ff                   test   %edi,%edi
>          74 05                   je     <eval_nospec_test+0x9>
>          0f ae e8                lfence
>          90                      nop
>          c3                      ret
>          0f ae e8                lfence
>          90                      nop
>          c3                      ret
> 
> which has the correct number of lfence's, and in the correct place.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Like Jan I wonder what the clang devs think of this solution.  Is
there any test in clang to assert that comments won't be stripped from
asm blocks before optimization?

> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> ---
>  xen/arch/x86/include/asm/nospec.h | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/include/asm/nospec.h 
> b/xen/arch/x86/include/asm/nospec.h
> index 5312ae4c6f31..7150e76b87fb 100644
> --- a/xen/arch/x86/include/asm/nospec.h
> +++ b/xen/arch/x86/include/asm/nospec.h
> @@ -10,15 +10,26 @@
>  static always_inline bool barrier_nospec_true(void)
>  {
>  #ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH
> -    alternative("lfence", "", X86_FEATURE_SC_NO_BRANCH_HARDEN);
> +    alternative("lfence #nospec-true", "", X86_FEATURE_SC_NO_BRANCH_HARDEN);
>  #endif
>      return true;
>  }
>  
> +static always_inline bool barrier_nospec_false(void)
> +{
> +#ifdef CONFIG_SPECULATIVE_HARDEN_BRANCH
> +    alternative("lfence #nospec-false", "", X86_FEATURE_SC_NO_BRANCH_HARDEN);
> +#endif
> +    return false;
> +}
> +
>  /* Allow to protect evaluation of conditionals with respect to speculation */
>  static always_inline bool evaluate_nospec(bool condition)
>  {
> -    return condition ? barrier_nospec_true() : !barrier_nospec_true();
> +    if ( condition )
> +        return barrier_nospec_true();
> +    else
> +        return barrier_nospec_false();
>  }

Is the switch from using a ternary operator also a requirement for
clang not optimizing this? (I would assume not, but better ask)

Thanks, Roger.