Re: [Qemu-devel] [PATCH 16/17] s390x: translate engine for s390x CPU

[Peter Maydell]

On 24 March 2011 15:58, Alexander Graf <address@hidden> wrote:
> diff --git a/target-s390x/translate.c b/target-s390x/translate.c

> +typedef struct DisasContext DisasContext;
> +struct DisasContext {
> +    uint64_t pc;
> +    int is_jmp;
> +    enum cc_op cc_op;
> +    CPUS390XState *env;
> +    struct TranslationBlock *tb;
> +};

I don't think anything actually uses dc->env, does it?
(I like the way that almost none of the translate.c code
gets a CPUState pointer, makes it hard to accidentally write
buggy code that relies on things not in the tb_flags.)

> +static char cpu_reg_names[10*3 + 6*4];

I can see code ins390x_translate_init() which sets this up, but
I can't see anything which uses it?

> +#if 0  /* reads four when it should read only 3 */
> +    case 2:

Is there any point having #if'd out broken code?
Either fix it and enable it, or just have a comment
to the effect that we could have optimised versions
for cases 2, 4, 5, 6 but currently do not.

> +    case 0x4:  /* LMG      R1,R3,D2(B2)     [RSE] */
> +    case 0x24: /* STMG     R1,R3,D2(B2)     [RSE] */
> +    case 0x26: /* STMH     R1,R3,D2(B2)     [RSE] */
> +    case 0x96: /* LMH      R1,R3,D2(B2)     [RSE] */
> +        /* Apparently, unrolling lmg/stmg of any size gains performance -
> +           even for very long ones... */

Doesn't this take you over MAX_OP_PER_INSTR for some cases?

> +            tmp2 = tcg_const_i64((((uint64_t)i2) << 48) | 
> 0x0000ffffffffffffULL);

This line is over 80 chars, as are a handful of others in this file.

> +    case 0xa: /* SVC    I         [RR] */
> +        insn = ld_code2(s->pc);
> +        debug_insn(insn);
> +        i = insn & 0xff;
> +#ifdef CONFIG_USER_ONLY
> +        s->pc += 2;
> +#endif
> +        update_psw_addr(s);
> +        gen_op_calc_cc(s);

Why do we only need to update s->pc if CONFIG_USER_ONLY?
Not saying it's wrong, but it could use an explanatory comment...

-- PMM