Re: [Qemu-devel] [PATCH 2/5] Blackfin: initial port

["Andreas Färber" <afaerber@suse.de>]

Hi,

Am 17.06.2013 09:16, schrieb Mike Frysinger:
> diff --git a/target-bfin/cpu-qom.h b/target-bfin/cpu-qom.h
> new file mode 100644
> index 0000000..697797b
> --- /dev/null
> +++ b/target-bfin/cpu-qom.h

For a new target, a separate cpu-qom.h should be unnecessary - it has
become impossible to include it without cpu.h. Just inline it into cpu.h
and group CPUState vs. CPUBfinState stuff together there.

> @@ -0,0 +1,61 @@
> +/*
> + * QEMU Blackfin CPU
> + *
> + * Copyright 2007-2013 Mike Frysinger
> + * Copyright 2007-2011 Analog Devices, Inc.
> + *
> + * Licensed under the Lesser GPL 2 or later.
> + */
> +
> +#ifndef QEMU_BFIN_CPU_QOM_H
> +#define QEMU_BFIN_CPU_QOM_H
> +
> +#include "qom/cpu.h"
> +
> +#define TYPE_BFIN_CPU "bfin-cpu"
> +
> +#define BFIN_CPU_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(BfinCPUClass, (klass), TYPE_BFIN_CPU)
> +#define BFIN_CPU(obj) \
> +    OBJECT_CHECK(BfinCPU, (obj), TYPE_BFIN_CPU)
> +#define BFIN_CPU_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(BfinCPUClass, (obj), TYPE_BFIN_CPU)
> +
> +/**
> + * BfinCPUClass:
> + * @parent_reset: The parent class' reset handler.
> + *
> + * An Bfin CPU model.
> + */
> +typedef struct BfinCPUClass {
> +    /*< private >*/
> +    CPUClass parent_class;
> +    /*< public >*/
> +
> +    void (*parent_reset)(CPUState *cpu);
> +} BfinCPUClass;
> +
> +/**
> + * BfinCPU:

QOM types should have verbose, readable names. Please use BlackfinCPU,
BlackfinCPUClass, TYPE_BLACKFIN_CPU.
By contrast, CPUBfinState, bfin_cpu_... and bfin-cpu are totally fine, I
guess.

> + * @env: #CPUArchState

Please don't use CPUArchState anywhere in bfin code except for the
#define CPUArchState CPUBfinState.

> + *
> + * An Bfin CPU.
> + */
> +typedef struct BfinCPU {
> +    /*< private >*/
> +    CPUState parent_obj;
> +    /*< public >*/
> +
> +    CPUArchState env;
> +} BfinCPU;
> +
> +static inline BfinCPU *bfin_env_get_cpu(CPUArchState *env)

CPUbfinState *env

> +{
> +    return BFIN_CPU(container_of(env, BfinCPU, env));

I've just posted a patch to drop these FOO_CPU() casts from all targets.
While there's no ACK yet, there was agreement on v1, so please just
return the container_of() here.

> +}
> +
> +#define ENV_GET_CPU(e) CPU(bfin_env_get_cpu(e))
> +
> +#define ENV_OFFSET offsetof(BfinCPU, env)
> +
> +#endif
> diff --git a/target-bfin/cpu.c b/target-bfin/cpu.c
> new file mode 100644
> index 0000000..871a1a1
> --- /dev/null
> +++ b/target-bfin/cpu.c
> @@ -0,0 +1,55 @@
> +/*
> + * QEMU Blackfin CPU
> + *
> + * Copyright 2007-2013 Mike Frysinger
> + * Copyright 2007-2011 Analog Devices, Inc.
> + *
> + * Licensed under the Lesser GPL 2 or later.
> + */
> +
> +#include "cpu.h"
> +#include "qemu-common.h"
> +
> +
> +/* CPUClass::reset() */
> +static void bfin_cpu_reset(CPUState *s)
> +{
> +    BfinCPU *cpu = BFIN_CPU(s);
> +    CPUArchState *env = &cpu->env;

CPUBfinState *

Missing memset(env, 0, offsetof(CPUBfinState, breakpoints)).

> +
> +    env->pc = 0xEF000000;
> +}
> +
> +static void bfin_cpu_initfn(Object *obj)
> +{
> +    CPUState *cs = CPU(obj);
> +    BfinCPU *cpu = BFIN_CPU(obj);
> +    CPUArchState *env = &cpu->env;
> +
> +    cs->env_ptr = env;
> +    cpu_exec_init(env);
> +}
> +
> +static void bfin_cpu_class_init(ObjectClass *oc, void *data)
> +{
> +    CPUClass *cc = CPU_CLASS(oc);
> +
> +    cc->reset = bfin_cpu_reset;
> +}
> +
> +static const TypeInfo bfin_cpu_type_info = {
> +    .name = TYPE_BFIN_CPU,
> +    .parent = TYPE_CPU,
> +    .instance_size = sizeof(BfinCPU),
> +    .instance_init = bfin_cpu_initfn,
> +    .abstract = false,
> +    .class_size = sizeof(BfinCPUClass),
> +    .class_init = bfin_cpu_class_init,
> +};
> +
> +static void bfin_cpu_register_types(void)
> +{
> +    type_register_static(&bfin_cpu_type_info);
> +}
> +
> +type_init(bfin_cpu_register_types)
> diff --git a/target-bfin/cpu.h b/target-bfin/cpu.h
> new file mode 100644
> index 0000000..d288197
> --- /dev/null
> +++ b/target-bfin/cpu.h
> @@ -0,0 +1,236 @@
> +/*
> + * Blackfin emulation
> + *
> + * Copyright 2007-2013 Mike Frysinger
> + * Copyright 2007-2011 Analog Devices, Inc.
> + *
> + * Licensed under the Lesser GPL 2 or later.
> + */
> +
> +#ifndef CPU_BFIN_H
> +#define CPU_BFIN_H
> +
> +struct DisasContext;
> +
> +#define TARGET_LONG_BITS 32
> +
> +#define ELF_MACHINE	EM_BLACKFIN
> +
> +#define CPUArchState struct CPUBfinState
> +
> +#include "config.h"
> +#include "qemu-common.h"
> +#include "exec/cpu-defs.h"
> +
> +#define TARGET_HAS_ICE 1
> +
> +#define EXCP_SYSCALL        0
> +#define EXCP_SOFT_BP        1
> +#define EXCP_STACK_OVERFLOW 3
> +#define EXCP_SINGLE_STEP    0x10
> +#define EXCP_TRACE_FULL     0x11
> +#define EXCP_UNDEF_INST     0x21
> +#define EXCP_ILL_INST       0x22
> +#define EXCP_DCPLB_VIOLATE  0x23
> +#define EXCP_DATA_MISALGIN  0x24
> +#define EXCP_UNRECOVERABLE  0x25
> +#define EXCP_DCPLB_MISS     0x26
> +#define EXCP_DCPLB_MULT     0x27
> +#define EXCP_EMU_WATCH      0x28
> +#define EXCP_MISALIG_INST   0x2a
> +#define EXCP_ICPLB_PROT     0x2b
> +#define EXCP_ICPLB_MISS     0x2c
> +#define EXCP_ICPLB_MULT     0x2d
> +#define EXCP_ILL_SUPV       0x2e
> +#define EXCP_ABORT          0x100
> +#define EXCP_DBGA           0x101
> +#define EXCP_OUTC           0x102
> +
> +#define CPU_INTERRUPT_NMI   CPU_INTERRUPT_TGT_EXT_1
> +
> +#define BFIN_L1_CACHE_BYTES 32
> +
> +/* Blackfin does 1K/4K/1M/4M, but for now only support 4k */
> +#define TARGET_PAGE_BITS    12
> +#define NB_MMU_MODES        2
> +
> +#define TARGET_PHYS_ADDR_SPACE_BITS 32
> +#define TARGET_VIRT_ADDR_SPACE_BITS 32
> +
> +#define cpu_init cpu_bfin_init
> +#define cpu_exec cpu_bfin_exec
> +#define cpu_gen_code cpu_bfin_gen_code
> +#define cpu_signal_handler cpu_bfin_signal_handler
> +
> +/* Indexes into astat array; matches bitpos in hardware too */
> +enum {
> +    ASTAT_AZ = 0,
> +    ASTAT_AN,
> +    ASTAT_AC0_COPY,
> +    ASTAT_V_COPY,
> +    ASTAT_CC = 5,
> +    ASTAT_AQ,
> +    ASTAT_RND_MOD = 8,
> +    ASTAT_AC0 = 12,
> +    ASTAT_AC1,
> +    ASTAT_AV0 = 16,
> +    ASTAT_AV0S,
> +    ASTAT_AV1,
> +    ASTAT_AV1S,
> +    ASTAT_V = 24,
> +    ASTAT_VS
> +};
> +
> +typedef struct CPUBfinState {
> +    CPU_COMMON
> +    int personality;
> +
> +    uint32_t dreg[8];
> +    uint32_t preg[8];
> +    uint32_t ireg[4];
> +    uint32_t mreg[4];
> +    uint32_t breg[4];
> +    uint32_t lreg[4];
> +    uint64_t areg[2];
> +    uint32_t rets;
> +    uint32_t lcreg[2], ltreg[2], lbreg[2];
> +    uint32_t cycles[2];
> +    uint32_t uspreg;
> +    uint32_t seqstat;
> +    uint32_t syscfg;
> +    uint32_t reti;
> +    uint32_t retx;
> +    uint32_t retn;
> +    uint32_t rete;
> +    uint32_t emudat;
> +    uint32_t pc;
> +
> +    /* ASTAT bits; broken up for speeeeeeeed */
> +    uint32_t astat[32];
> +    /* ASTAT delayed helpers */
> +    uint32_t astat_op, astat_arg[3];

Are you sure this field placement is what you want? Usually reset
memset()s all fields up to breakpoints (inside CPU_COMMON) so registers
are usually placed before CPU_COMMON.

Any field that is not a register accessed by TCG should rather be in
BlackfinCPU or BlackfinCPUClass - personality sounds like a candidate?

> +} CPUBfinState;
> +#define spreg preg[6]
> +#define fpreg preg[7]
> +
> +static inline uint32_t bfin_astat_read(CPUArchState *env)
> +{
> +    unsigned int i, ret;
> +
> +    ret = 0;
> +    for (i = 0; i < 32; ++i)
> +        ret |= (env->astat[i] << i);
> +
> +    return ret;
> +}
> +
> +static inline void bfin_astat_write(CPUArchState *env, uint32_t astat)
> +{
> +    unsigned int i;
> +    for (i = 0; i < 32; ++i)
> +        env->astat[i] = !!(astat & (1 << i));
> +}
> +
> +enum astat_ops {
> +    ASTAT_OP_NONE,
> +    ASTAT_OP_DYNAMIC,
> +    ASTAT_OP_ABS,
> +    ASTAT_OP_ABS_VECTOR,
> +    ASTAT_OP_ADD16,
> +    ASTAT_OP_ADD32,
> +    ASTAT_OP_ASHIFT16,
> +    ASTAT_OP_ASHIFT32,
> +    ASTAT_OP_COMPARE_SIGNED,
> +    ASTAT_OP_COMPARE_UNSIGNED,
> +    ASTAT_OP_LOGICAL,
> +    ASTAT_OP_LSHIFT16,
> +    ASTAT_OP_LSHIFT32,
> +    ASTAT_OP_LSHIFT_RT16,
> +    ASTAT_OP_LSHIFT_RT32,
> +    ASTAT_OP_MIN_MAX,
> +    ASTAT_OP_MIN_MAX_VECTOR,
> +    ASTAT_OP_NEGATE,
> +    ASTAT_OP_SUB16,
> +    ASTAT_OP_SUB32,
> +    ASTAT_OP_VECTOR_ADD_ADD,    /* +|+ */
> +    ASTAT_OP_VECTOR_ADD_SUB,    /* +|- */
> +    ASTAT_OP_VECTOR_SUB_SUB,    /* -|- */
> +    ASTAT_OP_VECTOR_SUB_ADD,    /* -|+ */
> +};
> +
> +typedef void (*hwloop_callback)(struct DisasContext *dc, int loop);
> +
> +typedef struct DisasContext {
> +    CPUArchState *env;
> +    struct TranslationBlock *tb;
> +    /* The current PC we're decoding (could be middle of parallel insn) */
> +    target_ulong pc;
> +    /* Length of current insn (2/4/8) */
> +    target_ulong insn_len;
> +
> +    /* For delayed ASTAT handling */
> +    enum astat_ops astat_op;
> +
> +    /* For hardware loop processing */
> +    hwloop_callback hwloop_callback;
> +    void *hwloop_data;
> +
> +    /* Was a DISALGNEXCPT used in this parallel insn ? */
> +    int disalgnexcpt;
> +
> +    int is_jmp;
> +    int mem_idx;
> +} DisasContext;
> +
> +void do_interrupt(CPUArchState *env);

do_interrupt() has recently been converted to a CPUClass hook.

> +CPUArchState *cpu_init(const char *cpu_model);
> +int cpu_exec(CPUArchState *s);
> +int cpu_bfin_signal_handler(int host_signum, void *pinfo, void *puc);
> +
> +extern const char * const greg_names[];
> +extern const char *get_allreg_name(int grp, int reg);
> +
> +#define MMU_KERNEL_IDX 0
> +#define MMU_USER_IDX   1
> +
> +int cpu_bfin_handle_mmu_fault(CPUArchState *env, target_ulong address, int rw,
> +                              int mmu_idx);
> +#define cpu_handle_mmu_fault cpu_bfin_handle_mmu_fault
> +
> +#if defined(CONFIG_USER_ONLY)
> +static inline void cpu_clone_regs(CPUArchState *env, target_ulong newsp)
> +{
> +    if (newsp)
> +        env->spreg = newsp;
> +}
> +#endif

Note there's a pending patch moving cpu_clone_regs() to
linux-user/*/target_cpu.h.

> +
> +#include "exec/cpu-all.h"
> +#include "cpu-qom.h"
> +
> +static inline bool cpu_has_work(CPUState *cpu)
> +{
> +    return (cpu->interrupt_request & (CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI));
> +}
> +
> +#include "exec/exec-all.h"
> +
> +static inline void cpu_pc_from_tb(CPUArchState *env, TranslationBlock *tb)
> +{
> +    env->pc = tb->pc;
> +}
> +

> +static inline target_ulong cpu_get_pc(CPUArchState *env)
> +{
> +    return env->pc;
> +}

Unused?

> +
> +static inline void cpu_get_tb_cpu_state(CPUArchState *env, target_ulong *pc,
> +                                        target_ulong *cs_base, int *flags)
> +{
> +    *pc = cpu_get_pc(env);
> +    *cs_base = 0;
> +    *flags = env->astat[ASTAT_RND_MOD];
> +}
> +
> +#endif
[...]
> diff --git a/target-bfin/translate.c b/target-bfin/translate.c
> new file mode 100644
> index 0000000..a619f66
> --- /dev/null
> +++ b/target-bfin/translate.c
[...]
> +
> +CPUArchState *cpu_init(const char *cpu_model)
> +{
> +    BfinCPU *cpu;
> +    CPUArchState *env;
> +    static int tcg_initialized = 0;
> +
> +    cpu = BFIN_CPU(object_new(TYPE_BFIN_CPU));
> +    env = &cpu->env;
> +
> +    cpu_reset(CPU(cpu));
> +    qemu_init_vcpu(env);
> +
> +    if (tcg_initialized)
> +        return env;
> +
> +    tcg_initialized = 1;

Please place this into the CPU realizefn. Note that qemu_init_vcpu() is
being moved to generic code with my next pull.

In light of possible bfin-softmmu support, please turn this function
into BlackfinCPU *cpu_bfin_init(const char *cpu_model) and place
cpu_init() as a static inline compatibility wrapper into cpu.h.

> +
> +#define GEN_HELPER 2
> +#include "helper.h"
> +
> +    cpu_env = tcg_global_reg_new_ptr(TCG_AREG0, "env");
> +
> +    cpu_pc = tcg_global_mem_new(TCG_AREG0,
> +        offsetof(CPUArchState, pc), "PC");
> +    cpu_cc = tcg_global_mem_new(TCG_AREG0,
> +        offsetof(CPUArchState, astat[ASTAT_CC]), "CC");
> +
> +    /*cpu_astat_op = tcg_global_mem_new(TCG_AREG0,
> +        offsetof(CPUArchState, astat_op), "astat_op");*/
> +    cpu_astat_arg[0] = tcg_global_mem_new(TCG_AREG0,
> +        offsetof(CPUArchState, astat_arg[0]), "astat_arg[0]");
> +    cpu_astat_arg[1] = tcg_global_mem_new(TCG_AREG0,
> +        offsetof(CPUArchState, astat_arg[1]), "astat_arg[1]");
> +    cpu_astat_arg[2] = tcg_global_mem_new(TCG_AREG0,
> +        offsetof(CPUArchState, astat_arg[2]), "astat_arg[2]");
> +
> +    cpu_areg[0] = tcg_global_mem_new_i64(TCG_AREG0,
> +        offsetof(CPUArchState, areg[0]), "A0");
> +    cpu_areg[1] = tcg_global_mem_new_i64(TCG_AREG0,
> +        offsetof(CPUArchState, areg[1]), "A1");
> +
> +    bfin_tcg_new_set(dreg, 0);
> +    bfin_tcg_new_set(preg, 8);
> +    bfin_tcg_new_set(ireg, 16);
> +    bfin_tcg_new_set(mreg, 20);
> +    bfin_tcg_new_set(breg, 24);
> +    bfin_tcg_new_set(lreg, 28);
> +    bfin_tcg_new(rets, 39);
> +    bfin_tcg_new(lcreg[0], 48);
> +    bfin_tcg_new(ltreg[0], 49);
> +    bfin_tcg_new(lbreg[0], 50);
> +    bfin_tcg_new(lcreg[1], 51);
> +    bfin_tcg_new(ltreg[1], 52);
> +    bfin_tcg_new(lbreg[1], 53);
> +    bfin_tcg_new_set(cycles, 54);
> +    bfin_tcg_new(uspreg, 56);
> +    bfin_tcg_new(seqstat, 57);
> +    bfin_tcg_new(syscfg, 58);
> +    bfin_tcg_new(reti, 59);
> +    bfin_tcg_new(retx, 60);
> +    bfin_tcg_new(retn, 61);
> +    bfin_tcg_new(rete, 62);
> +    bfin_tcg_new(emudat, 63);
> +
> +    return env;
> +}
> +
> +#define _astat_printf(bit) cpu_fprintf(f, "%s" #bit " ", (env->astat[ASTAT_##bit] ? "" : "~"))
> +void cpu_dump_state(CPUArchState *env, FILE *f,
> +                    int (*cpu_fprintf)(FILE *f, const char *fmt, ...),
> +                    int flags)
> +{

This will be converted to a CPUClass hook in my next pull.

[...]
> +static void
> +gen_intermediate_code_internal(CPUArchState *env, TranslationBlock *tb,
> +                               int search_pc)
> +{

Please use BlackfinCPU and bool arguments here, I am about to convert
all other targets (github.com/afaerber/qemu-cpu.git qom-cpu-11).

[...]
> +
> +void gen_intermediate_code(CPUArchState *env, struct TranslationBlock *tb)
> +{
> +    gen_intermediate_code_internal(env, tb, 0);
> +}
> +
> +void gen_intermediate_code_pc(CPUArchState *env, struct TranslationBlock *tb)
> +{
> +    gen_intermediate_code_internal(env, tb, 1);
> +}
> +
> +void restore_state_to_opc(CPUArchState *env, TranslationBlock *tb, int pc_pos)
> +{
> +    env->pc = tcg_ctx.gen_opc_pc[pc_pos];
> +}
> +
> +#include "bfin-sim.c"

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg