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.
> +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.
> +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.
> +#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?
> +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...
> + 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.
> + /* 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?
> +
> + for (i = 0; i < 4; i++) {
> + words[i] = cpu_ldl_code(env, ctx->base.pc_next + i *
> sizeof(size4u_t));
> + }
And this?
> + /*
> + * 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;
> +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~
