Re: [PATCH 3/3] x86: short-circuit certain cpu_has_* when x86-64-v{2,3} are in effect

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Wed, Jul 12, 2023 at 8:36 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> Certain fallback code can be made subject to DCE this way. Note that
> CX16 has no compiler provided manifest constant, so CONFIG_* are used
> there instead. Note also that we don't have cpu_has_movbe nor
> cpu_has_lzcnt (aka cpu_has_abm).
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Jason Andryuk <jandryuk@xxxxxxxxx>

One thought below.

> ---
> Of course we could use IS_ENABLED(CONFIG_X86_64_V<n>) everywhere, but as
> CX16 shows this isn't necessarily better than the #if/#else approach
> based on compiler-provided manifest constants. While not really intended
> to be used that way, it looks as if we could also use
> IS_ENABLED(__POPCNT__) and alike.
>
> We could go further and also short-circuit SSE*, AVX and alike, which we
> don't use outside of the emulator.
>
> --- a/xen/arch/x86/include/asm/cpufeature.h
> +++ b/xen/arch/x86/include/asm/cpufeature.h
> @@ -76,13 +76,19 @@ static inline bool boot_cpu_has(unsigned
>  #define cpu_has_eist            boot_cpu_has(X86_FEATURE_EIST)
>  #define cpu_has_ssse3           boot_cpu_has(X86_FEATURE_SSSE3)
>  #define cpu_has_fma             boot_cpu_has(X86_FEATURE_FMA)
> -#define cpu_has_cx16            boot_cpu_has(X86_FEATURE_CX16)
> +#define cpu_has_cx16            (IS_ENABLED(CONFIG_X86_64_V2) || \
> +                                 IS_ENABLED(CONFIG_X86_64_V3) || \
> +                                 boot_cpu_has(X86_FEATURE_CX16))

If you think there may be more ABI selections in the future, it might
be better to express the "V$N" numerically and check >= 2.  Or you can
add a Kconfig CONFIG_X86_64_CX16 and select that as appropriate.  But
if there aren't going to be more of these, then this is fine.

Regards,
Jason