Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upstream patch for mainline Qemu for Aspeed timers #134

Open
bluecmd opened this issue Feb 13, 2019 · 3 comments
Open

Upstream patch for mainline Qemu for Aspeed timers #134

bluecmd opened this issue Feb 13, 2019 · 3 comments
Assignees
Labels
process Tracking bug for something that should be done

Comments

@bluecmd
Copy link
Contributor

bluecmd commented Feb 13, 2019

From #114 the discussion on the mailinglist was to use this following patch instead and upstream it straight to mainline. Not really sure about the reasons why but why not.

Quoting myself:

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.
@bluecmd bluecmd self-assigned this Feb 13, 2019
@bluecmd bluecmd added the process Tracking bug for something that should be done label Feb 13, 2019
@hugelgupf
Copy link
Member

Upstream won't take it? Wow.

@bluecmd
Copy link
Contributor Author

bluecmd commented Feb 17, 2019

It's complicated, but OpenBMC upstream took the original patch with a caveat note that that particular patch probably wouldn't make it mainline upstream. So this issue is to patch the mainline upstream.

@bluecmd
Copy link
Contributor Author

bluecmd commented Mar 14, 2019

From 5719d0770f212b2d3c3e9e30dd7e585b83a7f92b Mon Sep 17 00:00:00 2001
From: Christian Svensson <bluecmd@google.com>
Date: Thu, 14 Mar 2019 10:41:21 +0100
Subject: [PATCH] aspeed/timer: Ensure positive muldiv delta

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 delta being operated on is positive.

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 <bluecmd@google.com>
---
 hw/timer/aspeed_timer.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 5c786e5128..bd65b9a167 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -261,7 +261,11 @@ 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);
+            if (delta >= 0) {
+              t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate);
+            } else {
+              t->start -= muldiv64(-delta, NANOSECONDS_PER_SECOND, rate);
+            }
             aspeed_timer_mod(t);
         }
         break;
-- 
2.21.0.360.g471c308f928-goog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
process Tracking bug for something that should be done
Projects
None yet
Development

No branches or pull requests

2 participants