Re: [Qemu-devel] [PATCH 02/10 v11] linux-user: Support tilegx architecture in linux-user

[Peter Maydell <peter.maydell@linaro.org>]

On 30 May 2015 at 22:10, Chen Gang <xili_gchen_5257@hotmail.com> 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 <gang.chen.5i5j@gmail.com>
> ---
>  include/elf.h             |   2 +
>  linux-user/elfload.c      |  23 +++++
>  linux-user/main.c         | 236 ++++++++++++++++++++++++++++++++++++++++++++++
>  linux-user/syscall_defs.h |  14 ++-
>  4 files changed, 270 insertions(+), 5 deletions(-)
>
> diff --git a/include/elf.h b/include/elf.h
> index 4afd474..79859f0 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 0ba9706..fbf9212 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->pc = infop->entry;
> +    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 3f32db0..8e7fe86 100644
> --- a/linux-user/main.c
> +++ b/linux-user/main.c
> @@ -3416,6 +3416,231 @@ void cpu_loop(CPUS390XState *env)
>
>  #endif /* TARGET_S390X */
>
> +#ifdef TARGET_TILEGX
> +
> +static uint64_t get_regval(CPUTLGState *env, uint8_t reg)
> +{
> +    if (likely(reg < TILEGX_R_COUNT)) {
> +        return env->regs[reg];
> +    } else if (reg != TILEGX_R_ZERO) {
> +        fprintf(stderr, "invalid register r%d for reading.\n", reg);
> +        g_assert_not_reached();

You don't appear to be guaranteeing that the register value
is < TILEGX_R_COUNT anywhere: get_SrcA_X1() and friends
mask with 0x3f, but that only means you're guaranteed the
value is between 0 and 63, wherease TILEGX_R_COUNT is 56.
What does real hardware do if the encoded register value
is 56..63 ?

Also, if (something) {
          g_assert_not_reached();
      }

is an awkward way to write
    g_assert(!something);

> +    }
> +    return 0;
> +}
> +
> +static void set_regval(CPUTLGState *env, uint8_t reg, uint64_t val)
> +{
> +    if (likely(reg < TILEGX_R_COUNT)) {
> +        env->regs[reg] = val;
> +    } else if (reg != TILEGX_R_ZERO) {
> +        fprintf(stderr, "invalid register r%d for writing.\n", reg);
> +        g_assert_not_reached();
> +    }
> +}
> +
> +/*
> + * Compare the 8-byte contents of the CmpValue SPR with the 8-byte value in
> + * memory at the address held in the first source register. If the values are
> + * not equal, then no memory operation is performed. If the values are equal,
> + * the 8-byte quantity from the second source register is written into memory
> + * at the address held in the first source register. In either case, the result
> + * of the instruc- tion is the value read from memory. The compare and write to

stray "- ".

> + * memory are atomic and thus can be used for synchronization purposes. This
> + * instruction only operates for addresses aligned to a 8-byte boundary.
> + * Unaligned memory access causes an Unaligned Data Reference interrupt.
> + *
> + * Functional Description (64-bit)
> + *       uint64_t memVal = memoryReadDoubleWord (rf[SrcA]);
> + *       rf[Dest] = memVal;
> + *       if (memVal == SPR[CmpValueSPR])
> + *           memoryWriteDoubleWord (rf[SrcA], rf[SrcB]);
> + *
> + * Functional Description (32-bit)
> + *       uint64_t memVal = signExtend32 (memoryReadWord (rf[SrcA]));
> + *       rf[Dest] = memVal;
> + *       if (memVal == signExtend32 (SPR[CmpValueSPR]))
> + *           memoryWriteWord (rf[SrcA], rf[SrcB]);
> + *
> + *
> + * For exch(4), will no cmp spr.

Not sure what this sentence means?

> + */
> +static void do_exch(CPUTLGState *env, int8_t quad, int8_t cmp)

quad and cmp are just booleans, right? Why int8_t not bool?

> +{
> +    uint8_t rdst, rsrc, rsrcb;
> +    target_ulong addr, tmp;
> +    target_long val, sprval;
> +    target_siginfo_t info;
> +
> +    start_exclusive();
> +
> +    rdst = (env->excparam >> 16) & 0xff;
> +    rsrc = (env->excparam >> 8) & 0xff;
> +    rsrcb = env->excparam & 0xff;

Consider extract32().

> +
> +    addr = get_regval(env, rsrc);
> +    if (quad ? get_user_s64(val, addr) : get_user_s32(val, addr)) {
> +        goto do_sigsegv;
> +    }
> +    tmp = (target_ulong)val;  /* rdst may be the same to rsrcb, so buffer it */

Why do this, when we could just use a different variable
rather than trashing val below?

> +
> +    if (cmp) {
> +        if (quad) {
> +            sprval = (target_long)env->spregs[TILEGX_SPR_CMPEXCH];

Pointless cast.

> +        } else {
> +            sprval = (int32_t)(env->spregs[TILEGX_SPR_CMPEXCH] & 0xffffffff);

Clearer as
    sprval = sextract64(env->spregs[TILEGX_SPR_CMPEXCH], 0, 32);

> +        }
> +    }
> +
> +    if (!cmp || val == sprval) {
> +        val = get_regval(env, rsrcb);

If you say "target_long srcbval = ..." you don't trash val.

> +        if (quad ? put_user_u64(val, addr) : put_user_u32(val, addr)) {
> +            goto do_sigsegv;
> +        }
> +    }
> +
> +    set_regval(env, rdst, tmp);
> +
> +    end_exclusive();
> +    return;
> +
> +do_sigsegv:
> +    end_exclusive();
> +
> +    info.si_signo = TARGET_SIGSEGV;
> +    info.si_errno = 0;
> +    info.si_code = TARGET_SEGV_MAPERR;
> +    info._sifields._sigfault._addr = addr;
> +    queue_signal(env, TARGET_SIGSEGV, &info);
> +}
> +
> +static void do_fetch(CPUTLGState *env, int trapnr, int8_t quad)
> +{
> +    uint8_t rdst, rsrc, rsrcb;
> +    int8_t write = 1;
> +    target_ulong addr;
> +    target_long val, tmp;
> +    target_siginfo_t info;
> +
> +    start_exclusive();
> +
> +    rdst = (env->excparam >> 16) & 0xff;
> +    rsrc = (env->excparam >> 8) & 0xff;
> +    rsrcb = env->excparam & 0xff;
> +
> +    addr = get_regval(env, rsrc);
> +    if (quad ? get_user_s64(val, addr) : get_user_s32(val, addr)) {
> +        goto do_sigsegv;
> +    }
> +    tmp = val;  /* rdst may be the same to rsrcb, so buffer it */

Again, unnecessary copy when you could just use a different
variable for the value in rsrcb.

> +    val = get_regval(env, rsrcb);
> +    switch (trapnr) {
> +    case TILEGX_EXCP_OPCODE_FETCHADD:
> +    case TILEGX_EXCP_OPCODE_FETCHADD4:
> +    case TILEGX_EXCP_OPCODE_FETCHADDGEZ:
> +    case TILEGX_EXCP_OPCODE_FETCHADDGEZ4:
> +        val += tmp;
> +        if (trapnr == TILEGX_EXCP_OPCODE_FETCHADDGEZ) {
> +            if (val < 0) {
> +                write = 0;
> +            }
> +        } else if (trapnr == TILEGX_EXCP_OPCODE_FETCHADDGEZ4) {
> +            if ((int32_t)val < 0) {
> +                write = 0;
> +            }
> +        }

Just give these their own case blocks rather than
writing an if() that's conditioned on the same variable
we're switching on. The only duplication you're saving
is a single line addition.

> +        break;
> +    case TILEGX_EXCP_OPCODE_FETCHAND:
> +    case TILEGX_EXCP_OPCODE_FETCHAND4:
> +        val &= tmp;
> +        break;
> +    case TILEGX_EXCP_OPCODE_FETCHOR:
> +    case TILEGX_EXCP_OPCODE_FETCHOR4:
> +        val |= tmp;
> +        break;
> +    default:
> +        g_assert_not_reached();
> +    }
> +
> +    if (write) {
> +        if (quad ? put_user_u64(val, addr) : put_user_u32(val, addr)) {
> +            goto do_sigsegv;
> +        }
> +    }
> +
> +    set_regval(env, rdst, tmp);
> +
> +    end_exclusive();
> +    return;
> +
> +do_sigsegv:
> +    end_exclusive();
> +
> +    info.si_signo = TARGET_SIGSEGV;
> +    info.si_errno = 0;
> +    info.si_code = TARGET_SEGV_MAPERR;
> +    info._sifields._sigfault._addr = addr;
> +    queue_signal(env, TARGET_SIGSEGV, &info);
> +}
> +
> +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]);
> +            env->regs[TILEGX_R_ERR] = TILEGX_IS_ERRNO(env->regs[TILEGX_R_RE]) ?
> +                                          env->regs[TILEGX_R_RE] : 0;
> +            break;
> +        case TILEGX_EXCP_OPCODE_EXCH:
> +            do_exch(env, 1, 0);

these should be true, false assuming you change the arg type to bool.

> +            break;
> +        case TILEGX_EXCP_OPCODE_EXCH4:
> +            do_exch(env, 0, 0);
> +            break;
> +        case TILEGX_EXCP_OPCODE_CMPEXCH:
> +            do_exch(env, 1, 1);
> +            break;
> +        case TILEGX_EXCP_OPCODE_CMPEXCH4:
> +            do_exch(env, 0, 1);
> +            break;
> +        case TILEGX_EXCP_OPCODE_FETCHADD:
> +            do_fetch(env, trapnr, 1);
> +            break;
> +        case TILEGX_EXCP_OPCODE_FETCHADDGEZ:
> +        case TILEGX_EXCP_OPCODE_FETCHAND:
> +        case TILEGX_EXCP_OPCODE_FETCHOR:
> +            do_fetch(env, trapnr, 1);
> +            exit(1);

???

> +            break;
> +        case TILEGX_EXCP_OPCODE_FETCHADD4:
> +        case TILEGX_EXCP_OPCODE_FETCHADDGEZ4:
> +        case TILEGX_EXCP_OPCODE_FETCHAND4:
> +        case TILEGX_EXCP_OPCODE_FETCHOR4:
> +            do_fetch(env, trapnr, 0);
> +            exit(1);

Again.

> +            break;
> +        default:
> +            fprintf(stderr, "trapnr is %d[0x%x].\n", trapnr, trapnr);
> +            g_assert_not_reached();
> +        }
> +        process_pending_signals(env);
> +    }
> +}
> +
> +#endif
> +
>  THREAD CPUState *thread_cpu;
>
>  void task_settid(TaskState *ts)
> @@ -4389,6 +4614,17 @@ int main(int argc, char **argv, char **envp)
>              env->psw.mask = regs->psw.mask;
>              env->psw.addr = regs->psw.addr;
>      }
> +#elif defined(TARGET_TILEGX)
> +    {
> +        int i;
> +        for (i = 0; i < TILEGX_R_COUNT; i++) {
> +            env->regs[i] = regs->regs[i];
> +        }
> +        for (i = 0; i < TILEGX_SPR_COUNT; i++) {
> +            env->spregs[i] = 0;
> +        }
> +        env->pc = regs->pc;
> +    }
>  #else
>  #error unsupported target CPU
>  #endif
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index edd5f3c..e6af073 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -64,8 +64,9 @@
>  #endif
>
>  #if defined(TARGET_I386) || defined(TARGET_ARM) || defined(TARGET_SH4) \
> -    || defined(TARGET_M68K) || defined(TARGET_CRIS) || defined(TARGET_UNICORE32) \
> -    || defined(TARGET_S390X) || defined(TARGET_OPENRISC)
> +    || defined(TARGET_M68K) || defined(TARGET_CRIS) \
> +    || defined(TARGET_UNICORE32) || defined(TARGET_S390X) \
> +    || defined(TARGET_OPENRISC) || defined(TARGET_TILEGX)
>
>  #define TARGET_IOC_SIZEBITS    14
>  #define TARGET_IOC_DIRBITS     2
> @@ -365,7 +366,8 @@ int do_sigaction(int sig, const struct target_sigaction *act,
>      || defined(TARGET_PPC) || defined(TARGET_MIPS) || defined(TARGET_SH4) \
>      || defined(TARGET_M68K) || defined(TARGET_ALPHA) || defined(TARGET_CRIS) \
>      || defined(TARGET_MICROBLAZE) || defined(TARGET_UNICORE32) \
> -    || defined(TARGET_S390X) || defined(TARGET_OPENRISC)
> +    || defined(TARGET_S390X) || defined(TARGET_OPENRISC) \
> +    || defined(TARGET_TILEGX)
>
>  #if defined(TARGET_SPARC)
>  #define TARGET_SA_NOCLDSTOP    8u
> @@ -1871,7 +1873,7 @@ struct target_stat {
>      abi_ulong  target_st_ctime_nsec;
>      unsigned int __unused[2];
>  };
> -#elif defined(TARGET_OPENRISC)
> +#elif defined(TARGET_OPENRISC) || defined(TARGET_TILEGX)
>
>  /* These are the asm-generic versions of the stat and stat64 structures */
>
> @@ -2264,7 +2266,9 @@ struct target_flock {
>  struct target_flock64 {
>         short  l_type;
>         short  l_whence;
> -#if defined(TARGET_PPC) || defined(TARGET_X86_64) || defined(TARGET_MIPS) || defined(TARGET_SPARC) || defined(TARGET_HPPA) || defined (TARGET_MICROBLAZE)
> +#if defined(TARGET_PPC) || defined(TARGET_X86_64) || defined(TARGET_MIPS) \
> +    || defined(TARGET_SPARC) || defined(TARGET_HPPA) \
> +    || defined(TARGET_MICROBLAZE) || defined(TARGET_TILEGX)
>          int __pad;
>  #endif
>         unsigned long long l_start;
> --
> 1.9.3

thanks
-- PMM