| Message ID | 20230629020713.327745-1-npiggin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/ppc: Fix clock update drift | expand |
On 6/29/23 04:07, Nicholas Piggin wrote: > The clock update logic reads the clock twice to compute the new clock > value, with a value derived from the later time subtracted from a value > derived from the earlier time. The delta causes time to be lost. > > This can ultimately result in time becoming unsynchronized between CPUs > and that can cause OS lockups, timeouts, watchdogs, etc. This can be > seen running a KVM guest (that causes lots of TB updates) on a powernv > SMP machine. > > Fix this by reading the clock once. > > Cc: qemu-stable@nongnu.org > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> Fixes: dbdd25065e90 ("Implement time-base start/stop helpers. Implement PowerPC 6xx time-base enable input pin.") Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > --- > I also made a test case that can trigger this with kvm-unit-tests, but > it's been taking me a while to get that upstreamed. > > Thanks, > Nick > > hw/ppc/ppc.c | 33 +++++++++++++++++---------------- > 1 file changed, 17 insertions(+), 16 deletions(-) > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c > index 82e4408c5c..6233f43c01 100644 > --- a/hw/ppc/ppc.c > +++ b/hw/ppc/ppc.c > @@ -535,23 +535,24 @@ static inline void cpu_ppc_store_tb(ppc_tb_t *tb_env, uint64_t vmclk, > void cpu_ppc_store_tbl (CPUPPCState *env, uint32_t value) > { > ppc_tb_t *tb_env = env->tb_env; > + int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > 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, clock, tb_env->tb_offset); > tb &= 0xFFFFFFFF00000000ULL; > - cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > - &tb_env->tb_offset, tb | (uint64_t)value); > + cpu_ppc_store_tb(tb_env, clock, &tb_env->tb_offset, tb | (uint64_t)value); > } > > static inline void _cpu_ppc_store_tbu(CPUPPCState *env, uint32_t value) > { > ppc_tb_t *tb_env = env->tb_env; > + int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > 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, clock, tb_env->tb_offset); > tb &= 0x00000000FFFFFFFFULL; > - cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > - &tb_env->tb_offset, ((uint64_t)value << 32) | tb); > + cpu_ppc_store_tb(tb_env, clock, &tb_env->tb_offset, > + ((uint64_t)value << 32) | tb); > } > > void cpu_ppc_store_tbu (CPUPPCState *env, uint32_t value) > @@ -584,23 +585,24 @@ uint32_t cpu_ppc_load_atbu (CPUPPCState *env) > void cpu_ppc_store_atbl (CPUPPCState *env, uint32_t value) > { > ppc_tb_t *tb_env = env->tb_env; > + int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > 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, clock, tb_env->atb_offset); > tb &= 0xFFFFFFFF00000000ULL; > - cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > - &tb_env->atb_offset, tb | (uint64_t)value); > + cpu_ppc_store_tb(tb_env, clock, &tb_env->atb_offset, tb | (uint64_t)value); > } > > void cpu_ppc_store_atbu (CPUPPCState *env, uint32_t value) > { > ppc_tb_t *tb_env = env->tb_env; > + int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > 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, clock, tb_env->atb_offset); > tb &= 0x00000000FFFFFFFFULL; > - cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > - &tb_env->atb_offset, ((uint64_t)value << 32) | tb); > + cpu_ppc_store_tb(tb_env, clock, &tb_env->atb_offset, > + ((uint64_t)value << 32) | tb); > } > > uint64_t cpu_ppc_load_vtb(CPUPPCState *env) > @@ -622,14 +624,13 @@ void cpu_ppc_store_vtb(CPUPPCState *env, uint64_t value) > void cpu_ppc_store_tbu40(CPUPPCState *env, uint64_t value) > { > ppc_tb_t *tb_env = env->tb_env; > + int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > 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, clock, tb_env->tb_offset); > tb &= 0xFFFFFFUL; > tb |= (value & ~0xFFFFFFUL); > - cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > - &tb_env->tb_offset, tb); > + cpu_ppc_store_tb(tb_env, clock, &tb_env->tb_offset, tb); > } > > static void cpu_ppc_tb_stop (CPUPPCState *env)
On 29/06/2023 04:07, Nicholas Piggin wrote: > The clock update logic reads the clock twice to compute the new clock > value, with a value derived from the later time subtracted from a value > derived from the earlier time. The delta causes time to be lost. > > This can ultimately result in time becoming unsynchronized between CPUs > and that can cause OS lockups, timeouts, watchdogs, etc. This can be > seen running a KVM guest (that causes lots of TB updates) on a powernv > SMP machine. > > Fix this by reading the clock once. > > Cc: qemu-stable@nongnu.org > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- Nice! It must have been a nightmare to debug. Do you know where that ATB spr comes from? I can't relate it to the ISA. Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com> > I also made a test case that can trigger this with kvm-unit-tests, but > it's been taking me a while to get that upstreamed. > > Thanks, > Nick > > hw/ppc/ppc.c | 33 +++++++++++++++++---------------- > 1 file changed, 17 insertions(+), 16 deletions(-) > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c > index 82e4408c5c..6233f43c01 100644 > --- a/hw/ppc/ppc.c > +++ b/hw/ppc/ppc.c > @@ -535,23 +535,24 @@ static inline void cpu_ppc_store_tb(ppc_tb_t *tb_env, uint64_t vmclk, > void cpu_ppc_store_tbl (CPUPPCState *env, uint32_t value) > { > ppc_tb_t *tb_env = env->tb_env; > + int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > 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, clock, tb_env->tb_offset); > tb &= 0xFFFFFFFF00000000ULL; > - cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > - &tb_env->tb_offset, tb | (uint64_t)value); > + cpu_ppc_store_tb(tb_env, clock, &tb_env->tb_offset, tb | (uint64_t)value); > } > > static inline void _cpu_ppc_store_tbu(CPUPPCState *env, uint32_t value) > { > ppc_tb_t *tb_env = env->tb_env; > + int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > 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, clock, tb_env->tb_offset); > tb &= 0x00000000FFFFFFFFULL; > - cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > - &tb_env->tb_offset, ((uint64_t)value << 32) | tb); > + cpu_ppc_store_tb(tb_env, clock, &tb_env->tb_offset, > + ((uint64_t)value << 32) | tb); > } > > void cpu_ppc_store_tbu (CPUPPCState *env, uint32_t value) > @@ -584,23 +585,24 @@ uint32_t cpu_ppc_load_atbu (CPUPPCState *env) > void cpu_ppc_store_atbl (CPUPPCState *env, uint32_t value) > { > ppc_tb_t *tb_env = env->tb_env; > + int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > 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, clock, tb_env->atb_offset); > tb &= 0xFFFFFFFF00000000ULL; > - cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > - &tb_env->atb_offset, tb | (uint64_t)value); > + cpu_ppc_store_tb(tb_env, clock, &tb_env->atb_offset, tb | (uint64_t)value); > } > > void cpu_ppc_store_atbu (CPUPPCState *env, uint32_t value) > { > ppc_tb_t *tb_env = env->tb_env; > + int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > 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, clock, tb_env->atb_offset); > tb &= 0x00000000FFFFFFFFULL; > - cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > - &tb_env->atb_offset, ((uint64_t)value << 32) | tb); > + cpu_ppc_store_tb(tb_env, clock, &tb_env->atb_offset, > + ((uint64_t)value << 32) | tb); > } > > uint64_t cpu_ppc_load_vtb(CPUPPCState *env) > @@ -622,14 +624,13 @@ void cpu_ppc_store_vtb(CPUPPCState *env, uint64_t value) > void cpu_ppc_store_tbu40(CPUPPCState *env, uint64_t value) > { > ppc_tb_t *tb_env = env->tb_env; > + int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > 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, clock, tb_env->tb_offset); > tb &= 0xFFFFFFUL; > tb |= (value & ~0xFFFFFFUL); > - cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > - &tb_env->tb_offset, tb); > + cpu_ppc_store_tb(tb_env, clock, &tb_env->tb_offset, tb); > } > > static void cpu_ppc_tb_stop (CPUPPCState *env)
On 6/29/23 09:04, Frederic Barrat wrote: > > > On 29/06/2023 04:07, Nicholas Piggin wrote: >> The clock update logic reads the clock twice to compute the new clock >> value, with a value derived from the later time subtracted from a value >> derived from the earlier time. The delta causes time to be lost. >> >> This can ultimately result in time becoming unsynchronized between CPUs >> and that can cause OS lockups, timeouts, watchdogs, etc. This can be >> seen running a KVM guest (that causes lots of TB updates) on a powernv >> SMP machine. >> >> Fix this by reading the clock once. >> >> Cc: qemu-stable@nongnu.org >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> --- > > > Nice! It must have been a nightmare to debug. > Do you know where that ATB spr comes from? I can't relate it to the ISA. You should look at the e500 CPU (BookE) specs. C.
On Thu Jun 29, 2023 at 5:34 PM AEST, Cédric Le Goater wrote: > On 6/29/23 09:04, Frederic Barrat wrote: > > > > > > On 29/06/2023 04:07, Nicholas Piggin wrote: > >> The clock update logic reads the clock twice to compute the new clock > >> value, with a value derived from the later time subtracted from a value > >> derived from the earlier time. The delta causes time to be lost. > >> > >> This can ultimately result in time becoming unsynchronized between CPUs > >> and that can cause OS lockups, timeouts, watchdogs, etc. This can be > >> seen running a KVM guest (that causes lots of TB updates) on a powernv > >> SMP machine. > >> > >> Fix this by reading the clock once. > >> > >> Cc: qemu-stable@nongnu.org > >> Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > >> --- > > > > > > Nice! It must have been a nightmare to debug. > > Do you know where that ATB spr comes from? I can't relate it to the ISA. > > You should look at the e500 CPU (BookE) specs. Yeah it's in 2.07 in the Book E. It doesn't look like Linux uses it anywhere so I didn't actually test it, but the pattern looks the same as the others so I think it's should be right. Thanks, Nick
On 6/28/23 23:07, Nicholas Piggin wrote: > The clock update logic reads the clock twice to compute the new clock > value, with a value derived from the later time subtracted from a value > derived from the earlier time. The delta causes time to be lost. > > This can ultimately result in time becoming unsynchronized between CPUs > and that can cause OS lockups, timeouts, watchdogs, etc. This can be > seen running a KVM guest (that causes lots of TB updates) on a powernv > SMP machine. > > Fix this by reading the clock once. > > Cc: qemu-stable@nongnu.org > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- Queued in gitlab.com/danielhb/qemu/tree/ppc-next after adding the "Fixes" tag Cedric mentioned. Thanks, Daniel > I also made a test case that can trigger this with kvm-unit-tests, but > it's been taking me a while to get that upstreamed. > > Thanks, > Nick > > hw/ppc/ppc.c | 33 +++++++++++++++++---------------- > 1 file changed, 17 insertions(+), 16 deletions(-) > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c > index 82e4408c5c..6233f43c01 100644 > --- a/hw/ppc/ppc.c > +++ b/hw/ppc/ppc.c > @@ -535,23 +535,24 @@ static inline void cpu_ppc_store_tb(ppc_tb_t *tb_env, uint64_t vmclk, > void cpu_ppc_store_tbl (CPUPPCState *env, uint32_t value) > { > ppc_tb_t *tb_env = env->tb_env; > + int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > 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, clock, tb_env->tb_offset); > tb &= 0xFFFFFFFF00000000ULL; > - cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > - &tb_env->tb_offset, tb | (uint64_t)value); > + cpu_ppc_store_tb(tb_env, clock, &tb_env->tb_offset, tb | (uint64_t)value); > } > > static inline void _cpu_ppc_store_tbu(CPUPPCState *env, uint32_t value) > { > ppc_tb_t *tb_env = env->tb_env; > + int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > 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, clock, tb_env->tb_offset); > tb &= 0x00000000FFFFFFFFULL; > - cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > - &tb_env->tb_offset, ((uint64_t)value << 32) | tb); > + cpu_ppc_store_tb(tb_env, clock, &tb_env->tb_offset, > + ((uint64_t)value << 32) | tb); > } > > void cpu_ppc_store_tbu (CPUPPCState *env, uint32_t value) > @@ -584,23 +585,24 @@ uint32_t cpu_ppc_load_atbu (CPUPPCState *env) > void cpu_ppc_store_atbl (CPUPPCState *env, uint32_t value) > { > ppc_tb_t *tb_env = env->tb_env; > + int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > 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, clock, tb_env->atb_offset); > tb &= 0xFFFFFFFF00000000ULL; > - cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > - &tb_env->atb_offset, tb | (uint64_t)value); > + cpu_ppc_store_tb(tb_env, clock, &tb_env->atb_offset, tb | (uint64_t)value); > } > > void cpu_ppc_store_atbu (CPUPPCState *env, uint32_t value) > { > ppc_tb_t *tb_env = env->tb_env; > + int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > 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, clock, tb_env->atb_offset); > tb &= 0x00000000FFFFFFFFULL; > - cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > - &tb_env->atb_offset, ((uint64_t)value << 32) | tb); > + cpu_ppc_store_tb(tb_env, clock, &tb_env->atb_offset, > + ((uint64_t)value << 32) | tb); > } > > uint64_t cpu_ppc_load_vtb(CPUPPCState *env) > @@ -622,14 +624,13 @@ void cpu_ppc_store_vtb(CPUPPCState *env, uint64_t value) > void cpu_ppc_store_tbu40(CPUPPCState *env, uint64_t value) > { > ppc_tb_t *tb_env = env->tb_env; > + int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > 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, clock, tb_env->tb_offset); > tb &= 0xFFFFFFUL; > tb |= (value & ~0xFFFFFFUL); > - cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), > - &tb_env->tb_offset, tb); > + cpu_ppc_store_tb(tb_env, clock, &tb_env->tb_offset, tb); > } > > static void cpu_ppc_tb_stop (CPUPPCState *env)
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c index 82e4408c5c..6233f43c01 100644 --- a/hw/ppc/ppc.c +++ b/hw/ppc/ppc.c @@ -535,23 +535,24 @@ static inline void cpu_ppc_store_tb(ppc_tb_t *tb_env, uint64_t vmclk, void cpu_ppc_store_tbl (CPUPPCState *env, uint32_t value) { ppc_tb_t *tb_env = env->tb_env; + int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); 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, clock, tb_env->tb_offset); tb &= 0xFFFFFFFF00000000ULL; - cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), - &tb_env->tb_offset, tb | (uint64_t)value); + cpu_ppc_store_tb(tb_env, clock, &tb_env->tb_offset, tb | (uint64_t)value); } static inline void _cpu_ppc_store_tbu(CPUPPCState *env, uint32_t value) { ppc_tb_t *tb_env = env->tb_env; + int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); 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, clock, tb_env->tb_offset); tb &= 0x00000000FFFFFFFFULL; - cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), - &tb_env->tb_offset, ((uint64_t)value << 32) | tb); + cpu_ppc_store_tb(tb_env, clock, &tb_env->tb_offset, + ((uint64_t)value << 32) | tb); } void cpu_ppc_store_tbu (CPUPPCState *env, uint32_t value) @@ -584,23 +585,24 @@ uint32_t cpu_ppc_load_atbu (CPUPPCState *env) void cpu_ppc_store_atbl (CPUPPCState *env, uint32_t value) { ppc_tb_t *tb_env = env->tb_env; + int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); 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, clock, tb_env->atb_offset); tb &= 0xFFFFFFFF00000000ULL; - cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), - &tb_env->atb_offset, tb | (uint64_t)value); + cpu_ppc_store_tb(tb_env, clock, &tb_env->atb_offset, tb | (uint64_t)value); } void cpu_ppc_store_atbu (CPUPPCState *env, uint32_t value) { ppc_tb_t *tb_env = env->tb_env; + int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); 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, clock, tb_env->atb_offset); tb &= 0x00000000FFFFFFFFULL; - cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), - &tb_env->atb_offset, ((uint64_t)value << 32) | tb); + cpu_ppc_store_tb(tb_env, clock, &tb_env->atb_offset, + ((uint64_t)value << 32) | tb); } uint64_t cpu_ppc_load_vtb(CPUPPCState *env) @@ -622,14 +624,13 @@ void cpu_ppc_store_vtb(CPUPPCState *env, uint64_t value) void cpu_ppc_store_tbu40(CPUPPCState *env, uint64_t value) { ppc_tb_t *tb_env = env->tb_env; + int64_t clock = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); 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, clock, tb_env->tb_offset); tb &= 0xFFFFFFUL; tb |= (value & ~0xFFFFFFUL); - cpu_ppc_store_tb(tb_env, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), - &tb_env->tb_offset, tb); + cpu_ppc_store_tb(tb_env, clock, &tb_env->tb_offset, tb); } static void cpu_ppc_tb_stop (CPUPPCState *env)
The clock update logic reads the clock twice to compute the new clock value, with a value derived from the later time subtracted from a value derived from the earlier time. The delta causes time to be lost. This can ultimately result in time becoming unsynchronized between CPUs and that can cause OS lockups, timeouts, watchdogs, etc. This can be seen running a KVM guest (that causes lots of TB updates) on a powernv SMP machine. Fix this by reading the clock once. Cc: qemu-stable@nongnu.org Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- I also made a test case that can trigger this with kvm-unit-tests, but it's been taking me a while to get that upstreamed. Thanks, Nick hw/ppc/ppc.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)