QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Peter Maydell <peter.maydell@linaro.org>
To: qemu-devel@nongnu.org
Subject: [Bug 1859021] Re: [PATCH v1 1/2] target/arm: detect 64 bit overflow caused by high cval + voff
Date: Thu, 16 Jan 2020 18:45:12 -0000
Message-ID: <CAFEAcA9nNH9pu+8E_YYkiNtzePjZdrEBjK_9zJv+XJaSvcnhmA@mail.gmail.com> (raw)
Message-ID: <20200116184512.-GhyoV50p1lAQuquaf1g3NHI4JxuTU5gwvHnbV2-jwA@z> (raw)
In-Reply-To: <20200110161626.31943-2-alex.bennee@linaro.org>

On Fri, 10 Jan 2020 at 16:16, Alex Bennée <alex.bennee@linaro.org> 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: 1859021@bugs.launchpad.net
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  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

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1859021

Title:
  qemu-system-aarch64 (tcg):  cval + voff overflow not handled, causes
  qemu to hang

Status in QEMU:
  Confirmed

Bug description:
  The Armv8 architecture reference manual states that for any timer set
  (e.g. CNTP* and CNTV*), the condition for such timer to generate an
  interrupt (if enabled & unmasked) is:

  CVAL <= CNT(P/V)CT

  Although this is arguably sloppy coding, I have seen code that is
  therefore assuming it can set CVAL to a very high value (e.g.
  UINT64_MAX) and leave the interrupt enabled in CTL, and never get the
  interrupt.

  On latest master commit as the time of writing, there is an integer
  overflow in target/arm/helper.c gt_recalc_timer affecting the virtual
  timer when the interrupt is enabled in CTL:

      /* Next transition is when we hit cval */
      nexttick = gt->cval + offset;

  When this overflow happens, I notice that qemu is no longer responsive and that I have to SIGKILL the process:
      - qemu takes nearly all the cpu time of the cores it is running on (e.g. 50% cpu usage if running on half the cores) and is completely unresponsive
      - no guest interrupt (reported via -d int) is generated

  Here the minimal code example to reproduce the issue:

      mov     x0, #1
      msr     cntvoff_el2, x0
      mov     x0, #-1
      msr     cntv_cval_el0, x0
      mov     x0, #1
      msr     cntv_ctl_el0, x0 // interrupt generation enabled, not masked; qemu will start to hang here

  Options used:
  -nographic -machine virt,virtualization=on,gic-version=2,accel=tcg -cpu cortex-a57
  -smp 4 -m 1024 -kernel whatever.elf -d unimp,guest_errors,int -semihosting-config enable,target=native
  -serial mon:stdio

  Version used: 4.2

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1859021/+subscriptions


  parent reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-10 16:16 [PATCH v1 0/2] fix for bug 1859021 Alex Bennée
2020-01-10 16:16 ` [PATCH v1 1/2] target/arm: detect 64 bit overflow caused by high cval + voff Alex Bennée
2020-01-10 16:16   ` [Bug 1859021] " Alex Bennée
2020-01-16 18:45   ` Peter Maydell [this message]
2020-01-16 18:45     ` [Bug 1859021] " Peter Maydell
2020-01-17 11:50     ` Peter Maydell
2020-01-17 11:50       ` [Bug 1859021] " Peter Maydell
2020-01-10 16:16 ` [PATCH v1 2/2] tests/tcg: add a vtimer test for aarch64 Alex Bennée
2020-01-17 14:07   ` Peter Maydell
  -- strict thread matches above, loose matches on Subject: below --
2020-01-09 13:24 [Bug 1859021] [NEW] qemu-system-aarch64 (tcg): cval + voff overflow not handled, causes qemu to hang Alex Longwall
2020-01-09 14:44 ` [Bug 1859021] " Alex Bennée
2020-01-09 16:25 ` [RFC PATCH] tests/tcg: add a vtimer test for aarch64 Alex Bennée
2020-01-09 16:25   ` [Bug 1859021] Re: qemu-system-aarch64 (tcg): cval + voff overflow not handled, causes qemu to hang Alex Bennée

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAFEAcA9nNH9pu+8E_YYkiNtzePjZdrEBjK_9zJv+XJaSvcnhmA@mail.gmail.com \
    --to=peter.maydell@linaro.org \
    --cc=1859021@bugs.launchpad.net \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git