RE: [Qemu-devel] [PATCH 1/3] unicore32: add target-unicore32 directory for unicore32-linux-user support

[Guan Xuetao]

> -----Original Message-----
> From: Blue Swirl [mailto:address@hidden
> Sent: Saturday, March 26, 2011 8:51 PM
> To: Guan Xuetao
> Cc: address@hidden
> Subject: Re: [Qemu-devel] [PATCH 1/3] unicore32: add target-unicore32 
> directory for unicore32-linux-user support
> 
> On Thu, Mar 24, 2011 at 11:25 AM, Guan Xuetao <address@hidden> wrote:
> > unicore32: add target-unicore32 directory for unicore32-linux-user support
> >
> > Signed-off-by: Guan Xuetao <address@hidden>
> > ---
> >
> > diff --git a/target-unicore32/cpu.h b/target-unicore32/cpu.h
> > new file mode 100644
> > index 0000000..0688891
> > --- /dev/null
> > +++ b/target-unicore32/cpu.h
> > @@ -0,0 +1,184 @@
> > +/*
> > + * UniCore32 virtual CPU header
> > + *
> > + * Copyright (C) 2010-2011 GUAN Xue-tao
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +#ifndef __UC32_CPU_H__
> > +#define __UC32_CPU_H__
> 
> Please use CPU_UC32_H.
Applied.

> > +typedef struct CPUState_UniCore32 {
> > +    /* Regs for current mode.  */
> > +    uint32_t regs[32];
> > +    /* Frequently accessed ASR bits are stored separately for efficiently.
> > +       This contains all the other bits.  Use asr_{read,write} to access
> > +       the whole ASR.  */
> > +    uint32_t uncached_asr;
> > +    uint32_t bsr;
> > +
> > +    /* Banked registers.  */
> > +    uint32_t banked_bsr[6];
> > +    uint32_t banked_r29[6];
> > +    uint32_t banked_r30[6];
> > +
> > +    /* asr flag cache for faster execution */
> > +    uint32_t CF; /* 0 or 1 */
> > +    uint32_t VF; /* V is the bit 31. All other bits are undefined */
> > +    uint32_t NF; /* N is bit 31. All other bits are undefined.  */
> > +    uint32_t ZF; /* Z set if zero.  */
> > +
> > +    /* System control coprocessor (cp0) */
> > +    struct {
> > +        uint32_t c0_cpuid;
> > +        uint32_t c0_cachetype;
> > +        uint32_t c1_sys; /* System control register.  */
> > +        uint32_t c2_base; /* MMU translation table base.  */
> > +        uint32_t c3_faultstatus; /* Fault status registers.  */
> > +        uint32_t c4_faultaddr; /* Fault address registers.  */
> > +        uint32_t c5_cacheop; /* Cache operation registers.  */
> > +        uint32_t c6_tlbop; /* TLB operation registers. */
> > +    } cp0;
> > +
> > +    /* Internal CPU feature flags.  */
> > +    uint32_t features;
> 
> All CPUState fields up to breakpoints are cleared on reset by common
> code. I'd suppose CPU feature shouldn't change on reset, so please
> move this after CPU_COMMON.
Applied.

> > +CPUState *uc32_cpu_init(const char *cpu_model);
> > +int uc32_cpu_exec(CPUState *s);
> > +int uc32_cpu_signal_handler(int host_signum, void *pinfo, void *puc);
> > +int uc32_cpu_handle_mmu_fault(CPUState *env, target_ulong address, int rw,
> > +                              int mmu_idx, int is_softmuu);
> > +void uc32_cpu_list(FILE *f, int (*cpu_fprintf)(FILE *f, const char *fmt, 
> > ...));
> 
> fprintf_function cpu_fprintf
Applied.

> > +
> > +void cpu_reset(CPUState *env)
> > +{
> > +    uint32_t id;
> > +
> > +    if (qemu_loglevel_mask(CPU_LOG_RESET)) {
> > +        qemu_log("CPU Reset (CPU %d)\n", env->cpu_index);
> > +        log_cpu_state(env, 0);
> > +    }
> > +
> > +    id = env->cp0.c0_cpuid;
> > +    memset(env, 0, offsetof(CPUState, breakpoints));
> 
> This is done in common code.
Applied.

> 
> > +    if (id) {
> > +        cpu_reset_model_id(env, id);
> 
> I don't think this should be done when resetting, but only at CPU init.
Applied.

> > +CPUState *uc32_cpu_init(const char *cpu_model)
> > +{
> > +    CPUState *env;
> > +    uint32_t id;
> > +    static int inited = 1;
> > +
> > +    id = uc32_cpu_find_by_name(cpu_model);
> > +    if (id == 0) {
> > +        return NULL;
> > +    }
> > +    env = qemu_mallocz(sizeof(CPUState));
> > +    cpu_exec_init(env);
> > +    if (inited) {
> > +        inited = 0;
> > +        uc32_translate_init();
> > +    }
> > +
> > +    env->cpu_model_str = cpu_model;
> > +    env->cp0.c0_cpuid = id;
> > +    cpu_reset(env);
> 
> This is also performed by common code.
However, only I386/SPARC/PPC call cpu_reset() in linux-user/main.c.
Then the cpu_reset() codes had been merged in cpu_init() for unicore32.

> > +
> > +uint32_t asr_read(CPUState *env)
> > +{
> > +    int ZF;
> > +    ZF = (env->ZF == 0);
> > +    return env->uncached_asr | (env->NF & 0x80000000) | (ZF << 30) |
> > +        (env->CF << 29) | ((env->VF & 0x80000000) >> 3);
> > +}
> > +
> > +void asr_write(CPUState *env, uint32_t val, uint32_t mask)
> > +{
> > +    if (mask & ASR_NZCV) {
> > +        env->ZF = (~val) & ASR_Z;
> > +        env->NF = val;
> > +        env->CF = (val >> 29) & 1;
> > +        env->VF = (val << 3) & 0x80000000;
> > +    }
> > +
> > +    if ((env->uncached_asr ^ val) & mask & ASR_M) {
> > +        switch_mode(env, val & ASR_M);
> > +    }
> > +    mask &= ~ASR_NZCV;
> > +    env->uncached_asr = (env->uncached_asr & ~mask) | (val & mask);
> > +}
> 
> Helpers like this and especially HELPER stuff below belong to op_helper.c.
> 
> If a function is used by helpers and other parts, two sets of
> functions should be created. The functions in op_helper.c have direct
> access to global env variable, so they can just use that, there is no
> need to pass env to the helper. The versions for non-helper case must
> get the CPUState from the caller. See for example get_psr() and
> cpu_get_psr() in target-sparc/op_helper.c.
Applied.

> > diff --git a/target-unicore32/translate.c b/target-unicore32/translate.c
> > new file mode 100644
> > index 0000000..d03e9cb
> > --- /dev/null
> > +++ b/target-unicore32/translate.c
> > @@ -0,0 +1,2110 @@
> > +/*
> > + *  UniCore32 translation
> > + *
> > + * Copyright (C) 2010-2011 GUAN Xue-tao
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> 
> Links to CPU docs would be nice.
The docs are only in Chinese.
Useful link: http://www.pkunity.com/english%20version/engindex.htm

> > +
> > +#define ILLEGAL         cpu_abort(env,                                  \
> > +                        "Illegal UniCore32 instruction %x at line %d!", \
> > +                        insn, __LINE__)
> 
> This makes QEMU exit by guest action (by unprivileged user code in the
> guest even), which in general is not OK. What does a real UC32 chip do
> in this case, is an exception generated? Also, this would happen
> during translation, whereas it should only happen if execution ever
> reaches the code in question. For example, the illegal instruction
> could be data which is skipped, or changed by self-modifying code
> before it is reached.
All ILLEGAL instructions are privileged instructions.
Because I had only implemented linux-user mode, these instructions should not be
generated. And all these codes will be removed after softmmu mode implemented.

> > +
> > +#define gen_set_CF(var) tcg_gen_st_i32(var, cpu_env, offsetof(CPUState, 
> > CF))
> 
> For improved performance, you could consider implementing the lazy
> condition code system as used by some targets (see qemu-tech.texi).
The lazy condition method is not useful in unicore32 isa.
The condition flags are explicitly modified or ignored, for example, 
add instruction will neglect flags, and add.a instruction will modify flags.

> > +
> > +static inline void store_reg_bx(CPUState *env, DisasContext *s, int reg,
> 
> The functions in translate.c in general shouldn't access CPUState, but
> only DisasContext.
Applied.

> > +
> > +/* Disassemble an F64 instruction */
> > +static void disas_ucf64_insn(CPUState *env, DisasContext *s, uint32_t insn)
> > +{
> > +    if ((env->features & UC32_HWCAP_UCF64) == 0) {
> 
> If the feature is static (does not change after the CPU feature has
> been specified by -cpu command line), it should be accessed from
> DisasContext.
The feature is static. And this sentence could be removed.

> > +#define UCF64_DUMP_STATE
> > +void cpu_dump_state(CPUState *env, FILE *f,
> > +        int (*cpu_fprintf)(FILE *f, const char *fmt, ...), int flags)
> 
> fprintf_function cpu_fprintf,
Applied.

> > +
> > +#ifdef UCF64_DUMP_STATE
> > +    for (i = 0; i < 16; i++) {
> > +        d.d = env->ucf64.regs[i];
> > +        s0.i = d.l.lower;
> > +        s1.i = d.l.upper;
> > +        d0.f64 = d.d;
> > +        cpu_fprintf(f, "s%02d=%08x(%8g) s%02d=%08x(%8g) 
> > d%02d=%08x%08x(%8g)\n",
> > +                    i * 2, (int)s0.i, s0.s,
> > +                    i * 2 + 1, (int)s1.i, s1.s,
> > +                    i, (int)(uint32_t)d.l.upper, (int)(uint32_t)d.l.lower,
> 
> Instead of casts and the two %08x you could use PRIx64 and uint64_t.
Applied.

Thanks Blue Swirl.

Guan Xuetao