[PATCH] qemu: aspeed_timer: Use signed muldiv for timer resets

[PATCH] qemu: aspeed_timer: Use signed muldiv for timer resets

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

diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 9acd1de485..1a54d85e9d 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -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;
-- 
2.19.1.930.g4563a0d9d0-goog

Re: [PATCH] qemu: aspeed_timer: Use signed muldiv for timer resets

[Cédric Le Goater]

On 11/12/18 3:21 PM, Christian Svensson wrote:
> 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 <bluecmd@google.com>
> ---
>  hw/timer/aspeed_timer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> index 9acd1de485..1a54d85e9d 100644
> --- a/hw/timer/aspeed_timer.c
> +++ b/hw/timer/aspeed_timer.c
> @@ -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,

C. 

>              aspeed_timer_mod(t);
>          }
>          break;
> 

Re: [PATCH] qemu: aspeed_timer: Use signed muldiv for timer resets

[Christian Svensson]

[-- Attachment #1: Type: text/plain, Size: 624 bytes --]

Hi
On Mon, Nov 12, 2018 at 7:37 PM Cédric Le Goater <clg@kaod.org> wrote:
> 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.

[-- Attachment #2: Type: text/html, Size: 893 bytes --]

Re: [PATCH] qemu: aspeed_timer: Use signed muldiv for timer resets

[Andrew Jeffery]

On Tue, 13 Nov 2018, at 05:15, Christian Svensson wrote:
> Hi
> On Mon, Nov 12, 2018 at 7:37 PM Cédric Le Goater <clg@kaod.org> wrote:
> > 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.

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

Re: [PATCH] qemu: aspeed_timer: Use signed muldiv for timer resets

[Christian Svensson]

[-- Attachment #1: Type: text/plain, Size: 1000 bytes --]

On Tue, Nov 13, 2018 at 1:27 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> 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.

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.

[-- Attachment #2: Type: text/html, Size: 1320 bytes --]

Re: [PATCH] qemu: aspeed_timer: Use signed muldiv for timer resets

[Christian Svensson]

[-- Attachment #1: Type: text/plain, Size: 1169 bytes --]

Ping. What's left to be done here?

On Tue, Nov 13, 2018 at 9:58 AM Christian Svensson <blue@cmd.nu> wrote:

>
>
> On Tue, Nov 13, 2018 at 1:27 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> > 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.
>
> 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.
>
>

[-- Attachment #2: Type: text/html, Size: 1701 bytes --]

Re: [PATCH] qemu: aspeed_timer: Use signed muldiv for timer resets

[Andrew Jeffery]

On Fri, 11 Jan 2019, at 23:00, Christian Svensson wrote:
> 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

> 
> On Tue, Nov 13, 2018 at 9:58 AM Christian Svensson <blue@cmd.nu> wrote:
> 
> >
> >
> > On Tue, Nov 13, 2018 at 1:27 AM Andrew Jeffery <andrew@aj.id.au> wrote:
> > > 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.
> >
> > 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.
> >
> >

Re: [PATCH] qemu: aspeed_timer: Use signed muldiv for timer resets

[Christian Svensson]

[-- Attachment #1: Type: text/plain, Size: 645 bytes --]

On Mon, Jan 14, 2019, 03:14 Andrew Jeffery <andrew@aj.id.au wrote:

> On Fri, 11 Jan 2019, at 23:00, Christian Svensson wrote:
> > 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,

[-- Attachment #2: Type: text/html, Size: 1067 bytes --]

Re: [PATCH] qemu: aspeed_timer: Use signed muldiv for timer resets

[Andrew Jeffery]

On Mon, 14 Jan 2019, at 17:48, Christian Svensson wrote:
> On Mon, Jan 14, 2019, 03:14 Andrew Jeffery <andrew@aj.id.au wrote:
> 
> > On Fri, 11 Jan 2019, at 23:00, Christian Svensson wrote:
> > > 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?

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

Andrew