Re: [Qemu-devel] [PATCH v6 01/15] target-tricore: Add target stubs and qom-cpu

[Peter Maydell]

On 22 August 2014 17:52, Bastian Koppelmann
<address@hidden> wrote:
> Add TriCore target stubs, and QOM cpu, and Maintainer
>
> Signed-off-by: Bastian Koppelmann <address@hidden>
> ---

> +#define TYPE_TRICORE_CPU "tricore-cpu"
> +
> +#define TRICORE_CPU_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(TRICORECPUClass, (klass), TYPE_TRICORE_CPU)
> +#define TRICORE_CPU(obj) \
> +    OBJECT_CHECK(TRICORECPU, (obj), TYPE_TRICORE_CPU)
> +#define TRICORE_CPU_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(TRICORECPUClass, (obj), TYPE_TRICORE_CPU)
> +
> +typedef struct TRICORECPUClass {
> +    /*< private >*/
> +    CPUClass parent_class;
> +    /*< public >*/
> +
> +    DeviceRealize parent_realize;
> +    void (*parent_reset)(CPUState *cpu);
> +} TRICORECPUClass;

Googling suggests the correct capitalization for the CPU name
is "TriCore", which means this struct should be
TriCoreCPUClass.

> +
> +/**
> + * TRICORECPU:
> + * @env: #CPUTRICOREState
> + *
> + * A TRICORE CPU.
> + */
> +typedef struct TRICORECPU {
> +    /*< private >*/
> +    CPUState parent_obj;
> +    /*< public >*/
> +
> +    CPUTRICOREState env;
> +} TRICORECPU;

TriCoreCPU.
> +++ b/target-tricore/cpu.c
> @@ -0,0 +1,191 @@
> +/*
> + *  TRICORE emulation for qemu: main translation routines.

TriCore.

> +typedef struct TRICORECPUInfo {
> +    const char *name;
> +    void (*initfn)(Object *obj);
> +    void (*class_init)(ObjectClass *oc, void *data);
> +} TRICORECPUInfo;
> +
> +static const TRICORECPUInfo tricore_cpus[] = {
> +    { .name = "tc1796",      .initfn = tc1796_initfn },
> +    { .name = "aurix",       .initfn = aurix_initfn }
> +};

This list is missing the terminator:
    { .name = NULL }

This causes QEMU to segfault on startup if compiled
with clang. (Presumably GCC happens to put some zero
data next in the binary.)

> +++ b/target-tricore/cpu.h
> @@ -0,0 +1,400 @@
> +/*
> + *  TRICORE emulation for qemu: main CPU struct.
> + *
> + *  Copyright (c) 2012-2014 Bastian Koppelmann C-Lab/University Paderborn
> + *
> + * 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/>.
> + */
> +#if !defined(__TRICORE_CPU_H__)
> +#define __TRICORE_CPU_H__
> +
> +#include "tricore-defs.h"
> +#include "config.h"
> +#include "qemu-common.h"
> +#include "exec/cpu-defs.h"
> +#include "fpu/softfloat.h"
> +
> +#define ELF_MACHINE     EM_TRICORE
> +
> +#define CPUArchState struct CPUTRICOREState
> +
> +struct CPUTRICOREState;
> +
> +#define TRICORE_CPU_IRQ 0
> +#define TRICORE_CPU_FIQ 1

Does Tricore really have ARM-style IRQ and FIQ lines,
or is this just cut-n-paste from ARM?

> +struct tricore_boot_info;
> +
> +#define NB_MMU_MODES 3
> +
> +typedef struct tricore_def_t tricore_def_t;
> +
> +typedef struct CPUTRICOREState CPUTRICOREState;
> +struct CPUTRICOREState {
> +    /* GPR Register */
> +    target_ulong gpr_a[16];
> +    target_ulong gpr_d[16];

target_ulong in CPU state isn't a great idea usually
unless you really have PPC-style "register width
depends on CPU". If your CPU is always 32 bit then
it's better just to use uint32_t in my opinion.

> +    /* CSFR Register */
> +#define MASK_PCXI_PCPN 0xff000000
> +#define MASK_PCXI_PIE  0x00800000
> +#define MASK_PCXI_UL   0x00400000
> +#define MASK_PCXI_PCXS 0x000f0000
> +#define MASK_PCXI_PCXO 0x0000ffff

Don't put all these defines in the middle of the struct, please.

thanks
-- PMM