| Message ID | 20181112142137.160970-1-bluecmd@google.com |
|---|---|
| State | Rejected, archived |
| Headers | show |
| Series | qemu: aspeed_timer: Use signed muldiv for timer resets | expand |
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; >
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 <<a href="mailto:clg@kaod.org">clg@kaod.org</a>> wrote:<br>> isn't QEMU using the helpers from :<br>><br>> include/qemu/host-utils.h<br>><br>> which do about the same ?<br><div><br></div><div>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.</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>
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
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 <<a href="mailto:andrew@aj.id.au">andrew@aj.id.au</a>> wrote:<br>> By inspection the concept of the patch seems okay to me. However, the issue<br>> that host-utils.h takes care of in addition to providing helpers is support for<br>> those helpers on systems that don't support 128-bit integers. The patch should<br>> be sent to the upstream lists, and I don't think we should be breaking<br>> 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 >= 0) {</div><div> t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate); <br></div><div>} else {</div><div> t->start = (int64_t)t->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>
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'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 <<a href="mailto:blue@cmd.nu">blue@cmd.nu</a>> 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 <<a href="mailto:andrew@aj.id.au" target="_blank">andrew@aj.id.au</a>> wrote:<br>> By inspection the concept of the patch seems okay to me. However, the issue<br>> that host-utils.h takes care of in addition to providing helpers is support for<br>> those helpers on systems that don't support 128-bit integers. The patch should<br>> be sent to the upstream lists, and I don't think we should be breaking<br>> 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 >= 0) {</div><div> t->start += muldiv64(delta, NANOSECONDS_PER_SECOND, rate); <br></div><div>} else {</div><div> t->start = (int64_t)t->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>
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. > > > >
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 <<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> > Ping. What's left to be done here?<br> <br> I've taken Cedric's tree and pushed it to openbmc/qemu master, so the patch<br> is there. Did you send it upstream originally? That'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>
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
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;
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(-)