Re: [PATCH v2 2/4] target/nios2: Exteral Interrupt Controller (EIC)

[Richard Henderson]

On 2/24/22 03:48, Amir Gonnen wrote:
> +static void nios2_cpu_set_eic_irq(Nios2CPU *cpu, int level)
> +{
> +    CPUNios2State *env = &cpu->env;
> +    CPUState *cs = CPU(cpu);
> +
> +    env->irq_pending = level;
> +
> +    if (env->irq_pending && nios2_take_eic_irq(cpu)) {
> +        env->irq_pending = 0;
> +        cpu_interrupt(cs, CPU_INTERRUPT_HARD);
> +    } else if (!env->irq_pending) {
> +        cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
> +    }
> +}

This looks wrong.  Of course, so does nios2_cpu_set_irq, from which you've 
cribbed this.

(1) The unset of irq_pending looks wrong, though come to think of it, it's existence also 
looks wrong.  I think that it's completely redundant with cpu->interrupt_request, and that 
you should only use cpu_interrupt/cpu_reset_interrupt from this level.

Which also means that nios2_check_interrupts and callers all the way back up are also 
incorrect.  All that should be required from wrctl status is the return to the main loop 
that you get from DISAS_UPDATE and tcg_gen_exit_tb.

Which also means that ipending is implemented incorrectly.  The current manipulation of 
CR_IPENDING in nios2_cpu_set_irc should instead be manipulating an internal "external hw 
request", per Figure 3-2 in NII51003.

For our purposes, I think simply re-using env->regs[CR_IPENDING] as the external hw 
request word is the right thing to do.   But we need to update RDCTL to compute the 
correct value from CR_IPENDING & CR_IENABLE, and update WRCTL to ignore writes.


(2) Checking nios2_take_eic_irq here, or CR_STATUS_PIE in nios2_cpu_set_irq is incorrect. 
 If you check this here, then you'll miss the interrupt when the interrupt enable bit is 
reset.  These checks belong in nios2_cpu_exec_interrupt.

> +    if (cpu->rnmi) {
> +        return !(env->regs[CR_STATUS] & CR_STATUS_NMI);
> +    }

I think this should be a separate

    #define CPU_INTERRUPT_NMI  CPU_INTERRUPT_TGT_EXT_0


r~