Re: [Qemu-devel] [PATCH 04/12 v9] linux-user: Support tilegx architecture in linux-user

[Chen Gang]

On 4/10/15 05:44, Peter Maydell wrote:
> On 27 March 2015 at 10:52, Chen Gang <address@hidden> wrote:

[...]
>> +
>> +#define ELF_CLASS   ELFCLASS64
>> +#define ELF_DATA    ELFDATA2LSB
>> +#define ELF_ARCH    EM_TILEGX
>> +
>> +static inline void init_thread(struct target_pt_regs *regs,
>> +                               struct image_info *infop)
>> +{
>> +    regs->lr = infop->entry;
> 
> This is wrong (see later).
> 
>> +    regs->sp = infop->start_stack;
>> +
>> +}
>> +

[...]
>>
>> +#ifdef TARGET_TILEGX
>> +void cpu_loop(CPUTLGState *env)
>> +{
>> +    CPUState *cs = CPU(tilegx_env_get_cpu(env));
>> +    int trapnr;
>> +
>> +    while (1) {
>> +        cpu_exec_start(cs);
>> +        trapnr = cpu_tilegx_exec(env);
>> +        cpu_exec_end(cs);
>> +        switch (trapnr) {
>> +        case TILEGX_EXCP_SYSCALL:
>> +            env->regs[TILEGX_R_RE] = do_syscall(env, env->regs[TILEGX_R_NR],
>> +                                                env->regs[0], env->regs[1],
>> +                                                env->regs[2], env->regs[3],
>> +                                                env->regs[4], env->regs[5],
>> +                                                env->regs[6], env->regs[7]);
>> +            break;
>> +        default:
>> +            exit(-1);
> 
> Calling exit() with negative values is never right (exit codes
> are always positive), and in any case this is the wrong way to
> handle a "can't happen" case in code. If we can never get here
> then you want
>     g_assert_not_reached();
>

OK, thanks.

[...] 
>> +#elif defined(TARGET_TILEGX)
>> +    {
>> +        env->regs[0] = regs->r0;
>> +        env->regs[1] = regs->r1;
>> +        env->regs[2] = regs->r2;
>> +        env->regs[3] = regs->r3;
>> +        env->regs[4] = regs->r4;
>> +        env->regs[5] = regs->r5;
>> +        env->regs[6] = regs->r6;
>> +        env->regs[7] = regs->r7;
>> +        env->regs[8] = regs->r8;
>> +        env->regs[9] = regs->r9;
>> +        env->regs[10] = regs->r10;
>> +        env->regs[11] = regs->r11;
>> +        env->regs[12] = regs->r12;
>> +        env->regs[13] = regs->r13;
>> +        env->regs[14] = regs->r14;
>> +        env->regs[15] = regs->r15;
>> +        env->regs[16] = regs->r16;
>> +        env->regs[17] = regs->r17;
>> +        env->regs[18] = regs->r18;
>> +        env->regs[19] = regs->r19;
>> +        env->regs[20] = regs->r20;
>> +        env->regs[21] = regs->r21;
>> +        env->regs[22] = regs->r22;
>> +        env->regs[23] = regs->r23;
>> +        env->regs[24] = regs->r24;
>> +        env->regs[25] = regs->r25;
>> +        env->regs[26] = regs->r26;
>> +        env->regs[27] = regs->r27;
>> +        env->regs[28] = regs->r28;
>> +        env->regs[29] = regs->r29;
>> +        env->regs[30] = regs->r30;
>> +        env->regs[31] = regs->r31;
>> +        env->regs[32] = regs->r32;
>> +        env->regs[33] = regs->r33;
>> +        env->regs[34] = regs->r34;
>> +        env->regs[35] = regs->r35;
>> +        env->regs[36] = regs->r36;
>> +        env->regs[37] = regs->r37;
>> +        env->regs[38] = regs->r38;
>> +        env->regs[39] = regs->r39;
>> +        env->regs[40] = regs->r40;
>> +        env->regs[41] = regs->r41;
>> +        env->regs[42] = regs->r42;
>> +        env->regs[43] = regs->r43;
>> +        env->regs[44] = regs->r44;
>> +        env->regs[45] = regs->r45;
>> +        env->regs[46] = regs->r46;
>> +        env->regs[47] = regs->r47;
>> +        env->regs[48] = regs->r48;
>> +        env->regs[49] = regs->r49;
>> +        env->regs[50] = regs->r50;
>> +        env->regs[51] = regs->r51;
>> +        env->regs[52] = regs->r52; /* TILEGX_R_BP */
> 
> This is why you should have declared target_pt_regs
> with an array, because then you can use a loop to
> do this initialization.
> 

OK, thanks.

>> +        env->regs[53] = regs->tp;  /* TILEGX_R_TP */
>> +        env->regs[54] = regs->sp;  /* TILEGX_R_SP */
>> +        env->regs[55] = regs->lr;  /* TILEGX_R_LR */
>> +        env->pc = regs->lr;
> 
> Er, what? You should set the env->pc from the entry
> in target_pt_regs that corresponds to the PC, not the LR.
> (Which in turn means you need to set that field, not LR,
> in init_thread().)
> 

OK, thanks.

-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed