| Message ID | 20231109135439.1297135-1-leonid_komarianskyi@epam.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] target/arm: Add overflow check for gt_recalc_timer | expand |
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
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
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
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(+)