Re: [PATCH 2/2] xen/nospec: Allow evaluate_nospec() to short circuit constant expressions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.03.2024 17:10, Andrew Cooper wrote:
> --- a/xen/include/xen/nospec.h
> +++ b/xen/include/xen/nospec.h
> @@ -18,6 +18,15 @@ static always_inline bool evaluate_nospec(bool cond)
>  #ifndef arch_evaluate_nospec
>  #define arch_evaluate_nospec(cond) cond
>  #endif
> +
> +    /*
> +     * If the compiler can reduce the condition to a constant, then it won't
> +     * be emitting a conditional branch, and there's nothing needing
> +     * protecting.
> +     */
> +    if ( __builtin_constant_p(cond) )
> +        return cond;
> +
>      return arch_evaluate_nospec(cond);
>  }

While for now, even after having some hours for considering, I can't point
out anything concrete that could potentially become a problem here, I
still have the gut feeling that this would better be left in the arch
logic. (There's the oddity of what the function actually expands to if the
#define in context actually takes effect, but that's merely cosmetic.)

The one thing I'm firmly unhappy with is "won't" in the comment: We can't
know what the compiler will do. I've certainly known of compilers which
didn't as you indicate here. That was nothing remotely recent, but
ancient DOS/Windows ones. Still, unlike with e.g. __{get,put}_user_bad()
the compiler doing something unexpected would go entirely silently here.

The other (minor) aspect I'm not entirely happy with is that you insert
between the fallback #define and its use. I think (if we need such a
#define in the first place) the two would better stay close together.

As to the need for the #define: To me

static always_inline bool evaluate_nospec(bool cond)
{
#ifdef arch_evaluate_nospec
    return arch_evaluate_nospec(cond);
#else
    return cond;
#endif
}

or even

static always_inline bool evaluate_nospec(bool cond)
{
#ifdef arch_evaluate_nospec
    return arch_evaluate_nospec(cond);
#endif
    return cond;
}

reads no worse, but perhaps slightly better, and is then consistent with
block_speculation(). At which point the question about "insertion point"
here would hopefully also disappear, as this addition is meaningful only
ahead of the #else.

Jan