Re: [Qemu-devel] [PATCH] MIPS interrupts and -icount

[Aurelien Jarno]

On Sun, Jul 25, 2010 at 02:07:54AM +0200, Edgar E. Iglesias wrote:
> On Sun, Jul 25, 2010 at 12:55:45AM +0200, Aurelien Jarno wrote:
> > On Thu, Jul 22, 2010 at 01:32:18PM +0200, Edgar E. Iglesias wrote:
> > > Hi,
> > > 
> > > I'm seeing an error when emulating MIPS guests with -icount.
> > > In cpu_interrupt:
> > >   cpu_abort(env, "Raised interrupt while not in I/O function");
> > 
> > I am not able to reproduce the issue. Do you have more details how to
> > reproduce the problem?
> 
> You need a machine with devices that raise the hw interrupts. I didn't
> see the error on the images on the wiki though. But I've got a machine
> here that trigs it easily. Will check if I can publish it and an image.
> 

That would be nice if you can share it.

> > > It seems to me like the MIPS interrupt glue logic between interrupt
> > > controllers and the MIPS core is not modeled correctly.
> > 
> > It seems indeed that sometimes interrupt are triggered while not in 
> > I/O functions, your patch addresses part of the problem.
> > 
> > > When hw interrupt pending bits in CP0_Cause are set, the CPU should
> > > see the hw interrupt line as active. The CPU may or may not take the
> > > interrupt based on internal state (global irq mask etc) but the glue
> > > logic shouldn't care about that. Am I missing something here?
> > 
> > I don't think it is correct. On the real hardware, the interrupt line is
> > actually active only when all conditions are fulfilled.
> > 
> > The thing to remember is that the interrupts are level triggered. So if
> > an interrupt is masked, it should be rejected by the CPU, but could be
> > triggered again as soon as the interrupt mask is changed.
> 
> Agreed, that behaviour is what I'm trying to acheive. The problem now
> is that the the level triggered line, prior to CPU masking is beeing masked
> by external logic. IMO it shouldnt. See hw/mips_int.c and cpu-exec.c prior
> to the patch.

Actually all depends if you consider the MIPS interrupt controller part
of the CPU or not. It could be entirely modeled in the CPU, that is in 
cpu-exec.c or entirely modeled as a separate controller, that is in 
mips_int.c.

IMHO it should be in mips_int.c. It is an interrupt controller like
another that combines a few interrupt lines into a single one that feeds
the CPU. It is like for example the i8259, with the exception that the 
configuration is not done by load/store into MMIO area, but directly 
using CPU special registers. We should probably mark these instructions 
as I/O.

> > Even in the i386 case, if none of the APIC accept interrupts, the
> > glue logic doesn't transmit the interrupt to the CPU.
> > 
> > > The following patch fixes the problem. Tested by booting the mips and 
> > > mipsel
> > > images from http://wiki.qemu.org/Download. Also tested more with an
> > > experimental out-of-tree qemu machine I've got here running a linux-2.6.33
> > > kernel.
> > > 
> > > I'd appreciate comments.
> > > 
> > 
> > I don't think the patch is correct, all the places that are modified are
> > places where interruptions can actually be triggered. What is probably 
> > wrong in the current code is that some instructions that can trigger 
> > exceptions are not between gen_io_start() / gen_io_end() blocks. There
> > should be an I/O function in the QEMU sense, like when an interrupt is
> > enabled on the i8259.
> > > 
> > > commit c9af70e4587e1464b8019a059845492225733584
> > > Author: Edgar E. Iglesias <address@hidden>
> > > Date:   Thu Jul 22 13:14:52 2010 +0200
> > > 
> > >     mips: Correct MIPS interrupt glue logic for icount
> > >     
> > >     When hw interrupt pending bits in CP0_Cause are set, the CPU should
> > >     see the hw interrupt line as active. The CPU may or may not take the
> > >     interrupt based on internal state (global irq mask etc) but the glue
> > >     logic shouldn't care.
> > >     
> > >     This fixes MIPS external hw interrupts in combination with -icount.
> > >     
> > >     Signed-off-by: Edgar E. Iglesias <address@hidden>
> > > 
> > > diff --git a/hw/mips_int.c b/hw/mips_int.c
> > > index c30954c..80488ba 100644
> > > --- a/hw/mips_int.c
> > > +++ b/hw/mips_int.c
> > > @@ -24,22 +24,6 @@
> > >  #include "mips_cpudevs.h"
> > >  #include "cpu.h"
> > >  
> > > -/* Raise IRQ to CPU if necessary. It must be called every time the active
> > > -   IRQ may change */
> > > -void cpu_mips_update_irq(CPUState *env)
> > > -{
> > > -    if ((env->CP0_Status & (1 << CP0St_IE)) &&
> > > -        !(env->CP0_Status & (1 << CP0St_EXL)) &&
> > > -        !(env->CP0_Status & (1 << CP0St_ERL)) &&
> > > -        !(env->hflags & MIPS_HFLAG_DM)) {
> > > -        if ((env->CP0_Status & env->CP0_Cause & CP0Ca_IP_mask) &&
> > > -            !(env->interrupt_request & CPU_INTERRUPT_HARD)) {
> > > -            cpu_interrupt(env, CPU_INTERRUPT_HARD);
> > > - }
> > > -    } else
> > > -        cpu_reset_interrupt(env, CPU_INTERRUPT_HARD);
> > > -}
> > > -
> > 
> > Removing these checks is fine as long as they are moved somewhere else.
> > Otherwise interrupts are going to be triggered while they should not.
> > Currently there is nothing that prevent that, and with your patch, the
> > CPU will enter interrupt mode if an interrupt is triggered while already
> > in interrupt mode.
> 
> 
> The checks are already made in cpu-exec.c. On real hw, the PIC and external
> logic dont have access to the internal state of the CPU so they can't
> do any of those checks.

Ok, I haven't seen that. Indeed the checks should be on one side or
another, but not on both.

> > 
> > >  static void cpu_mips_irq_request(void *opaque, int irq, int level)
> > >  {
> > >      CPUState *env = (CPUState *)opaque;
> > > @@ -52,7 +36,12 @@ static void cpu_mips_irq_request(void *opaque, int 
> > > irq, int level)
> > >      } else {
> > >          env->CP0_Cause &= ~(1 << (irq + CP0Ca_IP));
> > >      }
> > > -    cpu_mips_update_irq(env);
> > > +
> > > +    if (env->CP0_Cause & CP0Ca_IP_mask) {
> > > +        cpu_interrupt(env, CPU_INTERRUPT_HARD);
> > > +    } else {
> > > +        cpu_reset_interrupt(env, CPU_INTERRUPT_HARD);
> > > +    }
> > >  }
> > 
> > Probably here?
> > 
> > >  void cpu_mips_irq_init_cpu(CPUState *env)
> > > diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> > > index 81051aa..1578850 100644
> > > --- a/target-mips/cpu.h
> > > +++ b/target-mips/cpu.h
> > > @@ -597,9 +597,6 @@ void cpu_mips_store_compare (CPUState *env, uint32_t 
> > > value);
> > >  void cpu_mips_start_count(CPUState *env);
> > >  void cpu_mips_stop_count(CPUState *env);
> > >  
> > > -/* mips_int.c */
> > > -void cpu_mips_update_irq (CPUState *env);
> > > -
> > >  /* helper.c */
> > >  int cpu_mips_handle_mmu_fault (CPUState *env, target_ulong address, int 
> > > rw,
> > >                                 int mmu_idx, int is_softmmu);
> > > diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> > > index 8ae510a..c963224 100644
> > > --- a/target-mips/op_helper.c
> > > +++ b/target-mips/op_helper.c
> > > @@ -1313,7 +1313,6 @@ void helper_mtc0_status (target_ulong arg1)
> > >          default: cpu_abort(env, "Invalid MMU mode!\n"); break;
> > >          }
> > >      }
> > > -    cpu_mips_update_irq(env);
> > >  }
> > 
> > Removing that means that changing the interrupt enable register can't
> > trigger exception anymore. This doesn't match real hardware anymore.
> 
> 
> The external hw line is already active in this case, the interrupt

It is not, as in your patch you are still masking CP0_Cause with
CP0Ca_IP_mask.

If an interruption is not enabled in CP0Ca_IP_mask, it doesn't trigger
the hw line. So when CP0Ca_IP_mask is changed, the interrupt is not
triggered.
 
> 
> > 
> > >  void helper_mttc0_status(target_ulong arg1)
> > > @@ -1359,12 +1358,6 @@ void helper_mtc0_cause (target_ulong arg1)
> > >          else
> > >              cpu_mips_start_count(env);
> > >      }
> > > -
> > > -    /* Handle the software interrupt as an hardware one, as they
> > > -       are very similar */
> > > -    if (arg1 & CP0Ca_IP_mask) {
> > > -        cpu_mips_update_irq(env);
> > > -    }
> > >  }
> > 
> > Same here, it's not possible to trigger software interrupt anymore.
> 
> Hmm, yes this part might cause problems. I'll check it out. Thanks.
> 
> 
> > 
> > >  void helper_mtc0_ebase (target_ulong arg1)
> > > @@ -1793,8 +1786,6 @@ target_ulong helper_di (void)
> > >      target_ulong t0 = env->CP0_Status;
> > >  
> > >      env->CP0_Status = t0 & ~(1 << CP0St_IE);
> > > -    cpu_mips_update_irq(env);
> > > -
> > >      return t0;
> > >  }
> > >  
> > > @@ -1803,8 +1794,6 @@ target_ulong helper_ei (void)
> > >      target_ulong t0 = env->CP0_Status;
> > >  
> > >      env->CP0_Status = t0 | (1 << CP0St_IE);
> > > -    cpu_mips_update_irq(env);
> > > -
> > >      return t0;
> > >  }
> > >  
> > 
> > Leaving form the interrupt should re-enable the interruptions, and thus
> > also trigger one if one is pending.
> 
> I don't think so because the line should already be active, enabling the
> IE flag should cause the CPU to take the interrupt.
> 

I haven't checked in details, but that's most probably true.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net