Re: [PATCH 05/37] target/i386: add core of new i386 decoder

[Richard Henderson <richard.henderson@linaro.org>]

On 9/12/22 00:03, Paolo Bonzini wrote:
> +    case X86_TYPE_B:  /* VEX.vvvv selects a GPR */
> +        op->unit = X86_OP_INT;
> +        op->n = s->vex_v;
> +        break;

Could use a comment for where missing vex prefix is diagnosed.
I guess it's one of the "vexN" group markers in the insn table?

> +    case X86_TYPE_S:  /* reg selects a segment register */
> +        op->unit = X86_OP_SEG;
> +        goto get_reg;
> +
> +        goto get_reg;

Stray goto.

> +
> +    case X86_TYPE_V:  /* reg in the modrm byte selects an XMM/YMM register */
> +        if (decode->e.special == X86_SPECIAL_MMX &&
> +            !(s->prefix & (PREFIX_DATA | PREFIX_REPZ | PREFIX_REPNZ))) {
> +    case X86_TYPE_P:  /* reg in the modrm byte selects an MMX register */
> +            op->unit = X86_OP_MMX;
> +        } else {
> +            op->unit = X86_OP_SSE;
> +        }
> +    get_reg:

Nesting P into the if works, but it's ugly.
Better to separate it out as

     case X86_TYPE_P:
         op->unit = X86_OP_MMX;
         goto get_reg;

> +    case X86_TYPE_W:  /* XMM/YMM modrm operand */
> +        if (decode->e.special == X86_SPECIAL_MMX &&
> +            !(s->prefix & (PREFIX_DATA | PREFIX_REPZ | PREFIX_REPNZ))) {
> +    case X86_TYPE_Q:  /* MMX modrm operand */
> +            op->unit = X86_OP_MMX;
> +        } else {
> +            op->unit = X86_OP_SSE;
> +        }
> +        goto get_modrm;

Likewise.

> +    case X86_TYPE_U:  /* R/M in the modrm byte selects an XMM/YMM register */
> +        if (decode->e.special == X86_SPECIAL_MMX &&
> +            !(s->prefix & (PREFIX_DATA | PREFIX_REPZ | PREFIX_REPNZ))) {
> +    case X86_TYPE_N:  /* R/M in the modrm byte selects an MMX register */
> +            op->unit = X86_OP_MMX;
> +        } else {
> +            op->unit = X86_OP_SSE;
> +        }
> +        goto get_modrm_reg;

Likewise.

> +    case X86_TYPE_H:  /* For AVX, VEX.vvvv selects an XMM/YMM register */
> +        if ((s->prefix & PREFIX_VEX)) {
> +            op->unit = X86_OP_SSE;
> +            op->n = s->vex_v;
> +            break;

Similar to X86_TYPE_B, should this diagnose error if missing VEX?

> +  e X86_TYPE_J:  /* Relative offset for a jump */
> +        op->unit = X86_OP_IMM;
> +        decode->immediate = insn_get_signed(env, s, op->ot);

Mailer damage?

> +        decode->immediate += s->pc - s->cs_base;

Please consider

https://lore.kernel.org/qemu-devel/20220906100932.343523-1-richard.henderson@linaro.org/

or at least the first half of the patch set, which rationalizes and consolidates the 
handing of s->cs_base.

> +    default:
> +        abort();

g_assert_not_reached().

> +static bool decode_insn(DisasContext *s, CPUX86State *env, X86DecodeFunc decode_func,
> +                        X86DecodedInsn *decode)
> +{
> +    X86OpEntry *e = &decode->e;
> +
> +    decode_func(s, env, e, &decode->b);
> +    while (e->is_decode) {
> +        e->is_decode = false;
> +        e->decode(s, env, e, &decode->b);
> +    }
> +
> +    /* First compute size of operands in order to initialize s->rip_offset.  */
> +    if (e->op0 != X86_TYPE_None) {
> +        if (!decode_op_size(s, e, e->s0, &decode->op[0].ot)) {
> +            return false;
> +        }
> +        if (e->op0 == X86_TYPE_I) {
> +            s->rip_offset += 1 << decode->op[0].ot;
> +        }
> +    }
> +    if (e->op1 != X86_TYPE_None) {
> +        if (!decode_op_size(s, e, e->s1, &decode->op[1].ot)) {
> +            return false;
> +        }
> +        if (e->op1 == X86_TYPE_I) {
> +            s->rip_offset += 1 << decode->op[1].ot;
> +        }
> +    }
> +    if (e->op2 != X86_TYPE_None) {
> +        if (!decode_op_size(s, e, e->s2, &decode->op[2].ot)) {
> +            return false;
> +        }
> +        if (e->op2 == X86_TYPE_I) {
> +            s->rip_offset += 1 << decode->op[2].ot;
> +        }
> +    }
> +    if (e->op3 != X86_TYPE_None) {
> +        assert(e->op3 == X86_TYPE_I && e->s3 == X86_SIZE_b);
> +        s->rip_offset += 1;
> +    }
> +
> +    if (e->op0 != X86_TYPE_None &&
> +        !decode_op(s, env, decode, &decode->op[0], e->op0, decode->b)) {
> +        return false;
> +    }
> +
> +    if (e->op1 != X86_TYPE_None &&
> +        !decode_op(s, env, decode, &decode->op[1], e->op1, decode->b)) {
> +        return false;
> +    }
> +
> +    if (e->op2 != X86_TYPE_None &&
> +        !decode_op(s, env, decode, &decode->op[2], e->op2, decode->b)) {
> +        return false;
> +    }
> +
> +    if (e->op3 != X86_TYPE_None) {
> +        decode->immediate = insn_get_signed(env, s, MO_8);
> +    }
> +
> +    return true;
> +}
> +
> +/* convert one instruction. s->base.is_jmp is set if the translation must
> +   be stopped. Return the next pc value */
> +static target_ulong disas_insn_new(DisasContext *s, CPUState *cpu, int b)

Note patch 2 from the cs_base cleanup above changes the return type from disas_insn to bool.

> +{
> +    CPUX86State *env = cpu->env_ptr;
> +    bool first = true;
> +    X86DecodedInsn decode;
> +    X86DecodeFunc decode_func = decode_root;
> +
> +#ifdef CONFIG_USER_ONLY
> +    if (limit) { --limit; }
> +#endif
> +    s->has_modrm = false;
> +#if 0
> +    s->pc_start = s->pc = s->base.pc_next;
> +    s->override = -1;
> +#ifdef TARGET_X86_64
> +    s->rex_w = false;
> +    s->rex_r = 0;
> +    s->rex_x = 0;
> +    s->rex_b = 0;
> +#endif
> +    s->prefix = 0;
> +    s->rip_offset = 0; /* for relative ip address */
> +    s->vex_l = 0;
> +    s->vex_v = 0;
> +    if (sigsetjmp(s->jmpbuf, 0) != 0) {
> +        gen_exception_gpf(s);
> +        return s->pc;
> +    }

Mainline has two longjmp error paths:
(1) insn too long: raise #GP,
(2) insn crosses page boundary, and isn't first in the TB:
     undo processing and defer insn to next TB.

> +static inline target_long insn_get_signed(CPUX86State *env, DisasContext *s, MemOp ot)

No need for inline.


r~