* [PATCH] target/arm: Add overflow check for gt_recalc_timer
@ 2023-04-06 15:16 Leonid Komarianskyi
2023-04-13 13:24 ` Peter Maydell
0 siblings, 1 reply; 5+ messages in thread
From: Leonid Komarianskyi @ 2023-04-06 15:16 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, qemu-arm, Volodymyr Babchuk, Dmytro Firsov,
Leonid Komarianskyi
If gt_timer is enabled before cval initialization on a virtualized
setup on QEMU, cval equals (UINT64_MAX - 1). Adding an offset value
to this causes an overflow that sets timer into the past, which leads
to infinite loop, because this timer fires immediately and calls
gt_recalc_timer() once more, which in turn sets the timer into the
past again and as a result, QEMU hangs. This patch adds check for
overflowing of the nexttick variable.
Suggested-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Co-Authored-By: Dmytro Firsov <dmytro_firsov@epam.com>
Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
---
target/arm/helper.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 2297626bfb..2fbba15040 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2618,6 +2618,7 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
int istatus = count - offset >= gt->cval;
uint64_t nexttick;
int irqstate;
+ bool nexttick_overflow = false;
gt->ctl = deposit32(gt->ctl, 2, 1, istatus);
@@ -2630,6 +2631,16 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
} else {
/* Next transition is when we hit cval */
nexttick = gt->cval + offset;
+ if (nexttick < offset) {
+ /*
+ * If gt->cval value is close to UINT64_MAX then adding
+ * to it offset can lead to overflow of nexttick variable.
+ * So, this check tests that arguments sum is less than any
+ * addend, and in case it is overflowed we have to mod timer
+ * to INT64_MAX.
+ */
+ nexttick_overflow = true;
+ }
}
/*
* Note that the desired next expiry time might be beyond the
@@ -2637,7 +2648,8 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
* set the timer for as far in the future as possible. When the
* timer expires we will reset the timer for any remaining period.
*/
- if (nexttick > INT64_MAX / gt_cntfrq_period_ns(cpu)) {
+ if ((nexttick > INT64_MAX / gt_cntfrq_period_ns(cpu))
+ || nexttick_overflow) {
timer_mod_ns(cpu->gt_timer[timeridx], INT64_MAX);
} else {
timer_mod(cpu->gt_timer[timeridx], nexttick);
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] target/arm: Add overflow check for gt_recalc_timer
2023-04-06 15:16 [PATCH] target/arm: Add overflow check for gt_recalc_timer Leonid Komarianskyi
@ 2023-04-13 13:24 ` Peter Maydell
2023-11-09 13:55 ` [PATCH v2] " Leonid Komarianskyi
0 siblings, 1 reply; 5+ messages in thread
From: Peter Maydell @ 2023-04-13 13:24 UTC (permalink / raw)
To: Leonid Komarianskyi
Cc: qemu-devel, qemu-arm, Volodymyr Babchuk, Dmytro Firsov
On Thu, 6 Apr 2023 at 16:16, Leonid Komarianskyi
<Leonid_Komarianskyi@epam.com> wrote:
>
> If gt_timer is enabled before cval initialization on a virtualized
> setup on QEMU, cval equals (UINT64_MAX - 1). Adding an offset value
> to this causes an overflow that sets timer into the past, which leads
> to infinite loop, because this timer fires immediately and calls
> gt_recalc_timer() once more, which in turn sets the timer into the
> past again and as a result, QEMU hangs. This patch adds check for
> overflowing of the nexttick variable.
This is https://gitlab.com/qemu-project/qemu/-/issues/60 --
thanks for sending a patch.
> Suggested-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> Co-Authored-By: Dmytro Firsov <dmytro_firsov@epam.com>
> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
> ---
> target/arm/helper.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 2297626bfb..2fbba15040 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2618,6 +2618,7 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
> int istatus = count - offset >= gt->cval;
> uint64_t nexttick;
> int irqstate;
> + bool nexttick_overflow = false;
>
> gt->ctl = deposit32(gt->ctl, 2, 1, istatus);
>
> @@ -2630,6 +2631,16 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
> } else {
> /* Next transition is when we hit cval */
> nexttick = gt->cval + offset;
> + if (nexttick < offset) {
> + /*
> + * If gt->cval value is close to UINT64_MAX then adding
> + * to it offset can lead to overflow of nexttick variable.
> + * So, this check tests that arguments sum is less than any
> + * addend, and in case it is overflowed we have to mod timer
> + * to INT64_MAX.
> + */
> + nexttick_overflow = true;
> + }
Rather than adding in a bool, I think I prefer the version
of the patch in one of the comments to the bug report:
/* Next transition is when we hit cval */
nexttick = gt->cval + offset;
+ if (nexttick < gt->cval) {
+ nexttick = UINT64_MAX;
+ }
i.e. we just saturate nexttick, and then let the existing handling
of "turns out nexttick is too big" handle things.
There is also a comment or two from me in the bug report pointing
out that the handling of wraparound is also wrong in the other
half of this if(); we should look at that too.
> }
> /*
> * Note that the desired next expiry time might be beyond the
> @@ -2637,7 +2648,8 @@ static void gt_recalc_timer(ARMCPU *cpu, int timeridx)
> * set the timer for as far in the future as possible. When the
> * timer expires we will reset the timer for any remaining period.
> */
> - if (nexttick > INT64_MAX / gt_cntfrq_period_ns(cpu)) {
> + if ((nexttick > INT64_MAX / gt_cntfrq_period_ns(cpu))
> + || nexttick_overflow) {
> timer_mod_ns(cpu->gt_timer[timeridx], INT64_MAX);
> } else {
> timer_mod(cpu->gt_timer[timeridx], nexttick);
> --
> 2.25.1
thanks
-- PMM
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] target/arm: Add overflow check for gt_recalc_timer
2023-04-13 13:24 ` Peter Maydell
@ 2023-11-09 13:55 ` Leonid Komarianskyi
2023-11-09 15:13 ` Leonid Komarianskyi
0 siblings, 1 reply; 5+ messages in thread
From: Leonid Komarianskyi @ 2023-11-09 13:55 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, qemu-arm, Volodymyr Babchuk, Dmytro Firsov,
Leonid Komarianskyi
If gt_timer is enabled before cval initialization on a virtualized
setup on QEMU, cval equals (UINT64_MAX - 1). Adding an offset value
to this causes an overflow that sets timer into the past, which leads
to infinite loop, because this timer fires immediately and calls
gt_recalc_timer() once more, which in turn sets the timer into the
past again and as a result, QEMU hangs. This patch adds check for
overflowing of the nexttick variable.
Suggested-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Co-Authored-By: Dmytro Firsov <dmytro_firsov@epam.com>
Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
---
target/arm/helper.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/target/arm/helper.c b/target/arm/helper.c
index 3b22596eab..b4aaa2965b 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -2665,6 +2665,16 @@ 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) {
+ /*
+ * If gt->cval value is close to UINT64_MAX then adding
+ * to it offset can lead to overflow of nexttick variable.
+ * So, this check tests that arguments sum is less than any
+ * addend, and in case it is overflowed we have to mod timer
+ * to INT64_MAX.
+ */
+ nexttick = UINT64_MAX;
+ }
}
/*
* Note that the desired next expiry time might be beyond the
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] target/arm: Add overflow check for gt_recalc_timer
2023-11-09 13:55 ` [PATCH v2] " Leonid Komarianskyi
@ 2023-11-09 15:13 ` Leonid Komarianskyi
2023-11-20 17:40 ` Peter Maydell
0 siblings, 1 reply; 5+ messages in thread
From: Leonid Komarianskyi @ 2023-11-09 15:13 UTC (permalink / raw)
To: peter.maydell; +Cc: qemu-devel, qemu-arm, Volodymyr Babchuk, Dmytro Firsov
Peter Maydell, thank you for your comments.
I apologize for so late response - returned to this issue and now I will
answer faster. I fixed the commit according to your recommendations,
please take a look at the new version.
> There is also a comment or two from me in the bug report pointing
> out that the handling of wraparound is also wrong in the other
> half of this if(); we should look at that too.
I read this topic and as I understand changing the other half of "if" is
not related to the reported issue. Since it affects running virtualized
setups on arm64 QEMU, e.g. Zephyr
(https://github.com/zephyrproject-rtos/zephyr/blob/main//boards/arm64/xenvm/doc/index.rst)
maybe is it worth merging at least this change?
Best regards,
Leonid Komarianskyi.
On 09.11.23 15:55, Leonid Komarianskyi wrote:
> If gt_timer is enabled before cval initialization on a virtualized
> setup on QEMU, cval equals (UINT64_MAX - 1). Adding an offset value
> to this causes an overflow that sets timer into the past, which leads
> to infinite loop, because this timer fires immediately and calls
> gt_recalc_timer() once more, which in turn sets the timer into the
> past again and as a result, QEMU hangs. This patch adds check for
> overflowing of the nexttick variable.
>
> Suggested-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> Co-Authored-By: Dmytro Firsov <dmytro_firsov@epam.com>
> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@epam.com>
> ---
> target/arm/helper.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 3b22596eab..b4aaa2965b 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2665,6 +2665,16 @@ 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) {
> + /*
> + * If gt->cval value is close to UINT64_MAX then adding
> + * to it offset can lead to overflow of nexttick variable.
> + * So, this check tests that arguments sum is less than any
> + * addend, and in case it is overflowed we have to mod timer
> + * to INT64_MAX.
> + */
> + nexttick = UINT64_MAX;
> + }
> }
> /*
> * Note that the desired next expiry time might be beyond the
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] target/arm: Add overflow check for gt_recalc_timer
2023-11-09 15:13 ` Leonid Komarianskyi
@ 2023-11-20 17:40 ` Peter Maydell
0 siblings, 0 replies; 5+ messages in thread
From: Peter Maydell @ 2023-11-20 17:40 UTC (permalink / raw)
To: Leonid Komarianskyi
Cc: qemu-devel, qemu-arm, Volodymyr Babchuk, Dmytro Firsov
On Thu, 9 Nov 2023 at 15:14, Leonid Komarianskyi
<Leonid_Komarianskyi@epam.com> wrote:
>
> Peter Maydell, thank you for your comments.
> I apologize for so late response - returned to this issue and now I will
> answer faster. I fixed the commit according to your recommendations,
> please take a look at the new version.
>
> > There is also a comment or two from me in the bug report pointing
> > out that the handling of wraparound is also wrong in the other
> > half of this if(); we should look at that too.
>
> I read this topic and as I understand changing the other half of "if" is
> not related to the reported issue. Since it affects running virtualized
> setups on arm64 QEMU, e.g. Zephyr
> (https://github.com/zephyrproject-rtos/zephyr/blob/main//boards/arm64/xenvm/doc/index.rst)
> maybe is it worth merging at least this change?
I feel they're really pretty much the same thing -- when we
added support for the timer offset registers we didn't correctly
update the arithmetic that calculates when the next interrupt
line transition happens.
I've just posted my version of a patch that I think should
fix both halves of the if():
https://patchew.org/QEMU/20231120173506.3729884-1-peter.maydell@linaro.org/
Thanks for prodding me into looking at this issue again -- I had
somehow got the mistaken impression that it only happened in
some weird icount sleep=off situations.
thanks
-- PMM
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-11-20 17:41 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-06 15:16 [PATCH] target/arm: Add overflow check for gt_recalc_timer Leonid Komarianskyi
2023-04-13 13:24 ` Peter Maydell
2023-11-09 13:55 ` [PATCH v2] " Leonid Komarianskyi
2023-11-09 15:13 ` Leonid Komarianskyi
2023-11-20 17:40 ` Peter Maydell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.