Re: [Qemu-devel] [PATCH] MIPS interrupts and -icount
On Sun, Dec 26, 2010 at 09:34:20AM +0100, Edgar E. Iglesias wrote:
> On Sat, Dec 25, 2010 at 11:22:14PM +0100, Aurelien Jarno wrote:
> > Hi,
> >
> > On Wed, Dec 22, 2010 at 05:12:39PM +0100, Edgar E. Iglesias wrote:
> > > Hi, I don't see this problem with the qemu.org test images and neither
> > > with my boards/images. I see QEMU basically not running at all when
> > > the guest is idle. Do you have more info on how to reproduce it?
> >
> > I am seeing the problem with the MIPS malta board and the images from:
> >
> > http://people.debian.org/~aurel32/qemu/mips/
> >
> > > If the CPU hw interrupt line is asserted it means some device is
> > > signaling interrupts. Maybe we are modling the wake up filter
> > > wrongly in target-mips/exec.h, maybe a real MIPS doesn't wakeup from
> > > sleep unless the irq passes the CPUs internal masking? The manuals
> > > are not really clear on this. I'm currently travelling and have
> > > no access to check with a real MIPS hw.
> >
> > According the manual I have checked, it is implementation dependent if
> > the CPU exits from the WAIT instruction when a non-enabled interrupt is
> > triggered. However for the few implementations I have checked (4k, 5k,
> > 34k), the CPU only wakes-up if the interrupt can be taken.
> >
> > > If your hw interrupt line is active all the time it sounds to me
> > > like if something is also wrong with either the guest software or a
> > > device model.
> >
> > The corresponding interrupt line is the timer one. It seems the kernel
> > sometimes choose to ignore the timer instead of stopping it. I am only
> > able to reproduce that with a dyntick enabled kernel.
> >
> > > I think the following patch should restore the previous wait for
> > > interrupt wakeup behaviour to let the MIPS sleep until an irq passes
> > > the internal masking (but I'm not sure this is how real MIPS does it):
> >
> > It does, thanks a lot.
> >
> > However, according to the manual I think we should also check if
> > interrupts are enabled (if they are disabled, an interrupt can't be
> > taken). I therefore propose the following patch:
> >
> > From 9c9e5f7ee1e897e408b1cd9f4c42ddf86c30aabe Mon Sep 17 00:00:00 2001
> > From: Aurelien Jarno
> > Date: Sat, 25 Dec 2010 22:56:32 +0100
> > Subject: [PATCH] target-mips: fix host CPU consumption when guest is idle
> >
> > When the CPU is in wait state, do not wake-up if an interrupt can't be
> > taken. This avoid host CPU running at 100% if a device (e.g. timer) has
> > an interrupt line left enabled.
> >
> > Also factorize code to check if interrupts are enabled in
> > cpu_mips_hw_interrupts_pending().
>
> Thanks Aurelien,
>
> It looks good, but one thing that worries me slightly is that streching
> the wakeup filter to include the IE related flags might break using the
> wait insn in polling mode.
>
> for example:
>
> di();
> init_hw();
> while (1) {
> wait_for_interrupt(); /* Power Save. */
> do_work();
> };
>
> I've seem similar constructions in bootcode/firmware for other archs.
> In this case I guess it would be using undefined behaviour on the mips
> though, so I'm OK with either patch.
Yes, the manuals are not fully clear, however it seems to be a possible
behaviour for some implementations, also it was the behaviour prior
to your patch. Note also that in the case above, the CPU can still be
woken-up by an NMI, though it doesn't seem to be implemented in QEMU
yet.
> At some point I'll see if I can check the IE flags behaviour with a real
> 34k and we can finetune the models with follow-up patches if needed.
Ok, that would be nice if you can do it.
> Acked-by: Edgar E. Iglesias
>
Thanks for your review.
Cheers,
Aurélien
--
Aurelien Jarno GPG: 1024D/F1BCDB73
aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH] MIPS interrupts and -icount
On Sat, Dec 25, 2010 at 11:22:14PM +0100, Aurelien Jarno wrote:
> Hi,
>
> On Wed, Dec 22, 2010 at 05:12:39PM +0100, Edgar E. Iglesias wrote:
> > Hi, I don't see this problem with the qemu.org test images and neither
> > with my boards/images. I see QEMU basically not running at all when
> > the guest is idle. Do you have more info on how to reproduce it?
>
> I am seeing the problem with the MIPS malta board and the images from:
>
> http://people.debian.org/~aurel32/qemu/mips/
>
> > If the CPU hw interrupt line is asserted it means some device is
> > signaling interrupts. Maybe we are modling the wake up filter
> > wrongly in target-mips/exec.h, maybe a real MIPS doesn't wakeup from
> > sleep unless the irq passes the CPUs internal masking? The manuals
> > are not really clear on this. I'm currently travelling and have
> > no access to check with a real MIPS hw.
>
> According the manual I have checked, it is implementation dependent if
> the CPU exits from the WAIT instruction when a non-enabled interrupt is
> triggered. However for the few implementations I have checked (4k, 5k,
> 34k), the CPU only wakes-up if the interrupt can be taken.
>
> > If your hw interrupt line is active all the time it sounds to me
> > like if something is also wrong with either the guest software or a
> > device model.
>
> The corresponding interrupt line is the timer one. It seems the kernel
> sometimes choose to ignore the timer instead of stopping it. I am only
> able to reproduce that with a dyntick enabled kernel.
>
> > I think the following patch should restore the previous wait for
> > interrupt wakeup behaviour to let the MIPS sleep until an irq passes
> > the internal masking (but I'm not sure this is how real MIPS does it):
>
> It does, thanks a lot.
>
> However, according to the manual I think we should also check if
> interrupts are enabled (if they are disabled, an interrupt can't be
> taken). I therefore propose the following patch:
>
> From 9c9e5f7ee1e897e408b1cd9f4c42ddf86c30aabe Mon Sep 17 00:00:00 2001
> From: Aurelien Jarno
> Date: Sat, 25 Dec 2010 22:56:32 +0100
> Subject: [PATCH] target-mips: fix host CPU consumption when guest is idle
>
> When the CPU is in wait state, do not wake-up if an interrupt can't be
> taken. This avoid host CPU running at 100% if a device (e.g. timer) has
> an interrupt line left enabled.
>
> Also factorize code to check if interrupts are enabled in
> cpu_mips_hw_interrupts_pending().
Thanks Aurelien,
It looks good, but one thing that worries me slightly is that streching
the wakeup filter to include the IE related flags might break using the
wait insn in polling mode.
for example:
di();
init_hw();
while (1) {
wait_for_interrupt(); /* Power Save. */
do_work();
};
I've seem similar constructions in bootcode/firmware for other archs.
In this case I guess it would be using undefined behaviour on the mips
though, so I'm OK with either patch.
At some point I'll see if I can check the IE flags behaviour with a real
34k and we can finetune the models with follow-up patches if needed.
Acked-by: Edgar E. Iglesias
Cheers
Re: [Qemu-devel] [PATCH] MIPS interrupts and -icount
Hi,
On Wed, Dec 22, 2010 at 05:12:39PM +0100, Edgar E. Iglesias wrote:
> Hi, I don't see this problem with the qemu.org test images and neither
> with my boards/images. I see QEMU basically not running at all when
> the guest is idle. Do you have more info on how to reproduce it?
I am seeing the problem with the MIPS malta board and the images from:
http://people.debian.org/~aurel32/qemu/mips/
> If the CPU hw interrupt line is asserted it means some device is
> signaling interrupts. Maybe we are modling the wake up filter
> wrongly in target-mips/exec.h, maybe a real MIPS doesn't wakeup from
> sleep unless the irq passes the CPUs internal masking? The manuals
> are not really clear on this. I'm currently travelling and have
> no access to check with a real MIPS hw.
According the manual I have checked, it is implementation dependent if
the CPU exits from the WAIT instruction when a non-enabled interrupt is
triggered. However for the few implementations I have checked (4k, 5k,
34k), the CPU only wakes-up if the interrupt can be taken.
> If your hw interrupt line is active all the time it sounds to me
> like if something is also wrong with either the guest software or a
> device model.
The corresponding interrupt line is the timer one. It seems the kernel
sometimes choose to ignore the timer instead of stopping it. I am only
able to reproduce that with a dyntick enabled kernel.
> I think the following patch should restore the previous wait for
> interrupt wakeup behaviour to let the MIPS sleep until an irq passes
> the internal masking (but I'm not sure this is how real MIPS does it):
It does, thanks a lot.
However, according to the manual I think we should also check if
interrupts are enabled (if they are disabled, an interrupt can't be
taken). I therefore propose the following patch:
>From 9c9e5f7ee1e897e408b1cd9f4c42ddf86c30aabe Mon Sep 17 00:00:00 2001
From: Aurelien Jarno
Date: Sat, 25 Dec 2010 22:56:32 +0100
Subject: [PATCH] target-mips: fix host CPU consumption when guest is idle
When the CPU is in wait state, do not wake-up if an interrupt can't be
taken. This avoid host CPU running at 100% if a device (e.g. timer) has
an interrupt line left enabled.
Also factorize code to check if interrupts are enabled in
cpu_mips_hw_interrupts_pending().
Based on a patch from Edgar E. Iglesias
Signed-off-by: Aurelien Jarno
---
cpu-exec.c |6 +-
target-mips/cpu.h |8
target-mips/exec.h | 18 +++---
3 files changed, 24 insertions(+), 8 deletions(-)
diff --git a/cpu-exec.c b/cpu-exec.c
index 39e5eea..8c9fb8b 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -454,11 +454,7 @@ int cpu_exec(CPUState *env1)
}
#elif defined(TARGET_MIPS)
if ((interrupt_request & CPU_INTERRUPT_HARD) &&
-cpu_mips_hw_interrupts_pending(env) &&
-(env->CP0_Status & (1 << CP0St_IE)) &&
-!(env->CP0_Status & (1 << CP0St_EXL)) &&
-!(env->CP0_Status & (1 << CP0St_ERL)) &&
-!(env->hflags & MIPS_HFLAG_DM)) {
+cpu_mips_hw_interrupts_pending(env)) {
/* Raise it */
env->exception_index = EXCP_EXT_INTERRUPT;
env->error_code = 0;
diff --git a/target-mips/cpu.h b/target-mips/cpu.h
index c1f211f..2419aa9 100644
--- a/target-mips/cpu.h
+++ b/target-mips/cpu.h
@@ -532,6 +532,14 @@ static inline int cpu_mips_hw_interrupts_pending(CPUState
*env)
int32_t status;
int r;
+if (!(env->CP0_Status & (1 << CP0St_IE)) ||
+(env->CP0_Status & (1 << CP0St_EXL)) ||
+(env->CP0_Status & (1 << CP0St_ERL)) ||
+(env->hflags & MIPS_HFLAG_DM)) {
+/* Interrupts are disabled */
+return 0;
+}
+
pending = env->CP0_Cause & CP0Ca_IP_mask;
status = env->CP0_Status & CP0Ca_IP_mask;
diff --git a/target-mips/exec.h b/target-mips/exec.h
index af61b54..1273654 100644
--- a/target-mips/exec.h
+++ b/target-mips/exec.h
@@ -19,10 +19,22 @@ register struct CPUMIPSState *env asm(AREG0);
static inline int cpu_has_work(CPUState *env)
{
-return (env->interrupt_request &
-(CPU_INTERRUPT_HARD | CPU_INTERRUPT_TIMER));
-}
+int has_work = 0;
+
+/* It is implementation dependent if non-enabled interrupts
+ wake-up the CPU, however most of the implementations only
+ check for interrupts that can be taken. */
+if ((env->interrupt_request & CPU_INTERRUPT_HARD) &&
+cpu_mips_hw_interrupts_pending(env)) {
+has_work = 1;
+}
+if (env->interrupt_request & CPU_INTERRUPT_TIMER) {
+has_work = 1;
+}
+
+return has_work;
+}
static inline int cpu_halted(CPUState *env)
{
--
Aurelien Jarno GPG: 1024D/F1BCDB73
aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH] MIPS interrupts and -icount
On Tue, Dec 21, 2010 at 11:28:43PM +0100, Aurelien Jarno wrote: > 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. Hi, I don't see this problem with the qemu.org test images and neither with my boards/images. I see QEMU basically not running at all when the guest is idle. Do you have more info on how to reproduce it? If the CPU hw interrupt line is asserted it means some device is signaling interrupts. Maybe we are modling the wake up filter wrongly in target-mips/exec.h, maybe a rea
Re: [Qemu-devel] [PATCH] MIPS interrupts and -icount
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. -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH] MIPS interrupts and -icount
On Sun, Jul 25, 2010 at 09:21:48AM +0200, Edgar E. Iglesias wrote: > On Sun, Jul 25, 2010 at 07:52:18AM +0200, Edgar E. Iglesias wrote: > > On Sun, Jul 25, 2010 at 06:44:41AM +0200, Aurelien Jarno 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. > > > > > > > > > > If we choose having the interrupt controller as part of the CPU, which > > > seems to be what you have chosen, the following patch should fix the > > > remaining issues (and prepare the work for vector interrupt support): > > > > Thanks, > > > > That looks nice, specially the removing of the helper_interrupt_restart > > parts :) > > > > I'll test it on my side and send you an ACK. > > > I've tested this with the same images as before, they still work. > I also created a synthetic testcase for the 2 software interrupt > lines and they seem to behave OK. Our linux port was not so > happy about seeing those raised though, so I had to hack a bit > to see them in action :) > > Acked-by: Edgar E. Iglesias > Tested-by: Edgar E. Iglesias > > Would you mind applying your patch on top of mine? > Thanks for the tests, I have just applied it. -- Aurelien Jarno GPG: 1024D/F1BCDB73 aurel...@aurel32.net http://www.aurel32.net
Re: [Qemu-devel] [PATCH] MIPS interrupts and -icount
On Sun, Jul 25, 2010 at 07:52:18AM +0200, Edgar E. Iglesias wrote:
> On Sun, Jul 25, 2010 at 06:44:41AM +0200, Aurelien Jarno 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.
> > >
> >
> > If we choose having the interrupt controller as part of the CPU, which
> > seems to be what you have chosen, the following patch should fix the
> > remaining issues (and prepare the work for vector interrupt support):
>
> Thanks,
>
> That looks nice, specially the removing of the helper_interrupt_restart
> parts :)
>
> I'll test it on my side and send you an ACK.
I've tested this with the same images as before, they still work.
I also created a synthetic testcase for the 2 software interrupt
lines and they seem to behave OK. Our linux port was not so
happy about seeing those raised though, so I had to hack a bit
to see them in action :)
Acked-by: Edgar E. Iglesias
Tested-by: Edgar E. Iglesias
Would you mind applying your patch on top of mine?
Thanks alot,
Edgar
>
> Thanks for your patience Aurelien,
> Edgar
>
>
> >
> >
> > diff --git a/hw/mips_int.c b/hw/mips_int.c
> > index 80488ba..477f6ab 100644
> > --- a/hw/mips_int.c
> > +++ b/hw/mips_int.c
> > @@ -54,3 +54,12 @@ void cpu_mips_irq_init_cpu(CPUState *env)
> > env->irq[i] = qi[i];
> > }
> > }
> > +
> > +void cpu_mips_soft_irq(CPUState *env, int irq, int level)
> > +{
> > +if (irq < 0 || irq > 2) {
> > +return;
> > +}
> > +
> > +qemu_set_irq(env->irq[irq], level);
> > +}
> > diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> > index 1578850..b8e6fee 100644
> > --- a/target-mips/cpu.h
> > +++ b/target-mips/cpu.h
> > @@ -597,6 +597,9 @@ 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_soft_irq(CPUState *env, int irq, int level);
> > +
> > /* 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/helper.h b/target-mips/helper.h
> > index a6ba75d..cb13fb2 100644
> > --- a/target-mips/helper.h
> > +++ b/target-mips/helper.h
> > @@ -2,7 +2,6 @@
> >
> > DEF_HELPER_2(raise_exception_err, void, i32, int)
> > DEF_HELPER_1(raise_exception, void, i32)
> > -DEF_HELPER_0(interrupt_restart, void)
> >
> > #ifdef TARGET_MIPS64
> > DEF_HELPER_3(ldl, tl, tl, tl, int)
> > diff --git a/tar
Re: [Qemu-devel] [PATCH] MIPS interrupts and -icount
On Sun, Jul 25, 2010 at 06:44:41AM +0200, Aurelien Jarno 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.
> >
>
> If we choose having the interrupt controller as part of the CPU, which
> seems to be what you have chosen, the following patch should fix the
> remaining issues (and prepare the work for vector interrupt support):
Thanks,
That looks nice, specially the removing of the helper_interrupt_restart
parts :)
I'll test it on my side and send you an ACK.
Thanks for your patience Aurelien,
Edgar
>
>
> diff --git a/hw/mips_int.c b/hw/mips_int.c
> index 80488ba..477f6ab 100644
> --- a/hw/mips_int.c
> +++ b/hw/mips_int.c
> @@ -54,3 +54,12 @@ void cpu_mips_irq_init_cpu(CPUState *env)
> env->irq[i] = qi[i];
> }
> }
> +
> +void cpu_mips_soft_irq(CPUState *env, int irq, int level)
> +{
> +if (irq < 0 || irq > 2) {
> +return;
> +}
> +
> +qemu_set_irq(env->irq[irq], level);
> +}
> diff --git a/target-mips/cpu.h b/target-mips/cpu.h
> index 1578850..b8e6fee 100644
> --- a/target-mips/cpu.h
> +++ b/target-mips/cpu.h
> @@ -597,6 +597,9 @@ 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_soft_irq(CPUState *env, int irq, int level);
> +
> /* 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/helper.h b/target-mips/helper.h
> index a6ba75d..cb13fb2 100644
> --- a/target-mips/helper.h
> +++ b/target-mips/helper.h
> @@ -2,7 +2,6 @@
>
> DEF_HELPER_2(raise_exception_err, void, i32, int)
> DEF_HELPER_1(raise_exception, void, i32)
> -DEF_HELPER_0(interrupt_restart, void)
>
> #ifdef TARGET_MIPS64
> DEF_HELPER_3(ldl, tl, tl, tl, int)
> diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
> index c963224..8482e69 100644
> --- a/target-mips/op_helper.c
> +++ b/target-mips/op_helper.c
> @@ -46,18 +46,6 @@ void helper_raise_exception (uint32_t exception)
> helper_raise_exception_err(exception, 0);
> }
>
> -void helper_interrupt_restart (void)
> -{
> -if (!(env->CP0_Status & (1 << CP0St_EXL)) &&
> -!(env->CP0_Status & (1 << CP0St_ERL)) &&
> -!(env->hflags & MIPS_HFLAG_DM) &&
> -(env->CP0_Status & (1 << CP0St_IE)) &&
> -(env->CP0_Status & env->CP0_Cause & CP0Ca_IP_mask)) {
> -env->CP0_Cause &= ~(0x1f << CP0Ca_EC);
> -helper_raise_exception(EXCP_EXT_INTERRUPT);
> -}
> -}
> -
> #if !defined(CONFIG_USER_ONLY)
>
Re: [Qemu-devel] [PATCH] MIPS interrupts and -icount
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 > > > > 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 sh
Re: [Qemu-devel] [PATCH] MIPS interrupts and -icount
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.
>
If we choose having the interrupt controller as part of the CPU, which
seems to be what you have chosen, the following patch should fix the
remaining issues (and prepare the work for vector interrupt support):
diff --git a/hw/mips_int.c b/hw/mips_int.c
index 80488ba..477f6ab 100644
--- a/hw/mips_int.c
+++ b/hw/mips_int.c
@@ -54,3 +54,12 @@ void cpu_mips_irq_init_cpu(CPUState *env)
env->irq[i] = qi[i];
}
}
+
+void cpu_mips_soft_irq(CPUState *env, int irq, int level)
+{
+if (irq < 0 || irq > 2) {
+return;
+}
+
+qemu_set_irq(env->irq[irq], level);
+}
diff --git a/target-mips/cpu.h b/target-mips/cpu.h
index 1578850..b8e6fee 100644
--- a/target-mips/cpu.h
+++ b/target-mips/cpu.h
@@ -597,6 +597,9 @@ 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_soft_irq(CPUState *env, int irq, int level);
+
/* 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/helper.h b/target-mips/helper.h
index a6ba75d..cb13fb2 100644
--- a/target-mips/helper.h
+++ b/target-mips/helper.h
@@ -2,7 +2,6 @@
DEF_HELPER_2(raise_exception_err, void, i32, int)
DEF_HELPER_1(raise_exception, void, i32)
-DEF_HELPER_0(interrupt_restart, void)
#ifdef TARGET_MIPS64
DEF_HELPER_3(ldl, tl, tl, tl, int)
diff --git a/target-mips/op_helper.c b/target-mips/op_helper.c
index c963224..8482e69 100644
--- a/target-mips/op_helper.c
+++ b/target-mips/op_helper.c
@@ -46,18 +46,6 @@ void helper_raise_exception (uint32_t exception)
helper_raise_exception_err(exception, 0);
}
-void helper_interrupt_restart (void)
-{
-if (!(env->CP0_Status & (1 << CP0St_EXL)) &&
-!(env->CP0_Status & (1 << CP0St_ERL)) &&
-!(env->hflags & MIPS_HFLAG_DM) &&
-(env->CP0_Status & (1 << CP0St_IE)) &&
-(env->CP0_Status & env->CP0_Cause & CP0Ca_IP_mask)) {
-env->CP0_Cause &= ~(0x1f << CP0Ca_EC);
-helper_raise_exception(EXCP_EXT_INTERRUPT);
-}
-}
-
#if !defined(CONFIG_USER_ONLY)
static void do_restore_state (void *pc_ptr)
{
@@ -1346,6 +1334,7 @@ void helper_mtc0_cause (target_ulong arg1)
{
uint32_t mask = 0x00C00300;
uint32_t old = env->CP0_Cause;
+int i;
if (env->insn_flags & ISA_MIPS32R2)
mask |= 1 << CP0Ca_DC;
@@ -1358,6 +1347,13 @@ void helper_mtc0_cause (target_ulong arg1)
else
cpu_mips_start_count(env);
}
+
+/* Set/reset software interrupts */
+for (i = 0 ; i < 2 ; i++) {
+if ((old
Re: [Qemu-devel] [PATCH] MIPS interrupts and -icount
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
> > > 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
> > >
> > > 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 el
Re: [Qemu-devel] [PATCH] MIPS interrupts and -icount
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
> > 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
> >
> > 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 {
> > +
Re: [Qemu-devel] [PATCH] MIPS interrupts and -icount
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
> 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
>
> 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 ar
[Qemu-devel] [PATCH] 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
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
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);
-}
-
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);
+}
}
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);
}
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);
-}
}
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;
}
