Re: [PATCH] hw/ppc: Simplify clock update arithmetic

[Nicholas Piggin]

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 <npiggin@gmail.com>
>
> This patch has ben superseded by
>
>   
> 20230629020713.327745-1-npiggin@gmail.com/">https://patchwork.ozlabs.org/project/qemu-ppc/patch/20230629020713.327745-1-npiggin@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)