Firstly, thank you very much for your patient work!
On 2/28/15 01:36, Andreas Färber wrote: > Hi, > > "target-tilegx: Initial stub" or "...support"? No need to mention QEMU > (spelling!) in a QEMU commit. > OK, thanks. > Am 22.02.2015 um 14:33 schrieb Chen Gang S: >> It almost likes a template for adding an architecture target. > > That's a comment that's not really descriptive of what the patch does. > Instead, maybe mention that this is the configure and build system > support etc. and what name to use for enabling it from configure, that > it's linux-user only for now, ...? > OK, thanks. >> >> Signed-off-by: Chen Gang <gang.chen.5...@gmail.com> >> --- >> configure | 7 ++ >> default-configs/tilegx-linux-user.mak | 1 + >> target-tilegx/Makefile.objs | 1 + >> target-tilegx/cpu-qom.h | 72 +++++++++++++++ >> target-tilegx/cpu.c | 162 >> ++++++++++++++++++++++++++++++++++ >> target-tilegx/cpu.h | 85 ++++++++++++++++++ >> target-tilegx/helper.h | 0 >> target-tilegx/translate.c | 54 ++++++++++++ >> 8 files changed, 382 insertions(+) >> create mode 100644 default-configs/tilegx-linux-user.mak >> create mode 100644 target-tilegx/Makefile.objs >> create mode 100644 target-tilegx/cpu-qom.h >> create mode 100644 target-tilegx/cpu.c >> create mode 100644 target-tilegx/cpu.h >> create mode 100644 target-tilegx/helper.h >> create mode 100644 target-tilegx/translate.c >> >> diff --git a/configure b/configure >> index 7ba4bcb..23aa8f6 100755 >> --- a/configure >> +++ b/configure >> @@ -5191,6 +5191,9 @@ case "$target_name" in >> s390x) >> gdb_xml_files="s390x-core64.xml s390-acr.xml s390-fpr.xml" >> ;; >> + tilegx) >> + TARGET_ARCH=tilegx >> + ;; >> unicore32) >> ;; >> xtensa|xtensaeb) >> @@ -5363,6 +5366,10 @@ for i in $ARCH $TARGET_BASE_ARCH ; do >> echo "CONFIG_SPARC_DIS=y" >> $config_target_mak >> echo "CONFIG_SPARC_DIS=y" >> config-all-disas.mak >> ;; >> + tilegx*) >> + echo "CONFIG_TILEGX_DIS=y" >> $config_target_mak >> + echo "CONFIG_TILEGX_DIS=y" >> config-all-disas.mak >> + ;; > > Hadn't you been asked to drop these lines, as you are not yet adding any > disassembler code that uses it? > NO. And next, I shall drop them. >> xtensa*) >> echo "CONFIG_XTENSA_DIS=y" >> $config_target_mak >> echo "CONFIG_XTENSA_DIS=y" >> config-all-disas.mak > [...] >> diff --git a/target-tilegx/cpu-qom.h b/target-tilegx/cpu-qom.h >> new file mode 100644 >> index 0000000..e15a8b8 >> --- /dev/null >> +++ b/target-tilegx/cpu-qom.h >> @@ -0,0 +1,72 @@ >> +/* >> + * QEMU Tilegx CPU > > "TILE-Gx" according to > http://www.tilera.com/products/?ezchip=585&spage=614 - please fix > wherever used in textual form. > OK, thanks. >> + * >> + * Copyright (c) 2015 Chen Gang >> + * >> + * This library is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU Lesser General Public >> + * License as published by the Free Software Foundation; either >> + * version 2.1 of the License, or (at your option) any later version. >> + * >> + * This library is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * Lesser General Public License for more details. >> + * >> + * You should have received a copy of the GNU Lesser General Public >> + * License along with this library; if not, see >> + * <http://www.gnu.org/licenses/lgpl-2.1.html> >> + */ >> +#ifndef QEMU_TILEGX_CPU_QOM_H >> +#define QEMU_TILEGX_CPU_QOM_H >> + >> +#include "qom/cpu.h" >> + >> +#define TYPE_TILEGX_CPU "tilegx-cpu" >> + >> +#define TILEGX_CPU_CLASS(klass) \ >> + OBJECT_CLASS_CHECK(TilegxCPUClass, (klass), TYPE_TILEGX_CPU) >> +#define TILEGX_CPU(obj) \ >> + OBJECT_CHECK(TilegxCPU, (obj), TYPE_TILEGX_CPU) >> +#define TILEGX_CPU_GET_CLASS(obj) \ >> + OBJECT_GET_CLASS(TilegxCPUClass, (obj), TYPE_TILEGX_CPU) >> + >> +/** >> + * TilegxCPUClass: >> + * @parent_realize: The parent class' realize handler. >> + * @parent_reset: The parent class' reset handler. >> + * >> + * A Tilegx CPU model. >> + */ >> +typedef struct TilegxCPUClass { > > For the benefit of readers, please call this TileGXCPUClass ... > OK, thanks. >> + /*< private >*/ >> + CPUClass parent_class; >> + /*< public >*/ >> + >> + DeviceRealize parent_realize; >> + void (*parent_reset)(CPUState *cpu); >> +} TilegxCPUClass; >> + >> +/** >> + * TilegxCPU: >> + * @env: #CPUTLState >> + * >> + * A Tilegx CPU. >> + */ >> +typedef struct TilegxCPU { > > ... and TileGXCPU. (or TileGx...) > OK, thanks. I shall call it TileGXCPU. >> + /*< private >*/ >> + CPUState parent_obj; >> + uint64_t base_vectors; > > This should not be in here, the private section serves to hide the > parent field from documentation. > > base_vectors should also probably be after env, for performance reasons. > rth? > OK, thanks. At present, we do not use base_vectors, so I guess, we can just remove it, and the related implementation will be: typedef struct TileGXCPU { /*< private >*/ CPUState parent_obj; /*< public >*/ CPUS390XState env; } TileGXCPU; >> + /*< public >*/ >> + >> + CPUTLState env; > > Can this be more telling than TL please? > How about CPUTLGState? >> +} TilegxCPU; >> + >> +static inline TilegxCPU *tilegx_env_get_cpu(CPUTLState *env) >> +{ >> + return container_of(env, TilegxCPU, env); >> +} >> + >> +#define ENV_GET_CPU(e) CPU(tilegx_env_get_cpu(e)) >> + >> +#endif >> diff --git a/target-tilegx/cpu.c b/target-tilegx/cpu.c >> new file mode 100644 >> index 0000000..3dd66b5 >> --- /dev/null >> +++ b/target-tilegx/cpu.c >> @@ -0,0 +1,162 @@ >> +/* >> + * QEMU Tilegx CPU >> + * >> + * Copyright (c) 2015 Chen Gang >> + * >> + * This library is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU Lesser General Public >> + * License as published by the Free Software Foundation; either >> + * version 2.1 of the License, or (at your option) any later version. >> + * >> + * This library is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * Lesser General Public License for more details. >> + * >> + * You should have received a copy of the GNU Lesser General Public >> + * License along with this library; if not, see >> + * <http://www.gnu.org/licenses/lgpl-2.1.html> >> + */ >> + >> +#include "cpu.h" >> +#include "qemu-common.h" >> +#include "hw/qdev-properties.h" >> +#include "migration/vmstate.h" >> + >> +TilegxCPU *cpu_tilegx_init(const char *cpu_model) >> +{ >> + TilegxCPU *cpu; >> + >> + cpu = TILEGX_CPU(object_new(TYPE_TILEGX_CPU)); >> + >> + object_property_set_bool(OBJECT(cpu), true, "realized", NULL); >> + >> + return cpu; >> +} >> + >> +static void tilegx_cpu_set_pc(CPUState *cs, vaddr value) >> +{ >> + TilegxCPU *cpu = TILEGX_CPU(cs); >> + >> + cpu->env.pc = value; >> +} >> + >> +static bool tilegx_cpu_has_work(CPUState *cs) >> +{ >> + return true; >> +} >> + >> +static void tilegx_cpu_reset(CPUState *s) >> +{ >> + TilegxCPU *cpu = TILEGX_CPU(s); >> + TilegxCPUClass *mcc = TILEGX_CPU_GET_CLASS(cpu); > > Why mcc? Probably copied from a CPU with M. > Yes, we need another name for it. How about tcc? >> + CPUTLState *env = &cpu->env; >> + >> + mcc->parent_reset(s); >> + >> + memset(env, 0, sizeof(CPUTLState)); >> + tlb_flush(s, 1); >> +} >> + >> +static void tilegx_cpu_realizefn(DeviceState *dev, Error **errp) >> +{ >> + CPUState *cs = CPU(dev); >> + TilegxCPUClass *mcc = TILEGX_CPU_GET_CLASS(dev); > > Ditto. > OK. >> + >> + cpu_reset(cs); >> + qemu_init_vcpu(cs); >> + >> + mcc->parent_realize(dev, errp); >> +} >> + >> +static void tilegx_tcg_init(void) >> +{ >> +} >> + >> +static void tilegx_cpu_initfn(Object *obj) >> +{ >> + CPUState *cs = CPU(obj); >> + TilegxCPU *cpu = TILEGX_CPU(obj); >> + CPUTLState *env = &cpu->env; >> + static bool tcg_initialized; >> + >> + cs->env_ptr = env; >> + cpu_exec_init(env); >> + >> + if (tcg_enabled() && !tcg_initialized) { >> + tcg_initialized = true; >> + tilegx_tcg_init(); >> + } >> +} >> + >> +static const VMStateDescription vmstate_tilegx_cpu = { >> + .name = "cpu", >> + .unmigratable = 1, >> +}; >> + >> +static Property tilegx_properties[] = { > > "tilegx_cpu_properties" for consistency? > Originally, I copied from Microblaze, but at present, I guess, we can drop it (then also drop base_vectors). >> + DEFINE_PROP_UINT64("tilegx.base-vectors", TilegxCPU, base_vectors, 0), > > Why "tilegx." prefix? That would likely interfere with our QMP tools. > May it be "tilera.base-vectors"? But after all, I guess, we can just drop it, at present. > What is this actually used for? To someone not knowing the architecture > it may sound a bit strange to have a variable/property ...vectors that > is actually just one uint64 value. Someone recently added an API to add > a description for a property, and this seems to be calling for one. > Moving it to a patch that actually uses it would be another idea. > >> + DEFINE_PROP_END_OF_LIST(), >> +}; >> + >> +static void tilegx_cpu_do_interrupt(CPUState *cs) >> +{ >> + cs->exception_index = -1; >> +} >> + >> +static int tilegx_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int rw, >> + int mmu_idx) > > Indentation. > OK, thanks. >> +{ >> + cpu_dump_state(cs, stderr, fprintf, 0); >> + return 1; >> +} >> + >> +static bool tilegx_cpu_exec_interrupt(CPUState *cs, int interrupt_request) >> +{ >> + if (interrupt_request & CPU_INTERRUPT_HARD) { >> + tilegx_cpu_do_interrupt(cs); >> + return true; >> + } >> + return false; >> +} >> + >> +static void tilegx_cpu_class_init(ObjectClass *oc, void *data) >> +{ >> + DeviceClass *dc = DEVICE_CLASS(oc); >> + CPUClass *cc = CPU_CLASS(oc); >> + TilegxCPUClass *mcc = TILEGX_CPU_CLASS(oc); >> + >> + mcc->parent_realize = dc->realize; >> + dc->realize = tilegx_cpu_realizefn; >> + >> + mcc->parent_reset = cc->reset; >> + cc->reset = tilegx_cpu_reset; >> + >> + cc->has_work = tilegx_cpu_has_work; >> + cc->do_interrupt = tilegx_cpu_do_interrupt; >> + cc->cpu_exec_interrupt = tilegx_cpu_exec_interrupt; >> + cc->dump_state = NULL; >> + cc->set_pc = tilegx_cpu_set_pc; >> + cc->gdb_read_register = NULL; >> + cc->gdb_write_register = NULL; > > Is this really safe to do? If so, all fields are zero-initialized at > this point already, so no need to assign NULL or 0. > >> + cc->handle_mmu_fault = tilegx_cpu_handle_mmu_fault; >> + dc->vmsd = &vmstate_tilegx_cpu; >> + dc->props = tilegx_properties; >> + cc->gdb_num_core_regs = 0; >> +} >> + >> +static const TypeInfo tilegx_cpu_type_info = { >> + .name = TYPE_TILEGX_CPU, >> + .parent = TYPE_CPU, >> + .instance_size = sizeof(TilegxCPU), >> + .instance_init = tilegx_cpu_initfn, > > What about CPU models? Do the Gx72, Gx36, etc. differ only in core count > or also in instruction set features? > OK, thanks. It is already replied in another mail thread. For me, we need still let TYPE_TILEGX_CPU be "tilegx-cpu" (according to microblaze implementation). >> + .class_size = sizeof(TilegxCPUClass), >> + .class_init = tilegx_cpu_class_init, >> +}; >> + >> +static void tilegx_cpu_register_types(void) >> +{ >> + type_register_static(&tilegx_cpu_type_info); >> +} >> + >> +type_init(tilegx_cpu_register_types) >> diff --git a/target-tilegx/cpu.h b/target-tilegx/cpu.h >> new file mode 100644 >> index 0000000..09a2b26 >> --- /dev/null >> +++ b/target-tilegx/cpu.h >> @@ -0,0 +1,85 @@ >> +/* >> + * Tilegx virtual CPU header >> + * >> + * Copyright (c) 2015 Chen Gang >> + * >> + * This library is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU Lesser General Public >> + * License as published by the Free Software Foundation; either >> + * version 2 of the License, or (at your option) any later version. >> + * >> + * This library is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * General Public License for more details. >> + * >> + * You should have received a copy of the GNU Lesser General Public >> + * License along with this library; if not, see >> <http://www.gnu.org/licenses/>. >> + */ >> +#ifndef CPU_TILEGX_H >> +#define CPU_TILEGX_H >> + >> +#include "config.h" >> +#include "qemu-common.h" >> + >> +#define TARGET_LONG_BITS 64 >> + >> +#define CPUArchState struct CPUTLState > > Drop the struct here? You have a typedef below. > Hmm... originally, I really noticed about it, but after check the other target implementation (e.g microblaze, alpha, s390), I remained it. >> + >> +#include "exec/cpu-defs.h" >> +#include "fpu/softfloat.h" >> + >> +/* Tilegx register alias */ >> +#define TILEGX_R_RE 0 /* 0 register, for function/syscall return value >> */ >> +#define TILEGX_R_NR 10 /* 10 register, for syscall number */ >> +#define TILEGX_R_BP 52 /* 52 register, optional frame pointer */ >> +#define TILEGX_R_TP 53 /* TP register, thread local storage data */ >> +#define TILEGX_R_SP 54 /* SP register, stack pointer */ >> +#define TILEGX_R_LR 55 /* LR register, may save pc, but it is not pc */ >> + >> +typedef struct CPUTLState { >> + uint64_t regs[56]; >> + uint64_t pc; > > You have marked the CPU unmigratable. Might it make sense to still > prepare the corresponding VMState fields so that it does not get forgotten? > Excuse me, I am not quite familiar with it, could you provide more details? >> + CPU_COMMON >> +} CPUTLState; >> + >> +#include "cpu-qom.h" >> + >> +/* Tilegx memory attributes */ >> +#define TARGET_PAGE_BITS 16 /* Tilegx uses 64KB page size */ >> +#define MMAP_SHIFT TARGET_PAGE_BITS >> +#define TARGET_PHYS_ADDR_SPACE_BITS 42 /* Tilegx is 42 bit physical address >> */ >> +#define TARGET_VIRT_ADDR_SPACE_BITS 64 /* Tilegx has 64 bit virtual address >> */ >> +#define MMU_USER_IDX 0 /* independent from both qemu and architecture */ >> + >> +#include "exec/cpu-all.h" >> + >> +int cpu_tilegx_exec(CPUTLState *s); >> +int cpu_tilegx_signal_handler(int host_signum, void *pinfo, void *puc); >> + >> +#define cpu_exec cpu_tilegx_exec >> +#define cpu_gen_code cpu_tilegx_gen_code >> +#define cpu_signal_handler cpu_tilegx_signal_handler >> + >> +TilegxCPU *cpu_tilegx_init(const char *cpu_model); >> + >> +static inline CPUTLState *cpu_init(const char *cpu_model) >> +{ >> + TilegxCPU *cpu = cpu_tilegx_init(cpu_model); >> + if (cpu == NULL) { >> + return NULL; >> + } >> + return &cpu->env; >> +} >> + >> +static inline void cpu_get_tb_cpu_state(CPUTLState *env, target_ulong *pc, >> + target_ulong *cs_base, int *flags) >> +{ >> + *pc = env->pc; >> + *cs_base = 0; >> + *flags = 0; >> +} >> + >> +#include "exec/exec-all.h" >> + >> +#endif >> diff --git a/target-tilegx/helper.h b/target-tilegx/helper.h >> new file mode 100644 >> index 0000000..e69de29 > > Is this empty file actually included from somewhere in this patch? > Yes, it is for future use, I guess, at present, we can just drop it: - "translate.c" includes "exec/helper-gen.h". - "exec/helper-gen.h" includes "helper.h". >> diff --git a/target-tilegx/translate.c b/target-tilegx/translate.c >> new file mode 100644 >> index 0000000..5131fa7 >> --- /dev/null >> +++ b/target-tilegx/translate.c >> @@ -0,0 +1,54 @@ >> +/* >> + * QEMU Tilegx CPU >> + * >> + * Copyright (c) 2015 Chen Gang >> + * >> + * This library is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU Lesser General Public >> + * License as published by the Free Software Foundation; either >> + * version 2.1 of the License, or (at your option) any later version. >> + * >> + * This library is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + * Lesser General Public License for more details. >> + * >> + * You should have received a copy of the GNU Lesser General Public >> + * License along with this library; if not, see >> + * <http://www.gnu.org/licenses/lgpl-2.1.html> >> + */ >> + >> +#include "cpu.h" >> +#include "disas/disas.h" >> +#include "tcg-op.h" >> +#include "exec/helper-proto.h" >> +#include "exec/cpu_ldst.h" >> +#include "exec/helper-gen.h" >> + >> +static inline void gen_intermediate_code_internal(TilegxCPU *cpu, >> + TranslationBlock *tb, >> + bool search_pc) >> +{ >> + /* >> + * FIXME: after load elf64 tilegx binary successfully, it will quit, at >> + * present, and will implement the related features next. >> + */ >> + fprintf(stderr, "\nLoad elf64 tilegx successfully\n"); > > "Loaded" > OK, thanks. >> + fprintf(stderr, "reach code start position: [" TARGET_FMT_lx "] %s\n\n", > > "reached" > OK, thanks. >> + tb->pc, lookup_symbol(tb->pc)); > > This output is hopefully only temporary. But as a general reminder, > there is error_report() for error messages, and for debug messages > please define your own macros or use the qemu_log infrastructure instead > of fprintf(stderr, ...). > OK, thanks, I shall use qemu_log for it. Thanks -- Chen Gang Open, share, and attitude like air, water, and life which God blessed
