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

[Cédric Le Goater]

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.

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)