Hi Andreas, Thanks for your comment.
On Thu, May 17, 2012 at 10:14 PM, Andreas Färber <afaer...@suse.de> wrote: > Am 17.05.2012 10:35, schrieb Jia Liu: >> add the openrisc target stub and basic implementation. >> >> Signed-off-by: Jia Liu <pro...@gmail.com> >> --- >> diff --git a/target-openrisc/cpu-qom.h b/target-openrisc/cpu-qom.h >> new file mode 100644 >> index 0000000..8c936df >> --- /dev/null >> +++ b/target-openrisc/cpu-qom.h > > First of all, if you design your new target cleanly, there's no strict > need for a cpu-qom.h header - it mainly served to separate new QOM code > from legacy code. If you want, you can put the code directly into cpu.h > just as well. > I see the other targets patched by QOM, so I copied the code from mips without fully understand. >> @@ -0,0 +1,70 @@ >> +/* >> + * QEMU Openrisc CPU >> + * >> + * Copyright (c) 2012 Jia Liu <pro...@gmail.com> > > Minor nitpick: I guess this was copied from some other header? Uses a > two-space indentation here and one-space below. > Thanks, I'll fix this all in V2. >> + * >> + * 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 QEMU_OPENRISC_CPU_QOM_H >> +#define QEMU_OPENRISC_CPU_QOM_H >> + >> +#include "qemu/cpu.h" > >> +#include "cpu.h" > > Please don't. This was a big mistake of mine that I've been correcting > on the qom-next branch, onto which you should rebase a new target such > as this by the way. It leads to really ugly problems when someone > includes cpu-qom.h and the new struct below is not yet defined for > functions using it there. > I'm not truly understand it, did you mean I should not use QOM until you make it more perfect? >> + >> +#define TYPE_OPENRISC_CPU "or32" >> + >> +#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; > > Pleave avoid unnecessary uppercase spelling: OpenRISCCPUClass? That > distinguishes it from the all-uppercase cast macros. > > Or OpenriscCPUClass as you spell it elsewhere? > I used OPENRISCCPU in the code, if it is not good, I can change it into OpenriscCPU. >> + >> +/** >> + * OPENRISCCPU: >> + * @env: #CPUOPENRISCState >> + * >> + * A Openrisc CPU. >> + */ >> +typedef struct OPENRISCCPU { >> + /*< private >*/ >> + CPUState parent_obj; >> + /*< public >*/ >> + >> + CPUOPENRISCState env; >> +} OPENRISCCPU; > > OpenRISCCPU? 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)) >> + >> +#endif /* QEMU_OPENRISC_CPU_QOM_H */ >> diff --git a/target-openrisc/cpu.c b/target-openrisc/cpu.c >> new file mode 100644 >> index 0000000..01e65a2 >> --- /dev/null >> +++ b/target-openrisc/cpu.c >> @@ -0,0 +1,74 @@ >> +/* >> + * 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 "cpu-qom.h" > > cpu.h already does #include "cpu-qom.h", so no need to include it here > again. thanks, I'll delete it. > >> +#include "qemu-common.h" >> + >> + >> +/* CPUClass::reset() */ >> +static void openrisc_cpu_reset(CPUState *s) >> +{ >> + OPENRISCCPU *cpu = OPENRISC_CPU(s); >> + OPENRISCCPUClass *occ = OPENRISC_CPU_GET_CLASS(cpu); >> + CPUOPENRISCState *env = &cpu->env; >> + >> + occ->parent_reset(s); >> + >> + openrisc_reset(env); > > Shouldn't this be inline here? And don't forget to reset the common > fields, too. The usual way I think is to let a helper signal an > interrupt_request and then from outside the thread call cpu_reset(), > which will call the above function. > Thanks, I'll check this, and reset the other fields. > Be warned that over time more and more fields will be moved from > CPU_COMMON into CPUState, so env is not ideal long-term. > did you mean I should use a memset(0) clean the all fields here? >> + >> +} >> + >> +static void openrisc_cpu_initfn(Object *obj) >> +{ >> + OPENRISCCPU *cpu = OPENRISC_CPU(obj); >> + CPUOPENRISCState *env = &cpu->env; >> + >> + cpu_exec_init(env); >> + >> + env->flags = 0; >> + >> + cpu_reset(CPU(cpu)); > > This should be part of a realizefn, not of the initfn. We don't have the > former just yet, so it should go into the cpu_xxx_init() function for now. > sorry, you mean I should rename it static void openrisc_cpu_realizefn(Object *obj)? >> +} >> + >> +static void openrisc_cpu_class_init(ObjectClass *oc, void *data) >> +{ >> + OPENRISCCPUClass *occ = OPENRISC_CPU_CLASS(oc); >> + CPUClass *cc = CPU_CLASS(oc); >> + >> + occ->parent_reset = cc->reset; >> + cc->reset = openrisc_cpu_reset; >> +} >> + >> +static const TypeInfo openrisc_cpu_type_info = { >> + .name = TYPE_OPENRISC_CPU, >> + .parent = TYPE_CPU, >> + .instance_size = sizeof(OPENRISCCPU), >> + .instance_init = openrisc_cpu_initfn, >> + .abstract = false, >> + .class_size = sizeof(OPENRISCCPUClass), >> + .class_init = openrisc_cpu_class_init, >> +}; >> + >> +static void openrisc_cpu_register_types(void) >> +{ >> + type_register_static(&openrisc_cpu_type_info); >> +} >> + >> +type_init(openrisc_cpu_register_types) > [...] >> diff --git a/target-openrisc/helper.c b/target-openrisc/helper.c >> new file mode 100644 >> index 0000000..dcb61c9 >> --- /dev/null >> +++ b/target-openrisc/helper.c >> @@ -0,0 +1,67 @@ >> +/* >> + * 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 Library 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 >> + * Library General Public License for more details. >> + * >> + * You should have received a copy of the GNU Library General Public >> + * License along with this library; if not, write to the Free Software >> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301 >> USA >> + */ >> + >> +#include "cpu.h" >> +#include "qemu-common.h" >> +#include "gdbstub.h" >> +#include "helper.h" >> +#include "host-utils.h" >> +#if !defined(CONFIG_USER_ONLY) >> +#include "hw/loader.h" >> +#endif >> + >> +typedef struct { >> + const char *name; >> +} OPENRISCDef; >> + >> +static const OPENRISCDef openrisc_defs[] = { >> + {.name = "or1200",} >> +}; > > Don't. Use QOM subclasses for modelling CPU types. > OK, let me find some code to copy. I'm not sure about QOM for now, yet. >> + >> +void cpu_openrisc_list(FILE *f, fprintf_function cpu_fprintf) >> +{ >> + int i; >> + >> + cpu_fprintf(f, "Available CPUs:\n"); >> + for (i = 0; i < ARRAY_SIZE(openrisc_defs); ++i) { >> + cpu_fprintf(f, " %s\n", openrisc_defs[i].name); >> + } >> +} > > This can then walk types to list models, cf. target-arm. > Thanks, I'll read ARM's code. >> + >> +CPUOPENRISCState *cpu_openrisc_init(const char *cpu_model) > > This should return OpenRISCCPU instead. > > cpu_init() can return CPUOpenRISCState for backwards compatibility, > there's lots of examples on the list. > Thanks, I'll check and fix. >> +{ >> + CPUOPENRISCState *env; >> + static int tcg_inited; >> + >> + env = g_malloc0(sizeof(*env)); > > No. This needs to instantiate the QOM type with object_new(). > Thanks, I'll use object_new() >> + memset(env, 0, sizeof(*env)); >> + cpu_exec_init(env); > > This is already in the initn, so drop it here. > Thank you, I'll drop it. >> + qemu_init_vcpu(env); > > This is the second candidate for a realizefn. > >> + if (!tcg_inited) { > > if (tcg_enabled() && !tcg_inited) { > > Note that besides TCG there's qtest that your new target should support. > Sorry, I'm not sure about this. >> + tcg_inited = 1; >> + openrisc_translate_init(); >> + } >> + >> + return env; >> +} >> + >> +void do_interrupt(CPUOPENRISCState *env) >> +{ >> +} > [...] >> diff --git a/target-openrisc/machine.c b/target-openrisc/machine.c >> new file mode 100644 >> index 0000000..c7ac0ea >> --- /dev/null >> +++ b/target-openrisc/machine.c >> @@ -0,0 +1,76 @@ >> +/* >> + * 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 Library 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 >> + * Library General Public License for more details. >> + * >> + * You should have received a copy of the GNU Library General Public >> + * License along with this library; if not, write to the Free Software >> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301 >> USA >> + */ >> + >> +#include "hw/hw.h" >> +#include "hw/boards.h" >> +#include "kvm.h" >> + >> +static const VMStateDescription vmstate_cpu = { >> + .name = "cpu", >> + .fields = (VMStateField[]) { >> + VMSTATE_UINT32_ARRAY(gpr, CPUOPENRISCState, 32), >> + VMSTATE_UINT32(sr, CPUOPENRISCState), >> + VMSTATE_UINT32(epcr, CPUOPENRISCState), >> + VMSTATE_UINT32(eear, CPUOPENRISCState), >> + VMSTATE_UINT32(esr, CPUOPENRISCState), >> + VMSTATE_UINT32(fpcsr, CPUOPENRISCState), >> + VMSTATE_UINT32(pc, CPUOPENRISCState), >> + VMSTATE_UINT32(npc, CPUOPENRISCState), >> + VMSTATE_UINT32(ppc, CPUOPENRISCState), >> + VMSTATE_END_OF_LIST() >> + } >> +}; >> + >> +void cpu_save(QEMUFile *f, void *opaque) >> +{ >> + CPUOPENRISCState *env = (CPUOPENRISCState *)opaque; >> + unsigned int i; >> + >> + for (i = 0; i < 32; i++) { >> + qemu_put_betls(f, &env->gpr[i]); >> + } >> + >> + qemu_put_betls(f, &env->epcr); >> + qemu_put_betls(f, &env->eear); >> + qemu_put_betls(f, &env->esr); >> + >> + qemu_put_betls(f, &env->sr); >> + qemu_put_be32s(f, &env->pc); >> + qemu_put_be32s(f, &env->fpcsr); >> +} >> + >> +int cpu_load(QEMUFile *f, void *opaque, int version_id) >> +{ >> + CPUOPENRISCState *env = (CPUOPENRISCState *)opaque; >> + unsigned int i; >> + >> + for (i = 0; i < 32; i++) { >> + qemu_get_betls(f, &env->gpr[i]); >> + } >> + >> + qemu_get_betls(f, &env->epcr); >> + qemu_get_betls(f, &env->eear); >> + qemu_get_betls(f, &env->esr); >> + >> + qemu_get_betls(f, &env->sr); >> + qemu_get_betls(f, &env->pc); >> + qemu_get_be32s(f, &env->fpcsr); >> + tlb_flush(env, 1); >> + >> + return 0; >> +} > [snip] > > This is a mix of two ways of doing the same thing. You should only use > VMState for new code. > did you mean I should not use QEMUFile? > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg Thanks very much for your comment. And I hope I can get more information from you for I can not understand all of your comment, your comment is good and useful, but for me, a newbie, I need more time to follow your step. Regard, Jia.
