Re: [PATCH v1 1/2] target/arm: detect 64 bit overflow caused by high cval + voff
On Thu, 16 Jan 2020 at 18:45, Peter Maydell wrote:
> There's something odd going on with this code. Adding a bit of context:
>
> uint64_t offset = timeridx == GTIMER_VIRT ?
> cpu->env.cp15.cntvoff_el2 : 0;
> uint64_t count = gt_get_countervalue(&cpu->env);
> /* Note that this must be unsigned 64 bit arithmetic: */
> int istatus = count - offset >= gt->cval;
> [...]
> if (istatus) {
> /* Next transition is when count rolls back over to zero */
> nexttick = UINT64_MAX;
> } else {
> /* Next transition is when we hit cval */
> nexttick = gt->cval + offset;
> }
>
> I think this patch is correct, in that the 'nexttick' values
> are all absolute and this cval/offset combination implies
> that the next timer interrupt is going to be in a future
> so distant we can't even fit the duration in a uint64_t.
>
> But the other half of the 'if' also looks wrong: that's
> for the case of "timer has fired, how long until the
> wraparound causes the interrupt line to go low again?".
> UINT64_MAX is right for the EL1 case where offset is 0,
> but the offset might actually be set such that the wrap
> around happens fairly soon. We want to calculate the
> tick when (count - offset) hits 0, saturated to
> UINT64_MAX. It's getting late here and I couldn't figure
> out what that expression should be with 15 minutes of
> fiddling around with pen and paper diagrams. I'll have another
> go tomorrow if nobody else gets there first...
With a fresher brain:
For the if (istatus) branch we want the absolute tick
when (count - offset) wraps round to 0, saturated to UINT64_MAX.
I think this is:
if (offset <= count) {
nexttick = UINT64_MAX;
} else {
nexttick = offset;
}
Should we consider this a separate bugfix to go in its own patch?
thanks
-- PMM
Re: [PATCH v1 1/2] target/arm: detect 64 bit overflow caused by high cval + voff
On Fri, 10 Jan 2020 at 16:16, Alex Bennée wrote:
>
> If we don't detect this we will be stuck in a busy loop as we schedule
> a timer for before now which will continually trigger gt_recalc_timer
> even though we haven't reached the state required to trigger the IRQ.
>
> Bug: https://bugs.launchpad.net/bugs/1859021
> Cc: 1859...@bugs.launchpad.net
> Signed-off-by: Alex Bennée
> ---
> target/arm/helper.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 19a57a17da5..eb17106f7bd 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2481,6 +2481,9 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
> } else {
> /* Next transition is when we hit cval */
> nexttick = gt->cval + offset;
> +if (nexttick < gt->cval) {
> +nexttick = UINT64_MAX;
> +}
> }
There's something odd going on with this code. Adding a bit of context:
uint64_t offset = timeridx == GTIMER_VIRT ?
cpu->env.cp15.cntvoff_el2 : 0;
uint64_t count = gt_get_countervalue(&cpu->env);
/* Note that this must be unsigned 64 bit arithmetic: */
int istatus = count - offset >= gt->cval;
[...]
if (istatus) {
/* Next transition is when count rolls back over to zero */
nexttick = UINT64_MAX;
} else {
/* Next transition is when we hit cval */
nexttick = gt->cval + offset;
}
I think this patch is correct, in that the 'nexttick' values
are all absolute and this cval/offset combination implies
that the next timer interrupt is going to be in a future
so distant we can't even fit the duration in a uint64_t.
But the other half of the 'if' also looks wrong: that's
for the case of "timer has fired, how long until the
wraparound causes the interrupt line to go low again?".
UINT64_MAX is right for the EL1 case where offset is 0,
but the offset might actually be set such that the wrap
around happens fairly soon. We want to calculate the
tick when (count - offset) hits 0, saturated to
UINT64_MAX. It's getting late here and I couldn't figure
out what that expression should be with 15 minutes of
fiddling around with pen and paper diagrams. I'll have another
go tomorrow if nobody else gets there first...
thanks
-- PMM
[PATCH v1 1/2] target/arm: detect 64 bit overflow caused by high cval + voff
If we don't detect this we will be stuck in a busy loop as we schedule
a timer for before now which will continually trigger gt_recalc_timer
even though we haven't reached the state required to trigger the IRQ.
Bug: https://bugs.launchpad.net/bugs/1859021
Cc: 1859...@bugs.launchpad.net
Signed-off-by: Alex Bennée
---
target/arm/helper.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 19a57a17da5..eb17106f7bd 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2481,6 +2481,9 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
} else {
/* Next transition is when we hit cval */
nexttick = gt->cval + offset;
+if (nexttick < gt->cval) {
+nexttick = UINT64_MAX;
+}
}
/* Note that the desired next expiry time might be beyond the
* signed-64-bit range of a QEMUTimer -- in this case we just
--
2.20.1
