Re: [Qemu-devel] [PATCH 1/9] NiosII: Add support for the Altera NiosII soft-core CPU.

[Richard Henderson <rth@twiddle.net>]

Somehow the original patch set never arrived here.  Replying indirectly...

> On Sun, Sep 09, 2012 at 08:19:59PM -0400, crwulff@gmail.com wrote:
>> diff --git a/target-nios2/exec.h b/target-nios2/exec.h
...
>> +static inline int cpu_has_work(CPUState *env)
>> +{
>> +    return env->interrupt_request & (CPU_INTERRUPT_HARD | CPU_INTERRUPT_NMI);
>> +}
>> +
>> +static inline int cpu_halted(CPUState *env)
>> +{
>> +    if (!env->halted) {
>> +        return 0;
>> +    }
>> +
>> +    if (cpu_has_work(env)) {
>> +        env->halted = 0;
>> +        return 0;
>> +    }
>> +
>> +    return EXCP_HALTED;
>> +}
>> +
>> +static inline void cpu_pc_from_tb(CPUState *env, TranslationBlock *tb)
>> +{
>> +    env->regs[R_PC] = tb->pc;
>> +}

Do my eyes deceive me or do you have duplicates of these from cpu.h?

>> +++ b/target-nios2/instruction.c

Any particular reason you split this file out from translate.c?

>> +static void __break(DisasContext *dc, uint32_t code __attribute__((unused)))

Leading un is reserved to the compiler.  break1?  break_?

>> +/* I-Type instruction */
>> +struct i_type {
>> +    uint32_t op:6;
>> +    uint32_t imm16:16;
>> +    uint32_t b:5;
>> +    uint32_t a:5;
>> +} __attribute__((packed));
>> +
>> +union i_type_u {
>> +    uint32_t      v;
>> +    struct i_type i;
>> +};
>> +
>> +#define I_TYPE(instr, op) \
>> +    union i_type_u instr_u = { .v = op }; \
>> +    struct i_type *instr = &instr_u.i

This is an extremely unportable idea.

Bit field layout differs from big-endian to little-endian, and between
compiler abis.  The only reliable method of picking out a specific set
of bits is to shift and mask by hand.

Which you can still do with your I_TYPE/R_TYPE macros (which I do like),
but instead with different structure definitions and initialization.

>> +typedef struct DisasContext {
>> +    CPUNios2State           *env;
>> +    TCGv                    *cpu_R;
>> +    int                      is_jmp;
>> +    target_ulong             pc;
>> +    struct TranslationBlock *tb;
>> +} DisasContext;

Why are you copying cpu_R here, and using s->cpu_R everywhere?
Why not directly use the global variable cpu_R like everyone else?
I suppose it's related to translate.c vs instruction.c, but I've
already expressed an opinion there...

>> +/* Indirect stringification.  Doing two levels allows the parameter to be a
>> + * macro itself.  For example, compile with -DFOO=bar, __stringify(FOO)
>> + * converts to "bar".
>> + */

Is there any reason you'd want to do that for the instruction tables?

>> +#define __stringify_1(x...)     #x
>> +#define __stringify(x...)       __stringify_1(x)

... because there's that leading underscore again, and honestly you don't need
anything but

#define INSTRUCTION(N)  { #N, N }

>> +#include "dyngen-exec.h"

This is going away.

>> +void helper_memalign(uint32_t addr, uint32_t dr, uint32_t wr, uint32_t mask)
>> +{
>> +    if (addr & mask) {
>> +        qemu_log("unaligned access addr=%x mask=%x, wr=%d dr=r%d\n",
>> +                 addr, mask, wr, dr);
>> +        env->regs[CR_BADADDR] = addr;
>> +        env->regs[CR_EXCEPTION] = EXCP_UNALIGN << 2;
>> +        helper_raise_exception(EXCP_UNALIGN);
>> +    }
>> +}

This should be done with 

#define ALIGNED_ONLY

directly in the softmmu_template.h helpers.  C.f. target-sparc/ldst_helper.c.

>> +uint32_t helper_divs(uint32_t a, uint32_t b)
>> +{
>> +    return (int32_t)a / (int32_t)b;
>> +}
>> +
>> +uint32_t helper_divu(uint32_t a, uint32_t b)
>> +{
>> +    return a / b;
>> +}

(1) Missing divide by zero check.  This will generally put qemu into a loop.

(2) You could (and probably should) use tcg_gen_div{,u}_tl.
    I would only suggest external helper functions if you have to check for
    additional exceptions apart from X / 0, like -INT_MIN / -1.

>> +    /* Dump the CPU state to the log */
>> +    if (qemu_loglevel_mask(CPU_LOG_TB_IN_ASM)) {
>> +        qemu_log("--------------\n");
>> +        log_cpu_state(env, 0);
>> +    }

Don't log cpu state for in_asm.  That's a common bug across translators,
and all it does is cause double logging for "-d cpu,in_asm".

>> +        LOG_DIS("%8.8x:\t", dc->pc);

Use tcg_gen_debug_insn_start, which makes the tcg opcodes dumps pretty too.


r~