Re: [PATCH v2 07/42] Enforce VEX encoding restrictions

[Paolo Bonzini]

On 4/25/22 00:01, Paul Brook wrote:
> +/* If a VEX prefix is used then it must have V=1111b */
> +#define CHECK_AVX_V0(s) do { \
> +    CHECK_AVX(s); \
> +    if ((s->prefix & PREFIX_VEX) && (s->vex_v != 0)) \
> +        goto illegal_op; \
> +    } while (0)
> +

What do you think about

#define CHECK_AVX(s, flags) \
    do {
        if ((s->prefix & PREFIX_VEX) && !(env->hflags & HF_AVX_EN_MASK)) {
            goto illegal_op;
        }
        if ((flags) & SSE_OPF_AVX2) {
            CHECK_AVX2(s);
        }
        if ((flags) & SSE_OPF_AVX_128) {
            CHECK_AVX_128(s);
        }
        if ((flags) & SSE_OPF_V0) {
            CHECK_V0(s);
        }
    }

Macros such as CHECK_AVX_V0_128(s) would become CHECK_AVX(s, SSE_OPF_V0 
| SSE_OPF_AVX_128); a bit longer but still bearable.  And here you would 
have:

>           case 0x210: /* movss xmm, ea */
>               if (mod != 3) {
> +                CHECK_AVX_V0_128(s);
>                   gen_lea_modrm(env, s, modrm);
>                   gen_op_ld_v(s, MO_32, s->T0, s->A0);
>                   tcg_gen_st32_tl(s->T0, cpu_env,
> @@ -3379,6 +3432,7 @@ static void gen_sse(CPUX86State *env, DisasContext *s, 
> int b,
>                   tcg_gen_st32_tl(s->T0, cpu_env,
>                                   offsetof(CPUX86State, 
> xmm_regs[reg].ZMM_L(3)));
>               } else {
> +                CHECK_AVX_128(s);

    CHECK_AVX(s, SSE_OPF_AVX_128);
    if (mod != 3) {
        CHECK_V0(s);
        ...
    } else {
        ...
    }

Another possibility is to add SSE_OPF_V0_MEM (i.e. V0 if mod != 3), and use

    CHECK_AVX(s, SSE_OPF_AVX_128 | SSE_OPF_AVX_V0_MEM);


It's okay not to move _all_ flags checks in the macros, but for example 
here:

> +            if (op6.ext_mask == CPUID_EXT_AVX
> +                    && (s->prefix & PREFIX_VEX) == 0) {
> +                goto illegal_op;
> +            }
> +            if (op6.flags & SSE_OPF_AVX2) {
> +                CHECK_AVX2(s);
> +            }
> +
>               if (b1) {
> +                if (op6.flags & SSE_OPF_V0) {
> +                    CHECK_AVX_V0(s);
> +                } else {
> +                    CHECK_AVX(s);
> +                }
>                   op1_offset = offsetof(CPUX86State,xmm_regs[reg]);
> +
> +                if (op6.flags & SSE_OPF_MMX) {
> +                    CHECK_AVX2_256(s);
> +                }

there is a lot of room for using a flags-extended CHECK_AVX macro.


Also, SSE_OPF_V0 seems overloaded, because it means depending on the 
place in the code:

- always 2-operand

- 2-operand except if SCALAR && !CMP

- 2-operand except if SCALAR && !CMP && has REPZ/REPNZ prefixes

It is not clear to me if the former overlaps with the last (i.e. if 
there are any SCALAR && !CMP operations that are always 2-operand). If 
so, please use different constants for all three; if not, please use a 
different constant for the last, e.g. SSE_OPF_V0 and SSE_OPF_VEC_V0, so 
that the difference is visible in the flags-extended CHECK_AVX macro.

Also related to overloading, here and in patch 37 there is code like this:

> +            if (op7.flags & SSE_OPF_BLENDV && !(s->prefix & PREFIX_VEX)) {
> +                /* Only VEX encodings are valid for these blendv opcodes */
> +                goto illegal_op;
> +            }

If this is for all SSE_OPF_BLENDV operations, it can be handled in the 
flags-enabled CHECK_AVX() macro above.  If it is only for some, it 
should be a new flag SSE_OPF_VEX_ONLY.

Finally (replying here just to keep things together), patch 29 has "We 
abuse the SSE_OPF_SCALAR flag to select the memory operand width 
appropriately".  Please don't; use a separate function that takes in "b" 
and returns a bool, with just a switch statement in it.

> +            CHECK_AVX(s);
> +            scalar_op = (s->prefix & PREFIX_VEX)
> +                && (op7.flags & SSE_OPF_SCALAR)
> +                && !(op7.flags & SSE_OPF_CMP);
> +            if (is_xmm && (op7.flags & SSE_OPF_MMX)) {
> +                CHECK_AVX2_256(s);
> +            }

I think the is_xmm check is always true here (inside case 0x03a: case 
0x13a:, i.e. b is inside the 0x10..0x5f range)?

Paolo