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

[Peter Maydell]

On 30 May 2013 17:22, Fabien Chouteau <address@hidden> 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 <address@hidden>

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