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

[Peter Maydell]

On 27 March 2015 at 10:52, Chen Gang <address@hidden> wrote:
> Add main working flow feature, system call processing feature, and elf64
> tilegx binary loading feature, based on Linux kernel tilegx 64-bit
> implementation.
>
> Signed-off-by: Chen Gang <address@hidden>
> ---
>  include/elf.h        |  2 ++
>  linux-user/elfload.c | 23 ++++++++++++++
>  linux-user/main.c    | 86 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 111 insertions(+)
>
> diff --git a/include/elf.h b/include/elf.h
> index 3e75f05..154144e 100644
> --- a/include/elf.h
> +++ b/include/elf.h
> @@ -133,6 +133,8 @@ typedef int64_t  Elf64_Sxword;
>
>  #define EM_AARCH64  183
>
> +#define EM_TILEGX   191 /* TILE-Gx */
> +
>  /* This is the info that is needed to parse the dynamic section of the file 
> */
>  #define DT_NULL                0
>  #define DT_NEEDED      1
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index 399c021..2571cb8 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -1189,6 +1189,29 @@ static inline void init_thread(struct target_pt_regs 
> *regs, struct image_info *i
>
>  #endif /* TARGET_S390X */
>
> +#ifdef TARGET_TILEGX
> +
> +/* 42 bits real used address, a half for user mode */
> +#define ELF_START_MMAP (0x00000020000000000ULL)
> +
> +#define elf_check_arch(x) ((x) == EM_TILEGX)
> +
> +#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;
> +
> +}
> +
> +#define ELF_EXEC_PAGESIZE        65536 /* TILE-Gx page size is 64KB */
> +
> +#endif /* TARGET_TILEGX */
> +
>  #ifndef ELF_PLATFORM
>  #define ELF_PLATFORM (NULL)
>  #endif
> diff --git a/linux-user/main.c b/linux-user/main.c
> index 6e446de..ecfc80b 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -3418,6 +3418,32 @@ void cpu_loop(CPUS390XState *env)
>
>  #endif /* TARGET_S390X */
>
> +#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();

> +        }
> +        process_pending_signals(env);
> +    }
> +}
> +#endif
> +
>  THREAD CPUState *thread_cpu;
>
>  void task_settid(TaskState *ts)
> @@ -4392,6 +4418,66 @@ int main(int argc, char **argv, char **envp)
>              env->psw.mask = regs->psw.mask;
>              env->psw.addr = regs->psw.addr;
>      }
> +#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.

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

thanks
-- PMM