* [Qemu-devel] [PATCH] ARM: Fix disable interrupt for M profile
@ 2013-05-30 16:22 Fabien Chouteau
2013-05-30 17:26 ` Peter Maydell
0 siblings, 1 reply; 4+ messages in thread
From: Fabien Chouteau @ 2013-05-30 16:22 UTC (permalink / raw)
To: qemu-devel; +Cc: blauwirbel, peter.maydell, pbonzini, afaerber, rth
I'm not sure this was expected or not, but it looks like the "||" should
be a "&&". Otherwise it's not possible to disable interrupt.
Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
---
cpu-exec.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/cpu-exec.c b/cpu-exec.c
index 31c089d..079b6f0 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -462,8 +462,8 @@ int cpu_exec(CPUArchState *env)
We avoid this by disabling interrupts when
pc contains a magic address. */
if (interrupt_request & CPU_INTERRUPT_HARD
- && ((IS_M(env) && env->regs[15] < 0xfffffff0)
- || !(env->uncached_cpsr & CPSR_I))) {
+ && (IS_M(env) && env->regs[15] < 0xfffffff0)
+ && !(env->uncached_cpsr & CPSR_I)) {
env->exception_index = EXCP_IRQ;
cc->do_interrupt(cpu);
next_tb = 0;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] ARM: Fix disable interrupt for M profile
2013-05-30 16:22 [Qemu-devel] [PATCH] ARM: Fix disable interrupt for M profile Fabien Chouteau
@ 2013-05-30 17:26 ` Peter Maydell
2013-05-31 9:04 ` Fabien Chouteau
0 siblings, 1 reply; 4+ messages in thread
From: Peter Maydell @ 2013-05-30 17:26 UTC (permalink / raw)
To: Fabien Chouteau; +Cc: blauwirbel, pbonzini, rth, qemu-devel, afaerber
On 30 May 2013 17:22, Fabien Chouteau <chouteau@adacore.com> wrote:
> I'm not sure this was expected or not, but it looks like the "||" should
> be a "&&". Otherwise it's not possible to disable interrupt.
>
> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
We've had people trying to fiddle with this bit of code
before. I'd like to see a clear description of:
* the bug (ideally with a test case)
* why this patch fixes it (probably with reference to the
architecture manual and to what QEMU means when it sets
"CPSR_I" on M profile [hint: look at how we handle PRIMASK])
* a demonstration that it doesn't break non-M-profile
> @@ -462,8 +462,8 @@ int cpu_exec(CPUArchState *env)
> We avoid this by disabling interrupts when
> pc contains a magic address. */
> if (interrupt_request & CPU_INTERRUPT_HARD
> - && ((IS_M(env) && env->regs[15] < 0xfffffff0)
> - || !(env->uncached_cpsr & CPSR_I))) {
> + && (IS_M(env) && env->regs[15] < 0xfffffff0)
> + && !(env->uncached_cpsr & CPSR_I)) {
> env->exception_index = EXCP_IRQ;
> cc->do_interrupt(cpu);
> next_tb = 0;
...for instance this change means that the if() condition
will now never be satisfied for a non-M-profile core,
which is definitely wrong.
I think you're right that there is a bug in this conditional,
but your patch is not fixing it.
thanks
-- PMM
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] ARM: Fix disable interrupt for M profile
2013-05-30 17:26 ` Peter Maydell
@ 2013-05-31 9:04 ` Fabien Chouteau
2013-05-31 9:39 ` Peter Maydell
0 siblings, 1 reply; 4+ messages in thread
From: Fabien Chouteau @ 2013-05-31 9:04 UTC (permalink / raw)
To: Peter Maydell; +Cc: blauwirbel, pbonzini, rth, qemu-devel, afaerber
On 05/30/2013 07:26 PM, Peter Maydell wrote:
> On 30 May 2013 17:22, Fabien Chouteau <chouteau@adacore.com> wrote:
>> I'm not sure this was expected or not, but it looks like the "||" should
>> be a "&&". Otherwise it's not possible to disable interrupt.
>>
>> Signed-off-by: Fabien Chouteau <chouteau@adacore.com>
>
> We've had people trying to fiddle with this bit of code
> before. I'd like to see a clear description of:
> * the bug (ideally with a test case)
The bug: It's impossible to disable interrupt on M-profile.
Test case: I can try to build a binary, the source will be in Ada.
> * why this patch fixes it (probably with reference to the
> architecture manual and to what QEMU means when it sets
> "CPSR_I" on M profile [hint: look at how we handle PRIMASK])
I do not intend to do a full review of interrupt handling in the
M-profile. I just noticed that disabling interrupt was not effective,
and found this condition that looked faulty to me. I want to fix the
condition to match requirements of the comment above.
> * a demonstration that it doesn't break non-M-profile
>
Much more difficult for me, but that's what peer-review is for, right?.
>> @@ -462,8 +462,8 @@ int cpu_exec(CPUArchState *env)
>> We avoid this by disabling interrupts when
>> pc contains a magic address. */
>> if (interrupt_request & CPU_INTERRUPT_HARD
>> - && ((IS_M(env) & env->regs[15] < 0xfffffff0)
>> - || !(env->uncached_cpsr & CPSR_I))) {
>> + && (IS_M(env) && env->regs[15] < 0xfffffff0)
>> + && !(env->uncached_cpsr & CPSR_I)) {
>> env->exception_index = EXCP_IRQ;
>> cc->do_interrupt(cpu);
>> next_tb = 0;
>
> ...for instance this change means that the if() condition
> will now never be satisfied for a non-M-profile core,
> which is definitely wrong.
>
You're right I did this a little bit too quickly.
The expression should be:
if (interrupt_request & CPU_INTERRUPT_HARD
&& (!IS_M(env) || env->regs[15] < 0xfffffff0)
&& !(env->uncached_cpsr & CPSR_I)) {
I'll resend a patch, just in case.
Thanks,
--
Fabien Chouteau
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] ARM: Fix disable interrupt for M profile
2013-05-31 9:04 ` Fabien Chouteau
@ 2013-05-31 9:39 ` Peter Maydell
0 siblings, 0 replies; 4+ messages in thread
From: Peter Maydell @ 2013-05-31 9:39 UTC (permalink / raw)
To: Fabien Chouteau; +Cc: blauwirbel, pbonzini, rth, qemu-devel, afaerber
On 31 May 2013 10:04, Fabien Chouteau <chouteau@adacore.com> wrote:
> On 05/30/2013 07:26 PM, Peter Maydell wrote:
>> * why this patch fixes it (probably with reference to the
>> architecture manual and to what QEMU means when it sets
>> "CPSR_I" on M profile [hint: look at how we handle PRIMASK])
>
> I do not intend to do a full review of interrupt handling in the
> M-profile.
My point is really that somebody needs to. M profile for me
is low priority, but I know that a lot of our current
implementation is dubious at best. So until somebody does
step up who's willing to overhaul the M profile interrupt
and exception handling code, I take a fairly hard line
on reviewing band-aid patches which don't come with
sufficiently convincing explanation of why they are correct,
to at least avoid accidental regressions.
thanks
-- PMM
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-05-31 9:39 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-30 16:22 [Qemu-devel] [PATCH] ARM: Fix disable interrupt for M profile Fabien Chouteau
2013-05-30 17:26 ` Peter Maydell
2013-05-31 9:04 ` Fabien Chouteau
2013-05-31 9:39 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.