On Thu Jun 29, 2023 at 3:28 PM AEST, Cédric Le Goater wrote:
> On 6/25/23 14:20, 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. This can lead to an underflow in
> > subtractions in bits that are intended to cancel exactly. This might
> > not cause any real problem, but it is more complicated than necessary
> > to reason about.
> >
> > Simplify this by reading the clock once.
> >
> > Signed-off-by: Nicholas Piggin <npig...@gmail.com>
>
> This patch has ben superseded by
>
>
> https://patchwork.ozlabs.org/project/qemu-ppc/patch/20230629020713.327745-1-npig...@gmail.com/
>
> It is nice to add a v2 prefix, even if you change the change the subject.
Oh yes, I actually forgot I sent that one, I did a bit more testing and
decided it actually was causing the problem. Hence subject and changelog
rewrite.
Thanks,
Nick
>
> Thanks,
>
> C.
>
>
> > ---
> > 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 f4fe1767d6..5d0a09eb5e 100644
> > --- a/hw/ppc/ppc.c
> > +++ b/hw/ppc/ppc.c
> > @@ -536,23 +536,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)
> > @@ -585,23 +586,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)
> > @@ -623,14 +625,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)
