[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] MIPS interrupts and -icount
From: |
Aurelien Jarno |
Subject: |
Re: [Qemu-devel] [PATCH] MIPS interrupts and -icount |
Date: |
Sun, 25 Jul 2010 05:08:07 +0200 |
User-agent: |
Mutt/1.5.20 (2009-06-14) |
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