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.igles...@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 <ed...@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. -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurel...@aurel32.net http://www.aurel32.net
