Re: [Qemu-devel] [PATCH 01/15] Openrisc: add target stub

[Jia Liu]

Hi Andreas,

Thanks, I'm trying fix them all.

On Thu, May 17, 2012 at 10:14 PM, Andreas Färber <address@hidden> wrote:
> Am 17.05.2012 10:35, schrieb Jia Liu:
>> add the openrisc target stub and basic implementation.
>>
>> Signed-off-by: Jia Liu <address@hidden>
>> ---
>> 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 <address@hidden>
>
> 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 
>> <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.
>
>> +
>> +#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 <address@hidden>
>> + *
>> + * 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.
>
>> +#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.
>
> 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 = 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.
>
>> +}
>> +
>> +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 <address@hidden>
>> + *
>> + * 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.
>
>> +
>> +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.
>
>> +
>> +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 = 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 = 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 <address@hidden>
>> + *
>> + * 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.
>
> 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.