[PATCH v2 for-8.2 00/19] ppc: record-replay enablement and fixes

[PATCH v2 for-8.2 00/19] ppc: record-replay enablement and fixes

[Nicholas Piggin]

The patches in this series has been seen a few times in various
iterations.

There are two main pieces, some assorted small fixes and tests for
record-replay, plus a large set of decrementer fixes. I merged these
into one series rather than send decrementer fixes alone first, because
record-replay has been very good at uncovering timer problems, so it's
good to have those test cases in at the same time IMO.

Some of the fixes we might take to stable, but unclear which.
Decrementer fixes were a bit of a tangle so maybe we just leave those
alone since they work okay.

The decrementer is not emulated perfectly still. Underflow from -ve
to +ve is not implemented, for one. I started doing that but it's not
trivial so better stop here for now.

For record-replay, pseries is now quite solid with rr. Surely some
issues to iron out but it is becoming usable.

powernv record-replay has some known problems migrating edge-triggered
decrementer, and edge triggered msgsnd. Also it seems to get stuck in
xive init somewhere when replaying from checkpoint, so there is probably
some state in xive not being reset. But at least it runs the avocado
test and seems close to working, so I've added that test case so we
don't go backwards (ha!).

Other machine types might not be too far off if there is interest. I
found it quite difficult to find these problems though, reverse
debugging will sometimes just lock up, stop at wrong location, or abort
with wrong event. Difficult understand what went wrong. Worst case I had
to basically bisect the replay of the trace, and find the minimum length
of replay that hit the problem -- that sometimes would land near a
mtDEC or timer interrupt or similar.

Thanks,
Nick

Nicholas Piggin (19):
  ppc/vhyp: reset exception state when handling vhyp hcall
  ppc/vof: Fix missed fields in VOF cleanup
  hw/ppc/ppc.c: Tidy over-long lines
  hw/ppc: Introduce functions for conversion between timebase and
    nanoseconds
  host-utils: Add muldiv64_round_up
  hw/ppc: Round up the decrementer interval when converting to ns
  hw/ppc: Avoid decrementer rounding errors
  target/ppc: Sign-extend large decrementer to 64-bits
  hw/ppc: Always store the decrementer value
  target/ppc: Migrate DECR SPR
  hw/ppc: Reset timebase facilities on machine reset
  hw/ppc: Read time only once to perform decrementer write
  target/ppc: Fix CPU reservation migration for record-replay
  target/ppc: Fix timebase reset with record-replay
  spapr: Fix machine reset deadlock from replay-record
  spapr: Fix record-replay machine reset consuming too many events
  tests/avocado: boot ppc64 pseries replay-record test to Linux VFS
    mount
  tests/avocado: reverse-debugging cope with re-executing breakpoints
  tests/avocado: ppc64 reverse debugging tests for pseries and powernv

 hw/ppc/mac_oldworld.c              |   1 +
 hw/ppc/pegasos2.c                  |   1 +
 hw/ppc/pnv_core.c                  |   2 +
 hw/ppc/ppc.c                       | 236 +++++++++++++++++++----------
 hw/ppc/prep.c                      |   1 +
 hw/ppc/spapr.c                     |  32 +++-
 hw/ppc/spapr_cpu_core.c            |   2 +
 hw/ppc/vof.c                       |   2 +
 include/hw/ppc/ppc.h               |   3 +-
 include/hw/ppc/spapr.h             |   2 +
 include/qemu/host-utils.h          |  21 ++-
 target/ppc/compat.c                |  19 +++
 target/ppc/cpu.h                   |   3 +
 target/ppc/excp_helper.c           |   3 +
 target/ppc/machine.c               |  40 ++++-
 target/ppc/translate.c             |   4 +
 tests/avocado/replay_kernel.py     |   3 +-
 tests/avocado/reverse_debugging.py |  54 ++++++-
 18 files changed, 330 insertions(+), 99 deletions(-)

-- 
2.40.1

[PATCH v2 01/19] ppc/vhyp: reset exception state when handling vhyp hcall

[Nicholas Piggin]

Convention is to reset the exception_index and error_code after handling
an interrupt. The vhyp hcall handler fails to do this. This does not
appear to have ill effects because cpu_handle_exception() clears
exception_index later, but it is fragile and inconsistent. Reset the
exception state after handling vhyp hcall like other handlers.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/excp_helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 32e46e56b3..72ec2be92e 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -843,6 +843,7 @@ static void powerpc_excp_7xx(PowerPCCPU *cpu, int excp)
             PPCVirtualHypervisorClass *vhc =
                 PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
             vhc->hypercall(cpu->vhyp, cpu);
+            powerpc_reset_excp_state(cpu);
             return;
         }
 
@@ -1014,6 +1015,7 @@ static void powerpc_excp_74xx(PowerPCCPU *cpu, int excp)
             PPCVirtualHypervisorClass *vhc =
                 PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
             vhc->hypercall(cpu->vhyp, cpu);
+            powerpc_reset_excp_state(cpu);
             return;
         }
 
@@ -1526,6 +1528,7 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
             PPCVirtualHypervisorClass *vhc =
                 PPC_VIRTUAL_HYPERVISOR_GET_CLASS(cpu->vhyp);
             vhc->hypercall(cpu->vhyp, cpu);
+            powerpc_reset_excp_state(cpu);
             return;
         }
         if (env->insns_flags2 & PPC2_ISA310) {
-- 
2.40.1

[PATCH v2 02/19] ppc/vof: Fix missed fields in VOF cleanup

[Nicholas Piggin]

Failing to reset the of_instance_last makes ihandle allocation continue
to increase, which causes record-replay replay fail to match the
recorded trace.

Not resetting claimed_base makes VOF eventually run out of memory after
some resets.

Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
Fixes: fc8c745d501 ("spapr: Implement Open Firmware client interface")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/vof.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
index 18c3f92317..e3b430a81f 100644
--- a/hw/ppc/vof.c
+++ b/hw/ppc/vof.c
@@ -1024,6 +1024,8 @@ void vof_cleanup(Vof *vof)
     }
     vof->claimed = NULL;
     vof->of_instances = NULL;
+    vof->of_instance_last = 0;
+    vof->claimed_base = 0;
 }
 
 void vof_build_dt(void *fdt, Vof *vof)
-- 
2.40.1

Re: [PATCH v2 02/19] ppc/vof: Fix missed fields in VOF cleanup

[Alexey Kardashevskiy]

On 08/08/2023 14:19, Nicholas Piggin wrote:
> Failing to reset the of_instance_last makes ihandle allocation continue
> to increase, which causes record-replay replay fail to match the
> recorded trace.
> 
> Not resetting claimed_base makes VOF eventually run out of memory after
> some resets.
> 
> Cc: Alexey Kardashevskiy <aik@ozlabs.ru>
> Fixes: fc8c745d501 ("spapr: Implement Open Firmware client interface")
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>


Reviewed-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Cool to see it still in use :)


> ---
>   hw/ppc/vof.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/hw/ppc/vof.c b/hw/ppc/vof.c
> index 18c3f92317..e3b430a81f 100644
> --- a/hw/ppc/vof.c
> +++ b/hw/ppc/vof.c
> @@ -1024,6 +1024,8 @@ void vof_cleanup(Vof *vof)
>       }
>       vof->claimed = NULL;
>       vof->of_instances = NULL;
> +    vof->of_instance_last = 0;
> +    vof->claimed_base = 0;
>   }
>   
>   void vof_build_dt(void *fdt, Vof *vof)

-- 
Alexey

[PATCH v2 03/19] hw/ppc/ppc.c: Tidy over-long lines

[Nicholas Piggin]

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/ppc.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 0e0a3d93c3..09b82f68a8 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -497,7 +497,8 @@ uint64_t cpu_ppc_load_tbl (CPUPPCState *env)
         return env->spr[SPR_TBL];
     }
 
-    tb = cpu_ppc_get_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), tb_env->tb_offset);
+    tb = cpu_ppc_get_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+                        tb_env->tb_offset);
     trace_ppc_tb_load(tb);
 
     return tb;
@@ -508,7 +509,8 @@ static inline uint32_t _cpu_ppc_load_tbu(CPUPPCState *env)
     ppc_tb_t *tb_env = env->tb_env;
     uint64_t tb;
 
-    tb = cpu_ppc_get_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), tb_env->tb_offset);
+    tb = cpu_ppc_get_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+                        tb_env->tb_offset);
     trace_ppc_tb_load(tb);
 
     return tb >> 32;
@@ -565,7 +567,8 @@ uint64_t cpu_ppc_load_atbl (CPUPPCState *env)
     ppc_tb_t *tb_env = env->tb_env;
     uint64_t tb;
 
-    tb = cpu_ppc_get_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), tb_env->atb_offset);
+    tb = cpu_ppc_get_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+                        tb_env->atb_offset);
     trace_ppc_tb_load(tb);
 
     return tb;
@@ -576,7 +579,8 @@ uint32_t cpu_ppc_load_atbu (CPUPPCState *env)
     ppc_tb_t *tb_env = env->tb_env;
     uint64_t tb;
 
-    tb = cpu_ppc_get_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), tb_env->atb_offset);
+    tb = cpu_ppc_get_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+                        tb_env->atb_offset);
     trace_ppc_tb_load(tb);
 
     return tb >> 32;
@@ -1040,10 +1044,11 @@ clk_setup_cb cpu_ppc_tb_init (CPUPPCState *env, uint32_t freq)
         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);
+    tb_env->decr_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+                                      &cpu_ppc_decr_cb, cpu);
     if (env->has_hv_mode && !cpu->vhyp) {
-        tb_env->hdecr_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, &cpu_ppc_hdecr_cb,
-                                                cpu);
+        tb_env->hdecr_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+                                           &cpu_ppc_hdecr_cb, cpu);
     } else {
         tb_env->hdecr_timer = NULL;
     }
-- 
2.40.1

[PATCH v2 04/19] hw/ppc: Introduce functions for conversion between timebase and nanoseconds

[Nicholas Piggin]

These calculations are repeated several times, and they will become
a little more complicated with subsequent changes.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/ppc.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 09b82f68a8..423a3a117a 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -482,10 +482,20 @@ void ppce500_set_mpic_proxy(bool enabled)
 /*****************************************************************************/
 /* PowerPC time base and decrementer emulation */
 
+static uint64_t ns_to_tb(uint32_t freq, int64_t clock)
+{
+    return muldiv64(clock, freq, NANOSECONDS_PER_SECOND);
+}
+
+static int64_t tb_to_ns(uint32_t freq, uint64_t tb)
+{
+    return muldiv64(tb, NANOSECONDS_PER_SECOND, freq);
+}
+
 uint64_t cpu_ppc_get_tb(ppc_tb_t *tb_env, uint64_t vmclk, int64_t tb_offset)
 {
     /* TB time in tb periods */
-    return muldiv64(vmclk, tb_env->tb_freq, NANOSECONDS_PER_SECOND) + tb_offset;
+    return ns_to_tb(tb_env->tb_freq, vmclk) + tb_offset;
 }
 
 uint64_t cpu_ppc_load_tbl (CPUPPCState *env)
@@ -528,8 +538,7 @@ uint32_t cpu_ppc_load_tbu (CPUPPCState *env)
 static inline void cpu_ppc_store_tb(ppc_tb_t *tb_env, uint64_t vmclk,
                                     int64_t *tb_offsetp, uint64_t value)
 {
-    *tb_offsetp = value -
-        muldiv64(vmclk, tb_env->tb_freq, NANOSECONDS_PER_SECOND);
+    *tb_offsetp = value - ns_to_tb(tb_env->tb_freq, vmclk);
 
     trace_ppc_tb_store(value, *tb_offsetp);
 }
@@ -694,11 +703,11 @@ static inline int64_t _cpu_ppc_load_decr(CPUPPCState *env, uint64_t next)
 
     diff = next - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     if (diff >= 0) {
-        decr = muldiv64(diff, tb_env->decr_freq, NANOSECONDS_PER_SECOND);
+        decr = ns_to_tb(tb_env->decr_freq, diff);
     } else if (tb_env->flags & PPC_TIMER_BOOKE) {
         decr = 0;
     }  else {
-        decr = -muldiv64(-diff, tb_env->decr_freq, NANOSECONDS_PER_SECOND);
+        decr = -ns_to_tb(tb_env->decr_freq, -diff);
     }
     trace_ppc_decr_load(decr);
 
@@ -838,7 +847,7 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
 
     /* Calculate the next timer event */
     now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-    next = now + muldiv64(value, NANOSECONDS_PER_SECOND, tb_env->decr_freq);
+    next = now + tb_to_ns(tb_env->decr_freq, value);
     *nextp = next;
 
     /* Adjust timer */
@@ -1130,7 +1139,7 @@ static void cpu_4xx_fit_cb (void *opaque)
         /* Cannot occur, but makes gcc happy */
         return;
     }
-    next = now + muldiv64(next, NANOSECONDS_PER_SECOND, tb_env->tb_freq);
+    next = now + tb_to_ns(tb_env->tb_freq, next);
     if (next == now)
         next++;
     timer_mod(ppc40x_timer->fit_timer, next);
@@ -1158,8 +1167,7 @@ static void start_stop_pit (CPUPPCState *env, ppc_tb_t *tb_env, int is_excp)
     } else {
         trace_ppc4xx_pit_start(ppc40x_timer->pit_reload);
         now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-        next = now + muldiv64(ppc40x_timer->pit_reload,
-                              NANOSECONDS_PER_SECOND, tb_env->decr_freq);
+        next = now + tb_to_ns(tb_env->decr_freq, ppc40x_timer->pit_reload);
         if (is_excp)
             next += tb_env->decr_next - now;
         if (next == now)
@@ -1218,7 +1226,7 @@ static void cpu_4xx_wdt_cb (void *opaque)
         /* Cannot occur, but makes gcc happy */
         return;
     }
-    next = now + muldiv64(next, NANOSECONDS_PER_SECOND, tb_env->decr_freq);
+    next = now + tb_to_ns(tb_env->decr_freq, next);
     if (next == now)
         next++;
     trace_ppc4xx_wdt(env->spr[SPR_40x_TCR], env->spr[SPR_40x_TSR]);
-- 
2.40.1

[PATCH v2 05/19] host-utils: Add muldiv64_round_up

[Nicholas Piggin]

This will be used for converting time intervals in different base units
to host units, for the purpose of scheduling timers to emulate target
timers. Timers typically must not fire before their requested expiry
time but may fire some time afterward, so rounding up is the right way
to implement these.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 include/qemu/host-utils.h | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/include/qemu/host-utils.h b/include/qemu/host-utils.h
index 011618373e..e2a50a567f 100644
--- a/include/qemu/host-utils.h
+++ b/include/qemu/host-utils.h
@@ -56,6 +56,11 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
     return (__int128_t)a * b / c;
 }
 
+static inline uint64_t muldiv64_round_up(uint64_t a, uint32_t b, uint32_t c)
+{
+    return ((__int128_t)a * b + c - 1) / c;
+}
+
 static inline uint64_t divu128(uint64_t *plow, uint64_t *phigh,
                                uint64_t divisor)
 {
@@ -83,7 +88,8 @@ void mulu64(uint64_t *plow, uint64_t *phigh, uint64_t a, uint64_t b);
 uint64_t divu128(uint64_t *plow, uint64_t *phigh, uint64_t divisor);
 int64_t divs128(uint64_t *plow, int64_t *phigh, int64_t divisor);
 
-static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
+static inline uint64_t __muldiv64(uint64_t a, uint32_t b, uint32_t c,
+                                  bool round_up)
 {
     union {
         uint64_t ll;
@@ -99,12 +105,25 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
 
     u.ll = a;
     rl = (uint64_t)u.l.low * (uint64_t)b;
+    if (round_up) {
+        rl += c - 1;
+    }
     rh = (uint64_t)u.l.high * (uint64_t)b;
     rh += (rl >> 32);
     res.l.high = rh / c;
     res.l.low = (((rh % c) << 32) + (rl & 0xffffffff)) / c;
     return res.ll;
 }
+
+static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
+{
+    return __muldiv64(a, b, c, false);
+}
+
+static inline uint64_t muldiv64_round_up(uint64_t a, uint32_t b, uint32_t c)
+{
+    return __muldiv64(a, b, c, true);
+}
 #endif
 
 /**
-- 
2.40.1

Re: [PATCH v2 05/19] host-utils: Add muldiv64_round_up

[Cédric Le Goater]

Adding more reviewers since this patch is modifying a common service.

Thanks,

C.


On 8/8/23 06:19, Nicholas Piggin wrote:
> This will be used for converting time intervals in different base units
> to host units, for the purpose of scheduling timers to emulate target
> timers. Timers typically must not fire before their requested expiry
> time but may fire some time afterward, so rounding up is the right way
> to implement these.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   include/qemu/host-utils.h | 21 ++++++++++++++++++++-
>   1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/include/qemu/host-utils.h b/include/qemu/host-utils.h
> index 011618373e..e2a50a567f 100644
> --- a/include/qemu/host-utils.h
> +++ b/include/qemu/host-utils.h
> @@ -56,6 +56,11 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
>       return (__int128_t)a * b / c;
>   }
>   
> +static inline uint64_t muldiv64_round_up(uint64_t a, uint32_t b, uint32_t c)
> +{
> +    return ((__int128_t)a * b + c - 1) / c;
> +}
> +
>   static inline uint64_t divu128(uint64_t *plow, uint64_t *phigh,
>                                  uint64_t divisor)
>   {
> @@ -83,7 +88,8 @@ void mulu64(uint64_t *plow, uint64_t *phigh, uint64_t a, uint64_t b);
>   uint64_t divu128(uint64_t *plow, uint64_t *phigh, uint64_t divisor);
>   int64_t divs128(uint64_t *plow, int64_t *phigh, int64_t divisor);
>   
> -static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
> +static inline uint64_t __muldiv64(uint64_t a, uint32_t b, uint32_t c,
> +                                  bool round_up)
>   {
>       union {
>           uint64_t ll;
> @@ -99,12 +105,25 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
>   
>       u.ll = a;
>       rl = (uint64_t)u.l.low * (uint64_t)b;
> +    if (round_up) {
> +        rl += c - 1;
> +    }
>       rh = (uint64_t)u.l.high * (uint64_t)b;
>       rh += (rl >> 32);
>       res.l.high = rh / c;
>       res.l.low = (((rh % c) << 32) + (rl & 0xffffffff)) / c;
>       return res.ll;
>   }
> +
> +static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
> +{
> +    return __muldiv64(a, b, c, false);
> +}
> +
> +static inline uint64_t muldiv64_round_up(uint64_t a, uint32_t b, uint32_t c)
> +{
> +    return __muldiv64(a, b, c, true);
> +}
>   #endif
>   
>   /**

Re: [PATCH v2 05/19] host-utils: Add muldiv64_round_up

[Richard Henderson]

On 9/1/23 04:51, Cédric Le Goater wrote:
> Adding more reviewers since this patch is modifying a common service.
> 
> Thanks,
> 
> C.
> 
> 
> On 8/8/23 06:19, Nicholas Piggin wrote:
>> This will be used for converting time intervals in different base units
>> to host units, for the purpose of scheduling timers to emulate target
>> timers. Timers typically must not fire before their requested expiry
>> time but may fire some time afterward, so rounding up is the right way
>> to implement these.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>>   include/qemu/host-utils.h | 21 ++++++++++++++++++++-
>>   1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/qemu/host-utils.h b/include/qemu/host-utils.h
>> index 011618373e..e2a50a567f 100644
>> --- a/include/qemu/host-utils.h
>> +++ b/include/qemu/host-utils.h
>> @@ -56,6 +56,11 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
>>       return (__int128_t)a * b / c;
>>   }
>> +static inline uint64_t muldiv64_round_up(uint64_t a, uint32_t b, uint32_t c)
>> +{
>> +    return ((__int128_t)a * b + c - 1) / c;
>> +}
>> +
>>   static inline uint64_t divu128(uint64_t *plow, uint64_t *phigh,
>>                                  uint64_t divisor)
>>   {
>> @@ -83,7 +88,8 @@ void mulu64(uint64_t *plow, uint64_t *phigh, uint64_t a, uint64_t b);
>>   uint64_t divu128(uint64_t *plow, uint64_t *phigh, uint64_t divisor);
>>   int64_t divs128(uint64_t *plow, int64_t *phigh, int64_t divisor);
>> -static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
>> +static inline uint64_t __muldiv64(uint64_t a, uint32_t b, uint32_t c,
>> +                                  bool round_up)

Perhaps better avoiding the reserved name: muldiv64_internal?

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~


>>   {
>>       union {
>>           uint64_t ll;
>> @@ -99,12 +105,25 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
>>       u.ll = a;
>>       rl = (uint64_t)u.l.low * (uint64_t)b;
>> +    if (round_up) {
>> +        rl += c - 1;
>> +    }
>>       rh = (uint64_t)u.l.high * (uint64_t)b;
>>       rh += (rl >> 32);
>>       res.l.high = rh / c;
>>       res.l.low = (((rh % c) << 32) + (rl & 0xffffffff)) / c;
>>       return res.ll;
>>   }
>> +
>> +static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
>> +{
>> +    return __muldiv64(a, b, c, false);
>> +}
>> +
>> +static inline uint64_t muldiv64_round_up(uint64_t a, uint32_t b, uint32_t c)
>> +{
>> +    return __muldiv64(a, b, c, true);
>> +}
>>   #endif
>>   /**
> 
> 

Re: [PATCH v2 05/19] host-utils: Add muldiv64_round_up

[Nicholas Piggin]

On Sat Sep 2, 2023 at 3:02 AM AEST, Richard Henderson wrote:
> On 9/1/23 04:51, Cédric Le Goater wrote:
> > Adding more reviewers since this patch is modifying a common service.
> > 
> > Thanks,
> > 
> > C.
> > 
> > 
> > On 8/8/23 06:19, Nicholas Piggin wrote:
> >> This will be used for converting time intervals in different base units
> >> to host units, for the purpose of scheduling timers to emulate target
> >> timers. Timers typically must not fire before their requested expiry
> >> time but may fire some time afterward, so rounding up is the right way
> >> to implement these.
> >>
> >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> >> ---
> >>   include/qemu/host-utils.h | 21 ++++++++++++++++++++-
> >>   1 file changed, 20 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/qemu/host-utils.h b/include/qemu/host-utils.h
> >> index 011618373e..e2a50a567f 100644
> >> --- a/include/qemu/host-utils.h
> >> +++ b/include/qemu/host-utils.h
> >> @@ -56,6 +56,11 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
> >>       return (__int128_t)a * b / c;
> >>   }
> >> +static inline uint64_t muldiv64_round_up(uint64_t a, uint32_t b, uint32_t c)
> >> +{
> >> +    return ((__int128_t)a * b + c - 1) / c;
> >> +}
> >> +
> >>   static inline uint64_t divu128(uint64_t *plow, uint64_t *phigh,
> >>                                  uint64_t divisor)
> >>   {
> >> @@ -83,7 +88,8 @@ void mulu64(uint64_t *plow, uint64_t *phigh, uint64_t a, uint64_t b);
> >>   uint64_t divu128(uint64_t *plow, uint64_t *phigh, uint64_t divisor);
> >>   int64_t divs128(uint64_t *plow, int64_t *phigh, int64_t divisor);
> >> -static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
> >> +static inline uint64_t __muldiv64(uint64_t a, uint32_t b, uint32_t c,
> >> +                                  bool round_up)
>
> Perhaps better avoiding the reserved name: muldiv64_internal?

Thanks, that would be okay. Or could be muldiv64_rounding?

>
> Otherwise,
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
>
> r~
>
>
> >>   {
> >>       union {
> >>           uint64_t ll;
> >> @@ -99,12 +105,25 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
> >>       u.ll = a;
> >>       rl = (uint64_t)u.l.low * (uint64_t)b;
> >> +    if (round_up) {
> >> +        rl += c - 1;
> >> +    }
> >>       rh = (uint64_t)u.l.high * (uint64_t)b;
> >>       rh += (rl >> 32);
> >>       res.l.high = rh / c;
> >>       res.l.low = (((rh % c) << 32) + (rl & 0xffffffff)) / c;
> >>       return res.ll;
> >>   }
> >> +
> >> +static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
> >> +{
> >> +    return __muldiv64(a, b, c, false);
> >> +}
> >> +
> >> +static inline uint64_t muldiv64_round_up(uint64_t a, uint32_t b, uint32_t c)
> >> +{
> >> +    return __muldiv64(a, b, c, true);
> >> +}
> >>   #endif
> >>   /**
> > 
> > 

Re: [PATCH v2 05/19] host-utils: Add muldiv64_round_up

[Cédric Le Goater]

On 9/4/23 15:07, Nicholas Piggin wrote:
> On Sat Sep 2, 2023 at 3:02 AM AEST, Richard Henderson wrote:
>> On 9/1/23 04:51, Cédric Le Goater wrote:
>>> Adding more reviewers since this patch is modifying a common service.
>>>
>>> Thanks,
>>>
>>> C.
>>>
>>>
>>> On 8/8/23 06:19, Nicholas Piggin wrote:
>>>> This will be used for converting time intervals in different base units
>>>> to host units, for the purpose of scheduling timers to emulate target
>>>> timers. Timers typically must not fire before their requested expiry
>>>> time but may fire some time afterward, so rounding up is the right way
>>>> to implement these.
>>>>
>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>> ---
>>>>    include/qemu/host-utils.h | 21 ++++++++++++++++++++-
>>>>    1 file changed, 20 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/qemu/host-utils.h b/include/qemu/host-utils.h
>>>> index 011618373e..e2a50a567f 100644
>>>> --- a/include/qemu/host-utils.h
>>>> +++ b/include/qemu/host-utils.h
>>>> @@ -56,6 +56,11 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
>>>>        return (__int128_t)a * b / c;
>>>>    }
>>>> +static inline uint64_t muldiv64_round_up(uint64_t a, uint32_t b, uint32_t c)
>>>> +{
>>>> +    return ((__int128_t)a * b + c - 1) / c;
>>>> +}
>>>> +
>>>>    static inline uint64_t divu128(uint64_t *plow, uint64_t *phigh,
>>>>                                   uint64_t divisor)
>>>>    {
>>>> @@ -83,7 +88,8 @@ void mulu64(uint64_t *plow, uint64_t *phigh, uint64_t a, uint64_t b);
>>>>    uint64_t divu128(uint64_t *plow, uint64_t *phigh, uint64_t divisor);
>>>>    int64_t divs128(uint64_t *plow, int64_t *phigh, int64_t divisor);
>>>> -static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
>>>> +static inline uint64_t __muldiv64(uint64_t a, uint32_t b, uint32_t c,
>>>> +                                  bool round_up)
>>
>> Perhaps better avoiding the reserved name: muldiv64_internal?
> 
> Thanks, that would be okay. Or could be muldiv64_rounding?
> 
>>
>> Otherwise,
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

oh, and I already sent the PR with the Rb of Richard ... :/
Sorry about that. Can we fix it later ? Or I will respin with
the update.

Someone really ought to take over PPC. Daniel and I are real
busy on other subsystems. Volunteers ?

Thanks,

C.

>>
>>
>> r~
>>
>>
>>>>    {
>>>>        union {
>>>>            uint64_t ll;
>>>> @@ -99,12 +105,25 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
>>>>        u.ll = a;
>>>>        rl = (uint64_t)u.l.low * (uint64_t)b;
>>>> +    if (round_up) {
>>>> +        rl += c - 1;
>>>> +    }
>>>>        rh = (uint64_t)u.l.high * (uint64_t)b;
>>>>        rh += (rl >> 32);
>>>>        res.l.high = rh / c;
>>>>        res.l.low = (((rh % c) << 32) + (rl & 0xffffffff)) / c;
>>>>        return res.ll;
>>>>    }
>>>> +
>>>> +static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
>>>> +{
>>>> +    return __muldiv64(a, b, c, false);
>>>> +}
>>>> +
>>>> +static inline uint64_t muldiv64_round_up(uint64_t a, uint32_t b, uint32_t c)
>>>> +{
>>>> +    return __muldiv64(a, b, c, true);
>>>> +}
>>>>    #endif
>>>>    /**
>>>
>>>
> 

Re: [PATCH v2 05/19] host-utils: Add muldiv64_round_up

[Nicholas Piggin]

On Mon Sep 4, 2023 at 11:30 PM AEST, Cédric Le Goater wrote:
> On 9/4/23 15:07, Nicholas Piggin wrote:
> > On Sat Sep 2, 2023 at 3:02 AM AEST, Richard Henderson wrote:
> >> On 9/1/23 04:51, Cédric Le Goater wrote:
> >>> Adding more reviewers since this patch is modifying a common service.
> >>>
> >>> Thanks,
> >>>
> >>> C.
> >>>
> >>>
> >>> On 8/8/23 06:19, Nicholas Piggin wrote:
> >>>> This will be used for converting time intervals in different base units
> >>>> to host units, for the purpose of scheduling timers to emulate target
> >>>> timers. Timers typically must not fire before their requested expiry
> >>>> time but may fire some time afterward, so rounding up is the right way
> >>>> to implement these.
> >>>>
> >>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> >>>> ---
> >>>>    include/qemu/host-utils.h | 21 ++++++++++++++++++++-
> >>>>    1 file changed, 20 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/include/qemu/host-utils.h b/include/qemu/host-utils.h
> >>>> index 011618373e..e2a50a567f 100644
> >>>> --- a/include/qemu/host-utils.h
> >>>> +++ b/include/qemu/host-utils.h
> >>>> @@ -56,6 +56,11 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
> >>>>        return (__int128_t)a * b / c;
> >>>>    }
> >>>> +static inline uint64_t muldiv64_round_up(uint64_t a, uint32_t b, uint32_t c)
> >>>> +{
> >>>> +    return ((__int128_t)a * b + c - 1) / c;
> >>>> +}
> >>>> +
> >>>>    static inline uint64_t divu128(uint64_t *plow, uint64_t *phigh,
> >>>>                                   uint64_t divisor)
> >>>>    {
> >>>> @@ -83,7 +88,8 @@ void mulu64(uint64_t *plow, uint64_t *phigh, uint64_t a, uint64_t b);
> >>>>    uint64_t divu128(uint64_t *plow, uint64_t *phigh, uint64_t divisor);
> >>>>    int64_t divs128(uint64_t *plow, int64_t *phigh, int64_t divisor);
> >>>> -static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
> >>>> +static inline uint64_t __muldiv64(uint64_t a, uint32_t b, uint32_t c,
> >>>> +                                  bool round_up)
> >>
> >> Perhaps better avoiding the reserved name: muldiv64_internal?
> > 
> > Thanks, that would be okay. Or could be muldiv64_rounding?
> > 
> >>
> >> Otherwise,
> >> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> oh, and I already sent the PR with the Rb of Richard ... :/
> Sorry about that. Can we fix it later ? Or I will respin with
> the update.
>
> Someone really ought to take over PPC. Daniel and I are real
> busy on other subsystems. Volunteers ?

I suppose I should. I could try do the next PR after this one
is merged.

Thanks,
Nick

Re: [PATCH v2 05/19] host-utils: Add muldiv64_round_up

[Cédric Le Goater]

On 9/5/23 05:56, Nicholas Piggin wrote:
> On Mon Sep 4, 2023 at 11:30 PM AEST, Cédric Le Goater wrote:
>> On 9/4/23 15:07, Nicholas Piggin wrote:
>>> On Sat Sep 2, 2023 at 3:02 AM AEST, Richard Henderson wrote:
>>>> On 9/1/23 04:51, Cédric Le Goater wrote:
>>>>> Adding more reviewers since this patch is modifying a common service.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> C.
>>>>>
>>>>>
>>>>> On 8/8/23 06:19, Nicholas Piggin wrote:
>>>>>> This will be used for converting time intervals in different base units
>>>>>> to host units, for the purpose of scheduling timers to emulate target
>>>>>> timers. Timers typically must not fire before their requested expiry
>>>>>> time but may fire some time afterward, so rounding up is the right way
>>>>>> to implement these.
>>>>>>
>>>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>>>> ---
>>>>>>     include/qemu/host-utils.h | 21 ++++++++++++++++++++-
>>>>>>     1 file changed, 20 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/include/qemu/host-utils.h b/include/qemu/host-utils.h
>>>>>> index 011618373e..e2a50a567f 100644
>>>>>> --- a/include/qemu/host-utils.h
>>>>>> +++ b/include/qemu/host-utils.h
>>>>>> @@ -56,6 +56,11 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
>>>>>>         return (__int128_t)a * b / c;
>>>>>>     }
>>>>>> +static inline uint64_t muldiv64_round_up(uint64_t a, uint32_t b, uint32_t c)
>>>>>> +{
>>>>>> +    return ((__int128_t)a * b + c - 1) / c;
>>>>>> +}
>>>>>> +
>>>>>>     static inline uint64_t divu128(uint64_t *plow, uint64_t *phigh,
>>>>>>                                    uint64_t divisor)
>>>>>>     {
>>>>>> @@ -83,7 +88,8 @@ void mulu64(uint64_t *plow, uint64_t *phigh, uint64_t a, uint64_t b);
>>>>>>     uint64_t divu128(uint64_t *plow, uint64_t *phigh, uint64_t divisor);
>>>>>>     int64_t divs128(uint64_t *plow, int64_t *phigh, int64_t divisor);
>>>>>> -static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
>>>>>> +static inline uint64_t __muldiv64(uint64_t a, uint32_t b, uint32_t c,
>>>>>> +                                  bool round_up)
>>>>
>>>> Perhaps better avoiding the reserved name: muldiv64_internal?
>>>
>>> Thanks, that would be okay. Or could be muldiv64_rounding?
>>>
>>>>
>>>> Otherwise,
>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>
>> oh, and I already sent the PR with the Rb of Richard ... :/
>> Sorry about that. Can we fix it later ? Or I will respin with
>> the update.
>>
>> Someone really ought to take over PPC. Daniel and I are real
>> busy on other subsystems. Volunteers ?
> 
> I suppose I should. I could try do the next PR after this one
> is merged.

Let's continue offline.

Thanks,

C.

Re: [PATCH v2 05/19] host-utils: Add muldiv64_round_up

[Philippe Mathieu-Daudé]

On 5/9/23 05:56, Nicholas Piggin wrote:
> On Mon Sep 4, 2023 at 11:30 PM AEST, Cédric Le Goater wrote:

>> Someone really ought to take over PPC. Daniel and I are real
>> busy on other subsystems. Volunteers ?
> 
> I suppose I should. I could try do the next PR after this one
> is merged.

Thanks a lot for volunteering!

Phil.

Re: [PATCH v2 05/19] host-utils: Add muldiv64_round_up

[Cédric Le Goater]

On 9/4/23 15:07, Nicholas Piggin wrote:
> On Sat Sep 2, 2023 at 3:02 AM AEST, Richard Henderson wrote:
>> On 9/1/23 04:51, Cédric Le Goater wrote:
>>> Adding more reviewers since this patch is modifying a common service.
>>>
>>> Thanks,
>>>
>>> C.
>>>
>>>
>>> On 8/8/23 06:19, Nicholas Piggin wrote:
>>>> This will be used for converting time intervals in different base units
>>>> to host units, for the purpose of scheduling timers to emulate target
>>>> timers. Timers typically must not fire before their requested expiry
>>>> time but may fire some time afterward, so rounding up is the right way
>>>> to implement these.
>>>>
>>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>>> ---
>>>>    include/qemu/host-utils.h | 21 ++++++++++++++++++++-
>>>>    1 file changed, 20 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/qemu/host-utils.h b/include/qemu/host-utils.h
>>>> index 011618373e..e2a50a567f 100644
>>>> --- a/include/qemu/host-utils.h
>>>> +++ b/include/qemu/host-utils.h
>>>> @@ -56,6 +56,11 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
>>>>        return (__int128_t)a * b / c;
>>>>    }
>>>> +static inline uint64_t muldiv64_round_up(uint64_t a, uint32_t b, uint32_t c)
>>>> +{
>>>> +    return ((__int128_t)a * b + c - 1) / c;
>>>> +}
>>>> +
>>>>    static inline uint64_t divu128(uint64_t *plow, uint64_t *phigh,
>>>>                                   uint64_t divisor)
>>>>    {
>>>> @@ -83,7 +88,8 @@ void mulu64(uint64_t *plow, uint64_t *phigh, uint64_t a, uint64_t b);
>>>>    uint64_t divu128(uint64_t *plow, uint64_t *phigh, uint64_t divisor);
>>>>    int64_t divs128(uint64_t *plow, int64_t *phigh, int64_t divisor);
>>>> -static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
>>>> +static inline uint64_t __muldiv64(uint64_t a, uint32_t b, uint32_t c,
>>>> +                                  bool round_up)
>>
>> Perhaps better avoiding the reserved name: muldiv64_internal?
> 
> Thanks, that would be okay. Or could be muldiv64_rounding?

I picked muldiv64_rounding()

Thanks,

C.


> 
>>
>> Otherwise,
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>
>>
>> r~
>>
>>
>>>>    {
>>>>        union {
>>>>            uint64_t ll;
>>>> @@ -99,12 +105,25 @@ static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
>>>>        u.ll = a;
>>>>        rl = (uint64_t)u.l.low * (uint64_t)b;
>>>> +    if (round_up) {
>>>> +        rl += c - 1;
>>>> +    }
>>>>        rh = (uint64_t)u.l.high * (uint64_t)b;
>>>>        rh += (rl >> 32);
>>>>        res.l.high = rh / c;
>>>>        res.l.low = (((rh % c) << 32) + (rl & 0xffffffff)) / c;
>>>>        return res.ll;
>>>>    }
>>>> +
>>>> +static inline uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c)
>>>> +{
>>>> +    return __muldiv64(a, b, c, false);
>>>> +}
>>>> +
>>>> +static inline uint64_t muldiv64_round_up(uint64_t a, uint32_t b, uint32_t c)
>>>> +{
>>>> +    return __muldiv64(a, b, c, true);
>>>> +}
>>>>    #endif
>>>>    /**
>>>
>>>
> 

[PATCH v2 06/19] hw/ppc: Round up the decrementer interval when converting to ns

[Nicholas Piggin]

The rule of timers is typically that they should never expire before the
timeout, but some time afterward. Rounding timer intervals up when doing
conversion is the right thing to do.

Under most circumstances it is impossible observe the decrementer
interrupt before the dec register has triggered. However with icount
timing, problems can arise. For example setting DEC to 0 can schedule
the timer for now, causing it to fire before any more instructions
have been executed and DEC is still 0.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/ppc.c | 31 +++++++++++++++++++------------
 1 file changed, 19 insertions(+), 12 deletions(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 423a3a117a..13eb45f4b7 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -482,14 +482,26 @@ void ppce500_set_mpic_proxy(bool enabled)
 /*****************************************************************************/
 /* PowerPC time base and decrementer emulation */
 
+/*
+ * Conversion between QEMU_CLOCK_VIRTUAL ns and timebase (TB) ticks:
+ * TB ticks are arrived at by multiplying tb_freq then dividing by
+ * ns per second, and rounding down. TB ticks drive all clocks and
+ * timers in the target machine.
+ *
+ * Converting TB intervals to ns for the purpose of setting a
+ * QEMU_CLOCK_VIRTUAL timer should go the other way, but rounding
+ * up. Rounding down could cause the timer to fire before the TB
+ * value has been reached.
+ */
 static uint64_t ns_to_tb(uint32_t freq, int64_t clock)
 {
     return muldiv64(clock, freq, NANOSECONDS_PER_SECOND);
 }
 
-static int64_t tb_to_ns(uint32_t freq, uint64_t tb)
+/* virtual clock in TB ticks, not adjusted by TB offset */
+static int64_t tb_to_ns_round_up(uint32_t freq, uint64_t tb)
 {
-    return muldiv64(tb, NANOSECONDS_PER_SECOND, freq);
+    return muldiv64_round_up(tb, NANOSECONDS_PER_SECOND, freq);
 }
 
 uint64_t cpu_ppc_get_tb(ppc_tb_t *tb_env, uint64_t vmclk, int64_t tb_offset)
@@ -847,7 +859,7 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
 
     /* Calculate the next timer event */
     now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-    next = now + tb_to_ns(tb_env->decr_freq, value);
+    next = now + tb_to_ns_round_up(tb_env->decr_freq, value);
     *nextp = next;
 
     /* Adjust timer */
@@ -1139,9 +1151,7 @@ static void cpu_4xx_fit_cb (void *opaque)
         /* Cannot occur, but makes gcc happy */
         return;
     }
-    next = now + tb_to_ns(tb_env->tb_freq, next);
-    if (next == now)
-        next++;
+    next = now + tb_to_ns_round_up(tb_env->tb_freq, next);
     timer_mod(ppc40x_timer->fit_timer, next);
     env->spr[SPR_40x_TSR] |= 1 << 26;
     if ((env->spr[SPR_40x_TCR] >> 23) & 0x1) {
@@ -1167,11 +1177,10 @@ static void start_stop_pit (CPUPPCState *env, ppc_tb_t *tb_env, int is_excp)
     } else {
         trace_ppc4xx_pit_start(ppc40x_timer->pit_reload);
         now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-        next = now + tb_to_ns(tb_env->decr_freq, ppc40x_timer->pit_reload);
+        next = now + tb_to_ns_round_up(tb_env->decr_freq,
+                                       ppc40x_timer->pit_reload);
         if (is_excp)
             next += tb_env->decr_next - now;
-        if (next == now)
-            next++;
         timer_mod(tb_env->decr_timer, next);
         tb_env->decr_next = next;
     }
@@ -1226,9 +1235,7 @@ static void cpu_4xx_wdt_cb (void *opaque)
         /* Cannot occur, but makes gcc happy */
         return;
     }
-    next = now + tb_to_ns(tb_env->decr_freq, next);
-    if (next == now)
-        next++;
+    next = now + tb_to_ns_round_up(tb_env->decr_freq, next);
     trace_ppc4xx_wdt(env->spr[SPR_40x_TCR], env->spr[SPR_40x_TSR]);
     switch ((env->spr[SPR_40x_TSR] >> 30) & 0x3) {
     case 0x0:
-- 
2.40.1

[PATCH v2 07/19] hw/ppc: Avoid decrementer rounding errors

[Nicholas Piggin]

The decrementer register contains a relative time in timebase units.
When writing to DECR this is converted and stored as an absolute value
in nanosecond units, reading DECR converts back to relative timebase.

The tb<->ns conversion of the relative part can cause rounding such that
a value writen to the decrementer can read back a different, with time
held constant. This is a particular problem for a deterministic icount
and record-replay trace.

Fix this by storing the absolute value in timebase units rather than
nanoseconds. The math before:
  store:  decr_next = now_ns + decr * ns_per_sec / tb_per_sec
  load:        decr = (decr_next - now_ns) * tb_per_sec / ns_per_sec
  load(store): decr = decr * ns_per_sec / tb_per_sec * tb_per_sec /
                      ns_per_sec

After:
  store:  decr_next = now_ns * tb_per_sec / ns_per_sec + decr
  load:        decr = decr_next - now_ns * tb_per_sec / ns_per_sec
  load(store): decr = decr

Fixes: 9fddaa0c0cab ("PowerPC merge: real time TB and decrementer - faster and simpler exception handling (Jocelyn Mayer)")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/ppc.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 13eb45f4b7..a397820d9c 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -711,16 +711,17 @@ bool ppc_decr_clear_on_delivery(CPUPPCState *env)
 static inline int64_t _cpu_ppc_load_decr(CPUPPCState *env, uint64_t next)
 {
     ppc_tb_t *tb_env = env->tb_env;
-    int64_t decr, diff;
+    uint64_t now, n;
+    int64_t decr;
 
-    diff = next - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-    if (diff >= 0) {
-        decr = ns_to_tb(tb_env->decr_freq, diff);
-    } else if (tb_env->flags & PPC_TIMER_BOOKE) {
+    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    n = ns_to_tb(tb_env->decr_freq, now);
+    if (next > n && tb_env->flags & PPC_TIMER_BOOKE) {
         decr = 0;
-    }  else {
-        decr = -ns_to_tb(tb_env->decr_freq, -diff);
+    } else {
+        decr = next - n;
     }
+
     trace_ppc_decr_load(decr);
 
     return decr;
@@ -857,13 +858,18 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
         (*lower_excp)(cpu);
     }
 
-    /* Calculate the next timer event */
+    /*
+     * Calculate the next decrementer event and set a timer.
+     * decr_next is in timebase units to keep rounding simple. Note it is
+     * not adjusted by tb_offset because if TB changes via tb_offset changing,
+     * decrementer does not change, so not directly comparable with TB.
+     */
     now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-    next = now + tb_to_ns_round_up(tb_env->decr_freq, value);
+    next = ns_to_tb(tb_env->decr_freq, now) + value;
     *nextp = next;
 
     /* Adjust timer */
-    timer_mod(timer, next);
+    timer_mod(timer, tb_to_ns_round_up(tb_env->decr_freq, next));
 }
 
 static inline void _cpu_ppc_store_decr(PowerPCCPU *cpu, target_ulong decr,
@@ -1177,12 +1183,15 @@ static void start_stop_pit (CPUPPCState *env, ppc_tb_t *tb_env, int is_excp)
     } else {
         trace_ppc4xx_pit_start(ppc40x_timer->pit_reload);
         now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-        next = now + tb_to_ns_round_up(tb_env->decr_freq,
-                                       ppc40x_timer->pit_reload);
-        if (is_excp)
-            next += tb_env->decr_next - now;
+
+        if (is_excp) {
+            tb_env->decr_next += ppc40x_timer->pit_reload;
+        } else {
+            tb_env->decr_next = ns_to_tb(tb_env->decr_freq, now)
+                                + ppc40x_timer->pit_reload;
+        }
+        next = tb_to_ns_round_up(tb_env->decr_freq, tb_env->decr_next);
         timer_mod(tb_env->decr_timer, next);
-        tb_env->decr_next = next;
     }
 }
 
-- 
2.40.1

[PATCH v2 08/19] target/ppc: Sign-extend large decrementer to 64-bits

[Nicholas Piggin]

When storing a large decrementer value with the most significant
implemented bit set, it is to be treated as a negative and sign
extended.

This isn't hit for book3s DEC because of another bug, fixing it
in the next patch exposes this one and can cause additional
problems, so fix this first. It can be hit with HDECR and other
edge triggered types.

Fixes: a8dafa52518 ("target/ppc: Implement large decrementer support for TCG")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/ppc.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index a397820d9c..fb4784793c 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -743,7 +743,9 @@ target_ulong cpu_ppc_load_decr(CPUPPCState *env)
      * to 64 bits, otherwise it is a 32 bit value.
      */
     if (env->spr[SPR_LPCR] & LPCR_LD) {
-        return decr;
+        PowerPCCPU *cpu = env_archcpu(env);
+        PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+        return sextract64(decr, 0, pcc->lrg_decr_bits);
     }
     return (uint32_t) decr;
 }
@@ -762,7 +764,9 @@ target_ulong cpu_ppc_load_hdecr(CPUPPCState *env)
      * extended to 64 bits, otherwise it is 32 bits.
      */
     if (pcc->lrg_decr_bits > 32) {
-        return hdecr;
+        PowerPCCPU *cpu = env_archcpu(env);
+        PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+        return sextract64(hdecr, 0, pcc->lrg_decr_bits);
     }
     return (uint32_t) hdecr;
 }
-- 
2.40.1

Re: [PATCH v2 08/19] target/ppc: Sign-extend large decrementer to 64-bits

[Cédric Le Goater]

Nick,

On 8/8/23 06:19, Nicholas Piggin wrote:
> When storing a large decrementer value with the most significant
> implemented bit set, it is to be treated as a negative and sign
> extended.
> 
> This isn't hit for book3s DEC because of another bug, fixing it
> in the next patch exposes this one and can cause additional
> problems, so fix this first. It can be hit with HDECR and other
> edge triggered types.
> 
> Fixes: a8dafa52518 ("target/ppc: Implement large decrementer support for TCG")
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   hw/ppc/ppc.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index a397820d9c..fb4784793c 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -743,7 +743,9 @@ target_ulong cpu_ppc_load_decr(CPUPPCState *env)
>        * to 64 bits, otherwise it is a 32 bit value.
>        */
>       if (env->spr[SPR_LPCR] & LPCR_LD) {
> -        return decr;
> +        PowerPCCPU *cpu = env_archcpu(env);
> +        PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +        return sextract64(decr, 0, pcc->lrg_decr_bits);
>       }
>       return (uint32_t) decr;
>   }
> @@ -762,7 +764,9 @@ target_ulong cpu_ppc_load_hdecr(CPUPPCState *env)
>        * extended to 64 bits, otherwise it is 32 bits.
>        */
>       if (pcc->lrg_decr_bits > 32) {
> -        return hdecr;
> +        PowerPCCPU *cpu = env_archcpu(env);
> +        PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);

Why are 'cpu' and 'ppc' duplicated ?

Thanks,

C.


> +        return sextract64(hdecr, 0, pcc->lrg_decr_bits);
>       }
>       return (uint32_t) hdecr;
>   }

Re: [PATCH v2 08/19] target/ppc: Sign-extend large decrementer to 64-bits

[Nicholas Piggin]

On Fri Sep 1, 2023 at 10:25 PM AEST, Cédric Le Goater wrote:
> Nick,
>
> On 8/8/23 06:19, Nicholas Piggin wrote:
> > When storing a large decrementer value with the most significant
> > implemented bit set, it is to be treated as a negative and sign
> > extended.
> > 
> > This isn't hit for book3s DEC because of another bug, fixing it
> > in the next patch exposes this one and can cause additional
> > problems, so fix this first. It can be hit with HDECR and other
> > edge triggered types.
> > 
> > Fixes: a8dafa52518 ("target/ppc: Implement large decrementer support for TCG")
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >   hw/ppc/ppc.c | 8 ++++++--
> >   1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> > index a397820d9c..fb4784793c 100644
> > --- a/hw/ppc/ppc.c
> > +++ b/hw/ppc/ppc.c
> > @@ -743,7 +743,9 @@ target_ulong cpu_ppc_load_decr(CPUPPCState *env)
> >        * to 64 bits, otherwise it is a 32 bit value.
> >        */
> >       if (env->spr[SPR_LPCR] & LPCR_LD) {
> > -        return decr;
> > +        PowerPCCPU *cpu = env_archcpu(env);
> > +        PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> > +        return sextract64(decr, 0, pcc->lrg_decr_bits);
> >       }
> >       return (uint32_t) decr;
> >   }
> > @@ -762,7 +764,9 @@ target_ulong cpu_ppc_load_hdecr(CPUPPCState *env)
> >        * extended to 64 bits, otherwise it is 32 bits.
> >        */
> >       if (pcc->lrg_decr_bits > 32) {
> > -        return hdecr;
> > +        PowerPCCPU *cpu = env_archcpu(env);
> > +        PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>
> Why are 'cpu' and 'ppc' duplicated ?

Hmm.. cut and paste bug maybe, good catch.

I'll send an increment to tidy it if needed.

Thanks,
Nick

[PATCH v2 09/19] hw/ppc: Always store the decrementer value

[Nicholas Piggin]

When writing a value to the decrementer that raises an exception, the
irq is raised, but the value is not stored so the store doesn't appear
to have changed the register when it is read again.

Always store the write value to the register.

Fixes: e81a982aa53 ("PPC: Clean up DECR implementation")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/ppc.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index fb4784793c..d9a1cfbf91 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -841,6 +841,16 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
         return;
     }
 
+    /*
+     * Calculate the next decrementer event and set a timer.
+     * decr_next is in timebase units to keep rounding simple. Note it is
+     * not adjusted by tb_offset because if TB changes via tb_offset changing,
+     * decrementer does not change, so not directly comparable with TB.
+     */
+    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    next = ns_to_tb(tb_env->decr_freq, now) + value;
+    *nextp = next; /* nextp is in timebase units */
+
     /*
      * Going from 1 -> 0 or 0 -> -1 is the event to generate a DEC interrupt.
      *
@@ -862,16 +872,6 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
         (*lower_excp)(cpu);
     }
 
-    /*
-     * Calculate the next decrementer event and set a timer.
-     * decr_next is in timebase units to keep rounding simple. Note it is
-     * not adjusted by tb_offset because if TB changes via tb_offset changing,
-     * decrementer does not change, so not directly comparable with TB.
-     */
-    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-    next = ns_to_tb(tb_env->decr_freq, now) + value;
-    *nextp = next;
-
     /* Adjust timer */
     timer_mod(timer, tb_to_ns_round_up(tb_env->decr_freq, next));
 }
-- 
2.40.1

[PATCH v2 10/19] target/ppc: Migrate DECR SPR

[Nicholas Piggin]

TCG does not maintain the DEC reigster in the SPR array, so it does get
migrated. TCG also needs to re-start the decrementer timer on the
destination machine.

Load and store the decrementer into the SPR when migrating. This works
for the level-triggered (book3s) decrementer, and should be compatible
with existing KVM machines that do keep the DEC value there.

This fixes lost decrementer interrupt on migration that can cause
hangs, as well as other problems including record-replay bugs.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/machine.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index 8234e35d69..8a190c4853 100644
--- a/target/ppc/machine.c
+++ b/target/ppc/machine.c
@@ -209,6 +209,14 @@ static int cpu_pre_save(void *opaque)
     /* Used to retain migration compatibility for pre 6.0 for 601 machines. */
     env->hflags_compat_nmsr = 0;
 
+    if (tcg_enabled()) {
+        /*
+         * TCG does not maintain the DECR spr (unlike KVM) so have to save
+         * it here.
+         */
+        env->spr[SPR_DECR] = cpu_ppc_load_decr(env);
+    }
+
     return 0;
 }
 
@@ -319,6 +327,12 @@ static int cpu_post_load(void *opaque, int version_id)
         ppc_update_ciabr(env);
         ppc_update_daw0(env);
 #endif
+        /*
+         * TCG needs to re-start the decrementer timer and/or raise the
+         * interrupt. This works for level-triggered decrementer. Edge
+         * triggered types (including HDEC) would need to carry more state.
+         */
+        cpu_ppc_store_decr(env, env->spr[SPR_DECR]);
         pmu_mmcr01_updated(env);
     }
 
-- 
2.40.1

Re: [PATCH v2 10/19] target/ppc: Migrate DECR SPR

[Cédric Le Goater]

Hello Nick,

On 8/8/23 06:19, Nicholas Piggin wrote:
> TCG does not maintain the DEC reigster in the SPR array, so it does get
> migrated. TCG also needs to re-start the decrementer timer on the
> destination machine.
> 
> Load and store the decrementer into the SPR when migrating. This works
> for the level-triggered (book3s) decrementer, and should be compatible
> with existing KVM machines that do keep the DEC value there.
> 
> This fixes lost decrementer interrupt on migration that can cause
> hangs, as well as other problems including record-replay bugs.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   target/ppc/machine.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 8234e35d69..8a190c4853 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -209,6 +209,14 @@ static int cpu_pre_save(void *opaque)
>       /* Used to retain migration compatibility for pre 6.0 for 601 machines. */
>       env->hflags_compat_nmsr = 0;
>   
> +    if (tcg_enabled()) {
> +        /*
> +         * TCG does not maintain the DECR spr (unlike KVM) so have to save
> +         * it here.
> +         */
> +        env->spr[SPR_DECR] = cpu_ppc_load_decr(env);
> +    }
> +
>       return 0;
>   }
>   
> @@ -319,6 +327,12 @@ static int cpu_post_load(void *opaque, int version_id)
>           ppc_update_ciabr(env);
>           ppc_update_daw0(env);
>   #endif
> +        /*
> +         * TCG needs to re-start the decrementer timer and/or raise the
> +         * interrupt. This works for level-triggered decrementer. Edge
> +         * triggered types (including HDEC) would need to carry more state.
> +         */
> +        cpu_ppc_store_decr(env, env->spr[SPR_DECR]);
>           pmu_mmcr01_updated(env);
>       }


This doesn't apply. I am missing some patch ?

Thanks,

C.

Re: [PATCH v2 10/19] target/ppc: Migrate DECR SPR

[Nicholas Piggin]

On Wed Aug 9, 2023 at 10:56 PM AEST, Cédric Le Goater wrote:
> Hello Nick,
>
> On 8/8/23 06:19, Nicholas Piggin wrote:
> > TCG does not maintain the DEC reigster in the SPR array, so it does get
> > migrated. TCG also needs to re-start the decrementer timer on the
> > destination machine.
> > 
> > Load and store the decrementer into the SPR when migrating. This works
> > for the level-triggered (book3s) decrementer, and should be compatible
> > with existing KVM machines that do keep the DEC value there.
> > 
> > This fixes lost decrementer interrupt on migration that can cause
> > hangs, as well as other problems including record-replay bugs.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >   target/ppc/machine.c | 14 ++++++++++++++
> >   1 file changed, 14 insertions(+)
> > 
> > diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> > index 8234e35d69..8a190c4853 100644
> > --- a/target/ppc/machine.c
> > +++ b/target/ppc/machine.c
> > @@ -209,6 +209,14 @@ static int cpu_pre_save(void *opaque)
> >       /* Used to retain migration compatibility for pre 6.0 for 601 machines. */
> >       env->hflags_compat_nmsr = 0;
> >   
> > +    if (tcg_enabled()) {
> > +        /*
> > +         * TCG does not maintain the DECR spr (unlike KVM) so have to save
> > +         * it here.
> > +         */
> > +        env->spr[SPR_DECR] = cpu_ppc_load_decr(env);
> > +    }
> > +
> >       return 0;
> >   }
> >   
> > @@ -319,6 +327,12 @@ static int cpu_post_load(void *opaque, int version_id)
> >           ppc_update_ciabr(env);
> >           ppc_update_daw0(env);
> >   #endif
> > +        /*
> > +         * TCG needs to re-start the decrementer timer and/or raise the
> > +         * interrupt. This works for level-triggered decrementer. Edge
> > +         * triggered types (including HDEC) would need to carry more state.
> > +         */
> > +        cpu_ppc_store_decr(env, env->spr[SPR_DECR]);
> >           pmu_mmcr01_updated(env);
> >       }
>
>
> This doesn't apply. I am missing some patch ?

Oh, it's a small dependency on the debug patches, sorry forgot to
mention (see the ciabr/daw0 in the top of the context).

Thanks,
Nick

[PATCH v2 11/19] hw/ppc: Reset timebase facilities on machine reset

[Nicholas Piggin]

Lower interrupts, delete timers, and set time facility registers
back to initial state on machine reset.

This is not so important for record-replay since timebase and
decrementer are migrated, but it gives a cleaner reset state.

Cc: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/mac_oldworld.c   |  1 +
 hw/ppc/pegasos2.c       |  1 +
 hw/ppc/pnv_core.c       |  2 ++
 hw/ppc/ppc.c            | 46 +++++++++++++++++++++++------------------
 hw/ppc/prep.c           |  1 +
 hw/ppc/spapr_cpu_core.c |  2 ++
 include/hw/ppc/ppc.h    |  3 ++-
 7 files changed, 35 insertions(+), 21 deletions(-)

diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 510ff0eaaf..9acc7adfc9 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -81,6 +81,7 @@ static void ppc_heathrow_reset(void *opaque)
 {
     PowerPCCPU *cpu = opaque;
 
+    cpu_ppc_tb_reset(&cpu->env);
     cpu_reset(CPU(cpu));
 }
 
diff --git a/hw/ppc/pegasos2.c b/hw/ppc/pegasos2.c
index 075367d94d..bd397cf2b5 100644
--- a/hw/ppc/pegasos2.c
+++ b/hw/ppc/pegasos2.c
@@ -99,6 +99,7 @@ static void pegasos2_cpu_reset(void *opaque)
         cpu->env.gpr[1] = 2 * VOF_STACK_SIZE - 0x20;
         cpu->env.nip = 0x100;
     }
+    cpu_ppc_tb_reset(&cpu->env);
 }
 
 static void pegasos2_pci_irq(void *opaque, int n, int level)
diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
index 9b39d527de..8c7afe037f 100644
--- a/hw/ppc/pnv_core.c
+++ b/hw/ppc/pnv_core.c
@@ -61,6 +61,8 @@ static void pnv_core_cpu_reset(PnvCore *pc, PowerPCCPU *cpu)
     hreg_compute_hflags(env);
     ppc_maybe_interrupt(env);
 
+    cpu_ppc_tb_reset(env);
+
     pcc->intc_reset(pc->chip, cpu);
 }
 
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index d9a1cfbf91..f391acc39e 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -944,23 +944,6 @@ void cpu_ppc_store_purr(CPUPPCState *env, uint64_t value)
                      &tb_env->purr_offset, value);
 }
 
-static void cpu_ppc_set_tb_clk (void *opaque, uint32_t freq)
-{
-    CPUPPCState *env = opaque;
-    PowerPCCPU *cpu = env_archcpu(env);
-    ppc_tb_t *tb_env = env->tb_env;
-
-    tb_env->tb_freq = freq;
-    tb_env->decr_freq = freq;
-    /* There is a bug in Linux 2.4 kernels:
-     * 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, 32);
-    _cpu_ppc_store_hdecr(cpu, 0xFFFFFFFF, 0xFFFFFFFF, 32);
-    cpu_ppc_store_purr(env, 0x0000000000000000ULL);
-}
-
 static void timebase_save(PPCTimebase *tb)
 {
     uint64_t ticks = cpu_get_host_ticks();
@@ -1062,7 +1045,7 @@ const VMStateDescription vmstate_ppc_timebase = {
 };
 
 /* Set up (once) timebase frequency (in Hz) */
-clk_setup_cb cpu_ppc_tb_init (CPUPPCState *env, uint32_t freq)
+void cpu_ppc_tb_init(CPUPPCState *env, uint32_t freq)
 {
     PowerPCCPU *cpu = env_archcpu(env);
     ppc_tb_t *tb_env;
@@ -1083,9 +1066,32 @@ clk_setup_cb cpu_ppc_tb_init (CPUPPCState *env, uint32_t freq)
     } else {
         tb_env->hdecr_timer = NULL;
     }
-    cpu_ppc_set_tb_clk(env, freq);
 
-    return &cpu_ppc_set_tb_clk;
+    tb_env->tb_freq = freq;
+    tb_env->decr_freq = freq;
+}
+
+void cpu_ppc_tb_reset(CPUPPCState *env)
+{
+    PowerPCCPU *cpu = env_archcpu(env);
+    ppc_tb_t *tb_env = env->tb_env;
+
+    timer_del(tb_env->decr_timer);
+    ppc_set_irq(cpu, PPC_INTERRUPT_DECR, 0);
+    tb_env->decr_next = 0;
+    if (tb_env->hdecr_timer != NULL) {
+        timer_del(tb_env->hdecr_timer);
+        ppc_set_irq(cpu, PPC_INTERRUPT_HDECR, 0);
+        tb_env->hdecr_next = 0;
+    }
+
+    /* There is a bug in Linux 2.4 kernels:
+     * if a decrementer exception is pending when it enables msr_ee at startup,
+     * it's not ready to handle it...
+     */
+    cpu_ppc_store_decr(env, -1);
+    cpu_ppc_store_hdecr(env, -1);
+    cpu_ppc_store_purr(env, 0x0000000000000000ULL);
 }
 
 void cpu_ppc_tb_free(CPUPPCState *env)
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index d9231c7317..f6fd35fcb9 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -67,6 +67,7 @@ static void ppc_prep_reset(void *opaque)
     PowerPCCPU *cpu = opaque;
 
     cpu_reset(CPU(cpu));
+    cpu_ppc_tb_reset(&cpu->env);
 }
 
 
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index b482d9754a..91fae56573 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -74,6 +74,8 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
 
     kvm_check_mmu(cpu, &error_fatal);
 
+    cpu_ppc_tb_reset(env);
+
     spapr_irq_cpu_intc_reset(spapr, cpu);
 }
 
diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
index e095c002dc..17a8dfc107 100644
--- a/include/hw/ppc/ppc.h
+++ b/include/hw/ppc/ppc.h
@@ -54,7 +54,8 @@ struct ppc_tb_t {
                                                */
 
 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);
+void cpu_ppc_tb_init(CPUPPCState *env, uint32_t freq);
+void cpu_ppc_tb_reset(CPUPPCState *env);
 void cpu_ppc_tb_free(CPUPPCState *env);
 void cpu_ppc_hdecr_init(CPUPPCState *env);
 void cpu_ppc_hdecr_exit(CPUPPCState *env);
-- 
2.40.1

[PATCH v2 12/19] hw/ppc: Read time only once to perform decrementer write

[Nicholas Piggin]

Reading the time more than once to perform an operation always increases
complexity and fragility due to introduced deltas. Simplify the
decrementer write by reading the clock once for the operation.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/ppc.c | 84 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 53 insertions(+), 31 deletions(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index f391acc39e..a0ee064b1d 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -708,13 +708,13 @@ bool ppc_decr_clear_on_delivery(CPUPPCState *env)
     return ((tb_env->flags & flags) == PPC_DECR_UNDERFLOW_TRIGGERED);
 }
 
-static inline int64_t _cpu_ppc_load_decr(CPUPPCState *env, uint64_t next)
+static inline int64_t __cpu_ppc_load_decr(CPUPPCState *env, int64_t now,
+                                          uint64_t next)
 {
     ppc_tb_t *tb_env = env->tb_env;
-    uint64_t now, n;
+    uint64_t n;
     int64_t decr;
 
-    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     n = ns_to_tb(tb_env->decr_freq, now);
     if (next > n && tb_env->flags & PPC_TIMER_BOOKE) {
         decr = 0;
@@ -727,16 +727,12 @@ static inline int64_t _cpu_ppc_load_decr(CPUPPCState *env, uint64_t next)
     return decr;
 }
 
-target_ulong cpu_ppc_load_decr(CPUPPCState *env)
+static target_ulong _cpu_ppc_load_decr(CPUPPCState *env, int64_t now)
 {
     ppc_tb_t *tb_env = env->tb_env;
     uint64_t decr;
 
-    if (kvm_enabled()) {
-        return env->spr[SPR_DECR];
-    }
-
-    decr = _cpu_ppc_load_decr(env, tb_env->decr_next);
+    decr = __cpu_ppc_load_decr(env, now, tb_env->decr_next);
 
     /*
      * If large decrementer is enabled then the decrementer is signed extened
@@ -750,14 +746,23 @@ target_ulong cpu_ppc_load_decr(CPUPPCState *env)
     return (uint32_t) decr;
 }
 
-target_ulong cpu_ppc_load_hdecr(CPUPPCState *env)
+target_ulong cpu_ppc_load_decr(CPUPPCState *env)
+{
+    if (kvm_enabled()) {
+        return env->spr[SPR_DECR];
+    } else {
+        return _cpu_ppc_load_decr(env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+    }
+}
+
+static target_ulong _cpu_ppc_load_hdecr(CPUPPCState *env, int64_t now)
 {
     PowerPCCPU *cpu = env_archcpu(env);
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     ppc_tb_t *tb_env = env->tb_env;
     uint64_t hdecr;
 
-    hdecr =  _cpu_ppc_load_decr(env, tb_env->hdecr_next);
+    hdecr =  __cpu_ppc_load_decr(env, now, tb_env->hdecr_next);
 
     /*
      * If we have a large decrementer (POWER9 or later) then hdecr is sign
@@ -771,6 +776,11 @@ target_ulong cpu_ppc_load_hdecr(CPUPPCState *env)
     return (uint32_t) hdecr;
 }
 
+target_ulong cpu_ppc_load_hdecr(CPUPPCState *env)
+{
+    return _cpu_ppc_load_hdecr(env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
+}
+
 uint64_t cpu_ppc_load_purr (CPUPPCState *env)
 {
     ppc_tb_t *tb_env = env->tb_env;
@@ -815,7 +825,7 @@ 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,
+static void __cpu_ppc_store_decr(PowerPCCPU *cpu, int64_t now, uint64_t *nextp,
                                  QEMUTimer *timer,
                                  void (*raise_excp)(void *),
                                  void (*lower_excp)(PowerPCCPU *),
@@ -824,7 +834,7 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
 {
     CPUPPCState *env = &cpu->env;
     ppc_tb_t *tb_env = env->tb_env;
-    uint64_t now, next;
+    uint64_t next;
     int64_t signed_value;
     int64_t signed_decr;
 
@@ -836,18 +846,12 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
 
     trace_ppc_decr_store(nr_bits, decr, value);
 
-    if (kvm_enabled()) {
-        /* KVM handles decrementer exceptions, we don't need our own timer */
-        return;
-    }
-
     /*
      * Calculate the next decrementer event and set a timer.
      * decr_next is in timebase units to keep rounding simple. Note it is
      * not adjusted by tb_offset because if TB changes via tb_offset changing,
      * decrementer does not change, so not directly comparable with TB.
      */
-    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
     next = ns_to_tb(tb_env->decr_freq, now) + value;
     *nextp = next; /* nextp is in timebase units */
 
@@ -876,12 +880,13 @@ static void __cpu_ppc_store_decr(PowerPCCPU *cpu, uint64_t *nextp,
     timer_mod(timer, tb_to_ns_round_up(tb_env->decr_freq, next));
 }
 
-static inline void _cpu_ppc_store_decr(PowerPCCPU *cpu, target_ulong decr,
-                                       target_ulong value, int nr_bits)
+static inline void _cpu_ppc_store_decr(PowerPCCPU *cpu, int64_t now,
+                                       target_ulong decr, target_ulong value,
+                                       int nr_bits)
 {
     ppc_tb_t *tb_env = cpu->env.tb_env;
 
-    __cpu_ppc_store_decr(cpu, &tb_env->decr_next, tb_env->decr_timer,
+    __cpu_ppc_store_decr(cpu, now, &tb_env->decr_next, tb_env->decr_timer,
                          tb_env->decr_timer->cb, &cpu_ppc_decr_lower,
                          tb_env->flags, decr, value, nr_bits);
 }
@@ -890,13 +895,22 @@ void cpu_ppc_store_decr(CPUPPCState *env, target_ulong value)
 {
     PowerPCCPU *cpu = env_archcpu(env);
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+    int64_t now;
+    target_ulong decr;
     int nr_bits = 32;
 
+    if (kvm_enabled()) {
+        /* KVM handles decrementer exceptions, we don't need our own timer */
+        return;
+    }
+
     if (env->spr[SPR_LPCR] & LPCR_LD) {
         nr_bits = pcc->lrg_decr_bits;
     }
 
-    _cpu_ppc_store_decr(cpu, cpu_ppc_load_decr(env), value, nr_bits);
+    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    decr = _cpu_ppc_load_decr(env, now);
+    _cpu_ppc_store_decr(cpu, now, decr, value, nr_bits);
 }
 
 static void cpu_ppc_decr_cb(void *opaque)
@@ -906,14 +920,15 @@ static void cpu_ppc_decr_cb(void *opaque)
     cpu_ppc_decr_excp(cpu);
 }
 
-static inline void _cpu_ppc_store_hdecr(PowerPCCPU *cpu, target_ulong hdecr,
-                                        target_ulong value, int nr_bits)
+static inline void _cpu_ppc_store_hdecr(PowerPCCPU *cpu, int64_t now,
+                                        target_ulong hdecr, target_ulong value,
+                                        int nr_bits)
 {
     ppc_tb_t *tb_env = cpu->env.tb_env;
 
     if (tb_env->hdecr_timer != NULL) {
         /* HDECR (Book3S 64bit) is edge-based, not level like DECR */
-        __cpu_ppc_store_decr(cpu, &tb_env->hdecr_next, tb_env->hdecr_timer,
+        __cpu_ppc_store_decr(cpu, now, &tb_env->hdecr_next, tb_env->hdecr_timer,
                              tb_env->hdecr_timer->cb, &cpu_ppc_hdecr_lower,
                              PPC_DECR_UNDERFLOW_TRIGGERED,
                              hdecr, value, nr_bits);
@@ -924,9 +939,12 @@ void cpu_ppc_store_hdecr(CPUPPCState *env, target_ulong value)
 {
     PowerPCCPU *cpu = env_archcpu(env);
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+    int64_t now;
+    target_ulong hdecr;
 
-    _cpu_ppc_store_hdecr(cpu, cpu_ppc_load_hdecr(env), value,
-                         pcc->lrg_decr_bits);
+    now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+    hdecr = _cpu_ppc_load_hdecr(env, now);
+    _cpu_ppc_store_hdecr(cpu, now, hdecr, value, pcc->lrg_decr_bits);
 }
 
 static void cpu_ppc_hdecr_cb(void *opaque)
@@ -936,12 +954,16 @@ static void cpu_ppc_hdecr_cb(void *opaque)
     cpu_ppc_hdecr_excp(cpu);
 }
 
-void cpu_ppc_store_purr(CPUPPCState *env, uint64_t value)
+static void _cpu_ppc_store_purr(CPUPPCState *env, int64_t now, uint64_t value)
 {
     ppc_tb_t *tb_env = env->tb_env;
 
-    cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
-                     &tb_env->purr_offset, value);
+    cpu_ppc_store_tb(tb_env, now, &tb_env->purr_offset, value);
+}
+
+void cpu_ppc_store_purr(CPUPPCState *env, uint64_t value)
+{
+    _cpu_ppc_store_purr(env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), value);
 }
 
 static void timebase_save(PPCTimebase *tb)
-- 
2.40.1

[PATCH v2 13/19] target/ppc: Fix CPU reservation migration for record-replay

[Nicholas Piggin]

ppc only migrates reserve_addr, so the destination machine can get a
valid reservation with an incorrect reservation value of 0. Prior to
commit 392d328abe753 ("target/ppc: Ensure stcx size matches larx"),
this could permit a stcx. to incorrectly succeed. That commit
inadvertently fixed that bug because the target machine starts with an
impossible reservation size of 0, so any stcx. will fail.

This behaviour is permitted by the ISA because reservation loss may
have implementation-dependent cause. What's more, with KVM machines it
is impossible save or reasonably restore reservation state. However if
the vmstate is being used for record-replay, the reservation must be
saved and restored exactly in order for execution from snapshot to
match the record.

This patch deprecates the existing incomplete reserve_addr vmstate,
and adds a new vmstate subsection with complete reservation state.
The new vmstate is needed only when record-replay mode is active.

Acked-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/cpu.h       |  2 ++
 target/ppc/machine.c   | 26 ++++++++++++++++++++++++--
 target/ppc/translate.c |  4 ++++
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 2777ea3110..9e491e05eb 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1121,7 +1121,9 @@ struct CPUArchState {
     target_ulong reserve_addr;   /* Reservation address */
     target_ulong reserve_length; /* Reservation larx op size (bytes) */
     target_ulong reserve_val;    /* Reservation value */
+#if defined(TARGET_PPC64)
     target_ulong reserve_val2;
+#endif
 
     /* These are used in supervisor mode only */
     target_ulong msr;      /* machine state register */
diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index 8a190c4853..ad7b4f6338 100644
--- a/target/ppc/machine.c
+++ b/target/ppc/machine.c
@@ -10,6 +10,7 @@
 #include "qemu/main-loop.h"
 #include "kvm_ppc.h"
 #include "power8-pmu.h"
+#include "sysemu/replay.h"
 
 static void post_load_update_msr(CPUPPCState *env)
 {
@@ -690,6 +691,27 @@ static const VMStateDescription vmstate_compat = {
     }
 };
 
+static bool reservation_needed(void *opaque)
+{
+    return (replay_mode != REPLAY_MODE_NONE);
+}
+
+static const VMStateDescription vmstate_reservation = {
+    .name = "cpu/reservation",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .needed = reservation_needed,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINTTL(env.reserve_addr, PowerPCCPU),
+        VMSTATE_UINTTL(env.reserve_length, PowerPCCPU),
+        VMSTATE_UINTTL(env.reserve_val, PowerPCCPU),
+#if defined(TARGET_PPC64)
+        VMSTATE_UINTTL(env.reserve_val2, PowerPCCPU),
+#endif
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 const VMStateDescription vmstate_ppc_cpu = {
     .name = "cpu",
     .version_id = 5,
@@ -711,8 +733,7 @@ const VMStateDescription vmstate_ppc_cpu = {
         VMSTATE_UINTTL_ARRAY(env.spr, PowerPCCPU, 1024),
         VMSTATE_UINT64(env.spe_acc, PowerPCCPU),
 
-        /* Reservation */
-        VMSTATE_UINTTL(env.reserve_addr, PowerPCCPU),
+        VMSTATE_UNUSED(sizeof(target_ulong)), /* was env.reserve_addr */
 
         /* Supervisor mode architected state */
         VMSTATE_UINTTL(env.msr, PowerPCCPU),
@@ -741,6 +762,7 @@ const VMStateDescription vmstate_ppc_cpu = {
         &vmstate_tlbemb,
         &vmstate_tlbmas,
         &vmstate_compat,
+        &vmstate_reservation,
         NULL
     }
 };
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index b8c7f38ccd..4a60aefd8f 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -77,7 +77,9 @@ static TCGv cpu_xer, cpu_so, cpu_ov, cpu_ca, cpu_ov32, cpu_ca32;
 static TCGv cpu_reserve;
 static TCGv cpu_reserve_length;
 static TCGv cpu_reserve_val;
+#if defined(TARGET_PPC64)
 static TCGv cpu_reserve_val2;
+#endif
 static TCGv cpu_fpscr;
 static TCGv_i32 cpu_access_type;
 
@@ -151,9 +153,11 @@ void ppc_translate_init(void)
     cpu_reserve_val = tcg_global_mem_new(cpu_env,
                                          offsetof(CPUPPCState, reserve_val),
                                          "reserve_val");
+#if defined(TARGET_PPC64)
     cpu_reserve_val2 = tcg_global_mem_new(cpu_env,
                                           offsetof(CPUPPCState, reserve_val2),
                                           "reserve_val2");
+#endif
 
     cpu_fpscr = tcg_global_mem_new(cpu_env,
                                    offsetof(CPUPPCState, fpscr), "fpscr");
-- 
2.40.1

[PATCH v2 14/19] target/ppc: Fix timebase reset with record-replay

[Nicholas Piggin]

Timebase save uses a random number for a legacy vmstate field, which
makes rr snapshot loading unbalanced. The easiest way to deal with this
is just to skip the rng if record-replay is active.

Reviewed-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/ppc.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index a0ee064b1d..87df914600 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -32,6 +32,7 @@
 #include "qemu/main-loop.h"
 #include "qemu/error-report.h"
 #include "sysemu/kvm.h"
+#include "sysemu/replay.h"
 #include "sysemu/runstate.h"
 #include "kvm_ppc.h"
 #include "migration/vmstate.h"
@@ -976,8 +977,14 @@ static void timebase_save(PPCTimebase *tb)
         return;
     }
 
-    /* not used anymore, we keep it for compatibility */
-    tb->time_of_the_day_ns = qemu_clock_get_ns(QEMU_CLOCK_HOST);
+    if (replay_mode == REPLAY_MODE_NONE) {
+        /* not used anymore, we keep it for compatibility */
+        tb->time_of_the_day_ns = qemu_clock_get_ns(QEMU_CLOCK_HOST);
+    } else {
+        /* simpler for record-replay to avoid this event, compat not needed */
+        tb->time_of_the_day_ns = 0;
+    }
+
     /*
      * tb_offset is only expected to be changed by QEMU so
      * there is no need to update it from KVM here
-- 
2.40.1

[PATCH v2 15/19] spapr: Fix machine reset deadlock from replay-record

[Nicholas Piggin]

When the machine is reset to load a new snapshot while being debugged
with replay-record, it is done from another thread, so the CPU does
not run the register setting operations. Set CPU registers directly in
machine reset.

Cc: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/spapr.c         | 20 ++++++++++++++++++--
 include/hw/ppc/spapr.h |  1 +
 target/ppc/compat.c    | 19 +++++++++++++++++++
 target/ppc/cpu.h       |  1 +
 4 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 1c8b8d57a7..7d84244f03 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1322,6 +1322,22 @@ void spapr_set_all_lpcrs(target_ulong value, target_ulong mask)
     }
 }
 
+/* May be used when the machine is not running */
+void spapr_init_all_lpcrs(target_ulong value, target_ulong mask)
+{
+    CPUState *cs;
+    CPU_FOREACH(cs) {
+        PowerPCCPU *cpu = POWERPC_CPU(cs);
+        CPUPPCState *env = &cpu->env;
+        target_ulong lpcr;
+
+        lpcr = env->spr[SPR_LPCR];
+        lpcr &= ~(LPCR_HR | LPCR_UPRT);
+        ppc_store_lpcr(cpu, lpcr);
+    }
+}
+
+
 static bool spapr_get_pate(PPCVirtualHypervisor *vhyp, PowerPCCPU *cpu,
                            target_ulong lpid, ppc_v3_pate_t *entry)
 {
@@ -1583,7 +1599,7 @@ int spapr_reallocate_hpt(SpaprMachineState *spapr, int shift, Error **errp)
     }
     /* We're setting up a hash table, so that means we're not radix */
     spapr->patb_entry = 0;
-    spapr_set_all_lpcrs(0, LPCR_HR | LPCR_UPRT);
+    spapr_init_all_lpcrs(0, LPCR_HR | LPCR_UPRT);
     return 0;
 }
 
@@ -1661,7 +1677,7 @@ static void spapr_machine_reset(MachineState *machine, ShutdownCause reason)
     spapr_ovec_cleanup(spapr->ov5_cas);
     spapr->ov5_cas = spapr_ovec_new();
 
-    ppc_set_compat_all(spapr->max_compat_pvr, &error_fatal);
+    ppc_init_compat_all(spapr->max_compat_pvr, &error_fatal);
 
     /*
      * This is fixing some of the default configuration of the XIVE
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 538b2dfb89..f47e8419a5 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -1012,6 +1012,7 @@ bool spapr_check_pagesize(SpaprMachineState *spapr, hwaddr pagesize,
 #define SPAPR_OV5_XIVE_BOTH     0x80 /* Only to advertise on the platform */
 
 void spapr_set_all_lpcrs(target_ulong value, target_ulong mask);
+void spapr_init_all_lpcrs(target_ulong value, target_ulong mask);
 hwaddr spapr_get_rtas_addr(void);
 bool spapr_memory_hot_unplug_supported(SpaprMachineState *spapr);
 
diff --git a/target/ppc/compat.c b/target/ppc/compat.c
index 7949a24f5a..ebef2cccec 100644
--- a/target/ppc/compat.c
+++ b/target/ppc/compat.c
@@ -229,6 +229,25 @@ int ppc_set_compat_all(uint32_t compat_pvr, Error **errp)
     return 0;
 }
 
+/* To be used when the machine is not running */
+int ppc_init_compat_all(uint32_t compat_pvr, Error **errp)
+{
+    CPUState *cs;
+
+    CPU_FOREACH(cs) {
+        PowerPCCPU *cpu = POWERPC_CPU(cs);
+        int ret;
+
+        ret = ppc_set_compat(cpu, compat_pvr, errp);
+
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
+    return 0;
+}
+
 int ppc_compat_max_vthreads(PowerPCCPU *cpu)
 {
     const CompatInfo *compat = compat_by_pvr(cpu->compat_pvr);
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 9e491e05eb..f8fe0db5cd 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1504,6 +1504,7 @@ int ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp);
 
 #if !defined(CONFIG_USER_ONLY)
 int ppc_set_compat_all(uint32_t compat_pvr, Error **errp);
+int ppc_init_compat_all(uint32_t compat_pvr, Error **errp);
 #endif
 int ppc_compat_max_vthreads(PowerPCCPU *cpu);
 void ppc_compat_add_property(Object *obj, const char *name,
-- 
2.40.1

[PATCH v2 16/19] spapr: Fix record-replay machine reset consuming too many events

[Nicholas Piggin]

spapr_machine_reset gets a random number to populate the device-tree
rng seed with. When loading a snapshot for record-replay, the machine
is reset again, and that tries to consume the random event record
again, crashing due to inconsistent record

Fix this by saving the seed to populate the device tree with, and
skipping the rng on snapshot load.

Acked-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/ppc/spapr.c         | 12 +++++++++---
 include/hw/ppc/spapr.h |  1 +
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 7d84244f03..ecfbdb0030 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1022,7 +1022,6 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt, bool reset)
 {
     MachineState *machine = MACHINE(spapr);
     SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
-    uint8_t rng_seed[32];
     int chosen;
 
     _FDT(chosen = fdt_add_subnode(fdt, 0, "chosen"));
@@ -1100,8 +1099,7 @@ static void spapr_dt_chosen(SpaprMachineState *spapr, void *fdt, bool reset)
         spapr_dt_ov5_platform_support(spapr, fdt, chosen);
     }
 
-    qemu_guest_getrandom_nofail(rng_seed, sizeof(rng_seed));
-    _FDT(fdt_setprop(fdt, chosen, "rng-seed", rng_seed, sizeof(rng_seed)));
+    _FDT(fdt_setprop(fdt, chosen, "rng-seed", spapr->fdt_rng_seed, 32));
 
     _FDT(spapr_dt_ovec(fdt, chosen, spapr->ov5_cas, "ibm,architecture-vec-5"));
 }
@@ -1654,6 +1652,14 @@ static void spapr_machine_reset(MachineState *machine, ShutdownCause reason)
     void *fdt;
     int rc;
 
+    if (reason != SHUTDOWN_CAUSE_SNAPSHOT_LOAD) {
+        /*
+         * Record-replay snapshot load must not consume random, this was
+         * already replayed from initial machine reset.
+         */
+        qemu_guest_getrandom_nofail(spapr->fdt_rng_seed, 32);
+    }
+
     pef_kvm_reset(machine->cgs, &error_fatal);
     spapr_caps_apply(spapr);
 
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index f47e8419a5..f4bd204d86 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -204,6 +204,7 @@ struct SpaprMachineState {
     uint32_t fdt_size;
     uint32_t fdt_initial_size;
     void *fdt_blob;
+    uint8_t fdt_rng_seed[32];
     long kernel_size;
     bool kernel_le;
     uint64_t kernel_addr;
-- 
2.40.1

[PATCH v2 17/19] tests/avocado: boot ppc64 pseries replay-record test to Linux VFS mount

[Nicholas Piggin]

This the ppc64 record-replay test is able to replay the full kernel boot
so try enabling it.

Acked-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/avocado/replay_kernel.py | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/avocado/replay_kernel.py b/tests/avocado/replay_kernel.py
index 79c607b0e7..a18610542e 100644
--- a/tests/avocado/replay_kernel.py
+++ b/tests/avocado/replay_kernel.py
@@ -255,8 +255,7 @@ def test_ppc64_pseries(self):
         kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
 
         kernel_command_line = self.KERNEL_COMMON_COMMAND_LINE + 'console=hvc0'
-        # icount is not good enough for PPC64 for complete boot yet
-        console_pattern = 'Kernel command line: %s' % kernel_command_line
+        console_pattern = 'VFS: Cannot open root device'
         self.run_rr(kernel_path, kernel_command_line, console_pattern)
 
     def test_ppc64_powernv(self):
-- 
2.40.1

[PATCH v2 18/19] tests/avocado: reverse-debugging cope with re-executing breakpoints

[Nicholas Piggin]

The reverse-debugging test creates a trace, then replays it and:

1. Steps the first 10 instructions and records their addresses.
2. Steps backward and verifies their addresses match.
3. Runs to (near) the end of the trace.
4. Sets breakpoints on the first 10 instructions.
5. Continues backward and verifies execution stops at the last
   breakpoint.

Step 5 breaks if any of the other 9 breakpoints are re-executed in the
trace after the 10th instruction is run, because those will be
unexpectedly hit when reverse continuing. This situation does arise
with the ppc pseries machine, the SLOF bios branches to its own entry
point.

Deal with this by switching steps 3 and 4, so the trace will be run to
the end *or* one of the breakpoints being re-executed. Step 5 then
reverses from there to the 10th instruction will not hit a breakpoint in
between, by definition.

Another step is added between steps 2 and 3, which steps forward over
the first 10 instructions and verifies their addresses, to support this.

Reviewed-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/avocado/reverse_debugging.py | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/tests/avocado/reverse_debugging.py b/tests/avocado/reverse_debugging.py
index 680c314cfc..7d1a478df1 100644
--- a/tests/avocado/reverse_debugging.py
+++ b/tests/avocado/reverse_debugging.py
@@ -150,16 +150,33 @@ def reverse_debugging(self, shift=7, args=None):
             self.check_pc(g, addr)
             logger.info('found position %x' % addr)
 
-        logger.info('seeking to the end (icount %s)' % (last_icount - 1))
-        vm.qmp('replay-break', icount=last_icount - 1)
-        # continue - will return after pausing
-        g.cmd(b'c', b'T02thread:01;')
+        # visit the recorded instruction in forward order
+        logger.info('stepping forward')
+        for addr in steps:
+            self.check_pc(g, addr)
+            self.gdb_step(g)
+            logger.info('found position %x' % addr)
 
+        # set breakpoints for the instructions just stepped over
         logger.info('setting breakpoints')
         for addr in steps:
             # hardware breakpoint at addr with len=1
             g.cmd(b'Z1,%x,1' % addr, b'OK')
 
+        # this may hit a breakpoint if first instructions are executed
+        # again
+        logger.info('continuing execution')
+        vm.qmp('replay-break', icount=last_icount - 1)
+        # continue - will return after pausing
+        # This could stop at the end and get a T02 return, or by
+        # re-executing one of the breakpoints and get a T05 return.
+        g.cmd(b'c')
+        if self.vm_get_icount(vm) == last_icount - 1:
+            logger.info('reached the end (icount %s)' % (last_icount - 1))
+        else:
+            logger.info('hit a breakpoint again at %x (icount %s)' %
+                        (self.get_pc(g), self.vm_get_icount(vm)))
+
         logger.info('running reverse continue to reach %x' % steps[-1])
         # reverse continue - will return after stopping at the breakpoint
         g.cmd(b'bc', b'T05thread:01;')
-- 
2.40.1

[PATCH v2 19/19] tests/avocado: ppc64 reverse debugging tests for pseries and powernv

[Nicholas Piggin]

These machines run reverse-debugging well enough to pass basic tests.
Wire them up.

Reviewed-by: Pavel Dovgalyuk <Pavel.Dovgalyuk@ispras.ru>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 tests/avocado/reverse_debugging.py | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/tests/avocado/reverse_debugging.py b/tests/avocado/reverse_debugging.py
index 7d1a478df1..fc47874eda 100644
--- a/tests/avocado/reverse_debugging.py
+++ b/tests/avocado/reverse_debugging.py
@@ -233,3 +233,32 @@ def test_aarch64_virt(self):
 
         self.reverse_debugging(
             args=('-kernel', kernel_path))
+
+class ReverseDebugging_ppc64(ReverseDebugging):
+    """
+    :avocado: tags=accel:tcg
+    """
+
+    REG_PC = 0x40
+
+    # unidentified gitlab timeout problem
+    @skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
+    def test_ppc64_pseries(self):
+        """
+        :avocado: tags=arch:ppc64
+        :avocado: tags=machine:pseries
+        """
+        # SLOF branches back to its entry point, which causes this test
+        # to take the 'hit a breakpoint again' path. That's not a problem,
+        # just slightly different than the other machines.
+        self.endian_is_le = False
+        self.reverse_debugging()
+
+    @skipIf(os.getenv('GITLAB_CI'), 'Running on GitLab')
+    def test_ppc64_powernv(self):
+        """
+        :avocado: tags=arch:ppc64
+        :avocado: tags=machine:powernv
+        """
+        self.endian_is_le = False
+        self.reverse_debugging()
-- 
2.40.1

Re: [PATCH v2 for-8.2 00/19] ppc: record-replay enablement and fixes

[Cédric Le Goater]

Hello,

On 8/8/23 06:19, Nicholas Piggin wrote:
> The patches in this series has been seen a few times in various
> iterations.
> 
> There are two main pieces, some assorted small fixes and tests for
> record-replay, plus a large set of decrementer fixes. I merged these
> into one series rather than send decrementer fixes alone first, because
> record-replay has been very good at uncovering timer problems, so it's
> good to have those test cases in at the same time IMO.
> 
> Some of the fixes we might take to stable, but unclear which.
> Decrementer fixes were a bit of a tangle so maybe we just leave those
> alone since they work okay.
> 
> The decrementer is not emulated perfectly still. Underflow from -ve
> to +ve is not implemented, for one. I started doing that but it's not
> trivial so better stop here for now.
> 
> For record-replay, pseries is now quite solid with rr. Surely some
> issues to iron out but it is becoming usable.
> 
> powernv record-replay has some known problems migrating edge-triggered
> decrementer, and edge triggered msgsnd. Also it seems to get stuck in
> xive init somewhere when replaying from checkpoint, so there is probably
> some state in xive not being reset. But at least it runs the avocado
> test and seems close to working, so I've added that test case so we
> don't go backwards (ha!).
> 
> Other machine types might not be too far off if there is interest. I
> found it quite difficult to find these problems though, reverse
> debugging will sometimes just lock up, stop at wrong location, or abort
> with wrong event. Difficult understand what went wrong. Worst case I had
> to basically bisect the replay of the trace, and find the minimum length
> of replay that hit the problem -- that sometimes would land near a
> mtDEC or timer interrupt or similar.
> 
> Thanks,
> Nick
> 
> Nicholas Piggin (19):
>    ppc/vhyp: reset exception state when handling vhyp hcall
>    ppc/vof: Fix missed fields in VOF cleanup
>    hw/ppc/ppc.c: Tidy over-long lines
>    hw/ppc: Introduce functions for conversion between timebase and
>      nanoseconds
>    host-utils: Add muldiv64_round_up
>    hw/ppc: Round up the decrementer interval when converting to ns
>    hw/ppc: Avoid decrementer rounding errors
>    target/ppc: Sign-extend large decrementer to 64-bits
>    hw/ppc: Always store the decrementer value
>    target/ppc: Migrate DECR SPR
>    hw/ppc: Reset timebase facilities on machine reset
>    hw/ppc: Read time only once to perform decrementer write
>    target/ppc: Fix CPU reservation migration for record-replay
>    target/ppc: Fix timebase reset with record-replay
>    spapr: Fix machine reset deadlock from replay-record
>    spapr: Fix record-replay machine reset consuming too many events
>    tests/avocado: boot ppc64 pseries replay-record test to Linux VFS
>      mount
>    tests/avocado: reverse-debugging cope with re-executing breakpoints
>    tests/avocado: ppc64 reverse debugging tests for pseries and powernv
> 
>   hw/ppc/mac_oldworld.c              |   1 +
>   hw/ppc/pegasos2.c                  |   1 +
>   hw/ppc/pnv_core.c                  |   2 +
>   hw/ppc/ppc.c                       | 236 +++++++++++++++++++----------
>   hw/ppc/prep.c                      |   1 +
>   hw/ppc/spapr.c                     |  32 +++-
>   hw/ppc/spapr_cpu_core.c            |   2 +
>   hw/ppc/vof.c                       |   2 +
>   include/hw/ppc/ppc.h               |   3 +-
>   include/hw/ppc/spapr.h             |   2 +
>   include/qemu/host-utils.h          |  21 ++-
>   target/ppc/compat.c                |  19 +++
>   target/ppc/cpu.h                   |   3 +
>   target/ppc/excp_helper.c           |   3 +
>   target/ppc/machine.c               |  40 ++++-
>   target/ppc/translate.c             |   4 +
>   tests/avocado/replay_kernel.py     |   3 +-
>   tests/avocado/reverse_debugging.py |  54 ++++++-
>   18 files changed, 330 insertions(+), 99 deletions(-)
> 

I am preparing a PR with this series. It is time to take a look at it if you
haven't already !

Thanks,

C.