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

[Aurelien Jarno]

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

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

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