From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:34163) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SV1TK-0000v6-NL for qemu-devel@nongnu.org; Thu, 17 May 2012 10:14:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SV1TD-0001c7-IC for qemu-devel@nongnu.org; Thu, 17 May 2012 10:14:30 -0400 Received: from cantor2.suse.de ([195.135.220.15]:60263 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SV1TD-0001bh-5Q for qemu-devel@nongnu.org; Thu, 17 May 2012 10:14:23 -0400 Message-ID: <4FB507B7.2060806@suse.de> Date: Thu, 17 May 2012 16:14:15 +0200 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1337243758-11802-1-git-send-email-proljc@gmail.com> <1337243758-11802-2-git-send-email-proljc@gmail.com> In-Reply-To: <1337243758-11802-2-git-send-email-proljc@gmail.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 01/15] Openrisc: add target stub List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jia Liu Cc: qemu-devel@nongnu.org Am 17.05.2012 10:35, schrieb Jia Liu: > add the openrisc target stub and basic implementation. >=20 > Signed-off-by: Jia Liu > --- > 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. > @@ -0,0 +1,70 @@ > +/* > + * QEMU Openrisc CPU > + * > + * Copyright (c) 2012 Jia Liu Minor nitpick: I guess this was copied from some other header? Uses a two-space indentation here and one-space below. > + * > + * 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 . > + */ > + > +#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. > + > +#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? > + > +/** > + * 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 > + * > + * 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 . > + */ > + > +#include "cpu.h" > +#include "cpu-qom.h" cpu.h already does #include "cpu-qom.h", so no need to include it here again. > +#include "qemu-common.h" > + > + > +/* CPUClass::reset() */ > +static void openrisc_cpu_reset(CPUState *s) > +{ > + OPENRISCCPU *cpu =3D OPENRISC_CPU(s); > + OPENRISCCPUClass *occ =3D OPENRISC_CPU_GET_CLASS(cpu); > + CPUOPENRISCState *env =3D &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. Be warned that over time more and more fields will be moved from CPU_COMMON into CPUState, so env is not ideal long-term. > + > +} > + > +static void openrisc_cpu_initfn(Object *obj) > +{ > + OPENRISCCPU *cpu =3D OPENRISC_CPU(obj); > + CPUOPENRISCState *env =3D &cpu->env; > + > + cpu_exec_init(env); > + > + env->flags =3D 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= . > +} > + > +static void openrisc_cpu_class_init(ObjectClass *oc, void *data) > +{ > + OPENRISCCPUClass *occ =3D OPENRISC_CPU_CLASS(oc); > + CPUClass *cc =3D CPU_CLASS(oc); > + > + occ->parent_reset =3D cc->reset; > + cc->reset =3D openrisc_cpu_reset; > +} > + > +static const TypeInfo openrisc_cpu_type_info =3D { > + .name =3D TYPE_OPENRISC_CPU, > + .parent =3D TYPE_CPU, > + .instance_size =3D sizeof(OPENRISCCPU), > + .instance_init =3D openrisc_cpu_initfn, > + .abstract =3D false, > + .class_size =3D sizeof(OPENRISCCPUClass), > + .class_init =3D 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 > + * > + * 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[] =3D { > + {.name =3D "or1200",} > +}; Don't. Use QOM subclasses for modelling CPU types. > + > +void cpu_openrisc_list(FILE *f, fprintf_function cpu_fprintf) > +{ > + int i; > + > + cpu_fprintf(f, "Available CPUs:\n"); > + for (i =3D 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. > + > +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. > +{ > + CPUOPENRISCState *env; > + static int tcg_inited; > + > + env =3D g_malloc0(sizeof(*env)); No. This needs to instantiate the QOM type with object_new(). > + memset(env, 0, sizeof(*env)); > + cpu_exec_init(env); This is already in the initn, so drop it here. > + 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. > + tcg_inited =3D 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 > + * > + * 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 =3D { > + .name =3D "cpu", > + .fields =3D (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 =3D (CPUOPENRISCState *)opaque; > + unsigned int i; > + > + for (i =3D 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 =3D (CPUOPENRISCState *)opaque; > + unsigned int i; > + > + for (i =3D 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. Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg