Re: [Qemu-devel] [PATCH 1/6 v4] target-tilegx: Firstly add to qemu with minimized features

[Andreas Färber]

Hi,

"target-tilegx: Initial stub" or "...support"? No need to mention QEMU
(spelling!) in a QEMU commit.

Am 22.02.2015 um 14:33 schrieb Chen Gang S:
> It almost likes a template for adding an architecture target.

That's a comment that's not really descriptive of what the patch does.
Instead, maybe mention that this is the configure and build system
support etc. and what name to use for enabling it from configure, that
it's linux-user only for now, ...?

> 
> Signed-off-by: Chen Gang <address@hidden>
> ---
>  configure                             |   7 ++
>  default-configs/tilegx-linux-user.mak |   1 +
>  target-tilegx/Makefile.objs           |   1 +
>  target-tilegx/cpu-qom.h               |  72 +++++++++++++++
>  target-tilegx/cpu.c                   | 162 
> ++++++++++++++++++++++++++++++++++
>  target-tilegx/cpu.h                   |  85 ++++++++++++++++++
>  target-tilegx/helper.h                |   0
>  target-tilegx/translate.c             |  54 ++++++++++++
>  8 files changed, 382 insertions(+)
>  create mode 100644 default-configs/tilegx-linux-user.mak
>  create mode 100644 target-tilegx/Makefile.objs
>  create mode 100644 target-tilegx/cpu-qom.h
>  create mode 100644 target-tilegx/cpu.c
>  create mode 100644 target-tilegx/cpu.h
>  create mode 100644 target-tilegx/helper.h
>  create mode 100644 target-tilegx/translate.c
> 
> diff --git a/configure b/configure
> index 7ba4bcb..23aa8f6 100755
> --- a/configure
> +++ b/configure
> @@ -5191,6 +5191,9 @@ case "$target_name" in
>    s390x)
>      gdb_xml_files="s390x-core64.xml s390-acr.xml s390-fpr.xml"
>    ;;
> +  tilegx)
> +    TARGET_ARCH=tilegx
> +  ;;
>    unicore32)
>    ;;
>    xtensa|xtensaeb)
> @@ -5363,6 +5366,10 @@ for i in $ARCH $TARGET_BASE_ARCH ; do
>      echo "CONFIG_SPARC_DIS=y"  >> $config_target_mak
>      echo "CONFIG_SPARC_DIS=y"  >> config-all-disas.mak
>    ;;
> +  tilegx*)
> +    echo "CONFIG_TILEGX_DIS=y"  >> $config_target_mak
> +    echo "CONFIG_TILEGX_DIS=y"  >> config-all-disas.mak
> +  ;;

Hadn't you been asked to drop these lines, as you are not yet adding any
disassembler code that uses it?

>    xtensa*)
>      echo "CONFIG_XTENSA_DIS=y"  >> $config_target_mak
>      echo "CONFIG_XTENSA_DIS=y"  >> config-all-disas.mak
[...]
> diff --git a/target-tilegx/cpu-qom.h b/target-tilegx/cpu-qom.h
> new file mode 100644
> index 0000000..e15a8b8
> --- /dev/null
> +++ b/target-tilegx/cpu-qom.h
> @@ -0,0 +1,72 @@
> +/*
> + * QEMU Tilegx CPU

"TILE-Gx" according to
http://www.tilera.com/products/?ezchip=585&spage=614 - please fix
wherever used in textual form.

> + *
> + * Copyright (c) 2015 Chen Gang
> + *
> + * 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.1 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/lgpl-2.1.html>
> + */
> +#ifndef QEMU_TILEGX_CPU_QOM_H
> +#define QEMU_TILEGX_CPU_QOM_H
> +
> +#include "qom/cpu.h"
> +
> +#define TYPE_TILEGX_CPU "tilegx-cpu"
> +
> +#define TILEGX_CPU_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(TilegxCPUClass, (klass), TYPE_TILEGX_CPU)
> +#define TILEGX_CPU(obj) \
> +    OBJECT_CHECK(TilegxCPU, (obj), TYPE_TILEGX_CPU)
> +#define TILEGX_CPU_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(TilegxCPUClass, (obj), TYPE_TILEGX_CPU)
> +
> +/**
> + * TilegxCPUClass:
> + * @parent_realize: The parent class' realize handler.
> + * @parent_reset: The parent class' reset handler.
> + *
> + * A Tilegx CPU model.
> + */
> +typedef struct TilegxCPUClass {

For the benefit of readers, please call this TileGXCPUClass ...

> +    /*< private >*/
> +    CPUClass parent_class;
> +    /*< public >*/
> +
> +    DeviceRealize parent_realize;
> +    void (*parent_reset)(CPUState *cpu);
> +} TilegxCPUClass;
> +
> +/**
> + * TilegxCPU:
> + * @env: #CPUTLState
> + *
> + * A Tilegx CPU.
> + */
> +typedef struct TilegxCPU {

... and TileGXCPU. (or TileGx...)

> +    /*< private >*/
> +    CPUState parent_obj;
> +    uint64_t base_vectors;

This should not be in here, the private section serves to hide the
parent field from documentation.

base_vectors should also probably be after env, for performance reasons.
rth?

> +    /*< public >*/
> +
> +    CPUTLState env;

Can this be more telling than TL please?

> +} TilegxCPU;
> +
> +static inline TilegxCPU *tilegx_env_get_cpu(CPUTLState *env)
> +{
> +    return container_of(env, TilegxCPU, env);
> +}
> +
> +#define ENV_GET_CPU(e) CPU(tilegx_env_get_cpu(e))
> +
> +#endif
> diff --git a/target-tilegx/cpu.c b/target-tilegx/cpu.c
> new file mode 100644
> index 0000000..3dd66b5
> --- /dev/null
> +++ b/target-tilegx/cpu.c
> @@ -0,0 +1,162 @@
> +/*
> + * QEMU Tilegx CPU
> + *
> + *  Copyright (c) 2015 Chen Gang
> + *
> + * 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.1 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/lgpl-2.1.html>
> + */
> +
> +#include "cpu.h"
> +#include "qemu-common.h"
> +#include "hw/qdev-properties.h"
> +#include "migration/vmstate.h"
> +
> +TilegxCPU *cpu_tilegx_init(const char *cpu_model)
> +{
> +    TilegxCPU *cpu;
> +
> +    cpu = TILEGX_CPU(object_new(TYPE_TILEGX_CPU));
> +
> +    object_property_set_bool(OBJECT(cpu), true, "realized", NULL);
> +
> +    return cpu;
> +}
> +
> +static void tilegx_cpu_set_pc(CPUState *cs, vaddr value)
> +{
> +    TilegxCPU *cpu = TILEGX_CPU(cs);
> +
> +    cpu->env.pc = value;
> +}
> +
> +static bool tilegx_cpu_has_work(CPUState *cs)
> +{
> +    return true;
> +}
> +
> +static void tilegx_cpu_reset(CPUState *s)
> +{
> +    TilegxCPU *cpu = TILEGX_CPU(s);
> +    TilegxCPUClass *mcc = TILEGX_CPU_GET_CLASS(cpu);

Why mcc? Probably copied from a CPU with M.

> +    CPUTLState *env = &cpu->env;
> +
> +    mcc->parent_reset(s);
> +
> +    memset(env, 0, sizeof(CPUTLState));
> +    tlb_flush(s, 1);
> +}
> +
> +static void tilegx_cpu_realizefn(DeviceState *dev, Error **errp)
> +{
> +    CPUState *cs = CPU(dev);
> +    TilegxCPUClass *mcc = TILEGX_CPU_GET_CLASS(dev);

Ditto.

> +
> +    cpu_reset(cs);
> +    qemu_init_vcpu(cs);
> +
> +    mcc->parent_realize(dev, errp);
> +}
> +
> +static void tilegx_tcg_init(void)
> +{
> +}
> +
> +static void tilegx_cpu_initfn(Object *obj)
> +{
> +    CPUState *cs = CPU(obj);
> +    TilegxCPU *cpu = TILEGX_CPU(obj);
> +    CPUTLState *env = &cpu->env;
> +    static bool tcg_initialized;
> +
> +    cs->env_ptr = env;
> +    cpu_exec_init(env);
> +
> +    if (tcg_enabled() && !tcg_initialized) {
> +        tcg_initialized = true;
> +        tilegx_tcg_init();
> +    }
> +}
> +
> +static const VMStateDescription vmstate_tilegx_cpu = {
> +    .name = "cpu",
> +    .unmigratable = 1,
> +};
> +
> +static Property tilegx_properties[] = {

"tilegx_cpu_properties" for consistency?

> +    DEFINE_PROP_UINT64("tilegx.base-vectors", TilegxCPU, base_vectors, 0),

Why "tilegx." prefix? That would likely interfere with our QMP tools.

What is this actually used for? To someone not knowing the architecture
it may sound a bit strange to have a variable/property ...vectors that
is actually just one uint64 value. Someone recently added an API to add
a description for a property, and this seems to be calling for one.
Moving it to a patch that actually uses it would be another idea.

> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void tilegx_cpu_do_interrupt(CPUState *cs)
> +{
> +    cs->exception_index = -1;
> +}
> +
> +static int tilegx_cpu_handle_mmu_fault(CPUState *cs, vaddr address, int rw,
> +                            int mmu_idx)

Indentation.

> +{
> +    cpu_dump_state(cs, stderr, fprintf, 0);
> +    return 1;
> +}
> +
> +static bool tilegx_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
> +{
> +    if (interrupt_request & CPU_INTERRUPT_HARD) {
> +        tilegx_cpu_do_interrupt(cs);
> +        return true;
> +    }
> +    return false;
> +}
> +
> +static void tilegx_cpu_class_init(ObjectClass *oc, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(oc);
> +    CPUClass *cc = CPU_CLASS(oc);
> +    TilegxCPUClass *mcc = TILEGX_CPU_CLASS(oc);
> +
> +    mcc->parent_realize = dc->realize;
> +    dc->realize = tilegx_cpu_realizefn;
> +
> +    mcc->parent_reset = cc->reset;
> +    cc->reset = tilegx_cpu_reset;
> +
> +    cc->has_work = tilegx_cpu_has_work;
> +    cc->do_interrupt = tilegx_cpu_do_interrupt;
> +    cc->cpu_exec_interrupt = tilegx_cpu_exec_interrupt;
> +    cc->dump_state = NULL;
> +    cc->set_pc = tilegx_cpu_set_pc;
> +    cc->gdb_read_register = NULL;
> +    cc->gdb_write_register = NULL;

Is this really safe to do? If so, all fields are zero-initialized at
this point already, so no need to assign NULL or 0.

> +    cc->handle_mmu_fault = tilegx_cpu_handle_mmu_fault;
> +    dc->vmsd = &vmstate_tilegx_cpu;
> +    dc->props = tilegx_properties;
> +    cc->gdb_num_core_regs = 0;
> +}
> +
> +static const TypeInfo tilegx_cpu_type_info = {
> +    .name = TYPE_TILEGX_CPU,
> +    .parent = TYPE_CPU,
> +    .instance_size = sizeof(TilegxCPU),
> +    .instance_init = tilegx_cpu_initfn,

What about CPU models? Do the Gx72, Gx36, etc. differ only in core count
or also in instruction set features?

> +    .class_size = sizeof(TilegxCPUClass),
> +    .class_init = tilegx_cpu_class_init,
> +};
> +
> +static void tilegx_cpu_register_types(void)
> +{
> +    type_register_static(&tilegx_cpu_type_info);
> +}
> +
> +type_init(tilegx_cpu_register_types)
> diff --git a/target-tilegx/cpu.h b/target-tilegx/cpu.h
> new file mode 100644
> index 0000000..09a2b26
> --- /dev/null
> +++ b/target-tilegx/cpu.h
> @@ -0,0 +1,85 @@
> +/*
> + *  Tilegx virtual CPU header
> + *
> + *  Copyright (c) 2015 Chen Gang
> + *
> + * 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
> + * 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_TILEGX_H
> +#define CPU_TILEGX_H
> +
> +#include "config.h"
> +#include "qemu-common.h"
> +
> +#define TARGET_LONG_BITS 64
> +
> +#define CPUArchState struct CPUTLState

Drop the struct here? You have a typedef below.

> +
> +#include "exec/cpu-defs.h"
> +#include "fpu/softfloat.h"
> +
> +/* Tilegx register alias */
> +#define TILEGX_R_RE  0   /*  0 register, for function/syscall return value */
> +#define TILEGX_R_NR  10  /* 10 register, for syscall number */
> +#define TILEGX_R_BP  52  /* 52 register, optional frame pointer */
> +#define TILEGX_R_TP  53  /* TP register, thread local storage data */
> +#define TILEGX_R_SP  54  /* SP register, stack pointer */
> +#define TILEGX_R_LR  55  /* LR register, may save pc, but it is not pc */
> +
> +typedef struct CPUTLState {
> +    uint64_t regs[56];
> +    uint64_t pc;

You have marked the CPU unmigratable. Might it make sense to still
prepare the corresponding VMState fields so that it does not get forgotten?

> +    CPU_COMMON
> +} CPUTLState;
> +
> +#include "cpu-qom.h"
> +
> +/* Tilegx memory attributes */
> +#define TARGET_PAGE_BITS 16  /* Tilegx uses 64KB page size */
> +#define MMAP_SHIFT TARGET_PAGE_BITS
> +#define TARGET_PHYS_ADDR_SPACE_BITS 42 /* Tilegx is 42 bit physical address 
> */
> +#define TARGET_VIRT_ADDR_SPACE_BITS 64 /* Tilegx has 64 bit virtual address 
> */
> +#define MMU_USER_IDX    0  /* independent from both qemu and architecture */
> +
> +#include "exec/cpu-all.h"
> +
> +int cpu_tilegx_exec(CPUTLState *s);
> +int cpu_tilegx_signal_handler(int host_signum, void *pinfo, void *puc);
> +
> +#define cpu_exec cpu_tilegx_exec
> +#define cpu_gen_code cpu_tilegx_gen_code
> +#define cpu_signal_handler cpu_tilegx_signal_handler
> +
> +TilegxCPU *cpu_tilegx_init(const char *cpu_model);
> +
> +static inline CPUTLState *cpu_init(const char *cpu_model)
> +{
> +    TilegxCPU *cpu = cpu_tilegx_init(cpu_model);
> +    if (cpu == NULL) {
> +        return NULL;
> +    }
> +    return &cpu->env;
> +}
> +
> +static inline void cpu_get_tb_cpu_state(CPUTLState *env, target_ulong *pc,
> +                                        target_ulong *cs_base, int *flags)
> +{
> +    *pc = env->pc;
> +    *cs_base = 0;
> +    *flags = 0;
> +}
> +
> +#include "exec/exec-all.h"
> +
> +#endif
> diff --git a/target-tilegx/helper.h b/target-tilegx/helper.h
> new file mode 100644
> index 0000000..e69de29

Is this empty file actually included from somewhere in this patch?

> diff --git a/target-tilegx/translate.c b/target-tilegx/translate.c
> new file mode 100644
> index 0000000..5131fa7
> --- /dev/null
> +++ b/target-tilegx/translate.c
> @@ -0,0 +1,54 @@
> +/*
> + * QEMU Tilegx CPU
> + *
> + *  Copyright (c) 2015 Chen Gang
> + *
> + * 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.1 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/lgpl-2.1.html>
> + */
> +
> +#include "cpu.h"
> +#include "disas/disas.h"
> +#include "tcg-op.h"
> +#include "exec/helper-proto.h"
> +#include "exec/cpu_ldst.h"
> +#include "exec/helper-gen.h"
> +
> +static inline void gen_intermediate_code_internal(TilegxCPU *cpu,
> +                                                  TranslationBlock *tb,
> +                                                  bool search_pc)
> +{
> +    /*
> +     * FIXME: after load elf64 tilegx binary successfully, it will quit, at
> +     * present, and will implement the related features next.
> +     */
> +    fprintf(stderr, "\nLoad elf64 tilegx successfully\n");

"Loaded"

> +    fprintf(stderr, "reach code start position: [" TARGET_FMT_lx "] %s\n\n",

"reached"

> +            tb->pc, lookup_symbol(tb->pc));

This output is hopefully only temporary. But as a general reminder,
there is error_report() for error messages, and for debug messages
please define your own macros or use the qemu_log infrastructure instead
of fprintf(stderr, ...).

> +    exit(0);
> +}
> +
> +void gen_intermediate_code(CPUTLState *env, struct TranslationBlock *tb)
> +{
> +    gen_intermediate_code_internal(tilegx_env_get_cpu(env), tb, false);
> +}
> +
> +void gen_intermediate_code_pc(CPUTLState *env, struct TranslationBlock *tb)
> +{
> +    gen_intermediate_code_internal(tilegx_env_get_cpu(env), tb, true);
> +}
> +
> +void restore_state_to_opc(CPUTLState *env, TranslationBlock *tb, int pc_pos)
> +{
> +}

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)