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

[Alexander Graf]

On 28.03.2011, at 17:40, Peter Maydell wrote:

> 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.)

Good idea :)

> 
>> +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?

It's used during the setup (and needs to be kept alive after that - hence the 
global):

    p = cpu_reg_names;
    for (i = 0; i < 16; i++) {
        snprintf(p, cpu_reg_names_size, "r%d", i);
        regs[i] = tcg_global_mem_new(TCG_AREG0,
                                     offsetof(CPUState, regs[i]), p);
        p += (i < 10) ? 3 : 4;
        cpu_reg_names_size -= (i < 10) ? 3 : 4;
    }


> 
>> +#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.

I wanted to keep it in as documentation. But yeah, might make sense to just 
remove it.

> 
>> +    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?

I haven't encountered any case where it does.

> 
>> +            tmp2 = tcg_const_i64((((uint64_t)i2) << 48) | 
>> 0x0000ffffffffffffULL);
> 
> This line is over 80 chars, as are a handful of others in this file.

Yeah, I generally see the 80 char limit as soft limit and make it hard on ~90. 
If a line is only over it by very little, readability doesn't improve by 
breaking it up. So far, everyone agreed to that approach :).

> 
>> +    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...

The user code needs to know where it jumps back to, while the exception 
generation code needs to get the exact position it was in to generate some more 
metadata. I'm not sure a comment really would be helpful here - it's an 
implementation detail that is hard to explain properly in a few words.


Alex