[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/6 v4] target-tilegx: Firstly add to qemu with
From: |
Chen Gang S |
Subject: |
Re: [Qemu-devel] [PATCH 1/6 v4] target-tilegx: Firstly add to qemu with minimized features |
Date: |
Sat, 28 Feb 2015 13:20:02 +0800 |
User-agent: |
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 |
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
[Qemu-devel] [PATCH 3/6 v4] linux-user: tilegx: Add target features support within qemu, Chen Gang S, 2015/02/22
[Qemu-devel] [PATCH 4/6 v4] linux-user: Support tilegx architecture in syscall, Chen Gang S, 2015/02/22
[Qemu-devel] [PATCH 5/6 v4] linux-user: Support tilegx architecture in linux-user, Chen Gang S, 2015/02/22
[Qemu-devel] [PATCH 6/6 v4] linux-user/syscall.c: Switch all macros which are not defined in tilegx, Chen Gang S, 2015/02/22