MIPS interrupts and -icount

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");

It seems to me like the MIPS interrupt glue logic between interrupt
controllers and the MIPS core is not modeled correctly.

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?

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.

Thanks,
Edgar


commit c9af70e4587e1464b8019a059845492225733584
Author: Edgar E. Iglesias <edgar.iglesias@gmail.com>
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 <edgar@axis.com>

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?

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

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 <edgar.iglesias@gmail.com>
> 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 <edgar@axis.com>
> 
> 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.

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

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

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

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.


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


> 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 <edgar.iglesias@gmail.com>
> > 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 <edgar@axis.com>
> > 
> > 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.


> 
> >  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
should be taken as soon as the TB that unmasks the IE flag ends.



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

Thanks,
Edgar

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 <edgar.iglesias@gmail.com>
> > > 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 <edgar@axis.com>
> > > 
> > > 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.

On Sun, Jul 25, 2010 at 05:08:07AM +0200, Aurelien Jarno wrote:
> 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.


Hi,

I agree that it's not obvious where things should be modeled, I'll try to
explain my view.

As a first step I'm trying to model a MIPS configured with Vectored
Interrupts. We've got external interrupt logic feeding the hw
interrupt lines. These lines are level triggered, held active by
the external logic as long as interrupts are pending. Regardless
of wether the CPU want's to take the interrupt now or later. In fact,
there is no way to access the internal flags from RTL logic located
here (AFAIK). In my mind, this layers pretty much ends in hw/mips_int.c.

Internally in the MIPS core, I'm guessing there is logic that simpliy
applies the internal CPU masks, outputing a single internal IRQ line
that decides wether the CPU should take the IRQ or not. Here, things like
IE flags etc matter. I don't have access to RTL on the MIPS side so I'm
just guessing here.

In my mind, we should model this latter part by asserting INTERRUPT_HARD
from hw/mips_int.c whenever any hw lines are active and letting the
CPU in cpu-exec.c decide when to take the interrupt by applying it's
internal masking.


> > > 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 <edgar.iglesias@gmail.com>
> > > > 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 <edgar@axis.com>
> > > > 
> > > > 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.


I think there is a bit of confusion here. CP0Ca_IP_mask never changes,
it's a fix mask (like a helper define) that helps us extract the hw
lines from the Cause register.

env->CP0_Cause & CP0Ca_IP_mask, simply means "are any hw irqs active?".

So, if hw interrupts are pending, INTERRUPT_HARD should be asserted
and the CPU should be taking the interrupt as soon as the current TB
ends and the internal flags (IE and others) satisfy the conditions.

Thanks,
Edgar

On Sun, Jul 25, 2010 at 07:46:49AM +0200, Edgar E. Iglesias wrote:
> On Sun, Jul 25, 2010 at 05:08:07AM +0200, Aurelien Jarno wrote:
> > 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.
> 
> 
> Hi,
> 
> I agree that it's not obvious where things should be modeled, I'll try to
> explain my view.
> 
> As a first step I'm trying to model a MIPS configured with Vectored
> Interrupts. We've got external interrupt logic feeding the hw
> interrupt lines. These lines are level triggered, held active by
> the external logic as long as interrupts are pending. Regardless
> of wether the CPU want's to take the interrupt now or later. In fact,
> there is no way to access the internal flags from RTL logic located
> here (AFAIK). In my mind, this layers pretty much ends in hw/mips_int.c.
> 
> Internally in the MIPS core, I'm guessing there is logic that simpliy
> applies the internal CPU masks, outputing a single internal IRQ line
> that decides wether the CPU should take the IRQ or not. Here, things like
> IE flags etc matter. I don't have access to RTL on the MIPS side so I'm
> just guessing here.
> 
> In my mind, we should model this latter part by asserting INTERRUPT_HARD
> from hw/mips_int.c whenever any hw lines are active and letting the
> CPU in cpu-exec.c decide when to take the interrupt by applying it's
> internal masking.
> 

Sorry to come back so long after this discussion, but I now have another
argument. This commit causes a regression, the host CPU is now always at
100%. QEMU spent all its time looping because the CPU interrupt line is
asserted.

Not asserting the CPU interrupt line when interrupts are disabled fixes
the issue.