Patchwork Disable interrupts on Cortex M3 (lm3s6965evb)

login
register
mail settings
Submitter Sebastian Huber
Date June 5, 2011, 2:17 p.m.
Message ID <4DEB900F.5070206@embedded-brains.de>
Download mbox | patch
Permalink /patch/98765/
State New
Headers show

Comments

Sebastian Huber - June 5, 2011, 2:17 p.m.
On 05/06/11 15:44, Peter Maydell wrote:
> On 5 June 2011 14:06, Sebastian Huber
> <sebastian.huber@embedded-brains.de> wrote:
>   
>> I think the interrupt handling logic for ARMv7M is wrong in cpu-exec.c
>> line 470.  Please have a look at the attached patch.
>>     
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -470,8 +470,8 @@ int cpu_exec(CPUState *env1)
>                         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))) {
> +                        && !(env->uncached_cpsr & CPSR_I)
> +                        && (IS_M(env) && env->regs[15] < 0xfffffff0)) {
>                          env->exception_index = EXCP_IRQ;
>                          do_interrupt(env);
>                          next_tb = 0;
>
> This doesn't look right -- it changes the behaviour in the
> case where we aren't an M profile CPU.
>   

Yes, you are right.  Please see attached version.

> In any case, M profile exception priority handling is sufficiently
> complicated that any change which only looks at PRIMASK (which is
> effectively what the change to look at CPSR_I here is doing) is
> almost certainly wrong. I think that whatever is raising the
> interrupt should be looking at the CPU priority and not raising it
> in the first place.
>   

Yes.  Please have a look at:

http://lists.nongnu.org/archive/html/qemu-devel/2011-05/msg03132.html

It is also not possible to set the priority of the standard exceptions
like PendSC etc. via the  System Handler Priority Register 1-3 (this
part is missing in gic_dist_{read, write}b()).

> (It looks suspiciously as if most of the v7M priority handling
> is simply missing from QEMU, ie you have bigger problems than
> can be fixed by a small patch like this...)
>   

Yes, but the current behaviour is definitely not right.  Since the
PRIMASK is mapped to the I bit in the CPSR I guessed that this was the
right place to fix it.
Peter Maydell - June 5, 2011, 2:57 p.m.
On 5 June 2011 15:17, Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
> On 05/06/11 15:44, Peter Maydell wrote:

>> In any case, M profile exception priority handling is sufficiently
>> complicated that any change which only looks at PRIMASK (which is
>> effectively what the change to look at CPSR_I here is doing) is
>> almost certainly wrong. I think that whatever is raising the
>> interrupt should be looking at the CPU priority and not raising it
>> in the first place.

> Yes.  Please have a look at:
>
> http://lists.nongnu.org/archive/html/qemu-devel/2011-05/msg03132.html

That patch does correct an error in the decode of the basepri
registers (and so we might as well apply it), but it does not
affect the fact that the underlying v7m.basepri field is not
actually used for anything.

>> (It looks suspiciously as if most of the v7M priority handling
>> is simply missing from QEMU, ie you have bigger problems than
>> can be fixed by a small patch like this...)

> Yes, but the current behaviour is definitely not right.  Since the
> PRIMASK is mapped to the I bit in the CPSR I guessed that this was the
> right place to fix it.

I agree that the current behaviour is not right. However, to fix
this problem you need to work on a larger scale than attempting
to apply two line patches which fix your particular use case.

-- PMM
Sebastian Huber - June 5, 2011, 4:32 p.m.
On 05/06/11 16:57, Peter Maydell wrote:
> On 5 June 2011 15:17, Sebastian Huber
> <sebastian.huber@embedded-brains.de> wrote:
>   
>> On 05/06/11 15:44, Peter Maydell wrote:
[...]
>>> (It looks suspiciously as if most of the v7M priority handling
>>> is simply missing from QEMU, ie you have bigger problems than
>>> can be fixed by a small patch like this...)
>>>       
>   
>> Yes, but the current behaviour is definitely not right.  Since the
>> PRIMASK is mapped to the I bit in the CPSR I guessed that this was the
>> right place to fix it.
>>     
> I agree that the current behaviour is not right. However, to fix
> this problem you need to work on a larger scale than attempting
> to apply two line patches which fix your particular use case.
>   

I agree, but you have to start somewhere.  What is "this problem"?  Is
that we have no execution priority (in the sense of the ARMv7
architecture, B1.3.2 Exceptions), but instead use a mapping to CPSR_I
and CPSR_F?
Peter Maydell - June 5, 2011, 7:23 p.m.
On 5 June 2011 17:32, Sebastian Huber
<sebastian.huber@embedded-brains.de> wrote:
> On 05/06/11 16:57, Peter Maydell wrote:
>> I agree that the current behaviour is not right. However, to fix
>> this problem you need to work on a larger scale than attempting
>> to apply two line patches which fix your particular use case.
>
> I agree, but you have to start somewhere.  What is "this problem"?  Is
> that we have no execution priority (in the sense of the ARMv7
> architecture, B1.3.2 Exceptions), but instead use a mapping to CPSR_I
> and CPSR_F?

There is some notion of priority, see gic_update()
in hw/arm_gic.c; but it is only within the gic and is not
dealing with v7M specific issues.

At the moment I am mostly just warning you that you're entering
difficult territory; if I have time to read the qemu v7m code
more carefully next week I may have more concrete opinions.

-- PMM

Patch

From 917f2491c1dc2525b24c635afe4459e55700149c Mon Sep 17 00:00:00 2001
From: Sebastian Huber <sebastian.huber@embedded-brains.de>
Date: Sun, 5 Jun 2011 14:57:17 +0200
Subject: [PATCH] Fixed interrupt handling for ARMv7M.

Signed-off-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
---
 cpu-exec.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cpu-exec.c b/cpu-exec.c
index 6ddd8dd..d1e9816 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -470,8 +470,8 @@  int cpu_exec(CPUState *env1)
                        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))) {
+                        && !(env->uncached_cpsr & CPSR_I)
+                        && (!IS_M(env) || env->regs[15] < 0xfffffff0)) {
                         env->exception_index = EXCP_IRQ;
                         do_interrupt(env);
                         next_tb = 0;
-- 
1.7.1