| Message ID | 1369930950-18564-1-git-send-email-chouteau@adacore.com |
|---|---|
| State | New |
| Headers | show |
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
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,
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
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;
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(-)