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

[Serge Vakulenko]

On Wed, Jul 1, 2015 at 4:07 AM, Aurelien Jarno <address@hidden> wrote:
> 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().

That's correct. Thanks for pointing this out. I will remove the unneeded code.

>>      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.

I agree - I'll fix it.

>> +        }
>> +    } 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.

Nice idea. It makes the code really simple.

>>  };
>>
>>  #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.

Agreed.

>>      } 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.

According to MIPS PRA spec (MD00090), VInt bit becomes do-not-care
when Config3.VEIC is enabled. So it makes little sense to rely on it's
value here. And it makes it easier to understand this way. EIC and
VInt modes are quite different: better not to mix them up.

Thanks,
--Serge

>>              offset = 0x200 + vector * spacing;
>>          }
>> --
>> 1.9.1
>>
>>
>>
>
> --
> Aurelien Jarno                          GPG: 4096R/1DDD8C9B
> address@hidden                 http://www.aurel32.net