Re: [Qemu-devel] [PATCH v2 01/17] Openrisc: add target stubs

["Andreas Färber" <afaerber@suse.de>]

Am 27.05.2012 07:32, schrieb Jia Liu:
> add openrisc target stubs.
> 
> Signed-off-by: Jia Liu <proljc@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.

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. ;)

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.

[...]
> 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 <proljc@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.

> 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 <proljc@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?

> +
> +#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?

> +
> +#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.

> +
> +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().

> +#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().

> +
> +#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.

> +}
> +
> +#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 <proljc@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.

> +
> +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().

> +
> +    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.

> +}

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 <proljc@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.

> +
> +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