Re: [PATCH 08/37] target/i386: add CPUID feature checks to new decoder

[Richard Henderson]

On 9/12/22 00:03, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   target/i386/tcg/decode-new.c.inc | 51 ++++++++++++++++++++++++++++++++
>   target/i386/tcg/decode-new.h     | 20 +++++++++++++
>   2 files changed, 71 insertions(+)
>
> diff --git a/target/i386/tcg/decode-new.c.inc b/target/i386/tcg/decode-new.c.inc
> index 7f76051b2d..a9b8b6c05f 100644
> --- a/target/i386/tcg/decode-new.c.inc
> +++ b/target/i386/tcg/decode-new.c.inc
> @@ -83,6 +83,7 @@
>   #define X86_OP_ENTRY0(op, ...)                                    \
>       X86_OP_ENTRY3(op, None, None, None, None, None, None, ## __VA_ARGS__)
>   
> +#define cpuid(feat) .cpuid = X86_FEAT_##feat,
>   #define i64 .special = X86_SPECIAL_i64,
>   #define o64 .special = X86_SPECIAL_o64,
>   #define xchg .special = X86_SPECIAL_Locked,
> @@ -506,6 +507,52 @@ static bool decode_insn(DisasContext *s, CPUX86State *env, 
> X86DecodeFunc decode_
>       return true;
>   }
>   
> +static bool has_cpuid_feature(DisasContext *s, X86CPUIDFeature cpuid)
> +{
> +    switch (cpuid) {
> +    case X86_FEAT_None:
> +        return true;
> +    case X86_FEAT_MOVBE:
> +        return (s->cpuid_ext_features & CPUID_EXT_MOVBE);
> +    case X86_FEAT_PCLMULQDQ:
> +        return (s->cpuid_ext_features & CPUID_EXT_PCLMULQDQ);
> +    case X86_FEAT_SSE:
> +        return (s->cpuid_ext_features & CPUID_SSE);
> +    case X86_FEAT_SSE2:
> +        return (s->cpuid_ext_features & CPUID_SSE2);
> +    case X86_FEAT_SSE3:
> +        return (s->cpuid_ext_features & CPUID_EXT_SSE3);
> +    case X86_FEAT_SSSE3:
> +        return (s->cpuid_ext_features & CPUID_EXT_SSSE3);
> +    case X86_FEAT_SSE41:
> +        return (s->cpuid_ext_features & CPUID_EXT_SSE41);
> +    case X86_FEAT_SSE42:
> +        return (s->cpuid_ext_features & CPUID_EXT_SSE42);
> +    case X86_FEAT_AES:
> +        if (s->vex_l) {
> +            return (s->cpuid_7_0_ecx_features & CPUID_7_0_ECX_VAES);
> +        } else {
> +            return (s->cpuid_ext_features & CPUID_EXT_AES);
> +        }
> +    case X86_FEAT_AVX:
> +        return (s->cpuid_ext_features & CPUID_EXT_AVX);
> +
> +    case X86_FEAT_SSE4A:
> +        return (s->cpuid_ext3_features & CPUID_EXT3_SSE4A);
> +
> +    case X86_FEAT_ADX:
> +        return (s->cpuid_7_0_ebx_features & CPUID_7_0_EBX_ADX);
> +    case X86_FEAT_BMI1:
> +        return (s->cpuid_7_0_ebx_features & CPUID_7_0_EBX_BMI1);
> +    case X86_FEAT_BMI2:
> +        return (s->cpuid_7_0_ebx_features & CPUID_7_0_EBX_BMI2);
> +    case X86_FEAT_AVX2:
> +        return (s->cpuid_7_0_ebx_features & CPUID_7_0_EBX_AVX2);
> +    default:
> +        abort();
> +    }

g_assert_not_reached().

I'll also note that for cases like this, where every case returns, and is intended to be 
present, it can be better to leave the default case outside the switch, so that the 
compiler can Werror early for missing enumerator entries instead of aborting at runtime.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~