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

[Chen Gang S]

Firstly, thank you very much for your patient work!


On 2/28/15 01:36, Andreas Färber wrote:
> Hi,
> 
> "target-tilegx: Initial stub" or "...support"? No need to mention QEMU
> (spelling!) in a QEMU commit.
> 

OK, thanks.

> 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, ...?
> 

OK, thanks.

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

NO. And next, I shall drop them.

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

OK, thanks.

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

OK, thanks.

>> +    /*< 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...)
> 

OK, thanks. I shall call it TileGXCPU.

>> +    /*< 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?
> 

OK, thanks. At present, we do not use base_vectors, so I guess, we can
just remove it, and the related implementation will be:

typedef struct TileGXCPU {
    /*< private >*/
    CPUState parent_obj;
    /*< public >*/

    CPUS390XState env;
} TileGXCPU;


>> +    /*< public >*/
>> +
>> +    CPUTLState env;
> 
> Can this be more telling than TL please?
>

How about CPUTLGState?
 
>> +} 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.
> 

Yes, we need another name for it. How about tcc?

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

OK.

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

Originally, I copied from Microblaze, but at present, I guess, we can
drop it (then also drop base_vectors).

>> +    DEFINE_PROP_UINT64("tilegx.base-vectors", TilegxCPU, base_vectors, 0),
> 
> Why "tilegx." prefix? That would likely interfere with our QMP tools.
> 

May it be "tilera.base-vectors"?

But after all, I guess, we can just drop it, at present.

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

OK, thanks.

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

OK, thanks. It is already replied in another mail thread.

For me, we need still let TYPE_TILEGX_CPU be "tilegx-cpu" (according to
microblaze implementation).

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

Hmm... originally, I really noticed about it, but after check the other
target implementation (e.g microblaze, alpha, s390), I remained it.

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

Excuse me, I am not quite familiar with it, could you provide more
details?

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

Yes, it is for future use, I guess, at present, we can just drop it:

 - "translate.c" includes "exec/helper-gen.h".

 - "exec/helper-gen.h" includes "helper.h".


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

OK, thanks.

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

OK, thanks.

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

OK, thanks, I shall use qemu_log for it.

Thanks
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed