1. Patchew
  2. QEMU
  3. View series

[PATCH] hw/ppc: Simplify clock update arithmetic

Nicholas Piggin posted 1 patch 10 months, 3 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230625122012.15503-1-npiggin@gmail.com
Maintainers: Daniel Henrique Barboza <danielhb413@gmail.com>, "Cédric Le Goater" <clg@kaod.org>, David Gibson <david@gibson.dropbear.id.au>, Greg Kurz <groug@kaod.org>
hw/ppc/ppc.c | 33 +++++++++++++++++----------------
1 file changed, 17 insertions(+), 16 deletions(-)
[PATCH] hw/ppc: Simplify clock update arithmetic
Posted by Nicholas Piggin 10 months, 3 weeks ago 
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>
---
 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)
-- 
2.40.1
Re: [PATCH] hw/ppc: Simplify clock update arithmetic
Posted by Cédric Le Goater 10 months, 3 weeks ago 
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

  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)
Re: [PATCH] hw/ppc: Simplify clock update arithmetic
Posted by Nicholas Piggin 10 months, 3 weeks ago 
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
>
>   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)

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.