qemu: aspeed_timer: Use signed muldiv for timer resets

PATCH In review Christian Svensson 4 participants 9 messages Nov 12, 2018 – Feb 11, 2019 Arm/Aspeed machine support

Share & tools

Christian Svensson

author

2018-11-12 14:22 UTC #0 lore raw

PATCH 1/1 qemu: aspeed_timer: Use signed muldiv for timer resets

If the host decrements the counter register that results in a negative delta. This is then passed to muldiv64 which only handles unsigned numbers resulting in bogus results.

This fix ensures the data being operated on is signed before it is ultimately casted to the final unsigned value.

Test case: kexec a kernel using aspeed_timer and it will freeze on the second bootup when the kernel initializes the timer. With this patch that no longer happens and the timer appears to run OK.

Signed-off-by Christian Svensson <[email protected]>

---
 hw/timer/aspeed_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

hw/timer/aspeed_timer.c +1 -1

index 9acd1de485..1a54d85e9d 100644
@@ -253,7 +253,7 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
             int64_t delta = (int64_t) value - (int64_t) calculate_ticks(t, now);
             uint32_t rate = calculate_rate(t);
 
-            t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate);
+            t->start = (int64_t)t->start + ((__int128_t)delta * NANOSECONDS_PER_SECOND / rate);
             aspeed_timer_mod(t);
         }
         break;

Signature · 1 line

2.19.1.930.g4563a0d9d0-goog

Christian Svensson

2018-11-12 18:45 UTC #2 lore raw

In reply to Cédric Le Goater

Quoted 5 lines · Cédric Le Goater

isn't QEMU using the helpers from :
include/qemu/host-utils.h
which do about the same ?

Multdiv64 only takes unsigned ints, and while I'm not familiar with the details of how that propagates to the division, it seems to result in nonsensical values for negative inputs.
Before copying this logic here I looked for an equivalent for signed ints but did not find any suitable one, and the expansion seemed simple enough.

You can see more details in
https://github.com/openbmc/qemu/issues/14#issuecomment-437692215.

Cédric Le Goater

2018-11-12 19:55 UTC #1 lore raw

In reply to Christian Svensson

Quoted 21 lines · Christian Svensson

If the host decrements the counter register that results in a negative delta. This is then passed to muldiv64 which only handles unsigned numbers resulting in bogus results.
This fix ensures the data being operated on is signed before it is ultimately casted to the final unsigned value.
Test case: kexec a kernel using aspeed_timer and it will freeze on the second bootup when the kernel initializes the timer. With this patch that no longer happens and the timer appears to run OK.
Signed-off-by Christian Svensson <[email protected]>
---
 hw/timer/aspeed_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
hw/timer/aspeed_timer.c +0 -0
index 9acd1de485..1a54d85e9d 100644

hw/timer/aspeed_timer.c static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg, · line 256

Quoted 5 lines

hw/timer/aspeed_timer.c +1 -1

@@ -253,7 +253,7 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
             int64_t delta = (int64_t) value - (int64_t) calculate_ticks(t, now);
             uint32_t rate = calculate_rate(t);
 
-            t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate);
+            t->start = (int64_t)t->start + ((__int128_t)delta * NANOSECONDS_PER_SECOND / rate);

isn't QEMU using the helpers from :

include/qemu/host-utils.h

which do about the same ?

Thanks,

Quoted 4 lines

hw/timer/aspeed_timer.c +0 -0

             aspeed_timer_mod(t);
         }
         break;

Andrew Jeffery

2018-11-13 00:27 UTC #3 lore raw

In reply to Christian Svensson

Quoted 8 lines · Christian Svensson

Hi
Quoted 5 lines · Cédric Le Goater
isn't QEMU using the helpers from :
include/qemu/host-utils.h
which do about the same ?

Quoted 8 lines

Multdiv64 only takes unsigned ints, and while I'm not familiar with the details of how that propagates to the division, it seems to result in nonsensical values for negative inputs.
Before copying this logic here I looked for an equivalent for signed ints but did not find any suitable one, and the expansion seemed simple enough.
You can see more details in
https://github.com/openbmc/qemu/issues/14#issuecomment-437692215.

By inspection the concept of the patch seems okay to me. However, the issue that host-utils.h takes care of in addition to providing helpers is support for those helpers on systems that don't support 128-bit integers. The patch should be sent to the upstream lists, and I don't think we should be breaking qemu-arm-* generally for systems that we might not care about.

Andrew

Christian Svensson

2018-11-13 08:58 UTC #4 lore raw

Quoted 1 line · Andrew Jeffery

By inspection the concept of the patch seems okay to me. However, the

issue

Quoted 1 line

that host-utils.h takes care of in addition to providing helpers is

support for

Quoted 1 line

those helpers on systems that don't support 128-bit integers. The patch

should

Quoted 2 lines

be sent to the upstream lists, and I don't think we should be breaking qemu-arm-* generally for systems that we might not care about.

Ah, yes - I missed that there were two - one for systems with int128 and one for them
without. Hm, that does complicate things.

Given that we only have one operand that is ever negative, how do you feel about a patch
that goes along the lines of:

if (delta >= 0) {
t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate);
} else {
t->start = (int64_t)t->start - muldiv64(-delta, NANOSECONDS_PER_SECOND,
rate);
}

That should avoid any issues I think, save us/me from implementing a signed muldiv,
but cost a bit on the readable code side.

Christian Svensson

2019-01-11 12:31 UTC #5 lore raw

Ping. What's left to be done here?

Quoted 32 lines · Christian Svensson

Quoted 1 line · Andrew Jeffery
By inspection the concept of the patch seems okay to me. However, the
issue
Quoted 1 line
that host-utils.h takes care of in addition to providing helpers is
support for
Quoted 1 line
those helpers on systems that don't support 128-bit integers. The patch
should
Quoted 2 lines
be sent to the upstream lists, and I don't think we should be breaking qemu-arm-* generally for systems that we might not care about.
Ah, yes - I missed that there were two - one for systems with int128 and one for them
without. Hm, that does complicate things.
Given that we only have one operand that is ever negative, how do you feel about a patch
that goes along the lines of:
if (delta >= 0) {
t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate);
} else {
   t->start = (int64_t)t->start - muldiv64(-delta, NANOSECONDS_PER_SECOND,
rate);
}
That should avoid any issues I think, save us/me from implementing a signed muldiv,
but cost a bit on the readable code side.

Andrew Jeffery

2019-01-14 02:14 UTC #6 lore raw

Quoted 1 line · Christian Svensson

Ping. What's left to be done here?

I've taken Cedric's tree and pushed it to openbmc/qemu master, so the patch is there. Did you send it upstream originally? That's where it should go, but that will probably require fixing the issue I pointed out last time.

Andrew

Quoted 35 lines · Christian Svensson

Quoted 33 lines · Andrew Jeffery
Quoted 1 line
By inspection the concept of the patch seems okay to me. However, the
issue
Quoted 1 line
that host-utils.h takes care of in addition to providing helpers is
support for
Quoted 1 line
those helpers on systems that don't support 128-bit integers. The patch
should
Quoted 2 lines
be sent to the upstream lists, and I don't think we should be breaking qemu-arm-* generally for systems that we might not care about.
Ah, yes - I missed that there were two - one for systems with int128 and one for them
without. Hm, that does complicate things.
Given that we only have one operand that is ever negative, how do you feel about a patch
that goes along the lines of:
if (delta >= 0) {
t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate);
} else {
   t->start = (int64_t)t->start - muldiv64(-delta, NANOSECONDS_PER_SECOND,
rate);
}
That should avoid any issues I think, save us/me from implementing a signed muldiv,
but cost a bit on the readable code side.

Christian Svensson

author

2019-01-14 07:18 UTC #7 lore raw

Quoted 8 lines · On Mon, Jan 14, 2019, 03:14 Andrew Jeffery <[email protected] wrote:

Quoted 1 line · Christian Svensson
Ping. What's left to be done here?
I've taken Cedric's tree and pushed it to openbmc/qemu master, so the patch is there. Did you send it upstream originally? That's where it should go, but that
will probably require fixing the issue I pointed out last time.

Ah, wierd, I swore I checked the repository last week but I must have checked my own fork or something. Thanks for pulling the patch.

As for fixing the issues, any specific concerns with the proposal I replied with back in November?

Thanks,

Andrew Jeffery

2019-02-11 22:52 UTC #8 lore raw

Quoted 11 lines · Christian Svensson

Quoted 8 lines · On Mon, Jan 14, 2019, 03:14 Andrew Jeffery <[email protected] wrote:
Quoted 1 line · Christian Svensson
Ping. What's left to be done here?
I've taken Cedric's tree and pushed it to openbmc/qemu master, so the patch is there. Did you send it upstream originally? That's where it should go, but that
will probably require fixing the issue I pointed out last time.

Quoted 5 lines

Ah, wierd, I swore I checked the repository last week but I must have checked my own fork or something. Thanks for pulling the patch.
As for fixing the issues, any specific concerns with the proposal I replied with back in November?

Sorry for the late reply here. What was the suggestion? I'll try to find the thread in the mean time.

Andrew