[Qemu-devel] [PATCH] ARM: Fix disable interrupt for M profile

[Qemu-devel] [PATCH] ARM: Fix disable interrupt for M profile

[Fabien Chouteau]

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

Re: [Qemu-devel] [PATCH] ARM: Fix disable interrupt for M profile

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] ARM: Fix disable interrupt for M profile

[Fabien Chouteau]

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

Re: [Qemu-devel] [PATCH] ARM: Fix disable interrupt for M profile

[Peter Maydell]

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