Patchwork [8/9] sparc64: interrupt trap handling

login
register
mail settings
Submitter Igor V. Kovalenko
Date Jan. 5, 2010, 11:19 p.m.
Message ID <20100105231943.6526.42311.stgit@skyserv>
Download mbox | patch
Permalink /patch/42238/
State New
Headers show

Comments

Igor V. Kovalenko - Jan. 5, 2010, 11:19 p.m.
From: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>

cpu_check_irqs
- handle SOFTINT register TICK and STICK timer bits
- only check interrupt levels greater than PIL value
- handle preemption by higher level traps

cpu_exec
- handle CPU_INTERRUPT_HARD only if interrupts are enabled
- PIL 15 is not special level on sparcv9

Signed-off-by: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
---
 cpu-exec.c |   40 ++++++++++++++++++++++------------------
 hw/sun4u.c |   52 +++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 59 insertions(+), 33 deletions(-)
Blue Swirl - Jan. 6, 2010, 5 p.m.
On Tue, Jan 5, 2010 at 11:19 PM, Igor V. Kovalenko
<igor.v.kovalenko@gmail.com> wrote:
> From: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
>
> cpu_check_irqs
> - handle SOFTINT register TICK and STICK timer bits
> - only check interrupt levels greater than PIL value
> - handle preemption by higher level traps
>
> cpu_exec
> - handle CPU_INTERRUPT_HARD only if interrupts are enabled
> - PIL 15 is not special level on sparcv9
>
> Signed-off-by: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
> ---
>  cpu-exec.c |   40 ++++++++++++++++++++++------------------
>  hw/sun4u.c |   52 +++++++++++++++++++++++++++++++++++++---------------
>  2 files changed, 59 insertions(+), 33 deletions(-)
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index af4595b..65192c1 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -449,24 +449,28 @@ int cpu_exec(CPUState *env1)
>                         next_tb = 0;
>                     }
>  #elif defined(TARGET_SPARC)
> -                    if ((interrupt_request & CPU_INTERRUPT_HARD) &&
> -                       cpu_interrupts_enabled(env)) {
> -                       int pil = env->interrupt_index & 15;
> -                       int type = env->interrupt_index & 0xf0;
> -
> -                       if (((type == TT_EXTINT) &&
> -                            (pil == 15 || pil > env->psrpil)) ||
> -                           type != TT_EXTINT) {
> -                           env->interrupt_request &= ~CPU_INTERRUPT_HARD;
> -                            env->exception_index = env->interrupt_index;
> -                            do_interrupt(env);
> -                           env->interrupt_index = 0;
> -                        next_tb = 0;
> -                       }
> -                   } else if (interrupt_request & CPU_INTERRUPT_TIMER) {
> -                       //do_interrupt(0, 0, 0, 0, 0);
> -                       env->interrupt_request &= ~CPU_INTERRUPT_TIMER;
> -                   }
> +                    if ((interrupt_request & CPU_INTERRUPT_HARD)) {
> +                        if (cpu_interrupts_enabled(env)) {
> +                            int pil = env->interrupt_index & 0xf;
> +                            int type = env->interrupt_index & 0xf0;
> +
> +                            if (((type == TT_EXTINT) && (pil > env->psrpil)) ||
> +                                            type != TT_EXTINT) {

This removes the check for level 15, which is non-maskable on V8.

> +                                //env->interrupt_request &= ~CPU_INTERRUPT_HARD;

Remove.

> +                                if (env->interrupt_index > 0) {
> +                                    env->exception_index = env->interrupt_index;
> +                                    do_interrupt(env);
> +                                    next_tb = 0;
> +                                }
> +                            }
> +                        }
> +                    }
> +                    else if (interrupt_request & CPU_INTERRUPT_TIMER) {

'else if' etc. should be on the same line as '}'.

> +                        env->interrupt_request &= ~CPU_INTERRUPT_TIMER;
> +#if !defined(CONFIG_USER_ONLY)
> +                        cpu_check_irqs(env);
> +#endif
> +                    }
>  #elif defined(TARGET_ARM)
>                     if (interrupt_request & CPU_INTERRUPT_FIQ
>                         && !(env->uncached_cpsr & CPSR_F)) {
> diff --git a/hw/sun4u.c b/hw/sun4u.c
> index 9d46f08..84a8043 100644
> --- a/hw/sun4u.c
> +++ b/hw/sun4u.c
> @@ -233,29 +233,51 @@ void irq_info(Monitor *mon)
>
>  void cpu_check_irqs(CPUState *env)
>  {
> -    uint32_t pil = env->pil_in | (env->softint & ~SOFTINT_TIMER) |
> -        ((env->softint & SOFTINT_TIMER) << 14);
> +    uint32_t pil = env->pil_in | (env->softint & ~(SOFTINT_TM | SOFTINT_SM));
> +
> +    /* check if TM or SM in SOFTINT are set
> +       setting these also causes interrupt 14 */
> +    if (env->softint & (SOFTINT_TM | SOFTINT_SM))
> +        pil |= 1 << 14;
> +
> +    if (!pil) {
> +        if (env->interrupt_request & CPU_INTERRUPT_HARD) {
> +            CPUIRQ_DPRINTF("Reset CPU IRQ (current interrupt %X)\n",
> +                           env->interrupt_index);
> +            env->interrupt_index = 0;
> +            cpu_reset_interrupt(env, CPU_INTERRUPT_HARD);

All other architectures have this code in cpu-exec.c, why move?

Uppercase hex.

> +        }
> +        return;
> +    }
> +
> +    if (cpu_interrupts_enabled(env)) {
>
> -    if (pil && (env->interrupt_index == 0 ||
> -                (env->interrupt_index & ~15) == TT_EXTINT)) {
>         unsigned int i;
>
> -        for (i = 15; i > 0; i--) {
> +        for (i = 15; i > env->psrpil; i--) {
>             if (pil & (1 << i)) {
>                 int old_interrupt = env->interrupt_index;
> -
> -                env->interrupt_index = TT_EXTINT | i;
> -                if (old_interrupt != env->interrupt_index) {
> -                    CPUIRQ_DPRINTF("Set CPU IRQ %d\n", i);
> -                    cpu_interrupt(env, CPU_INTERRUPT_HARD);
> -                }
> +                int new_interrupt = TT_EXTINT | i;
> +
> +                    if (env->tl > 0 && cpu_tsptr(env)->tt > new_interrupt) {
> +                        CPUIRQ_DPRINTF("Not setting CPU IRQ: TL=%d "
> +                                "current %X >= pending %X\n",
> +                                env->tl, cpu_tsptr(env)->tt, new_interrupt);
> +                    }
> +                    else if (old_interrupt != new_interrupt) {

'else if' etc. should be on the same line as '}'.

> +                        env->interrupt_index = new_interrupt;
> +                        CPUIRQ_DPRINTF("Set CPU IRQ %d old=%X new=%X\n", i,
> +                                old_interrupt, new_interrupt);
> +                        cpu_interrupt(env, CPU_INTERRUPT_HARD);
> +                    }
>                 break;
>             }
>         }
> -    } else if (!pil && (env->interrupt_index & ~15) == TT_EXTINT) {
> -        CPUIRQ_DPRINTF("Reset CPU IRQ %d\n", env->interrupt_index & 15);
> -        env->interrupt_index = 0;
> -        cpu_reset_interrupt(env, CPU_INTERRUPT_HARD);
> +    }
> +    else {
> +        CPUIRQ_DPRINTF("Interrupts disabled, pil=%08X pil_in=%08X softint=%08X "
> +                "current interrupt %X\n",
> +                pil, env->pil_in, env->softint, env->interrupt_index);
>     }
>  }

Overall, it looks like there are some unnecessary changes so that it's
hard to see what is the fix.
Igor V. Kovalenko - Jan. 7, 2010, 5:24 p.m.
On Wed, Jan 6, 2010 at 8:00 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Tue, Jan 5, 2010 at 11:19 PM, Igor V. Kovalenko
> <igor.v.kovalenko@gmail.com> wrote:
>> From: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
>>
>> cpu_check_irqs
>> - handle SOFTINT register TICK and STICK timer bits
>> - only check interrupt levels greater than PIL value
>> - handle preemption by higher level traps
>>
>> cpu_exec
>> - handle CPU_INTERRUPT_HARD only if interrupts are enabled
>> - PIL 15 is not special level on sparcv9
>>
>> Signed-off-by: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
>> ---
>>  cpu-exec.c |   40 ++++++++++++++++++++++------------------
>>  hw/sun4u.c |   52 +++++++++++++++++++++++++++++++++++++---------------
>>  2 files changed, 59 insertions(+), 33 deletions(-)
>>
>> diff --git a/cpu-exec.c b/cpu-exec.c
>> index af4595b..65192c1 100644
>> --- a/cpu-exec.c
>> +++ b/cpu-exec.c
>> @@ -449,24 +449,28 @@ int cpu_exec(CPUState *env1)
>>                         next_tb = 0;
>>                     }
>>  #elif defined(TARGET_SPARC)
>> -                    if ((interrupt_request & CPU_INTERRUPT_HARD) &&
>> -                       cpu_interrupts_enabled(env)) {
>> -                       int pil = env->interrupt_index & 15;
>> -                       int type = env->interrupt_index & 0xf0;
>> -
>> -                       if (((type == TT_EXTINT) &&
>> -                            (pil == 15 || pil > env->psrpil)) ||
>> -                           type != TT_EXTINT) {
>> -                           env->interrupt_request &= ~CPU_INTERRUPT_HARD;
>> -                            env->exception_index = env->interrupt_index;
>> -                            do_interrupt(env);
>> -                           env->interrupt_index = 0;
>> -                        next_tb = 0;
>> -                       }
>> -                   } else if (interrupt_request & CPU_INTERRUPT_TIMER) {
>> -                       //do_interrupt(0, 0, 0, 0, 0);
>> -                       env->interrupt_request &= ~CPU_INTERRUPT_TIMER;
>> -                   }
>> +                    if ((interrupt_request & CPU_INTERRUPT_HARD)) {
>> +                        if (cpu_interrupts_enabled(env)) {
>> +                            int pil = env->interrupt_index & 0xf;
>> +                            int type = env->interrupt_index & 0xf0;
>> +
>> +                            if (((type == TT_EXTINT) && (pil > env->psrpil)) ||
>> +                                            type != TT_EXTINT) {
>
> This removes the check for level 15, which is non-maskable on V8.

I'll implement cpu_pil_allowed() to hide v8 vs v9 difference wrt psrpil.

>> diff --git a/hw/sun4u.c b/hw/sun4u.c
>> index 9d46f08..84a8043 100644
>> --- a/hw/sun4u.c
>> +++ b/hw/sun4u.c
>> @@ -233,29 +233,51 @@ void irq_info(Monitor *mon)
>>
>>  void cpu_check_irqs(CPUState *env)
>>  {
>> -    uint32_t pil = env->pil_in | (env->softint & ~SOFTINT_TIMER) |
>> -        ((env->softint & SOFTINT_TIMER) << 14);
>> +    uint32_t pil = env->pil_in | (env->softint & ~(SOFTINT_TM | SOFTINT_SM));
>> +
>> +    /* check if TM or SM in SOFTINT are set
>> +       setting these also causes interrupt 14 */
>> +    if (env->softint & (SOFTINT_TM | SOFTINT_SM))
>> +        pil |= 1 << 14;
>> +
>> +    if (!pil) {
>> +        if (env->interrupt_request & CPU_INTERRUPT_HARD) {
>> +            CPUIRQ_DPRINTF("Reset CPU IRQ (current interrupt %X)\n",
>> +                           env->interrupt_index);
>> +            env->interrupt_index = 0;
>> +            cpu_reset_interrupt(env, CPU_INTERRUPT_HARD);
>
> All other architectures have this code in cpu-exec.c, why move?

Not really a move (from cpu-exec.c) - see below, it was there already,
and sparc32 code is similar. PPC can do that in cpu-exec.c since it
exposes pending_interrupts flag. Other arches also seem to reset HARD
flag wherever fits.

For sparc64 clearing flag in cpu-exec.c would require pulling some
code to deal with softint bits. That way we could reduce
cpu_check_irqs() to set separate flag "possible interrupt state
change" and return from tb so cpu-exec.c code would pick the flag and
deal with interrupt.

That implementation I'd like less than consolidating trigger logic
into cpu_check_irqs.

> Overall, it looks like there are some unnecessary changes so that it's
> hard to see what is the fix.

Since code used to be tabbed as well, there is a great deal of churn here.
I tried to outline fixes in the changeset header.

Patch

diff --git a/cpu-exec.c b/cpu-exec.c
index af4595b..65192c1 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -449,24 +449,28 @@  int cpu_exec(CPUState *env1)
                         next_tb = 0;
                     }
 #elif defined(TARGET_SPARC)
-                    if ((interrupt_request & CPU_INTERRUPT_HARD) &&
-			cpu_interrupts_enabled(env)) {
-			int pil = env->interrupt_index & 15;
-			int type = env->interrupt_index & 0xf0;
-
-			if (((type == TT_EXTINT) &&
-			     (pil == 15 || pil > env->psrpil)) ||
-			    type != TT_EXTINT) {
-			    env->interrupt_request &= ~CPU_INTERRUPT_HARD;
-                            env->exception_index = env->interrupt_index;
-                            do_interrupt(env);
-			    env->interrupt_index = 0;
-                        next_tb = 0;
-			}
-		    } else if (interrupt_request & CPU_INTERRUPT_TIMER) {
-			//do_interrupt(0, 0, 0, 0, 0);
-			env->interrupt_request &= ~CPU_INTERRUPT_TIMER;
-		    }
+                    if ((interrupt_request & CPU_INTERRUPT_HARD)) {
+                        if (cpu_interrupts_enabled(env)) {
+                            int pil = env->interrupt_index & 0xf;
+                            int type = env->interrupt_index & 0xf0;
+
+                            if (((type == TT_EXTINT) && (pil > env->psrpil)) ||
+                                            type != TT_EXTINT) {
+                                //env->interrupt_request &= ~CPU_INTERRUPT_HARD;
+                                if (env->interrupt_index > 0) {
+                                    env->exception_index = env->interrupt_index;
+                                    do_interrupt(env);
+                                    next_tb = 0;
+                                }
+                            }
+                        }
+                    }
+                    else if (interrupt_request & CPU_INTERRUPT_TIMER) {
+                        env->interrupt_request &= ~CPU_INTERRUPT_TIMER;
+#if !defined(CONFIG_USER_ONLY)
+                        cpu_check_irqs(env);
+#endif
+                    }
 #elif defined(TARGET_ARM)
                     if (interrupt_request & CPU_INTERRUPT_FIQ
                         && !(env->uncached_cpsr & CPSR_F)) {
diff --git a/hw/sun4u.c b/hw/sun4u.c
index 9d46f08..84a8043 100644
--- a/hw/sun4u.c
+++ b/hw/sun4u.c
@@ -233,29 +233,51 @@  void irq_info(Monitor *mon)
 
 void cpu_check_irqs(CPUState *env)
 {
-    uint32_t pil = env->pil_in | (env->softint & ~SOFTINT_TIMER) |
-        ((env->softint & SOFTINT_TIMER) << 14);
+    uint32_t pil = env->pil_in | (env->softint & ~(SOFTINT_TM | SOFTINT_SM));
+
+    /* check if TM or SM in SOFTINT are set
+       setting these also causes interrupt 14 */
+    if (env->softint & (SOFTINT_TM | SOFTINT_SM))
+        pil |= 1 << 14;
+
+    if (!pil) {
+        if (env->interrupt_request & CPU_INTERRUPT_HARD) {
+            CPUIRQ_DPRINTF("Reset CPU IRQ (current interrupt %X)\n",
+                           env->interrupt_index);
+            env->interrupt_index = 0;
+            cpu_reset_interrupt(env, CPU_INTERRUPT_HARD);
+        }
+        return;
+    }
+
+    if (cpu_interrupts_enabled(env)) {
 
-    if (pil && (env->interrupt_index == 0 ||
-                (env->interrupt_index & ~15) == TT_EXTINT)) {
         unsigned int i;
 
-        for (i = 15; i > 0; i--) {
+        for (i = 15; i > env->psrpil; i--) {
             if (pil & (1 << i)) {
                 int old_interrupt = env->interrupt_index;
-
-                env->interrupt_index = TT_EXTINT | i;
-                if (old_interrupt != env->interrupt_index) {
-                    CPUIRQ_DPRINTF("Set CPU IRQ %d\n", i);
-                    cpu_interrupt(env, CPU_INTERRUPT_HARD);
-                }
+                int new_interrupt = TT_EXTINT | i;
+
+                    if (env->tl > 0 && cpu_tsptr(env)->tt > new_interrupt) {
+                        CPUIRQ_DPRINTF("Not setting CPU IRQ: TL=%d "
+                                "current %X >= pending %X\n",
+                                env->tl, cpu_tsptr(env)->tt, new_interrupt);
+                    }
+                    else if (old_interrupt != new_interrupt) {
+                        env->interrupt_index = new_interrupt;
+                        CPUIRQ_DPRINTF("Set CPU IRQ %d old=%X new=%X\n", i,
+                                old_interrupt, new_interrupt);
+                        cpu_interrupt(env, CPU_INTERRUPT_HARD);
+                    }
                 break;
             }
         }
-    } else if (!pil && (env->interrupt_index & ~15) == TT_EXTINT) {
-        CPUIRQ_DPRINTF("Reset CPU IRQ %d\n", env->interrupt_index & 15);
-        env->interrupt_index = 0;
-        cpu_reset_interrupt(env, CPU_INTERRUPT_HARD);
+    }
+    else {
+        CPUIRQ_DPRINTF("Interrupts disabled, pil=%08X pil_in=%08X softint=%08X "
+                "current interrupt %X\n",
+                pil, env->pil_in, env->softint, env->interrupt_index);
     }
 }