* [PATCH] hw/ppc: Simplify clock update arithmetic
@ 2023-06-25 12:20 Nicholas Piggin
2023-06-29 5:28 ` Cédric Le Goater
0 siblings, 1 reply; 3+ messages in thread
From: Nicholas Piggin @ 2023-06-25 12:20 UTC (permalink / raw)
To: qemu-devel
Cc: Nicholas Piggin, qemu-ppc, Daniel Henrique Barboza,
Cédric Le Goater, David Gibson, Greg Kurz,
Harsh Prateek Bora
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] hw/ppc: Simplify clock update arithmetic
2023-06-25 12:20 [PATCH] hw/ppc: Simplify clock update arithmetic Nicholas Piggin
@ 2023-06-29 5:28 ` Cédric Le Goater
2023-06-29 7:42 ` Nicholas Piggin
0 siblings, 1 reply; 3+ messages in thread
From: Cédric Le Goater @ 2023-06-29 5:28 UTC (permalink / raw)
To: Nicholas Piggin, qemu-devel
Cc: qemu-ppc, Daniel Henrique Barboza, David Gibson, Greg Kurz,
Harsh Prateek Bora
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)
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] hw/ppc: Simplify clock update arithmetic
2023-06-29 5:28 ` Cédric Le Goater
@ 2023-06-29 7:42 ` Nicholas Piggin
0 siblings, 0 replies; 3+ messages in thread
From: Nicholas Piggin @ 2023-06-29 7:42 UTC (permalink / raw)
To: Cédric Le Goater, qemu-devel
Cc: qemu-ppc, Daniel Henrique Barboza, David Gibson, Greg Kurz,
Harsh Prateek Bora
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)
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-06-29 7:43 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-25 12:20 [PATCH] hw/ppc: Simplify clock update arithmetic Nicholas Piggin
2023-06-29 5:28 ` Cédric Le Goater
2023-06-29 7:42 ` Nicholas Piggin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).