Re: [Qemu-devel] [PATCH v17 5/9] target-avr: adding AVR interrupt handling

[Peter Maydell]

On 18 August 2016 at 13:07, Michael Rolnik <address@hidden> wrote:
> Signed-off-by: Michael Rolnik <address@hidden>
> ---
>  target-avr/helper.c | 55 
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
>
> diff --git a/target-avr/helper.c b/target-avr/helper.c
> index b48222d..8511fb7 100644
> --- a/target-avr/helper.c
> +++ b/target-avr/helper.c
> @@ -32,11 +32,66 @@
>  bool avr_cpu_exec_interrupt(CPUState *cs, int interrupt_request)
>  {
>      bool ret = false;
> +    CPUClass *cc = CPU_GET_CLASS(cs);
> +    AVRCPU *cpu = AVR_CPU(cs);
> +    CPUAVRState *env = &cpu->env;
> +
> +    if (interrupt_request & CPU_INTERRUPT_RESET) {
> +        if (cpu_interrupts_enabled(env)) {
> +            cs->exception_index = EXCP_RESET;
> +            cc->do_interrupt(cs);
> +
> +            cs->interrupt_request &= ~CPU_INTERRUPT_RESET;
> +
> +            ret = true;
> +        }
> +    }

Are you sure that you need to handle CPU_INTERRUPT_RESET here?
It looks to me like the code in cpu-exec.c should deal with it
for you.

> +    if (interrupt_request & CPU_INTERRUPT_HARD) {
> +        if (cpu_interrupts_enabled(env) && env->intsrc != 0) {
> +            int index = ctz32(env->intsrc);
> +            cs->exception_index = EXCP_INT(index);
> +            cc->do_interrupt(cs);
> +
> +            env->intsrc &= env->intsrc - 1; /* clear the interrupt */

I think clearing the env->intsrc bit should go in avr_cpu_do_interrupt().

> +            cs->interrupt_request &= ~CPU_INTERRUPT_HARD;

I'm not sure what the interrupt model for this CPU is,
but other CPU models don't do this, so maybe you don't
want to either. (The usual model is that CPU_INTERRUPT_HARD
corresponds to an interrupt input signal to the CPU;
for instance on ARM it's the IRQ line. When the signal
goes high we call cpu_interrupt(cs, CPU_INTERRUPT_HARD)
which sets the bit, and when it goes low we call
cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD) which clears
the bit.)

> +
> +            ret = true;
> +        }
> +    }
>      return ret;
>  }
>
>  void avr_cpu_do_interrupt(CPUState *cs)
>  {
> +    AVRCPU *cpu = AVR_CPU(cs);
> +    CPUAVRState *env = &cpu->env;
> +
> +    uint32_t ret = env->pc_w;
> +    int vector = 0;
> +    int size = avr_feature(env, AVR_FEATURE_JMP_CALL) ? 2 : 1;
> +    int base = 0; /* TODO: where to get it */
> +
> +    if (cs->exception_index == EXCP_RESET) {
> +        vector = 0;
> +    } else if (env->intsrc != 0) {
> +        vector = ctz32(env->intsrc) + 1;
> +    }

Should env->intsrc==0 really be treated like reset?
(If it's a can't-happen case then asserting would probably be good.)

> +
> +    if (avr_feature(env, AVR_FEATURE_3_BYTE_PC)) {
> +        cpu_stb_data(env, env->sp--, (ret & 0x0000ff));
> +        cpu_stb_data(env, env->sp--, (ret & 0x00ff00) >>  8);
> +        cpu_stb_data(env, env->sp--, (ret & 0xff0000) >> 16);
> +    } else if (avr_feature(env, AVR_FEATURE_2_BYTE_PC)) {
> +        cpu_stb_data(env, env->sp--, (ret & 0x0000ff));
> +        cpu_stb_data(env, env->sp--, (ret & 0x00ff00) >>  8);
> +    } else {
> +        cpu_stb_data(env, env->sp--, (ret & 0x0000ff));
> +    }
> +
> +    env->pc_w = base + vector * size;
> +    env->sregI = 0; /* clear Global Interrupt Flag */
> +
> +    cs->exception_index = -1;
>  }
>
>  int avr_cpu_memory_rw_debug(CPUState *cs, vaddr addr, uint8_t *buf,
> --
> 2.4.9 (Apple Git-60)

thanks
-- PMM