Skip to contentSkip to conversation

sparc64: tick timers

PATCH Applied Igor V. Kovalenko 3 participants 20 messages Jan 5, 2010 – Jan 7, 2010

lore
Share & tools
lore

Igor V. Kovalenko

author
#0 lore raw

cover sparc64: tick timers

The following series deals with tick timers for sparc64. It is not complete solution yet, comments are welcome.

First two changes are really debug helpers,
so are not strictly required.

Central part is addressing how traps are taken, which includes taking trap when PSTATE.IE is changed, PIL mask is changed, and SOFTINT is written to. I did not used CPU_INTERRUPT_TIMER at all, instead all interrupts are delivered with CPU_INTERRUPT_HARD.

Last change is reimplementing tick timers without periodic timer framework. sparc64 timers are essentially one-shot since they provide trap on tick counter match only so require reloading for next time period. Still it may be cleaner to fix sun4u use of periodic timers.

These changes allow recent linux kernel to start scheduler works, and count about 200 bogomips before crashing later.

---

Igor V. Kovalenko (9):
sparc64: trace pstate and global register set changes
sparc64: add PSR and PIL to cpu state dump
sparc64: use helper_wrpil to check pending irq on write
sparc64: check for pending irq when pil, pstate or softint is changed
sparc64: add macros to deal with softint and timer interrupt
sparc64: clear exception_index with -1 value
sparc64: move cpu_interrupts_enabled to cpu.h
sparc64: interrupt trap handling
sparc64: reimplement tick timers

 cpu-exec.c               |   40 ++++----
 hw/sun4u.c               |  234 +++++++++++++++++++++++++++++++++++++---------
 target-sparc/cpu.h       |   19 ++++
 target-sparc/exec.h      |   13 ---
 target-sparc/helper.c    |    1 
 target-sparc/helper.h    |    1 
 target-sparc/op_helper.c |   75 ++++++++++++++-
 target-sparc/translate.c |    5 -
 8 files changed, 304 insertions(+), 84 deletions(-)
Signature · 2 lines
Kind regards,
Igor V. Kovalenko
author
#1 lore raw

PATCH 1/9 sparc64: trace pstate and global register set changes

From: Igor V. Kovalenko <[email protected]>

Signed-off-by Igor V. Kovalenko <[email protected]>
---
 target-sparc/op_helper.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)
target-sparc/op_helper.c +20 -0
index dab2c25..26092e5 100644
@@ -11,6 +11,7 @@
 //#define DEBUG_UNASSIGNED
 //#define DEBUG_ASI
 //#define DEBUG_PCALL
+//#define DEBUG_PSTATE
 
 #ifdef DEBUG_MMU
 #define DPRINTF_MMU(fmt, ...)                                   \

@@ -31,6 +32,13 @@
     do { printf("ASI: " fmt , ## __VA_ARGS__); } while (0)
 #endif
 
+#ifdef DEBUG_PSTATE
+#define DPRINTF_PSTATE(fmt, ...)                                   \
+    do { printf("PSTATE: " fmt , ## __VA_ARGS__); } while (0)
+#else
+#define DPRINTF_PSTATE(fmt, ...) do {} while (0)
+#endif
+
 #ifdef TARGET_SPARC64
 #ifndef TARGET_ABI32
 #define AM_CHECK(env1) ((env1)->pstate & PS_AM)

@@ -3244,6 +3252,12 @@ static inline uint64_t *get_gregset(uint64_t pstate)
 {
     switch (pstate) {
     default:
+        DPRINTF_PSTATE("ERROR in get_gregset: active pstate bits=%lX %s %s %s",
+                pstate,
+                (pstate&PS_IG)?"IG":"",
+                (pstate&PS_MG)?"MG":"",
+                (pstate&PS_AG)?"AG":"" );
+        /* pass through to normal set of global registers */
     case 0:
         return env->bgregs;
     case PS_AG:

@@ -3269,12 +3283,18 @@ static inline void change_pstate(uint64_t new_pstate)
     new_pstate_regs = new_pstate & 0xc01;
 
     if (new_pstate_regs != pstate_regs) {
+        DPRINTF_PSTATE("change_pstate: switching regs old=%lX new=%lX\n",
+                       pstate_regs, new_pstate_regs);
         // Switch global register bank
         src = get_gregset(new_pstate_regs);
         dst = get_gregset(pstate_regs);
         memcpy32(dst, env->gregs);
         memcpy32(env->gregs, src);
     }
+    else {
+        DPRINTF_PSTATE("change_pstate: regs new=%lX (unchanged)\n",
+                       new_pstate_regs);
+    }
     env->pstate = new_pstate;
 }
#2 lore raw

Re: [Qemu-devel] [PATCH 1/9] sparc64: trace pstate and global register set changes

Quoted 36 lines · Igor V. Kovalenko

From: Igor V. Kovalenko <[email protected]>

Signed-off-by Igor V. Kovalenko <[email protected]>

---
 target-sparc/op_helper.c |   20 ++++++++++++++++++++

 1 files changed, 20 insertions(+), 0 deletions(-)
target-sparc/op_helper.c +8 -0
index dab2c25..26092e5 100644
@@ -11,6 +11,7 @@
  //#define DEBUG_UNASSIGNED
  //#define DEBUG_ASI
  //#define DEBUG_PCALL
+//#define DEBUG_PSTATE
 
  #ifdef DEBUG_MMU
  #define DPRINTF_MMU(fmt, ...)                                   \

@@ -31,6 +32,13 @@
     do { printf("ASI: " fmt , ## __VA_ARGS__); } while (0)
  #endif
 
+#ifdef DEBUG_PSTATE
+#define DPRINTF_PSTATE(fmt, ...)                                   \
+    do { printf("PSTATE: " fmt , ## __VA_ARGS__); } while (0)
+#else
+#define DPRINTF_PSTATE(fmt, ...) do {} while (0)
+#endif
+
  #ifdef TARGET_SPARC64
  #ifndef TARGET_ABI32
  #define AM_CHECK(env1) ((env1)->pstate & PS_AM)

@@ -3244,6 +3252,12 @@ static inline uint64_t *get_gregset(uint64_t pstate)
  {
     switch (pstate) {
Quoted 7 lines

    default:
+        DPRINTF_PSTATE("ERROR in get_gregset: active pstate bits=%lX %s %s %s",
+                pstate,
+                (pstate&PS_IG)?"IG":"",
+                (pstate&PS_MG)?"MG":"",
+                (pstate&PS_AG)?"AG":"" );
+        /* pass through to normal set of global registers */

%lX format is not correct on 64 bit hosts and nobody uses upper case hex, please use just %x. The spacing is too tight.

On UltraSPARC-IV and T1/2 the bits are interpreted differently (GL) so the printout would be wrong, but that can be disregarded for now.

target-sparc/op_helper.c static inline void change_pstate(uint64_t new_pstate) · line 3287

Quoted 9 lines

    case 0:
        return env->bgregs;
    case PS_AG:

target-sparc/op_helper.c +2 -0
@@ -3269,12 +3283,18 @@ static inline void change_pstate(uint64_t new_pstate)
     new_pstate_regs = new_pstate & 0xc01;
 
     if (new_pstate_regs != pstate_regs) {
+        DPRINTF_PSTATE("change_pstate: switching regs old=%lX new=%lX\n",
+                       pstate_regs, new_pstate_regs);

Same format issue here.

target-sparc/op_helper.c static inline void change_pstate(uint64_t new_pstate) · line 3297

Quoted 10 lines
target-sparc/op_helper.c +4 -0
         // Switch global register bank
         src = get_gregset(new_pstate_regs);
         dst = get_gregset(pstate_regs);
         memcpy32(dst, env->gregs);
         memcpy32(env->gregs, src);
     }
+    else {
+        DPRINTF_PSTATE("change_pstate: regs new=%lX (unchanged)\n",
+                       new_pstate_regs);
+    }

And here.

author
#3 lore raw

PATCH 2/9 sparc64: add PSR and PIL to cpu state dump

In reply to Igor V. Kovalenko

From: Igor V. Kovalenko <[email protected]>

Signed-off-by Igor V. Kovalenko <[email protected]>
---
 target-sparc/helper.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
target-sparc/helper.c +1 -0
index a06923a..0f0e583 100644
@@ -1452,6 +1452,7 @@ void cpu_dump_state(CPUState *env, FILE *f,
 #ifdef TARGET_SPARC64
     cpu_fprintf(f, "pstate: 0x%08x ccr: 0x%02x asi: 0x%02x tl: %d fprs: %d\n",
                 env->pstate, GET_CCR(env), env->asi, env->tl, env->fprs);
+    cpu_fprintf(f, "psr: 0x%08x pil=%x\n", GET_PSR(env), env->psrpil);
     cpu_fprintf(f, "cansave: %d canrestore: %d otherwin: %d wstate %d "
                 "cleanwin %d cwp %d\n",
                 env->cansave, env->canrestore, env->otherwin, env->wstate,
#4 lore raw

Re: [Qemu-devel] [PATCH 2/9] sparc64: add PSR and PIL to cpu state dump

Quoted 12 lines · Igor V. Kovalenko

From: Igor V. Kovalenko <[email protected]>

Signed-off-by Igor V. Kovalenko <[email protected]>

---
 target-sparc/helper.c |    1 +

 1 files changed, 1 insertions(+), 0 deletions(-)
target-sparc/helper.c +0 -0
index a06923a..0f0e583 100644

target-sparc/helper.c void cpu_dump_state(CPUState *env, FILE *f, · line 1455

Quoted 4 lines
target-sparc/helper.c +1 -0
@@ -1452,6 +1452,7 @@ void cpu_dump_state(CPUState *env, FILE *f,
  #ifdef TARGET_SPARC64
     cpu_fprintf(f, "pstate: 0x%08x ccr: 0x%02x asi: 0x%02x tl: %d fprs: %d\n",
                 env->pstate, GET_CCR(env), env->asi, env->tl, env->fprs);
+    cpu_fprintf(f, "psr: 0x%08x pil=%x\n", GET_PSR(env), env->psrpil);

PSR does not exist on Sparc64, instead we have separate registers, like VER, CWP and CCR. PIL part is OK.

author
#5 lore raw

PATCH 3/9 sparc64: use helper_wrpil to check pending irq on write

In reply to Igor V. Kovalenko

From: Igor V. Kovalenko <[email protected]>

Signed-off-by Igor V. Kovalenko <[email protected]>
---
 target-sparc/helper.h    |    1 +
 target-sparc/op_helper.c |   14 ++++++++++++++
 target-sparc/translate.c |    5 +----
 3 files changed, 16 insertions(+), 4 deletions(-)
target-sparc/helper.h +1 -0
index 4002b9e..6f103e7 100644
@@ -5,6 +5,7 @@ DEF_HELPER_0(rett, void)
 DEF_HELPER_1(wrpsr, void, tl)
 DEF_HELPER_0(rdpsr, tl)
 #else
+DEF_HELPER_1(wrpil, void, tl)
 DEF_HELPER_1(wrpstate, void, tl)
 DEF_HELPER_0(done, void)
 DEF_HELPER_0(retry, void)
target-sparc/op_helper.c +14 -0
index 26092e5..a7da0e4 100644
@@ -3303,6 +3303,20 @@ void helper_wrpstate(target_ulong new_state)
     change_pstate(new_state & 0xf3f);
 }
 
+void helper_wrpil(target_ulong new_pil)
+{
+    DPRINTF_PSTATE("helper_wrpil old=%X new=%X\n",
+                   env->psrpil, (uint32_t)new_pil);
+
+    env->psrpil = new_pil;
+
+#if !defined(CONFIG_USER_ONLY)
+    if (env->pstate & PS_IE) {
+        cpu_check_irqs(env);
+    }
+#endif
+}
+
 void helper_done(void)
 {
     trap_state* tsptr = cpu_tsptr(env);
target-sparc/translate.c +1 -4
index bf6df50..7e9f0cf 100644
@@ -3371,10 +3371,7 @@ static void disas_sparc_insn(DisasContext * dc)
                                                offsetof(CPUSPARCState, tl));
                                 break;
                             case 8: // pil
-                                tcg_gen_trunc_tl_i32(cpu_tmp32, cpu_tmp0);
-                                tcg_gen_st_i32(cpu_tmp32, cpu_env,
-                                               offsetof(CPUSPARCState,
-                                                        psrpil));
+                                gen_helper_wrpil(cpu_tmp0);
                                 break;
                             case 9: // cwp
                                 gen_helper_wrcwp(cpu_tmp0);
#6 lore raw

Re: [Qemu-devel] [PATCH 3/9] sparc64: use helper_wrpil to check pending irq on write

Quoted 32 lines · Igor V. Kovalenko

From: Igor V. Kovalenko <[email protected]>

Signed-off-by Igor V. Kovalenko <[email protected]>

---
 target-sparc/helper.h    |    1 +
 target-sparc/op_helper.c |   14 ++++++++++++++
 target-sparc/translate.c |    5 +----

 3 files changed, 16 insertions(+), 4 deletions(-)
target-sparc/helper.h +1 -0
index 4002b9e..6f103e7 100644
@@ -5,6 +5,7 @@ DEF_HELPER_0(rett, void)
  DEF_HELPER_1(wrpsr, void, tl)
  DEF_HELPER_0(rdpsr, tl)
  #else
+DEF_HELPER_1(wrpil, void, tl)
  DEF_HELPER_1(wrpstate, void, tl)
  DEF_HELPER_0(done, void)
  DEF_HELPER_0(retry, void)
target-sparc/op_helper.c +3 -0
index 26092e5..a7da0e4 100644
@@ -3303,6 +3303,20 @@ void helper_wrpstate(target_ulong new_state)
     change_pstate(new_state & 0xf3f);
  }
 
+void helper_wrpil(target_ulong new_pil)
+{
+    DPRINTF_PSTATE("helper_wrpil old=%X new=%X\n",

target-sparc/op_helper.c void helper_wrpstate(target_ulong new_state) · line 3318

Quoted 10 lines
target-sparc/op_helper.c +10 -0
+                   env->psrpil, (uint32_t)new_pil);
+
+    env->psrpil = new_pil;
+
+#if !defined(CONFIG_USER_ONLY)
+    if (env->pstate & PS_IE) {
+        cpu_check_irqs(env);
+    }
+#endif
+}

It's not possible to write to PIL in user mode, so the whole code inside the function should be surrounded by #if !defined(CONFIG_USER_ONLY)/#endif. Please use lowercase hex.

I'm wondering about using target_ulong instead of uint32_t (type of env->psrpil). TL makes the generated code shorter, uint32_t would decrease TCG register pressure on 32 bit hosts. This is not performance critical, so either way should be OK.

author
#7 lore raw

PATCH 4/9 sparc64: check for pending irq when pil, pstate or softint is changed

In reply to Igor V. Kovalenko

From: Igor V. Kovalenko <[email protected]>

Signed-off-by Igor V. Kovalenko <[email protected]>
---
 target-sparc/op_helper.c |   39 ++++++++++++++++++++++++++++++++++++---
 1 files changed, 36 insertions(+), 3 deletions(-)
target-sparc/op_helper.c +36 -3
index a7da0e4..b1978cb 100644
@@ -3301,6 +3301,12 @@ static inline void change_pstate(uint64_t new_pstate)
 void helper_wrpstate(target_ulong new_state)
 {
     change_pstate(new_state & 0xf3f);
+
+#if !defined(CONFIG_USER_ONLY)
+    if (env->pstate & PS_IE) {
+        cpu_check_irqs(env);
+    }
+#endif
 }
 
 void helper_wrpil(target_ulong new_pil)

@@ -3328,6 +3334,14 @@ void helper_done(void)
     change_pstate((tsptr->tstate >> 8) & 0xf3f);
     PUT_CWP64(env, tsptr->tstate & 0xff);
     env->tl--;
+
+    DPRINTF_PSTATE("... helper_done tl=%d\n", env->tl);
+
+#if !defined(CONFIG_USER_ONLY)
+    if (env->pstate & PS_IE) {
+        cpu_check_irqs(env);
+    }
+#endif
 }
 
 void helper_retry(void)

@@ -3341,21 +3355,40 @@ void helper_retry(void)
     change_pstate((tsptr->tstate >> 8) & 0xf3f);
     PUT_CWP64(env, tsptr->tstate & 0xff);
     env->tl--;
+
+    DPRINTF_PSTATE("... helper_retry tl=%d\n", env->tl);
+
+#if !defined(CONFIG_USER_ONLY)
+    if (env->pstate & PS_IE) {
+        cpu_check_irqs(env);
+    }
+#endif
+}
+
+static void do_modify_softint(const char* operation, uint32_t value)
+{
+    if (env->softint != value) {
+        env->softint = value;
+        DPRINTF_PSTATE(": %s new %08X\n", operation, env->softint);
+#if !defined(CONFIG_USER_ONLY)
+        cpu_check_irqs(env);
+#endif
+    }
 }
 
 void helper_set_softint(uint64_t value)
 {
-    env->softint |= (uint32_t)value;
+    do_modify_softint("helper_set_softint" , env->softint | (uint32_t)value);
 }
 
 void helper_clear_softint(uint64_t value)
 {
-    env->softint &= (uint32_t)~value;
+    do_modify_softint("helper_clear_softint" , env->softint & (uint32_t)~value);
 }
 
 void helper_write_softint(uint64_t value)
 {
-    env->softint = (uint32_t)value;
+    do_modify_softint("helper_write_softint", (uint32_t)value);
 }
 #endif
#8 lore raw

Re: [Qemu-devel] [PATCH 4/9] sparc64: check for pending irq when pil, pstate or softint is changed

Quoted 48 lines · Igor V. Kovalenko

From: Igor V. Kovalenko <[email protected]>

Signed-off-by Igor V. Kovalenko <[email protected]>

---
 target-sparc/op_helper.c |   39 ++++++++++++++++++++++++++++++++++++---

 1 files changed, 36 insertions(+), 3 deletions(-)
target-sparc/op_helper.c +19 -0
index a7da0e4..b1978cb 100644
@@ -3301,6 +3301,12 @@ static inline void change_pstate(uint64_t new_pstate)
  void helper_wrpstate(target_ulong new_state)
  {
     change_pstate(new_state & 0xf3f);
+
+#if !defined(CONFIG_USER_ONLY)
+    if (env->pstate & PS_IE) {
+        cpu_check_irqs(env);
+    }
+#endif
  }
 
  void helper_wrpil(target_ulong new_pil)

@@ -3328,6 +3334,14 @@ void helper_done(void)
     change_pstate((tsptr->tstate >> 8) & 0xf3f);
     PUT_CWP64(env, tsptr->tstate & 0xff);
     env->tl--;
+
+    DPRINTF_PSTATE("... helper_done tl=%d\n", env->tl);
+
+#if !defined(CONFIG_USER_ONLY)
+    if (env->pstate & PS_IE) {
+        cpu_check_irqs(env);
+    }
+#endif
  }
 
  void helper_retry(void)

@@ -3341,21 +3355,40 @@ void helper_retry(void)
     change_pstate((tsptr->tstate >> 8) & 0xf3f);
     PUT_CWP64(env, tsptr->tstate & 0xff);
     env->tl--;
+
+    DPRINTF_PSTATE("... helper_retry tl=%d\n", env->tl);
+
+#if !defined(CONFIG_USER_ONLY)
+    if (env->pstate & PS_IE) {

target-sparc/op_helper.c void helper_retry(void) · line 3372

Quoted 10 lines
target-sparc/op_helper.c +10 -0
+        cpu_check_irqs(env);
+    }
+#endif
+}
+
+static void do_modify_softint(const char* operation, uint32_t value)
+{
+    if (env->softint != value) {
+        env->softint = value;
+        DPRINTF_PSTATE(": %s new %08X\n", operation, env->softint);

Uppercase hex again. Otherwise the patch looks OK.

Maybe the #if CONFIG_USER_ONLY/#endif could be avoided if cpu_check_irqs would be #defined as a dummy macro for CONFIG_USER_ONLY in cpu.h.

author
#9 lore raw

PATCH 5/9 sparc64: add macros to deal with softint and timer interrupt

In reply to Igor V. Kovalenko

From: Igor V. Kovalenko <[email protected]>

Signed-off-by Igor V. Kovalenko <[email protected]>
---
 target-sparc/cpu.h |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
target-sparc/cpu.h +6 -0
index 1fe4d0f..0dba241 100644
@@ -394,6 +394,8 @@ typedef struct CPUSPARCState {
     uint64_t fprs;
     uint64_t tick_cmpr, stick_cmpr;
     void *tick, *stick;
+#define TICK_NPT_MASK        0x8000000000000000ULL
+#define TICK_SOFTINT_DISABLE 0x8000000000000000ULL
     uint64_t gsr;
     uint32_t gl; // UA2005
     /* UA 2005 hyperprivileged registers */

@@ -402,6 +404,10 @@ typedef struct CPUSPARCState {
     uint32_t softint;
 #define SOFTINT_TIMER   1
 #define SOFTINT_STIMER  (1 << 16)
+#define SOFTINT_INTRMASK (0xFFFE)
+#define SOFTINT_TM       (1 << 0)
+#define SOFTINT_SM       (1 << 16)
+#define SOFTINT_REG_MASK (SOFTINT_SM|SOFTINT_INTRMASK|SOFTINT_TM)
 #endif
     sparc_def_t *def;
 } CPUSPARCState;
#10 lore raw

Re: [Qemu-devel] [PATCH 5/9] sparc64: add macros to deal with softint and timer interrupt

Quoted 12 lines · Igor V. Kovalenko

From: Igor V. Kovalenko <[email protected]>

Signed-off-by Igor V. Kovalenko <[email protected]>

---
 target-sparc/cpu.h |    6 ++++++

 1 files changed, 6 insertions(+), 0 deletions(-)
target-sparc/cpu.h +0 -0
index 1fe4d0f..0dba241 100644

target-sparc/cpu.h typedef struct CPUSPARCState { · line 398

Quoted 5 lines
target-sparc/cpu.h +2 -0
@@ -394,6 +394,8 @@ typedef struct CPUSPARCState {
     uint64_t fprs;
     uint64_t tick_cmpr, stick_cmpr;
     void *tick, *stick;
+#define TICK_NPT_MASK        0x8000000000000000ULL
+#define TICK_SOFTINT_DISABLE 0x8000000000000000ULL

Please move the TICK_NPT and TICK_INT_DIS macros from sun4u.c to here.

target-sparc/cpu.h typedef struct CPUSPARCState { · line 409

Quoted 10 lines
target-sparc/cpu.h +0 -0
     uint64_t gsr;

    uint32_t gl; // UA2005
    /* UA 2005 hyperprivileged registers */

target-sparc/cpu.h +3 -0
@@ -402,6 +404,10 @@ typedef struct CPUSPARCState {
     uint32_t softint;
  #define SOFTINT_TIMER   1
  #define SOFTINT_STIMER  (1 << 16)
+#define SOFTINT_INTRMASK (0xFFFE)
+#define SOFTINT_TM       (1 << 0)
+#define SOFTINT_SM       (1 << 16)

Why the duplicate definitions?

Quoted 8 lines
target-sparc/cpu.h +1 -0
+#define SOFTINT_REG_MASK (SOFTINT_SM|SOFTINT_INTRMASK|SOFTINT_TM)
  #endif
     sparc_def_t *def;
  } CPUSPARCState;
author
#11 lore raw

PATCH 6/9 sparc64: clear exception_index with -1 value

In reply to Igor V. Kovalenko

From: Igor V. Kovalenko <[email protected]>

Signed-off-by Igor V. Kovalenko <[email protected]>
---
 target-sparc/op_helper.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
target-sparc/op_helper.c +1 -1
index b1978cb..94f1c7a 100644
@@ -3535,7 +3535,7 @@ void do_interrupt(CPUState *env)
     env->tbr |= ((env->tl > 1) ? 1 << 14 : 0) | (intno << 5);
     env->pc = env->tbr;
     env->npc = env->pc + 4;
-    env->exception_index = 0;
+    env->exception_index = -1;
 }
 #else
 #ifdef DEBUG_PCALL
Applied
#12 lore raw

Re: [Qemu-devel] [PATCH 6/9] sparc64: clear exception_index with -1 value

Thanks, applied.

Quoted 24 lines · Igor V. Kovalenko

From: Igor V. Kovalenko <[email protected]>

Signed-off-by Igor V. Kovalenko <[email protected]>

---
 target-sparc/op_helper.c |    2 +-

 1 files changed, 1 insertions(+), 1 deletions(-)
target-sparc/op_helper.c +1 -1
index b1978cb..94f1c7a 100644
@@ -3535,7 +3535,7 @@ void do_interrupt(CPUState *env)
     env->tbr |= ((env->tl > 1) ? 1 << 14 : 0) | (intno << 5);
     env->pc = env->tbr;
     env->npc = env->pc + 4;
-    env->exception_index = 0;
+    env->exception_index = -1;
  }
  #else
  #ifdef DEBUG_PCALL
#13 lore raw

Re: [Qemu-devel] [PATCH 6/9] sparc64: clear exception_index with -1 value

What's the effect of the patch? Don't we need it for sparc32 too? The code looks similar.

2010/1/6 Blue Swirl <[email protected]>:

Quoted 31 lines

Thanks, applied.

Quoted 24 lines · Igor V. Kovalenko

From: Igor V. Kovalenko <[email protected]>

Signed-off-by Igor V. Kovalenko <[email protected]>

---
 target-sparc/op_helper.c |    2 +-

 1 files changed, 1 insertions(+), 1 deletions(-)
target-sparc/op_helper.c +1 -1
index b1978cb..94f1c7a 100644
@@ -3535,7 +3535,7 @@ void do_interrupt(CPUState *env)
     env->tbr |= ((env->tl > 1) ? 1 << 14 : 0) | (intno << 5);
     env->pc = env->tbr;
     env->npc = env->pc + 4;
-    env->exception_index = 0;
+    env->exception_index = -1;
  }
  #else
  #ifdef DEBUG_PCALL
Signature · 4 lines
Regards,
Artyom Tarasenko

solaris/sparc under qemu blog: http://tyom.blogspot.com/
author
#14 lore raw

Re: [Qemu-devel] [PATCH 6/9] sparc64: clear exception_index with -1 value

Quoted 15 lines · Artyom Tarasenko

What's the effect of the patch? Don't we need it for sparc32 too? The code looks similar.

2010/1/6 Blue Swirl <[email protected]>:

Quoted 11 lines

Thanks, applied.

Quoted 7 lines · Igor V. Kovalenko

From: Igor V. Kovalenko <[email protected]>

Signed-off-by Igor V. Kovalenko <[email protected]>

---
 target-sparc/op_helper.c |    2 +-

 1 files changed, 1 insertions(+), 1 deletions(-)
Quoted 13 lines
Quoted 5 lines
Quoted 5 lines
target-sparc/op_helper.c +0 -0
index b1978cb..94f1c7a 100644
@@ -3535,7 +3535,7 @@ void do_interrupt(CPUState *env)
Quoted 8 lines

target-sparc/op_helper.c void do_interrupt(CPUState *env) · line 3541

Quoted 8 lines
target-sparc/op_helper.c +1 -1
     env->tbr |= ((env->tl > 1) ? 1 << 14 : 0) | (intno << 5);
     env->pc = env->tbr;
     env->npc = env->pc + 4;
-    env->exception_index = 0;
+    env->exception_index = -1;
  }
  #else
  #ifdef DEBUG_PCALL

Right, but that's out of scope for this changeset.

In fact sparc32 should have the same issue, as well as alpha, s390 and sh4. Check in cpu_exec() for valid exception_index is "greater or equal to zero" so all those targets should be corrected. On sh4 we even check for "invalid value of exception_index is -1"

Signature · 2 lines
Kind regards,
Igor V. Kovalenko
#15 lore raw

Re: [Qemu-devel] [PATCH 6/9] sparc64: clear exception_index with -1 value

Quoted 33 lines · Igor Kovalenko
Quoted 15 lines · Artyom Tarasenko

What's the effect of the patch? Don't we need it for sparc32 too? The code looks similar.

2010/1/6 Blue Swirl <[email protected]>:

Quoted 11 lines

Thanks, applied.

Quoted 7 lines · Igor V. Kovalenko

From: Igor V. Kovalenko <[email protected]>

Signed-off-by Igor V. Kovalenko <[email protected]>

---
 target-sparc/op_helper.c |    2 +-

 1 files changed, 1 insertions(+), 1 deletions(-)
Quoted 13 lines
Quoted 5 lines
Quoted 5 lines
target-sparc/op_helper.c +0 -0
index b1978cb..94f1c7a 100644
@@ -3535,7 +3535,7 @@ void do_interrupt(CPUState *env)
Quoted 8 lines

target-sparc/op_helper.c void do_interrupt(CPUState *env) · line 3541

Quoted 8 lines
target-sparc/op_helper.c +1 -1
     env->tbr |= ((env->tl > 1) ? 1 << 14 : 0) | (intno << 5);
     env->pc = env->tbr;
     env->npc = env->pc + 4;
-    env->exception_index = 0;
+    env->exception_index = -1;
  }
  #else
  #ifdef DEBUG_PCALL

Right, but that's out of scope for this changeset.

Quoted 4 lines

In fact sparc32 should have the same issue, as well as alpha, s390 and sh4. Check in cpu_exec() for valid exception_index is "greater or equal to zero" so all those targets should be corrected. On sh4 we even check for "invalid value of exception_index is -1"

Thanks, I applied the fix for Sparc32. Here's also an untested fix for other architectures.

From c93e1ac4b8a862fc1a58cdb33bb07a51c5b30733 Mon Sep 17 00:00:00 2001
From: Blue Swirl <[email protected]>
Date: Thu, 7 Jan 2010 20:02:12 +0000
Subject: [PATCH] Fix incorrect exception_index use

env->exception_index should be cleared with -1, not 0.

See also 821b19fe923ac49a24cdb4af902584fdd019cee6.

Spotted by Igor Kovalenko.

Signed-off-by Blue Swirl <[email protected]>
---
 target-alpha/helper.c |    2 +-
 target-s390x/kvm.c    |    4 ++--
 target-sh4/helper.c   |    2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)
target-alpha/helper.c +1 -1
index be7d37b..1e0bc4a 100644
@@ -467,7 +467,7 @@ void do_interrupt (CPUState *env)
 
     env->ipr[IPR_EXC_ADDR] = env->pc | 1;
     excp = env->exception_index;
-    env->exception_index = 0;
+    env->exception_index = -1;
     env->error_code = 0;
     /* XXX: disable interrupts and memory mapping */
     if (env->ipr[IPR_PAL_BASE] != -1ULL) {
target-s390x/kvm.c +2 -2
index 0992563..0199a65 100644
@@ -186,7 +186,7 @@ static void kvm_s390_interrupt_internal(CPUState *env, int type, uint32_t parm,
     }
 
     env->halted = 0;
-    env->exception_index = 0;
+    env->exception_index = -1;
 
     kvmint.type = type;
     kvmint.parm = parm;

@@ -325,7 +325,7 @@ static int s390_cpu_restart(CPUState *env)
 {
     kvm_s390_interrupt(env, KVM_S390_RESTART, 0);
     env->halted = 0;
-    env->exception_index = 0;
+    env->exception_index = -1;
     qemu_cpu_kick(env);
     dprintf("DONE: SIGP cpu restart: %p\n", env);
     return 0;
target-sh4/helper.c +1 -1
index 088d36a..8c94564 100644
@@ -38,7 +38,7 @@ int cpu_sh4_handle_mmu_fault(CPUState * env, target_ulong address, int rw,
 			     int mmu_idx, int is_softmmu)
 {
     env->tea = address;
-    env->exception_index = 0;
+    env->exception_index = -1;
     switch (rw) {
     case 0:
         env->exception_index = 0x0a0;
Signature · 1 line
1.5.6.5
author
#16 lore raw

PATCH 7/9 sparc64: move cpu_interrupts_enabled to cpu.h

In reply to Igor V. Kovalenko

From: Igor V. Kovalenko <[email protected]>

- to be used by cpu_check_irqs

Signed-off-by Igor V. Kovalenko <[email protected]>
---
 target-sparc/cpu.h  |   13 +++++++++++++
 target-sparc/exec.h |   13 -------------
 2 files changed, 13 insertions(+), 13 deletions(-)
target-sparc/cpu.h +13 -0
index 0dba241..f1584d4 100644
@@ -566,6 +566,19 @@ static inline int cpu_mmu_index(CPUState *env1)
 #endif
 }
 
+static inline int cpu_interrupts_enabled(CPUState *env1)
+{
+#if !defined (TARGET_SPARC64)
+    if (env1->psret != 0)
+        return 1;
+#else
+    if (env1->pstate & PS_IE)
+        return 1;
+#endif
+
+    return 0;
+}
+
 static inline int cpu_fpu_enabled(CPUState *env1)
 {
 #if defined(CONFIG_USER_ONLY)
target-sparc/exec.h +0 -13
index e120d6f..3e021e9 100644
@@ -24,19 +24,6 @@ static inline void regs_to_env(void)
 /* op_helper.c */
 void do_interrupt(CPUState *env);
 
-static inline int cpu_interrupts_enabled(CPUState *env1)
-{
-#if !defined (TARGET_SPARC64)
-    if (env1->psret != 0)
-        return 1;
-#else
-    if (env1->pstate & PS_IE)
-        return 1;
-#endif
-
-    return 0;
-}
-
 static inline int cpu_has_work(CPUState *env1)
 {
     return (env1->interrupt_request & CPU_INTERRUPT_HARD) &&
author
#17 lore raw

PATCH 8/9 sparc64: interrupt trap handling

In reply to Igor V. Kovalenko

From: Igor V. Kovalenko <[email protected]>

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 <[email protected]>
---
 cpu-exec.c |   40 ++++++++++++++++++++++------------------
 hw/sun4u.c |   52 +++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 59 insertions(+), 33 deletions(-)
cpu-exec.c +22 -18
index af4595b..65192c1 100644
@@ -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)) {
hw/sun4u.c +37 -15

66 lines not on this page · raw message

#18 lore raw

Re: [Qemu-devel] [PATCH 8/9] sparc64: interrupt trap handling

Quoted 40 lines · Igor V. Kovalenko

From: Igor V. Kovalenko <[email protected]>

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 <[email protected]>

---
 cpu-exec.c |   40 ++++++++++++++++++++++------------------ hw/sun4u.c |   52 +++++++++++++++++++++++++++++++++++++---------------

 2 files changed, 59 insertions(+), 33 deletions(-)
cpu-exec.c +0 -15
index af4595b..65192c1 100644
@@ -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) {

cpu-exec.c int cpu_exec(CPUState *env1) · line 458

Quoted 10 lines
cpu-exec.c +7 -3
-                       //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.

cpu-exec.c int cpu_exec(CPUState *env1) · line 459

Quoted 1 line
cpu-exec.c +1 -0
+                                //env->interrupt_request &= ~CPU_INTERRUPT_HARD;

Remove.

cpu-exec.c int cpu_exec(CPUState *env1) · line 468

Quoted 9 lines
cpu-exec.c +9 -0
+                                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 '}'.

Quoted 21 lines
cpu-exec.c +5 -0
+                        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)) {

hw/sun4u.c +3 -2
index 9d46f08..84a8043 100644
@@ -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

hw/sun4u.c void irq_info(Monitor *mon) · line 248

Quoted 10 lines
hw/sun4u.c +10 -0
+       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.

Quoted 18 lines
hw/sun4u.c +6 -7
+        }
+        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);

hw/sun4u.c void irq_info(Monitor *mon) · line 267

Quoted 10 lines
hw/sun4u.c +8 -2
-                    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 '}'.

Quoted 9 lines
hw/sun4u.c +5 -1
+                        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) {

hw/sun4u.c void irq_info(Monitor *mon) · line 282

Quoted 10 lines
hw/sun4u.c +5 -3
-        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.

author
#19 lore raw

Re: [Qemu-devel] [PATCH 8/9] sparc64: interrupt trap handling

Quoted 46 lines · Blue Swirl
Quoted 44 lines · Igor V. Kovalenko

From: Igor V. Kovalenko <[email protected]>

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 <[email protected]>

---
 cpu-exec.c |   40 ++++++++++++++++++++++------------------ hw/sun4u.c |   52 +++++++++++++++++++++++++++++++++++++---------------

 2 files changed, 59 insertions(+), 33 deletions(-)
cpu-exec.c +1 -18
index af4595b..65192c1 100644
@@ -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)) {
Quoted 8 lines

cpu-exec.c int cpu_exec(CPUState *env1) · line 458

Quoted 6 lines
cpu-exec.c +6 -0
+                        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.

Quoted 17 lines
Quoted 17 lines
hw/sun4u.c +7 -2
index 9d46f08..84a8043 100644
@@ -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;
+
Quoted 8 lines

hw/sun4u.c void irq_info(Monitor *mon) · line 248

Quoted 6 lines
hw/sun4u.c +6 -0
+    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.

Quoted 2 lines

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.

Signature · 2 lines
Kind regards,
Igor V. Kovalenko