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

[Chris Wulff]

Thanks for the feedback. Nothing jumps out at me that I disagree with. I'll get it updated and another patch out once I get the chance to incorporate the info.

> > +#include "hw/sysbus.h"
> > +#include "dyngen-exec.h"
> > +#include "cpu.h"
> > +
> > +struct altera_iic {
>
> CamelCase

Yep. I missed that one in reviewing the coding standard and the checkpatch script apparently doesn't flag it. I'll clean up that one an the others like it.

> > +
> > +#if !defined(CONFIG_USER_ONLY)
> > +    struct nios2_mmu mmu;
> > +#endif
> > +
> > +    CPU_COMMON
> > +} CPUNios2State;
> > +
> > +#include "cpu-qom.h"
> > +
> > +extern Nios2CPU *cpu_nios2_init(const char *cpu_model);
>
> 'extern' is useless for functions.

True. I will remove for consistency with other code.

> > +extern int cpu_nios2_exec(CPUNios2State *s);
> > +extern void cpu_nios2_close(CPUNios2State *s);
> > +extern void do_interrupt(CPUNios2State *env);
> > +extern int cpu_nios2_signal_handler(int host_signum, void *pinfo, void *puc);
> > +extern void dump_mmu(FILE *f, fprintf_function cpu_fprintf, CPUNios2State *env);
> > +
> > +#define TARGET_PHYS_ADDR_SPACE_BITS 32
> > +#define TARGET_VIRT_ADDR_SPACE_BITS 32
> > +
> > +extern Nios2CPU *g_cpu;
>
> Just say no to globals.

> > +#ifndef NIOS2_EXEC_H
> > +#define NIOS2_EXEC_H
> > +
> > +#include "dyngen-exec.h"
>
> Dyngen model is being obsoleted in coming days. Please make all
> helpers which need access to CPU state take the state as parameter.
>
> > +
> > +register struct CPUNios2State *env asm(AREG0);
>
> This is already defined in dyngen-exec.h, but see above. Maybe you are
> not using Git HEAD? No target uses exec.h anymore.
>
> > +
> > +#include "cpu.h"
> > +#include "exec-all.h"
> > +
> > +#if !defined(CONFIG_USER_ONLY)
> > +#include "softmmu_exec.h"
> > +#endif
> > +
>
> The functions below should go to cpu.h.

I've been working on this on and off for months now so it is very well possible that I referenced older code that still did use some of this stuff. I'll review the latest to see how this is being done now. I agree that globals aren't good. I'll see if I can find how to get the information passed along to the appropriate places that need it.

> > +void do_interrupt(CPUNios2State *env)
> > +{
> > +    switch (env->exception_index) {
> > +    case EXCP_IRQ:
> > +        assert(env->regs[CR_STATUS] & CR_STATUS_PIE);
>
> I hope this is not a guest triggerable assert.

It shouldn't be. But maybe this should just be a debug message and not trigger an exception if this is the case. We should never be trying to send an IRQ exception to the guest if interrupts are globally disabled.

> > +    if (has_mmu && (addr < 0xC0000000)) {
> > +        hit = mmu_translate(env, &lu, addr, 0, 0);
> > +        if (hit) {
> > +            vaddr = addr & TARGET_PAGE_MASK;
> > +            paddr = lu.paddr + vaddr - lu.vaddr;
> > +        } else {
> > +            cpu_abort(env, "cpu_get_phys_page debug MISS: %08X\n", addr);
>
> cpu_get_phys_page_debug is used for memory access from monitor.
> Aborting because user specified a bad address is rather draconian.

I was seeing this come from the disassembly code when I had a mismatch between the TLB in the processor emulation and the one that QEMU knows about (due to improper removal of QEMU tlb entries when they were overwritten in the NiosII TLB.) I can make this into a debug message though instead. I'm not seeing this come out at all now that I've fixed my TLB implementation.

> > +    default:
> > +        cpu_abort(dc->env, "Incorrect load size %d\n", size);
>
> Maybe assert instead.
>
> > +        break;
> > +    }
> > +}


> > + */
> > +
> > +/* I-Type instruction */
> > +struct i_type {
>
> IType
>
> > +    uint32_t op:6;
> > +    uint32_t imm16:16;
> > +    uint32_t b:5;
> > +    uint32_t a:5;
> > +} __attribute__((packed));
>
> QEMU_PACKED

Yep. I will change this and any others.

> > +#define __stringify_1(x...)     #x
> > +#define __stringify(x...)       __stringify_1(x)
>
> We already have glue and stringify, no need to add another.

Ok, didn't see those. This was some of the bits I originally got from nios2sim-ng, but I can easily clean that up.


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

> > +
> > +void tlb_fill(CPUNios2State *env1, target_ulong addr, int is_write, int mmu_idx,
> > +              uintptr_t retaddr)
> > +{
> > +    TranslationBlock *tb;
> > +    CPUNios2State *saved_env;
> > +    int ret;
> > +
> > +    saved_env = env;
> > +    env = env1;
>
> References to global env should be removed, the CPU state should be
> passed around instead. Please check for example Sparc or recent
> commits to s390x.

Ok, I'll take a look at that example. Definitely good to get rid of globals.


 -- Chris Wulff