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
