Hi Andreas, On Sun, May 27, 2012 at 8:44 PM, Andreas Färber <afaer...@suse.de> wrote: > Am 27.05.2012 07:32, schrieb Jia Liu: >> add openrisc target stubs. >> >> Signed-off-by: Jia Liu <pro...@gmail.com> > > Minor nitpick: I'd recommend to stick to the typographic conventions > outlined here: > https://live.gnome.org/Git/CommitMessages > > In particular please start the sentence with a capital A. GNOME > recommend a lowercase topic (we usually use the file/directory mainly > affected) and uppercase beginning of the actual description, e.g. > > target-or32: Add target stubs > > Add OpenRISC target stubs. >
Thanks, I'll fix all of them. > Signed-off-by: ... > > Writing it that way is not mandatory but when you're reposting and > fixing the English grammar you can just as well make it perfect. ;) > I'm trying, all the time... > As Stefan pointed out, www.opencores.org writes it as OpenRISC, not > Openrisc. I saw no prominent notice whether OpenRISC may be a trademark > but better to respect their naming, seeing all the misspellings of QEMU. > Thanks, I'll fix all of them. > [...] >> diff --git a/target-openrisc/cpu.c b/target-openrisc/cpu.c >> new file mode 100644 >> index 0000000..ef3ffb1 >> --- /dev/null >> +++ b/target-openrisc/cpu.c >> @@ -0,0 +1,24 @@ >> +/* >> + * QEMU Openrisc CPU >> + * >> + * Copyright (c) 2012 Jia Liu <pro...@gmail.com> >> + * >> + * 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 >> + * 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/>. >> + */ >> + >> +#include "cpu.h" >> +#include "qemu-common.h" >> +#if !defined(CONFIG_USER_ONLY) >> +#include "hw/loader.h" >> +#endif > > Missing TypeInfo, missing class_init, missing initfn (where you might > want to move the openrisc_translate_init() call btw, following Igor's > example), missing reset function. This cannot all be deferred to a later > patch. > I'm trying fix this, is target-i386 a good example of QOM? If so, I'll rewrite the code fellow target-i386. >> diff --git a/target-openrisc/cpu.h b/target-openrisc/cpu.h >> new file mode 100644 >> index 0000000..80018df >> --- /dev/null >> +++ b/target-openrisc/cpu.h >> @@ -0,0 +1,184 @@ >> +/* >> + * Openrisc virtual CPU header. >> + * >> + * Copyright (c) 2011-2012 Jia Liu <pro...@gmail.com> >> + * >> + * 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 >> + * 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/>. >> + */ >> + >> +#ifndef CPU_OPENRISC_H >> +#define CPU_OPENRISC_H >> + >> +#define TARGET_HAS_ICE 1 >> + >> +#define ELF_MACHINE EM_OPENRISC >> + >> +#define CPUArchState struct CPUOpenriscState >> + >> +#define TARGET_LONG_BITS 32 >> + >> +#include "config.h" >> +#include "qemu-common.h" >> +#include "cpu-defs.h" >> +#include "softfloat.h" >> +#include "qemu/cpu.h" >> + >> +struct CPUOpenriscState; >> + >> +#define NB_MMU_MODES 3 >> +#define MMU_NOMMU_IDX 0 >> +#define MMU_SUPERVISOR_IDX 1 >> +#define MMU_USER_IDX 2 > > Maybe make these three an enum? > Thanks, I'll make a enum for them. >> + >> +#define TARGET_PAGE_BITS 13 >> + >> +#define TARGET_PHYS_ADDR_SPACE_BITS 32 >> +#define TARGET_VIRT_ADDR_SPACE_BITS 32 >> + >> +/* Verison Register */ >> +#define SPR_VR 0x12000001 >> +#define SPR_CPUCFGR 0x12000001 >> + >> +/* Registers */ >> +enum { >> + R0 = 0, R1, R2, R3, R4, R5, R6, R7, R8, R9, R10, >> + R11, R12, R13, R14, R15, R16, R17, R18, R19, R20, >> + R21, R22, R23, R24, R25, R26, R27, R28, R29, R30, >> + R31 >> +}; >> + >> +/* Register aliases */ >> +enum { >> + R_ZERO = R0, >> + R_SP = R1, >> + R_FP = R2, >> + R_LR = R9, >> + R_RV = R11, >> + R_RVH = R12 >> +}; >> + >> +typedef struct CPUOpenriscState CPUOpenriscState; >> +struct CPUOpenriscState { >> + target_ulong gpr[32]; /* General registers */ >> + >> + CPU_COMMON >> + >> + target_ulong pc; /* Program counter */ >> + target_ulong npc; /* Next PC */ >> + target_ulong ppc; /* Prev PC */ >> + target_ulong jmp_pc; /* Jump PC */ >> + uint32_t flags; >> + /* Branch. */ >> + uint32_t btaken; /* the SR_F bit */ >> +}; > > Why are pc, etc. placed after CPU_COMMON? Are they not supposed to be reset? > Aha, sorry, I'll place them before CPU_COMMON. >> + >> +#define TYPE_OPENRISC_CPU "or32-cpu" >> + >> +#define OPENRISC_CPU_CLASS(klass) \ >> + OBJECT_CLASS_CHECK(OpenriscCPUClass, (klass), TYPE_OPENRISC_CPU) >> +#define OPENRISC_CPU(obj) \ >> + OBJECT_CHECK(OpenriscCPU, (obj), TYPE_OPENRISC_CPU) >> +#define OPENRISC_CPU_GET_CLASS(obj) \ >> + OBJECT_GET_CLASS(OpenriscCPUClass, (obj), TYPE_OPENRISC_CPU) >> + >> +/** >> + * OpenriscCPUClass: >> + * @parent_reset: The parent class' reset handler. >> + * >> + * A Openrisc CPU model. >> + */ >> +typedef struct OpenriscCPUClass { >> + /*< private >*/ >> + CPUClass parent_class; >> + /*< public >*/ >> + >> + void (*parent_reset)(CPUState *cpu); >> +} OpenriscCPUClass; >> + >> +/** >> + * OpenriscCPU: >> + * @env: #CPUOpenriscState >> + * >> + * A Openrisc CPU. >> + */ >> +typedef struct OpenriscCPU { >> + /*< private >*/ >> + CPUState parent_obj; >> + /*< public >*/ >> + >> + CPUOpenriscState env; >> +} OpenriscCPU; >> + >> +static inline OpenriscCPU *openrisc_env_get_cpu(CPUOpenriscState *env) >> +{ >> + return OPENRISC_CPU(container_of(env, OpenriscCPU, env)); >> +} >> + >> +#define ENV_GET_CPU(e) CPU(openrisc_env_get_cpu(e)) >> + >> +void openrisc_cpu_realize(OpenriscCPU *cpu); > > This should have a second Error **errp parameter, even if currently > unused. Please see target-i386 (target-arm is a bad example here). > Implementation is missing and it's not being used either. > Thanks, I'll read target-i386 code. >> + >> +void cpu_openrisc_list(FILE *f, fprintf_function cpu_fprintf); >> +OpenriscCPU *cpu_openrisc_init(const char *cpu_model); >> +int cpu_openrisc_exec(CPUOpenriscState *s); >> +void do_interrupt(CPUOpenriscState *env); >> +void openrisc_translate_init(void); >> + >> +#define cpu_list cpu_openrisc_list >> +#define cpu_exec cpu_openrisc_exec >> +#define cpu_gen_code cpu_openrisc_gen_code >> + >> +#define CPU_SAVE_VERSION 1 >> + >> +void openrisc_reset(CPUOpenriscState *env); > > Again, why the need? There's cpu_reset(). > Yes, I'll rename it. >> +#if !defined(CONFIG_USER_ONLY) >> +void openrisc_mmu_init(CPUOpenriscState *env); >> +int get_phys_nommu(CPUOpenriscState *env, target_phys_addr_t *physical, >> + int *prot, target_ulong address, int rw); >> +#endif >> + >> +static inline CPUOpenriscState *cpu_init(const char *cpu_model) >> +{ >> + return NULL; >> +} > > Needs to be implemented properly by calling cpu_openrisc_init(). > Thanks, I'll fix this. >> + >> +#include "cpu-all.h" >> + >> +static inline void cpu_get_tb_cpu_state(CPUOpenriscState *env, >> + target_ulong *pc, >> + target_ulong *cs_base, int *flags) >> +{ >> + *pc = env->pc; >> + *cs_base = 0; >> + *flags = 0; >> +} >> + >> +static inline int cpu_mmu_index(CPUOpenriscState *env) >> +{ >> + return 0; >> +} >> + >> +static inline bool cpu_has_work(CPUOpenriscState *env) >> +{ >> + return 1; > > The return type is bool, so please use true rather than 1. > Thanks, I'll fix this. >> +} >> + >> +#include "exec-all.h" >> + >> +static inline void cpu_pc_from_tb(CPUOpenriscState *env, TranslationBlock >> *tb) >> +{ >> + env->pc = tb->pc; >> +} >> + >> +#endif /* CPU_OPENRISC_H */ >> diff --git a/target-openrisc/helper.c b/target-openrisc/helper.c >> new file mode 100644 >> index 0000000..934f73b >> --- /dev/null >> +++ b/target-openrisc/helper.c >> @@ -0,0 +1,60 @@ >> +/* >> + * Openrisc helpers >> + * >> + * Copyright (c) 2011-2012 Jia Liu <pro...@gmail.com> >> + * >> + * 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 >> + * 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/>. >> + */ >> + >> +#include "cpu.h" >> +#include "qemu-common.h" >> +#include "gdbstub.h" >> +#include "host-utils.h" >> +#include "sysemu.h" >> + >> +void cpu_state_reset(CPUOpenriscState *env) >> +{ >> + cpu_reset(ENV_GET_CPU(env)); >> +} > > Had you rebased onto qom-next branch as requested, this would no longer > be necessary. > Nope, I didn't find qom-next branch. I'll clone qom-next git repo, and, which target should I fellow? i386? Sorry, I need a good example. >> + >> +OpenriscCPU *cpu_openrisc_init(const char *cpu_model) >> +{ >> + OpenriscCPU *cpu; >> + CPUOpenriscState *env; >> + static int inited; >> + inited = 0; >> + >> + if (!object_class_by_name(cpu_model)) { >> + return NULL; >> + } >> + cpu = OPENRISC_CPU(object_new(cpu_model)); >> + env = &cpu->env; >> + env->cpu_model_str = cpu_model; >> + /*realize openrisc cpu here*/ >> + >> + if (tcg_enabled() && !inited) { >> + inited = 1; >> + openrisc_translate_init(); >> + } >> + >> + cpu_state_reset(env); > > cpu_reset(). > Thanks, I'll fix it. >> + >> + return cpu; >> +} > > This function would best be placed into cpu.c. > >> + >> +void cpu_openrisc_list(FILE *f, fprintf_function cpu_fprintf) >> +{ >> + (*cpu_fprintf)(f, "Available CPUs:\n" >> + "or1200\n"); > > Nack. Do not hardcode CPU models. Register a class "or1200" in cpu.c and > call object_class_get_list() here, sort them and print the name of each. > Again, compare target-arm. > Thanks, I'll move the code form 02/17 and 03/17 to here. >> +} > > This function should go into cpu.c, too. It's only in helper.c for many > existing targets because cpu.c is pretty new. > >> diff --git a/target-openrisc/machine.c b/target-openrisc/machine.c >> new file mode 100644 >> index 0000000..31165fc >> --- /dev/null >> +++ b/target-openrisc/machine.c >> @@ -0,0 +1,31 @@ >> +/* >> + * Openrisc Machine >> + * >> + * Copyright (c) 2011-2012 Jia Liu <pro...@gmail.com> >> + * >> + * 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 >> + * 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/>. >> + */ >> + >> +#include "hw/hw.h" >> +#include "hw/boards.h" >> +#include "kvm.h" > > I doubt that there is KVM support for or32. > Thanks, I'll delete it. >> + >> +void cpu_save(QEMUFile *f, void *opaque) >> +{ >> +} >> + >> +int cpu_load(QEMUFile *f, void *opaque, int version_id) >> +{ >> + return 0; >> +} > [snip] > > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg Regards, Jia.