qemu: aspeed_timer: Use signed muldiv for timer resets

Message ID 20181112142137.160970-1-bluecmd@google.com
State New
Headers show
Series
  • qemu: aspeed_timer: Use signed muldiv for timer resets
Related show

Commit Message

Christian Svensson Nov. 12, 2018, 2:21 p.m.
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(-)

Comments

Cédric Le Goater Nov. 12, 2018, 6:37 p.m. | #1
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;
>
Christian Svensson Nov. 12, 2018, 6:45 p.m. | #2
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.
<div dir="ltr"><div dir="ltr">Hi<br>On Mon, Nov 12, 2018 at 7:37 PM Cédric Le Goater &lt;<a href="mailto:clg@kaod.org">clg@kaod.org</a>&gt; wrote:<br>&gt; isn&#39;t QEMU using the helpers from :<br>&gt;<br>&gt;         include/qemu/host-utils.h<br>&gt;<br>&gt; which do about the same ?<br><div><br></div><div>Multdiv64 only takes unsigned ints, and while I&#39;m not familiar with the details of how that propagates to the division, it seems to result in nonsensical values for negative inputs.</div><div>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.</div><div><br></div><div>You can see more details in <a href="https://github.com/openbmc/qemu/issues/14#issuecomment-437692215">https://github.com/openbmc/qemu/issues/14#issuecomment-437692215</a>.</div><div><br></div></div></div>
Andrew Jeffery Nov. 13, 2018, 12:27 a.m. | #3
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
Christian Svensson Nov. 13, 2018, 8:58 a.m. | #4
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.
<div dir="ltr"><br><br>On Tue, Nov 13, 2018 at 1:27 AM Andrew Jeffery &lt;<a href="mailto:andrew@aj.id.au">andrew@aj.id.au</a>&gt; wrote:<br>&gt; By inspection the concept of the patch seems okay to me. However, the issue<br>&gt; that host-utils.h takes care of in addition to providing helpers is support for<br>&gt; those helpers on systems that don&#39;t support 128-bit integers. The patch should<br>&gt; be sent to the upstream lists, and I don&#39;t think we should be breaking<br>&gt; qemu-arm-* generally for systems that we might not care about.<div><br></div><div>Ah, yes - I missed that there were two - one for systems with int128 and one for them</div><div>without. Hm, that does complicate things.</div><div><br></div><div>Given that we only have one operand that is ever negative, how do you feel about a patch</div><div>that goes along the lines of:</div><div><br></div><div>if (delta &gt;= 0) {</div><div>  t-&gt;start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate);  <br></div><div>} else {</div><div>  

t-&gt;start = (int64_t)t-&gt;start - muldiv64(-delta, NANOSECONDS_PER_SECOND, rate);</div><div>}</div><div><br></div><div>That should avoid any issues I think, save us/me from implementing a signed muldiv,</div><div>but cost a bit on the readable code side.</div><div><br></div></div>
Christian Svensson Jan. 11, 2019, 12:30 p.m. | #5
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.
>
>
<div dir="ltr">Ping. What&#39;s left to be done here?</div><br><div class="gmail_quote"><div dir="ltr">On Tue, Nov 13, 2018 at 9:58 AM Christian Svensson &lt;<a href="mailto:blue@cmd.nu">blue@cmd.nu</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><br>On Tue, Nov 13, 2018 at 1:27 AM Andrew Jeffery &lt;<a href="mailto:andrew@aj.id.au" target="_blank">andrew@aj.id.au</a>&gt; wrote:<br>&gt; By inspection the concept of the patch seems okay to me. However, the issue<br>&gt; that host-utils.h takes care of in addition to providing helpers is support for<br>&gt; those helpers on systems that don&#39;t support 128-bit integers. The patch should<br>&gt; be sent to the upstream lists, and I don&#39;t think we should be breaking<br>&gt; qemu-arm-* generally for systems that we might not care about.<div><br></div><div>Ah, yes - I missed that there were two - one for systems with int128 and one for them</div><div>without. Hm, that does complicate things.</div><div><br></div><div>Given that we only have one operand that is ever negative, how do you feel about a patch</div><div>that goes along the lines of:</div><div><br></div><div>if (delta &gt;= 0) {</div><div>  t-&gt;start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate);  <br></div><div>} else {</div><div>  

t-&gt;start = (int64_t)t-&gt;start - muldiv64(-delta, NANOSECONDS_PER_SECOND, rate);</div><div>}</div><div><br></div><div>That should avoid any issues I think, save us/me from implementing a signed muldiv,</div><div>but cost a bit on the readable code side.</div><div><br></div></div>
</blockquote></div>
Andrew Jeffery Jan. 14, 2019, 2:14 a.m. | #6
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.
> >
> >
Christian Svensson Jan. 14, 2019, 7:18 a.m. | #7
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,
<div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr">On Mon, Jan 14, 2019, 03:14 Andrew Jeffery &lt;<a href="mailto:andrew@aj.id.au">andrew@aj.id.au</a> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri, 11 Jan 2019, at 23:00, Christian Svensson wrote:<br>
&gt; Ping. What&#39;s left to be done here?<br>
<br>
I&#39;ve taken Cedric&#39;s tree and pushed it to openbmc/qemu master, so the patch<br>
is there. Did you send it upstream originally? That&#39;s where it should go, but that<br>
will probably require fixing the issue I pointed out last time.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">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.</div><div dir="auto"><br></div><div dir="auto">As for fixing the issues, any specific concerns with the proposal I replied with back in November?</div><div dir="auto"><br></div><div dir="auto">Thanks,</div></div>
Andrew Jeffery Feb. 11, 2019, 10:52 p.m. | #8
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

Patch

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;