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

[Jia Liu]

Hi Andreas,

On Sun, May 27, 2012 at 8:44 PM, Andreas Färber <address@hidden> wrote:
> Am 27.05.2012 07:32, schrieb Jia Liu:
>> add openrisc target stubs.
>>
>> Signed-off-by: Jia Liu <address@hidden>
>
> 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 <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 "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 <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/>.
>> + */
>> +
>> +#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 <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 "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 <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 "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.