Re: [Qemu-devel] [PATCH pic32 v2 3/5] Added support for external interrupt controller (EIC) mode.

[Aurelien Jarno]

On 2015-06-30 21:12, Serge Vakulenko wrote:
> Signed-off-by: Serge Vakulenko <address@hidden>
> ---
>  hw/mips/cputimer.c   | 17 +++++++++++++++--
>  hw/mips/mips_int.c   | 12 ++++++++++--
>  target-mips/cpu.h    |  9 ++++++++-
>  target-mips/helper.c | 20 ++++++++++++++------
>  4 files changed, 47 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/mips/cputimer.c b/hw/mips/cputimer.c
> index 94a29df..849d62c 100644
> --- a/hw/mips/cputimer.c
> +++ b/hw/mips/cputimer.c
> @@ -54,7 +54,13 @@ static void cpu_mips_timer_expire(CPUMIPSState *env)
>      if (env->insn_flags & ISA_MIPS32R2) {
>          env->CP0_Cause |= 1 << CP0Ca_TI;
>      }
> -    qemu_irq_raise(env->irq[(env->CP0_IntCtl >> CP0IntCtl_IPTI) & 0x7]);
> +    if (env->CP0_Config3 & (1 << CP0C3_VEIC)) {
> +        /* External interrupt controller mode. */
> +        env->eic_timer_irq(env, 1);
> +    } else {
> +        /* Legacy or vectored interrupt mode. */
> +        qemu_irq_raise(env->irq[(env->CP0_IntCtl >> CP0IntCtl_IPTI) & 0x7]);
> +    }
>  }
>  
>  uint32_t cpu_mips_get_count (CPUMIPSState *env)
> @@ -102,7 +108,14 @@ void cpu_mips_store_compare (CPUMIPSState *env, uint32_t 
> value)
>          cpu_mips_timer_update(env);
>      if (env->insn_flags & ISA_MIPS32R2)
>          env->CP0_Cause &= ~(1 << CP0Ca_TI);
> -    qemu_irq_lower(env->irq[(env->CP0_IntCtl >> CP0IntCtl_IPTI) & 0x7]);
> +
> +    if (env->CP0_Config3 & (1 << CP0C3_VEIC)) {
> +        /* External interrupt controller mode. */
> +        env->eic_timer_irq(env, 0);
> +    } else {
> +        /* Legacy or vectored interrupt mode. */
> +        qemu_irq_lower(env->irq[(env->CP0_IntCtl >> CP0IntCtl_IPTI) & 0x7]);
> +    }
>  }
>  
>  void cpu_mips_start_count(CPUMIPSState *env)
> diff --git a/hw/mips/mips_int.c b/hw/mips/mips_int.c
> index d740046..3452866 100644
> --- a/hw/mips/mips_int.c
> +++ b/hw/mips/mips_int.c
> @@ -32,7 +32,7 @@ static void cpu_mips_irq_request(void *opaque, int irq, int 
> level)
>      CPUMIPSState *env = &cpu->env;
>      CPUState *cs = CPU(cpu);
>  
> -    if (irq < 0 || irq > 7)
> +    if (irq < 0 || irq > 7 || (env->CP0_Config3 & (1 << CP0C3_VEIC)))
>          return;

In which case you go through these code path? The internal interrupt
controller should not be used with an external one, so this should not
be reached. Actually machines with an external interrupt controller
should not even call cpu_mips_irq_init_cpu().

>      if (level) {
> @@ -74,5 +74,13 @@ void cpu_mips_soft_irq(CPUMIPSState *env, int irq, int 
> level)
>          return;
>      }
>  
> -    qemu_set_irq(env->irq[irq], level);
> +    if (env->CP0_Config3 & (1 << CP0C3_VEIC)) {
> +        /* External interrupt controller mode. */
> +        if (level > 0) {
> +            env->eic_soft_irq(env, irq);

It means software interrupts can be asserted, but not deasserted with an
external vector interrupt. I don't think that is correct.

> +        }
> +    } else {
> +        /* Legacy or vectored interrupt mode. */
> +        qemu_set_irq(env->irq[irq], level);
> +    }
>  }
> diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> index c476166..ab830ee 100644
> --- a/target-mips/cpu.h
> +++ b/target-mips/cpu.h
> @@ -596,6 +596,11 @@ struct CPUMIPSState {
>      void *irq[8];
>      QEMUTimer *timer; /* Internal timer */
>      unsigned count_freq; /* rate of Count register */
> +
> +    /* Fields for external interrupt controller. */
> +    void *eic_context;
> +    void (*eic_timer_irq)(CPUMIPSState *env, int raise);
> +    void (*eic_soft_irq)(CPUMIPSState *env, int num);

I don't think this is the way to go. You should just define variable
like eic_timer_irq and eic_soft_irq similar to the irq[8] field above,
and allocate them with qemu_allocate_irqs(). That way you can pass the
context of the external interrupt controller.

>  };
>  
>  #include "cpu-qom.h"
> @@ -664,7 +669,9 @@ static inline int 
> cpu_mips_hw_interrupts_pending(CPUMIPSState *env)
>      if (env->CP0_Config3 & (1 << CP0C3_VEIC)) {
>          /* A MIPS configured with a vectorizing external interrupt controller
>             will feed a vector into the Cause pending lines. The core treats
> -           the status lines as a vector level, not as indiviual masks.  */
> +           the status lines as a vector level, not as individual masks.  */
> +        pending >>= CP0Ca_IP + 2;
> +        status >>= CP0Ca_IP + 2;
>          r = pending > status;

I don't think it's needed. As the pending and status field have been
masked above, we don't need to shift them to do the comparison.

>      } else {
>          /* A MIPS configured with compatibility or VInt (Vectored Interrupts)
> diff --git a/target-mips/helper.c b/target-mips/helper.c
> index 8e3204a..7e25998 100644
> --- a/target-mips/helper.c
> +++ b/target-mips/helper.c
> @@ -574,23 +574,31 @@ void mips_cpu_do_interrupt(CPUState *cs)
>              unsigned int vector;
>              unsigned int pending = (env->CP0_Cause & CP0Ca_IP_mask) >> 8;
>  
> -            pending &= env->CP0_Status >> 8;
>              /* Compute the Vector Spacing.  */
>              spacing = (env->CP0_IntCtl >> CP0IntCtl_VS) & ((1 << 6) - 1);
>              spacing <<= 5;
>  
> -            if (env->CP0_Config3 & (1 << CP0C3_VInt)) {
> +            if (env->CP0_Config3 & (1 << CP0C3_VEIC)) {
> +                /* For VEIC mode, the external interrupt controller feeds the
> +                 * vector through the CP0Cause IP lines. */
> +                vector = pending;
> +
> +                /* Architecturally, this is chip-specific behavior.
> +                 * TODO: some processors, like PIC32MZ,
> +                 * provide vector in a different way.
> +                 * Some processors, like PIC32, have a separate
> +                 * bit INTCON.MVEC to explicitly enable vectored mode,
> +                 * disabled by default. */
> +                spacing = 0;
> +            } else {
>                  /* For VInt mode, the MIPS computes the vector internally.  
> */
> +                pending &= env->CP0_Status >> 8;
>                  for (vector = 7; vector > 0; vector--) {
>                      if (pending & (1 << vector)) {
>                          /* Found it.  */
>                          break;
>                      }
>                  }
> -            } else {
> -                /* For VEIC mode, the external interrupt controller feeds the
> -                   vector through the CP0Cause IP lines.  */
> -                vector = pending;
>              }

The changes looks correct, but I do wonder why you swap the test on
CP0_Config3. It would be less changes to just move the pending masking
with CP0_Status inside the VInt mode.

>              offset = 0x200 + vector * spacing;
>          }
> -- 
> 1.9.1
> 
> 
> 

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
address@hidden                 http://www.aurel32.net