RE: [PATCH] Add minimal Hexagon target - First in a series of patches - linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files in target/hexagon/imported are from another project and therefore do not conform to qemu coding standards

[Taylor Simpson]

Thanks for the feedback Richard.

Responses below ...

Thanks,
Taylor


-----Original Message-----
From: Richard Henderson <address@hidden>
Sent: Tuesday, November 19, 2019 1:34 PM
To: Taylor Simpson <address@hidden>; address@hidden; address@hidden; 
address@hidden
Subject: Re: [PATCH] Add minimal Hexagon target - First in a series of patches 
- linux-user changes + linux-user/hexagon + skeleton of target/hexagon - Files 
in target/hexagon/imported are from another project and therefore do not 
conform to qemu coding standards

-------------------------------------------------------------------------
CAUTION: This email originated from outside of the organization.
-------------------------------------------------------------------------

On 11/19/19 12:58 AM, Taylor Simpson wrote:
> +static abi_ulong get_sigframe(struct target_sigaction *ka,
> +                              CPUHexagonState *regs, size_t
> +framesize) {
> +    abi_ulong sp = get_sp_from_cpustate(regs);
> +
> +    /* This is the X/Open sanctioned signal stack switching.  */
> +    sp = target_sigsp(sp, ka) - framesize;
> +
> +    sp &= ~7UL; /* align sp on 8-byte boundary */

QEMU_ALIGN_DOWN.

> diff --git a/linux-user/hexagon/sockbits.h
> b/linux-user/hexagon/sockbits.h new file mode 100644 index
> 0000000..85bd64a
> --- /dev/null
> +++ b/linux-user/hexagon/sockbits.h
> @@ -0,0 +1,3 @@
> +/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights
> +Reserved. */
> +
> +#include "../generic/sockbits.h"

All new files should have denote their license.

> +static inline const char *cpu_get_model(uint32_t eflags) {
> +    /* For now, treat anything newer than v60 as a v67 */
> +    /* FIXME - Disable instructions that are newer than the specified arch */
> +    if (eflags == 0x04 ||    /* v5  */
> +        eflags == 0x05 ||    /* v55 */
> +        eflags == 0x60 ||    /* v60 */
> +        eflags == 0x61 ||    /* v61 */
> +        eflags == 0x62 ||    /* v62 */
> +        eflags == 0x65 ||    /* v65 */
> +        eflags == 0x66 ||    /* v66 */
> +        eflags == 0x67) {    /* v67 */
> +        return "v67";
> +    }
> +    printf("eflags = 0x%x\n", eflags);

Left over debug.

> diff --git a/target/hexagon/Makefile.objs
> b/target/hexagon/Makefile.objs new file mode 100644 index
> 0000000..dfab6ee
> --- /dev/null
> +++ b/target/hexagon/Makefile.objs
> @@ -0,0 +1,6 @@
> +obj-y += \
> +    cpu.o \
> +    translate.o \
> +    op_helper.o
> +
> +CFLAGS += -I$(SRC_PATH)/target/hexagon/imported

Is this really better than

#include "imported/global_types.h"

etc?

> +++ b/target/hexagon/cpu-param.h
> @@ -0,0 +1,11 @@
> +/* Copyright (c) 2019 Qualcomm Innovation Center, Inc. All Rights
> +Reserved. */
> +
> +
> +#ifndef HEXAGON_CPU_PARAM_H
> +#define HEXAGON_CPU_PARAM_H
> +
> +# define TARGET_PHYS_ADDR_SPACE_BITS 36

Watch your spacing.

Does this really compile without TARGET_VIRT_ADDR_SPACE_BITS?

It's used in linux-user/main.c, but I suppose in a way that the preprocessor 
interprets it as 0.

> +const char * const hexagon_prednames[] = {
> +  "p0 ", "p1 ", "p2 ", "p3 "
> +};

Inter-string spacing probably belongs to the format not the name.
[Taylor] Could you elaborate?  Should I put spacing  after the commas?

> +static inline target_ulong hack_stack_ptrs(CPUHexagonState *env,
> +                                           target_ulong addr) {
> +    target_ulong stack_start = env->stack_start;
> +    target_ulong stack_size = 0x10000;
> +    target_ulong stack_adjust = env->stack_adjust;
> +
> +    if (stack_start + 0x1000 >= addr && addr >= (stack_start - stack_size)) {
> +        return addr - stack_adjust;
> +    }
> +    return addr;
> +}

An explanation would be welcome here.
[Taylor]  I will add a comment.  One of the main debugging techniques is to use 
"-d cpu" and compare against LLDB output when single stepping.  However, the 
target and qemu put the stacks in different locations.  This is used to 
compensate so the diff is cleaner.

> +static void hexagon_dump(CPUHexagonState *env, FILE *f) {
> +    static target_ulong last_pc;
> +    int i;
> +
> +    /*
> +     * When comparing with LLDB, it doesn't step through single-cycle
> +     * hardware loops the same way.  So, we just skip them here
> +     */
> +    if (env->gpr[HEX_REG_PC] == last_pc) {
> +        return;
> +    }

This seems mis-placed.
[Taylor] Hexagon has hardware controlled loops, so we can have a single packet 
that executes multiple times.  We don't want the dump to print every time.

> +#ifdef FIXME
> +    /*
> +     * LLDB bug swaps gp and ugp
> +     * FIXME when the LLDB bug is fixed
> +     */
> +    print_reg(f, env, HEX_REG_GP);
> +    print_reg(f, env, HEX_REG_UGP);
> +#else
> +    fprintf(f, "  %s = 0x" TARGET_FMT_lx "\n",
> +        hexagon_regnames[HEX_REG_GP],
> +        hack_stack_ptrs(env, env->gpr[HEX_REG_UGP]));
> +    fprintf(f, "  %s = 0x" TARGET_FMT_lx "\n",
> +        hexagon_regnames[HEX_REG_UGP],
> +        hack_stack_ptrs(env, env->gpr[HEX_REG_GP])); #endif
> +    print_reg(f, env, HEX_REG_PC);
> +#ifdef FIXME
> +    /*
> +     * Not modelled in qemu, print junk to minimize the diff's
> +     * with LLDB output
> +     */
> +    print_reg(f, env, HEX_REG_CAUSE);
> +    print_reg(f, env, HEX_REG_BADVA);
> +    print_reg(f, env, HEX_REG_CS0);
> +    print_reg(f, env, HEX_REG_CS1);
> +#else
> +    fprintf(f, "  cause = 0x000000db\n");
> +    fprintf(f, "  badva = 0x00000000\n");
> +    fprintf(f, "  cs0 = 0x00000000\n");
> +    fprintf(f, "  cs1 = 0x00000000\n"); #endif

Need we retain the fixme?
[Taylor] Those will be needed when we implement system mode.  So, I'll change 
the macro to CONFIG_USER_ONLY.

> +void hexagon_debug(CPUHexagonState *env) {
> +    hexagon_dump(env, stdout);
> +}

Is this to be called from the debugger?  From what location did you find it 
useful?  There are only certain locations in tcg that are self-consistent...
[Taylor] Yes, these are useful from the debugger.  I call it from a helper that 
takes CPUHexagonState as an argument.

> +    DEFINE_CPU(TYPE_HEXAGON_CPU_V67,              hexagon_v67_cpu_init),

Spacing?

> +#ifndef HEXAGON_CPU_H
> +#define HEXAGON_CPU_H
> +
> +/* FIXME - Change this to a command-line option */ #ifdef FIXME
> +#define DEBUG_HEX #endif #ifdef FIXME #define COUNT_HEX_HELPERS
> +#endif

Eh?

> +
> +/* Forward declaration needed by some of the header files */ typedef
> +struct CPUHexagonState CPUHexagonState;
> +
> +#include <fenv.h>
> +#include "qemu/osdep.h"

osdep.h should already have been included.
Indeed, it must be first for *.c files.

Why do you need fenv.h?

> +#include "global_types.h"
> +#include "max.h"
> +#include "iss_ver_registers.h"
> +
> +#define TARGET_PAGE_BITS 16     /* 64K pages */
> +/*
> + * For experimenting with oslib (4K pages)
> + * #define TARGET_PAGE_BITS 12
> + */
> +#define TARGET_LONG_BITS 32
> +#define TARGET_VIRT_ADDR_SPACE_BITS 32

Ah, to answer my earlier question, these belong to cpu-param.h.
Drop the "experimenting" comment.

> +
> +#include <time.h>

time.h is included by osdep.h.


> +#include "qemu/compiler.h"
> +#include "qemu-common.h"
> +#include "exec/cpu-defs.h"
> +#include "regs.h"
> +
> +#define TYPE_HEXAGON_CPU "hexagon-cpu"
> +
> +#define HEXAGON_CPU_TYPE_SUFFIX "-" TYPE_HEXAGON_CPU #define
> +HEXAGON_CPU_TYPE_NAME(name) (name HEXAGON_CPU_TYPE_SUFFIX) #define
> +CPU_RESOLVING_TYPE TYPE_HEXAGON_CPU
> +
> +#define TYPE_HEXAGON_CPU_V67             HEXAGON_CPU_TYPE_NAME("v67")
> +
> +#define MMU_USER_IDX 0
> +
> +#define TRANSLATE_FAIL 1
> +#define TRANSLATE_SUCCESS 0

What's this for?  This looks like it's cribbed from riscv, which oddly doesn't 
match the actual tlb_fill interface, which uses bool true for success.

> +
> +struct MemLog {
> +    vaddr_t va;
> +    size1u_t width;
> +    size4u_t data32;
> +    size8u_t data64;
> +};

Is this part of translation?  Maybe save this til you actually use it, and 
probably place in translate.h.
[Taylor] Good catch.  I can save them until they are actually used.  They are 
part of the execution state, so need to be in cpu.h though.

> +    /* For comparing with LLDB on target - see hack_stack_ptrs function */
> +    target_ulong stack_start;
> +    target_ulong stack_adjust;

Which, as you recall, doesn't actually have any commentary.

> +extern const char * const hexagon_regnames[]; extern const char *
> +const hexagon_prednames[];

Do these actually need declaring here?
Let's keep them private to cpu.c otherwise.

> +#define ENV_GET_CPU(e)  CPU(hexagon_env_get_cpu(e))
> +#define ENV_OFFSET      offsetof(HexagonCPU, env)

Obsolete. Remove.

> +int hexagon_gdb_read_register(CPUState *cpu, uint8_t *buf, int reg);
> +int hexagon_gdb_write_register(CPUState *cpu, uint8_t *buf, int reg);

Move these decls to e.g. internal.h?
They're not relevant to generic users of cpu.h

> +void QEMU_NORETURN do_raise_exception_err(CPUHexagonState *env,
> +                                          uint32_t exception,
> +uintptr_t pc);

Likewise.

> +void hexagon_translate_init(void);
> +void hexagon_debug(CPUHexagonState *env); void
> +hexagon_debug_vreg(CPUHexagonState *env, int regnum); void
> +hexagon_debug_qreg(CPUHexagonState *env, int regnum);

Likewise.

> +#ifdef COUNT_HEX_HELPERS
> +extern void print_helper_counts(void); #endif

Likewise.

> +static void decode_packet(CPUHexagonState *env, DisasContext *ctx) {
> +    size4u_t words[4];
> +    int i;
> +    /* Brute force way to make sure current PC is set */
> +    tcg_gen_movi_tl(hex_gpr[HEX_REG_PC], ctx->base.pc_next);

What's this for?
[Taylor] Honestly, I'm not sure.  If I remove it, nothing works - not even 
"hello world".

> +
> +    for (i = 0; i < 4; i++) {
> +        words[i] = cpu_ldl_code(env, ctx->base.pc_next + i * 
> sizeof(size4u_t));
> +    }

And this?
[Taylor] It's reading from the instruction memory.  The switch statement below 
uses it.

> +    /*
> +     * Brute force just enough to get the first program to execute.
> +     */
> +    switch (words[0]) {
> +    case 0x7800c806:                                /* r6 = #64 */
> +        tcg_gen_movi_tl(hex_gpr[6], 64);
> +        ctx->base.pc_next += 4;
> +        break;
> +    case 0x7800c020:                                /* r0 = #1 */
> +        tcg_gen_movi_tl(hex_gpr[0], 1);
> +        ctx->base.pc_next += 4;
> +        break;
> +    case 0x00044002:
> +        if (words[1] == 0x7800c001) {               /* r1 = ##0x400080 */
> +            tcg_gen_movi_tl(hex_gpr[1], 0x400080);
> +            ctx->base.pc_next += 8;
> +        } else {
> +            printf("ERROR: Unknown instruction 0x%x\n", words[1]);
> +            g_assert_not_reached();
> +        }
> +        break;
> +    case 0x7800c0e2:                                /* r2 = #7 */
> +        tcg_gen_movi_tl(hex_gpr[2], 7);
> +        ctx->base.pc_next += 4;
> +        break;
> +    case 0x5400c004:                               /* trap0(#1) */
> +    {
> +        TCGv excp_trap0 = tcg_const_tl(HEX_EXCP_TRAP0);
> +        gen_helper_raise_exception(cpu_env, excp_trap0);
> +        tcg_temp_free(excp_trap0);
> +        ctx->base.pc_next += 4;
> +        break;
> +    }
> +    case 0x7800cbc6:                               /* r6 = #94 */
> +        tcg_gen_movi_tl(hex_gpr[6], 94);
> +        ctx->base.pc_next += 4;
> +        break;
> +    case 0x7800cba6:                               /* r6 = #93 */
> +        tcg_gen_movi_tl(hex_gpr[6], 93);
> +        ctx->base.pc_next += 4;
> +        break;
> +    case 0x7800c000:                               /* r0 = #0 */
> +        tcg_gen_movi_tl(hex_gpr[0], 0);
> +        ctx->base.pc_next += 4;
> +        break;
> +    default:
> +        ctx->base.is_jmp = DISAS_TOO_MANY;
> +        ctx->base.pc_next += 4;
> +    }

I'm not especially keen on this, since it will just be removed in subsequent 
patches.  The initial patch must compile, but it need not do *anything* 
interesting.

C.f. 61766fe9e2d3 which is the initial commit for target/hppa, wherein the 
decoder is

> +static ExitStatus translate_one(DisasContext *ctx, uint32_t insn) {
> +    uint32_t opc = extract32(insn, 26, 6);
> +
> +    switch (opc) {
> +    default:
> +        break;
> +    }
> +    return gen_illegal(ctx);
> +}



> +}
> +
> +static void hexagon_tr_init_disas_context(DisasContextBase *dcbase,
> +                                          CPUState *cs) {
> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
> +
> +    ctx->mem_idx = ctx->base.tb->flags & TB_FLAGS_MMU_MASK;

Since you're not initializing this in cpu_get_tb_cpu_state, you might as well 
just use

    ctx->mem_idx = MMU_USER_IDX;
[Taylor] Should I be initialize this in cpu_get_bt_cpu_state?

> +static void hexagon_tr_translate_packet(DisasContextBase *dcbase,
> +CPUState *cpu) {
> +    DisasContext *ctx = container_of(dcbase, DisasContext, base);
> +    CPUHexagonState *env = cpu->env_ptr;
> +
> +    decode_packet(env, ctx);
> +
> +    if (ctx->base.is_jmp == DISAS_NEXT) {
> +        target_ulong page_start;
> +
> +        page_start = ctx->base.pc_first & TARGET_PAGE_MASK;
> +        if (ctx->base.pc_next - page_start >= TARGET_PAGE_SIZE) {
> +            ctx->base.is_jmp = DISAS_TOO_MANY;
> +        }
> +
> +#ifdef DEBUG_HEX
> +        /* When debugging, force the end of the TB after each packet */
> +        if (ctx->base.pc_next - ctx->base.pc_first >= 0x04) {
> +            ctx->base.is_jmp = DISAS_TOO_MANY;
> +        }
> +#endif
> +    }

As mentioned elsewhere, this latter should be handled by -singlestep.  The 
generic translator loop will have set max_insns to 1.


r~