[Qemu-devel] [PATCH v2] PPC: Clean up DECR implementation

[Qemu-devel] [PATCH v2] PPC: Clean up DECR implementation

[Alexander Graf]

There are 3 different variants of the decrementor for BookE and BookS.

The BookE variant sets TSR[DIS] to 1 when the DEC value becomes 1 or 0. TSR[DIS]
is then the indicator whether the decrementor interrupt line is asserted or not.

The old BookS variant treats DEC as an edge interrupt that gets triggered when
the DEC value's top bit turns 1 from 0.

The new BookS variant maintains the assertion bit inside DEC itself. Whenever
the DEC value becomes negative (top bit set) the DEC interrupt line is asserted.

So far we implemented mostly the old BookS variant. Let's do them all properly.

This fixes booting pseries ppc64 guest images in TCG mode for me.

Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - Use timer callback to trigger IRQ, it gets overridden by e500
---
 hw/ppc/ppc.c             | 92 +++++++++++++++++++++++++++++++++---------------
 include/hw/ppc/ppc.h     |  3 ++
 target-ppc/cpu.h         |  1 +
 target-ppc/excp_helper.c |  5 +--
 4 files changed, 71 insertions(+), 30 deletions(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 9c2a132..71df471 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -620,6 +620,13 @@ static void cpu_ppc_tb_start (CPUPPCState *env)
     }
 }
 
+bool ppc_decr_clear_on_delivery(CPUPPCState *env)
+{
+    ppc_tb_t *tb_env = env->tb_env;
+    int flags = PPC_DECR_UNDERFLOW_TRIGGERED | PPC_DECR_UNDERFLOW_LEVEL;
+    return ((tb_env->flags & flags) == PPC_DECR_UNDERFLOW_TRIGGERED);
+}
+
 static inline uint32_t _cpu_ppc_load_decr(CPUPPCState *env, uint64_t next)
 {
     ppc_tb_t *tb_env = env->tb_env;
@@ -677,6 +684,11 @@ static inline void cpu_ppc_decr_excp(PowerPCCPU *cpu)
     ppc_set_irq(cpu, PPC_INTERRUPT_DECR, 1);
 }
 
+static inline void cpu_ppc_decr_lower(PowerPCCPU *cpu)
+{
+    ppc_set_irq(cpu, PPC_INTERRUPT_DECR, 0);
+}
+
 static inline void cpu_ppc_hdecr_excp(PowerPCCPU *cpu)
 {
     /* Raise it */
@@ -684,11 +696,16 @@ static inline void cpu_ppc_hdecr_excp(PowerPCCPU *cpu)
     ppc_set_irq(cpu, PPC_INTERRUPT_HDECR, 1);
 }
 
+static inline void cpu_ppc_hdecr_lower(PowerPCCPU *cpu)
+{
+    ppc_set_irq(cpu, PPC_INTERRUPT_HDECR, 0);
+}
+
 static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
                                  QEMUTimer *timer,
-                                 void (*raise_excp)(PowerPCCPU *),
-                                 uint32_t decr, uint32_t value,
-                                 int is_excp)
+                                 void (*raise_excp)(void *),
+                                 void (*lower_excp)(PowerPCCPU *),
+                                 uint32_t decr, uint32_t value)
 {
     CPUPPCState *env = &cpu->env;
     ppc_tb_t *tb_env = env->tb_env;
@@ -702,59 +719,74 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
         return;
     }
 
-    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-    next = now + muldiv64(value, get_ticks_per_sec(), tb_env->decr_freq);
-    if (is_excp) {
-        next += *nextp - now;
+    /*
+     * Going from 2 -> 1, 1 -> 0 or 0 -> -1 is the event to generate a DEC
+     * interrupt.
+     *
+     * If we get a really small DEC value, we can assume that by the time we
+     * handled it we should inject an interrupt already.
+     *
+     * On MSB level based DEC implementations the MSB always means the interrupt
+     * is pending, so raise it on those.
+     *
+     * On MSB edge based DEC implementations the MSB going from 0 -> 1 triggers
+     * an edge interrupt, so raise it here too.
+     */
+    if ((value < 3) ||
+        ((tb_env->flags & PPC_DECR_UNDERFLOW_LEVEL) && (value & 0x80000000)) ||
+        ((tb_env->flags & PPC_DECR_UNDERFLOW_TRIGGERED) && (value & 0x80000000)
+          && !(decr & 0x80000000))) {
+        (*raise_excp)(cpu);
+        return;
     }
-    if (next == now) {
-        next++;
+
+    /* On MSB level based systems a 0 for the MSB stops interrupt delivery */
+    if (!(value & 0x80000000) && (tb_env->flags & PPC_DECR_UNDERFLOW_LEVEL)) {
+        (*lower_excp)(cpu);
     }
+
+    /* Calculate the next timer event */
+    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    next = now + muldiv64(value, get_ticks_per_sec(), tb_env->decr_freq);
     *nextp = next;
+
     /* Adjust timer */
     timer_mod(timer, next);
-
-    /* If we set a negative value and the decrementer was positive, raise an
-     * exception.
-     */
-    if ((tb_env->flags & PPC_DECR_UNDERFLOW_TRIGGERED)
-        && (value & 0x80000000)
-        && !(decr & 0x80000000)) {
-        (*raise_excp)(cpu);
-    }
 }
 
 static inline void _cpu_ppc_store_decr(PowerPCCPU *cpu, uint32_t decr,
-                                       uint32_t value, int is_excp)
+                                       uint32_t value)
 {
     ppc_tb_t *tb_env = cpu->env.tb_env;
 
     __cpu_ppc_store_decr(cpu, &tb_env->decr_next, tb_env->decr_timer,
-                         &cpu_ppc_decr_excp, decr, value, is_excp);
+                         tb_env->decr_timer->cb, &cpu_ppc_decr_lower, decr,
+                         value);
 }
 
 void cpu_ppc_store_decr (CPUPPCState *env, uint32_t value)
 {
     PowerPCCPU *cpu = ppc_env_get_cpu(env);
 
-    _cpu_ppc_store_decr(cpu, cpu_ppc_load_decr(env), value, 0);
+    _cpu_ppc_store_decr(cpu, cpu_ppc_load_decr(env), value);
 }
 
 static void cpu_ppc_decr_cb(void *opaque)
 {
     PowerPCCPU *cpu = opaque;
 
-    _cpu_ppc_store_decr(cpu, 0x00000000, 0xFFFFFFFF, 1);
+    cpu_ppc_decr_excp(cpu);
 }
 
 static inline void _cpu_ppc_store_hdecr(PowerPCCPU *cpu, uint32_t hdecr,
-                                        uint32_t value, int is_excp)
+                                        uint32_t value)
 {
     ppc_tb_t *tb_env = cpu->env.tb_env;
 
     if (tb_env->hdecr_timer != NULL) {
         __cpu_ppc_store_decr(cpu, &tb_env->hdecr_next, tb_env->hdecr_timer,
-                             &cpu_ppc_hdecr_excp, hdecr, value, is_excp);
+                             tb_env->hdecr_timer->cb, &cpu_ppc_hdecr_lower,
+                             hdecr, value);
     }
 }
 
@@ -762,14 +794,14 @@ void cpu_ppc_store_hdecr (CPUPPCState *env, uint32_t value)
 {
     PowerPCCPU *cpu = ppc_env_get_cpu(env);
 
-    _cpu_ppc_store_hdecr(cpu, cpu_ppc_load_hdecr(env), value, 0);
+    _cpu_ppc_store_hdecr(cpu, cpu_ppc_load_hdecr(env), value);
 }
 
 static void cpu_ppc_hdecr_cb(void *opaque)
 {
     PowerPCCPU *cpu = opaque;
 
-    _cpu_ppc_store_hdecr(cpu, 0x00000000, 0xFFFFFFFF, 1);
+    cpu_ppc_hdecr_excp(cpu);
 }
 
 static void cpu_ppc_store_purr(PowerPCCPU *cpu, uint64_t value)
@@ -792,8 +824,8 @@ static void cpu_ppc_set_tb_clk (void *opaque, uint32_t freq)
      * if a decrementer exception is pending when it enables msr_ee at startup,
      * it's not ready to handle it...
      */
-    _cpu_ppc_store_decr(cpu, 0xFFFFFFFF, 0xFFFFFFFF, 0);
-    _cpu_ppc_store_hdecr(cpu, 0xFFFFFFFF, 0xFFFFFFFF, 0);
+    _cpu_ppc_store_decr(cpu, 0xFFFFFFFF, 0xFFFFFFFF);
+    _cpu_ppc_store_hdecr(cpu, 0xFFFFFFFF, 0xFFFFFFFF);
     cpu_ppc_store_purr(cpu, 0x0000000000000000ULL);
 }
 
@@ -806,6 +838,10 @@ clk_setup_cb cpu_ppc_tb_init (CPUPPCState *env, uint32_t freq)
     tb_env = g_malloc0(sizeof(ppc_tb_t));
     env->tb_env = tb_env;
     tb_env->flags = PPC_DECR_UNDERFLOW_TRIGGERED;
+    if (env->insns_flags & PPC_SEGMENT_64B) {
+        /* All Book3S 64bit CPUs implement level based DEC logic */
+        tb_env->flags |= PPC_DECR_UNDERFLOW_LEVEL;
+    }
     /* Create new timer */
     tb_env->decr_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &cpu_ppc_decr_cb, cpu);
     if (0) {
diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
index 835418a..d71bd07 100644
--- a/include/hw/ppc/ppc.h
+++ b/include/hw/ppc/ppc.h
@@ -44,6 +44,9 @@ struct ppc_tb_t {
 #define PPC_DECR_ZERO_TRIGGERED      (1 << 3) /* Decr interrupt triggered when
                                                * the decrementer reaches zero.
                                                */
+#define PPC_DECR_UNDERFLOW_LEVEL     (1 << 4) /* Decr interrupt active when
+                                               * the most significant bit is 1.
+                                               */
 
 uint64_t cpu_ppc_get_tb(ppc_tb_t *tb_env, uint64_t vmclk, int64_t tb_offset);
 clk_setup_cb cpu_ppc_tb_init (CPUPPCState *env, uint32_t freq);
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 2719c08..d498340 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1133,6 +1133,7 @@ uint64_t cpu_ppc_load_atbl (CPUPPCState *env);
 uint32_t cpu_ppc_load_atbu (CPUPPCState *env);
 void cpu_ppc_store_atbl (CPUPPCState *env, uint32_t value);
 void cpu_ppc_store_atbu (CPUPPCState *env, uint32_t value);
+bool ppc_decr_clear_on_delivery(CPUPPCState *env);
 uint32_t cpu_ppc_load_decr (CPUPPCState *env);
 void cpu_ppc_store_decr (CPUPPCState *env, uint32_t value);
 uint32_t cpu_ppc_load_hdecr (CPUPPCState *env);
diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
index 19bc6b6..4fa297d 100644
--- a/target-ppc/excp_helper.c
+++ b/target-ppc/excp_helper.c
@@ -723,7 +723,6 @@ void ppc_hw_interrupt(CPUPPCState *env)
     if ((msr_ee != 0 || msr_hv == 0 || msr_pr != 0) && hdice != 0) {
         /* Hypervisor decrementer exception */
         if (env->pending_interrupts & (1 << PPC_INTERRUPT_HDECR)) {
-            env->pending_interrupts &= ~(1 << PPC_INTERRUPT_HDECR);
             powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_HDECR);
             return;
         }
@@ -767,7 +766,9 @@ void ppc_hw_interrupt(CPUPPCState *env)
         }
         /* Decrementer exception */
         if (env->pending_interrupts & (1 << PPC_INTERRUPT_DECR)) {
-            env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DECR);
+            if (ppc_decr_clear_on_delivery(env)) {
+                env->pending_interrupts &= ~(1 << PPC_INTERRUPT_DECR);
+            }
             powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_DECR);
             return;
         }
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH v2] PPC: Clean up DECR implementation

[Tom Musta]

On 4/6/2014 3:55 PM, Alexander Graf wrote:
<snip>

> @@ -806,6 +838,10 @@ clk_setup_cb cpu_ppc_tb_init (CPUPPCState *env, uint32_t freq)
>      tb_env = g_malloc0(sizeof(ppc_tb_t));
>      env->tb_env = tb_env;
>      tb_env->flags = PPC_DECR_UNDERFLOW_TRIGGERED;
> +    if (env->insns_flags & PPC_SEGMENT_64B) {
> +        /* All Book3S 64bit CPUs implement level based DEC logic */
> +        tb_env->flags |= PPC_DECR_UNDERFLOW_LEVEL;
> +    }
>      /* Create new timer */
>      tb_env->decr_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &cpu_ppc_decr_cb, cpu);
>      if (0) {

Equating Book3S with PPC_SEGMENT_64B is clever ... is it too clever?  Especially since
the SLB Bridge is in the phased-out category and consequently we should expect future
Book3S implementations to not support this instruction category.

Re: [Qemu-devel] [PATCH v2] PPC: Clean up DECR implementation

[Alexander Graf]

On 04/08/2014 09:56 PM, Tom Musta wrote:
> On 4/6/2014 3:55 PM, Alexander Graf wrote:
> <snip>
>
>> @@ -806,6 +838,10 @@ clk_setup_cb cpu_ppc_tb_init (CPUPPCState *env, uint32_t freq)
>>       tb_env = g_malloc0(sizeof(ppc_tb_t));
>>       env->tb_env = tb_env;
>>       tb_env->flags = PPC_DECR_UNDERFLOW_TRIGGERED;
>> +    if (env->insns_flags & PPC_SEGMENT_64B) {
>> +        /* All Book3S 64bit CPUs implement level based DEC logic */
>> +        tb_env->flags |= PPC_DECR_UNDERFLOW_LEVEL;
>> +    }
>>       /* Create new timer */
>>       tb_env->decr_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &cpu_ppc_decr_cb, cpu);
>>       if (0) {
> Equating Book3S with PPC_SEGMENT_64B is clever ... is it too clever?  Especially since
> the SLB Bridge is in the phased-out category and consequently we should expect future
> Book3S implementations to not support this instruction category.

Maybe it's too clever :). I'm very open to suggestions on how to figure 
this out otherwise. Or maybe we should just rework the way timers get 
created and make them be part of the core itself?


Alex

Re: [Qemu-devel] [PATCH v2] PPC: Clean up DECR implementation

[Tom Musta]

On 4/8/2014 2:58 PM, Alexander Graf wrote:
> On 04/08/2014 09:56 PM, Tom Musta wrote:
>> On 4/6/2014 3:55 PM, Alexander Graf wrote:
>> <snip>
>>
>>> @@ -806,6 +838,10 @@ clk_setup_cb cpu_ppc_tb_init (CPUPPCState *env, uint32_t freq)
>>>       tb_env = g_malloc0(sizeof(ppc_tb_t));
>>>       env->tb_env = tb_env;
>>>       tb_env->flags = PPC_DECR_UNDERFLOW_TRIGGERED;
>>> +    if (env->insns_flags & PPC_SEGMENT_64B) {
>>> +        /* All Book3S 64bit CPUs implement level based DEC logic */
>>> +        tb_env->flags |= PPC_DECR_UNDERFLOW_LEVEL;
>>> +    }
>>>       /* Create new timer */
>>>       tb_env->decr_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &cpu_ppc_decr_cb, cpu);
>>>       if (0) {
>> Equating Book3S with PPC_SEGMENT_64B is clever ... is it too clever?  Especially since
>> the SLB Bridge is in the phased-out category and consequently we should expect future
>> Book3S implementations to not support this instruction category.
> 
> Maybe it's too clever :). I'm very open to suggestions on how to figure this out otherwise. Or maybe we should just rework the way timers get created and make them be part of the core itself?
> 

I don't think there are existing flags that successfully describe what you want.

It seems to me that an overall timer configuration mechanism would be nice to have.  This would include:
 - a list of timers supported by the processor (family)
 - read / write privileges for each timer.
 - additional attributes (e.g. edge triggered vs. level triggered DEC)

It would be great to have good default configurations for the current Book III-S and Book III-E.

Things like read/write perms have changed over time which makes this all that much more fun.

I need a little time to study Book III-E and review some old documents (e.g. 601, 401/403, etc.).
I'll propose something a little more concrete.

Re: [Qemu-devel] [PATCH v2] PPC: Clean up DECR implementation

[Tom Musta]

On 4/8/2014 2:58 PM, Alexander Graf wrote:
> On 04/08/2014 09:56 PM, Tom Musta wrote:
>> On 4/6/2014 3:55 PM, Alexander Graf wrote:
>> <snip>
>>
>>> @@ -806,6 +838,10 @@ clk_setup_cb cpu_ppc_tb_init (CPUPPCState *env, uint32_t freq)
>>>       tb_env = g_malloc0(sizeof(ppc_tb_t));
>>>       env->tb_env = tb_env;
>>>       tb_env->flags = PPC_DECR_UNDERFLOW_TRIGGERED;
>>> +    if (env->insns_flags & PPC_SEGMENT_64B) {
>>> +        /* All Book3S 64bit CPUs implement level based DEC logic */
>>> +        tb_env->flags |= PPC_DECR_UNDERFLOW_LEVEL;
>>> +    }
>>>       /* Create new timer */
>>>       tb_env->decr_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &cpu_ppc_decr_cb, cpu);
>>>       if (0) {
>> Equating Book3S with PPC_SEGMENT_64B is clever ... is it too clever?  Especially since
>> the SLB Bridge is in the phased-out category and consequently we should expect future
>> Book3S implementations to not support this instruction category.
> 
> Maybe it's too clever :). I'm very open to suggestions on how to figure this out otherwise. Or maybe we should just rework the way timers get created and make them be part of the core itself?
> 
> 
> Alex
> 

A somewhat more practical approach than redesigning timer init:  The phrasing introduced into Book3S
that corresponds to your UNDERFLOW_LEVEL flag has existed at least since ISA 2.03.  And 2.03 introduced
some new features, like SPE and Altivec.  So ...

    if (env->insns_flags & (PPC_SEGMENT_64B | PPC_SPE | PPC_ALTIVEC)) {
        /* All Book3S 64bit CPUs implement level based DEC logic */
        tb_env->flags |= PPC_DECR_UNDERFLOW_LEVEL;
    }

would catch a few more.  I'm not sure we get into this code for Book3E machines, but if you are worried
about that you could also ensure that insns_flags doesn't have PPC_BOOKE on.

Re: [Qemu-devel] [PATCH v2] PPC: Clean up DECR implementation

[Alexander Graf]

> Am 09.04.2014 um 21:33 schrieb Tom Musta <tommusta@gmail.com>:
> 
>> On 4/8/2014 2:58 PM, Alexander Graf wrote:
>>> On 04/08/2014 09:56 PM, Tom Musta wrote:
>>> On 4/6/2014 3:55 PM, Alexander Graf wrote:
>>> <snip>
>>> 
>>>> @@ -806,6 +838,10 @@ clk_setup_cb cpu_ppc_tb_init (CPUPPCState *env, uint32_t freq)
>>>>      tb_env = g_malloc0(sizeof(ppc_tb_t));
>>>>      env->tb_env = tb_env;
>>>>      tb_env->flags = PPC_DECR_UNDERFLOW_TRIGGERED;
>>>> +    if (env->insns_flags & PPC_SEGMENT_64B) {
>>>> +        /* All Book3S 64bit CPUs implement level based DEC logic */
>>>> +        tb_env->flags |= PPC_DECR_UNDERFLOW_LEVEL;
>>>> +    }
>>>>      /* Create new timer */
>>>>      tb_env->decr_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &cpu_ppc_decr_cb, cpu);
>>>>      if (0) {
>>> Equating Book3S with PPC_SEGMENT_64B is clever ... is it too clever?  Especially since
>>> the SLB Bridge is in the phased-out category and consequently we should expect future
>>> Book3S implementations to not support this instruction category.
>> 
>> Maybe it's too clever :). I'm very open to suggestions on how to figure this out otherwise. Or maybe we should just rework the way timers get created and make them be part of the core itself?
>> 
>> 
>> Alex
> 
> A somewhat more practical approach than redesigning timer init:  The phrasing introduced into Book3S
> that corresponds to your UNDERFLOW_LEVEL flag has existed at least since ISA 2.03.  And 2.03 introduced
> some new features, like SPE and Altivec.  So ...

e6500 supports Altivec :)

> 
>    if (env->insns_flags & (PPC_SEGMENT_64B | PPC_SPE | PPC_ALTIVEC)) {
>        /* All Book3S 64bit CPUs implement level based DEC logic */
>        tb_env->flags |= PPC_DECR_UNDERFLOW_LEVEL;
>    }
> 
> would catch a few more.  I'm not sure we get into this code for Book3E machines, but if you are worried
> about that you could also ensure that insns_flags doesn't have PPC_BOOKE on.

Phew, I'm not quite sure. We can do it as an interim solution, but really the timer is a core property rather than a machine device :). So eventually this should get reworked.


Alex