All of lore.kernel.org
 help / color / mirror / Atom feed
* [Bug 1872237] [NEW] SysTick reload behavior emulated incorrectly
@ 2020-04-11 19:59 Bryan Cantrill
  2020-04-11 21:05 ` [Bug 1872237] " Peter Maydell
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Bryan Cantrill @ 2020-04-11 19:59 UTC (permalink / raw)
  To: qemu-devel

Public bug reported:

QEMU's emuation of SysTick on ARM is incorrect with respect to reload
behavior.  This issue is described here, and also in a repository
dedicated to the issue:

  https://github.com/oxidecomputer/qemu-systick-bug


(What follows is in Markdown, which I understand that Launchpad does
not support; see the repository linked above for a rendering of it.)

Take this Rust program:

```rust
#![no_std]
#![no_main]

extern crate panic_semihosting;

use cortex_m_rt::entry;
use cortex_m_semihosting::hprintln;
use cortex_m::peripheral::syst::SystClkSource;
use cortex_m::peripheral::SYST;

fn delay(syst: &mut cortex_m::peripheral::SYST, ms: u32)
{
    /*
     * Configured for the LM3S6965, which has a default CPU clock of 12 Mhz
     */
    let reload = 12_000 * ms;

    syst.set_reload(reload);
    syst.clear_current();
    syst.enable_counter();

    hprintln!("waiting for {} ms (SYST_CVR={}) ...",
        ms, SYST::get_current()
    ).unwrap();

    while !syst.has_wrapped() {}

    hprintln!("  ... done (SYST_CVR={})\n",
SYST::get_current()).unwrap();

    syst.disable_counter();
}

#[entry]
fn main() -> ! {
    let p = cortex_m::Peripherals::take().unwrap();
    let mut syst = p.SYST;

    syst.set_clock_source(SystClkSource::Core);

    loop {
        delay(&mut syst, 1000);
        delay(&mut syst, 100);
    }
}
```

This program should oscillate between waiting for one second and waiting
for 100 milliseconds.  Under hardware, this is more or less what it does
(depending on core clock frequency); e.g., from an STM32F4107 (connected via
OCD and with semi-hosting enabled):

```
waiting for 1000 ms (SYST_CVR=11999949) ...
  ... done (SYST_CVR=11999902)

waiting for 100 ms (SYST_CVR=1199949) ...
  ... done (SYST_CVR=1199897)

waiting for 1000 ms (SYST_CVR=11999949) ...
  ... done (SYST_CVR=11999885)

waiting for 100 ms (SYST_CVR=1199949) ...
  ... done (SYST_CVR=1199897)

waiting for 1000 ms (SYST_CVR=11999949) ...
  ... done (SYST_CVR=11999885)

```

Under QEMU, however, its behavior is quite different:

```
$ cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.03s
     Running `qemu-system-arm -cpu cortex-m3 -machine lm3s6965evb -nographic -semihosting-config enable=on,target=native -kernel target/thumbv7m-none-eabi/debug/qemu-systick-bug`
waiting for 1000 ms (SYST_CVR=11999658) ...
  ... done (SYST_CVR=11986226)

waiting for 100 ms (SYST_CVR=0) ...
  ... done (SYST_CVR=1186560)

waiting for 1000 ms (SYST_CVR=1185996) ...
  ... done (SYST_CVR=11997350)

waiting for 100 ms (SYST_CVR=0) ...
  ... done (SYST_CVR=1186581)
```

In addition to the values being strangely wrong, the behavior is wrong:
the first wait correctly waits for 1000 ms -- but the subsequent wait
(which should be for 100 ms) is in fact 1000 ms, and the next wait (which
should be for 1000 ms) is in fact 100 ms.  (That is, it appears as if
the periods of the two delays have been switched.)

The problems is that the QEMU ARM emulation code does not reload SYST_CVR from
SYST_RVR if SYST_CSR.ENABLE is not set -- and moreover, that SYST_CVR is
not reloaded from SYST_RVR even when SYST_CSR.ENABLE becomes set.  This is
very explicit; from
<a
href="https://github.com/qemu/qemu/blob/8bac3ba57eecc466b7e73dabf7d19328a59f684e/hw/timer/armv7m_systick.c#L42-L60">hw/timer/armv7m_systick.c</a>:

```c
static void systick_reload(SysTickState *s, int reset)
{
    /* The Cortex-M3 Devices Generic User Guide says that "When the
     * ENABLE bit is set to 1, the counter loads the RELOAD value from the
     * SYST RVR register and then counts down". So, we need to check the
     * ENABLE bit before reloading the value.
     */
    trace_systick_reload();

    if ((s->control & SYSTICK_ENABLE) == 0) {
        return;
    }

    if (reset) {
        s->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
    }
    s->tick += (s->reload + 1) * systick_scale(s);
    timer_mod(s->timer, s->tick);
}
```

The statement in the code is stronger than the statement in the
<a href="https://static.docs.arm.com/ddi0403/eb/DDI0403E_B_armv7m_arm.pdf">ARMv7-M Architecture Reference Manual</a> (sec B3.3.1):

> Writing to SYST_CVR clears both the register and the COUNTFLAG status
> bit to zero. This causes the SysTick logic to reload SYST_CVR from SYST_RVR
> on the next timer clock. A write to SYST_CVR does not trigger the
> SysTick exception logic.

Note that this does not mention the behavior on a write to SYST_CVR when
SYST_CSR.ENABLE is not set -- and in particular, does *not* say that writes to
SYST_CVR will be ignored if SYST_CSR.ENABLE is not set.

Section 3.3.1 does go on to say:

> The SYST_CVR value is UNKNOWN on reset. Before enabling the SysTick counter,u
> software must write the required counter value to SYST_RVR, and then write
> to SYST_CVR. This clears SYST_CVR to zero. When enabled, the counter 
> reloads the value from SYST_RVR, and counts down from that value, rather]
> than from an arbitrary value.

(This is more or less what has been quoted in the implementation of
`systick_reload`, above.)  This note does **not** say, however, that writes
to SYST_CVR will be discarded when not enabled, but rather that the counting
will only begin (and the value in SYST_RVR loaded or reloaded) when
SYST_CSR.ENABLE becomes set.

While QEMU's behavior does not match that of the hardware (and is therefore
at some level malfunctioning), there is additional behavior that is very
clearly incorrect: once SYST_CSR.ENABLE is set, not only will SYST_CVR
continue to return 0 (that is, the counter will not be enabled),
SYST_CSR.COUNTFLAG will become set when the *old* value of SYST_RVR ticks
have elapsed!  This is wrong in both regards: if SYST_CVR is not counting
down, SYST_CSR.COUNTFLAG should never become set -- and it certainly
shouldn't match a value of SYST_RVR that has been overwritten in the
interim!

In terms of fixing this, it's helpful to understand the
<a
href="https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01217.html">context
around this change</a>:

> Consider the following pseudo code to configure SYSTICK (The
> recommended programming sequence from "the definitive guide to the
> arm cortex-m3"):
>    SYSTICK Reload Value Register = 0xffff
>    SYSTICK Current Value Register = 0
>    SYSTICK Control and Status Register = 0x7
>
> The pseudo code "SYSTICK Current Value Register = 0" leads to invoking
> systick_reload(). As a consequence, the systick.tick member is updated
> and the systick timer starts to count down when the ENABLE bit of
> SYSTICK Control and Status Register is cleared.
>
> The worst case is that: during the system initialization, the reset
> value of the SYSTICK Control and Status Register is 0x00000000. 
> When the code "SYSTICK Current Value Register = 0" is executed, the
> systick.tick member is accumulated with "(s->systick.reload + 1) *
> systick_scale(s)". The systick_scale() gets the external_ref_clock
> scale because the CLKSOURCE bit of the SYSTICK Control and Status
> Register is cleared. This is the incorrect behavior because of the
> code "SYSTICK Control and Status Register = 0x7". Actually, we want
> the processor clock instead of the external reference clock.
>
> This incorrect behavior defers the generation of the first interrupt. 
>
> The patch fixes the above-mentioned issue by setting the systick.tick
> member and modifying the systick timer only if the ENABLE bit of
> the SYSTICK Control and Status Register is set.
>
> In addition, the Cortex-M3 Devices Generic User Guide mentioned that
> "When ENABLE is set to 1, the counter loads the RELOAD value from the
> SYST RVR register and then counts down". This patch adheres to the
> statement of the user guide.

This fix is simply incorrect -- or at the least, incomplete:
a write to SYST_CVR must clear any cached state
such that a subsequent write to SYST_CSR.ENABLE will correctly cause
a reload.  Here is a diff that solves the problem without re-introducing
the behavior that the original fix was trying to correct:

```diff
diff --git a/hw/timer/armv7m_systick.c b/hw/timer/armv7m_systick.c
index 74c58bcf24..3f7b267c2d 100644
--- a/hw/timer/armv7m_systick.c
+++ b/hw/timer/armv7m_systick.c
@@ -181,6 +181,15 @@ static MemTxResult systick_write(void *opaque, hwaddr addr,
         break;
     case 0x8: /* SysTick Current Value.  Writes reload the timer.  */
         systick_reload(s, 1);
+
+        if ((s->control & SYSTICK_ENABLE) == 0) {
+            /*
+             * If we're not enabled, explicitly clear our tick value to
+             * assure that when we do become enabled, we correctly reload.
+             */
+            s->tick = 0;
+        }
+
         s->control &= ~SYSTICK_COUNTFLAG;
         break;
     default:
```

** Affects: qemu
     Importance: Undecided
         Status: New


** Tags: arm

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

Title:
  SysTick reload behavior emulated incorrectly

Status in QEMU:
  New

Bug description:
  QEMU's emuation of SysTick on ARM is incorrect with respect to reload
  behavior.  This issue is described here, and also in a repository
  dedicated to the issue:

    https://github.com/oxidecomputer/qemu-systick-bug

  
  (What follows is in Markdown, which I understand that Launchpad does
  not support; see the repository linked above for a rendering of it.)

  Take this Rust program:

  ```rust
  #![no_std]
  #![no_main]

  extern crate panic_semihosting;

  use cortex_m_rt::entry;
  use cortex_m_semihosting::hprintln;
  use cortex_m::peripheral::syst::SystClkSource;
  use cortex_m::peripheral::SYST;

  fn delay(syst: &mut cortex_m::peripheral::SYST, ms: u32)
  {
      /*
       * Configured for the LM3S6965, which has a default CPU clock of 12 Mhz
       */
      let reload = 12_000 * ms;

      syst.set_reload(reload);
      syst.clear_current();
      syst.enable_counter();

      hprintln!("waiting for {} ms (SYST_CVR={}) ...",
          ms, SYST::get_current()
      ).unwrap();

      while !syst.has_wrapped() {}

      hprintln!("  ... done (SYST_CVR={})\n",
  SYST::get_current()).unwrap();

      syst.disable_counter();
  }

  #[entry]
  fn main() -> ! {
      let p = cortex_m::Peripherals::take().unwrap();
      let mut syst = p.SYST;

      syst.set_clock_source(SystClkSource::Core);

      loop {
          delay(&mut syst, 1000);
          delay(&mut syst, 100);
      }
  }
  ```

  This program should oscillate between waiting for one second and waiting
  for 100 milliseconds.  Under hardware, this is more or less what it does
  (depending on core clock frequency); e.g., from an STM32F4107 (connected via
  OCD and with semi-hosting enabled):

  ```
  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999902)

  waiting for 100 ms (SYST_CVR=1199949) ...
    ... done (SYST_CVR=1199897)

  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999885)

  waiting for 100 ms (SYST_CVR=1199949) ...
    ... done (SYST_CVR=1199897)

  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999885)

  ```

  Under QEMU, however, its behavior is quite different:

  ```
  $ cargo run
      Finished dev [unoptimized + debuginfo] target(s) in 0.03s
       Running `qemu-system-arm -cpu cortex-m3 -machine lm3s6965evb -nographic -semihosting-config enable=on,target=native -kernel target/thumbv7m-none-eabi/debug/qemu-systick-bug`
  waiting for 1000 ms (SYST_CVR=11999658) ...
    ... done (SYST_CVR=11986226)

  waiting for 100 ms (SYST_CVR=0) ...
    ... done (SYST_CVR=1186560)

  waiting for 1000 ms (SYST_CVR=1185996) ...
    ... done (SYST_CVR=11997350)

  waiting for 100 ms (SYST_CVR=0) ...
    ... done (SYST_CVR=1186581)
  ```

  In addition to the values being strangely wrong, the behavior is wrong:
  the first wait correctly waits for 1000 ms -- but the subsequent wait
  (which should be for 100 ms) is in fact 1000 ms, and the next wait (which
  should be for 1000 ms) is in fact 100 ms.  (That is, it appears as if
  the periods of the two delays have been switched.)

  The problems is that the QEMU ARM emulation code does not reload SYST_CVR from
  SYST_RVR if SYST_CSR.ENABLE is not set -- and moreover, that SYST_CVR is
  not reloaded from SYST_RVR even when SYST_CSR.ENABLE becomes set.  This is
  very explicit; from
  <a
  href="https://github.com/qemu/qemu/blob/8bac3ba57eecc466b7e73dabf7d19328a59f684e/hw/timer/armv7m_systick.c#L42-L60">hw/timer/armv7m_systick.c</a>:

  ```c
  static void systick_reload(SysTickState *s, int reset)
  {
      /* The Cortex-M3 Devices Generic User Guide says that "When the
       * ENABLE bit is set to 1, the counter loads the RELOAD value from the
       * SYST RVR register and then counts down". So, we need to check the
       * ENABLE bit before reloading the value.
       */
      trace_systick_reload();

      if ((s->control & SYSTICK_ENABLE) == 0) {
          return;
      }

      if (reset) {
          s->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
      }
      s->tick += (s->reload + 1) * systick_scale(s);
      timer_mod(s->timer, s->tick);
  }
  ```

  The statement in the code is stronger than the statement in the
  <a href="https://static.docs.arm.com/ddi0403/eb/DDI0403E_B_armv7m_arm.pdf">ARMv7-M Architecture Reference Manual</a> (sec B3.3.1):

  > Writing to SYST_CVR clears both the register and the COUNTFLAG status
  > bit to zero. This causes the SysTick logic to reload SYST_CVR from SYST_RVR
  > on the next timer clock. A write to SYST_CVR does not trigger the
  > SysTick exception logic.

  Note that this does not mention the behavior on a write to SYST_CVR when
  SYST_CSR.ENABLE is not set -- and in particular, does *not* say that writes to
  SYST_CVR will be ignored if SYST_CSR.ENABLE is not set.

  Section 3.3.1 does go on to say:

  > The SYST_CVR value is UNKNOWN on reset. Before enabling the SysTick counter,u
  > software must write the required counter value to SYST_RVR, and then write
  > to SYST_CVR. This clears SYST_CVR to zero. When enabled, the counter 
  > reloads the value from SYST_RVR, and counts down from that value, rather]
  > than from an arbitrary value.

  (This is more or less what has been quoted in the implementation of
  `systick_reload`, above.)  This note does **not** say, however, that writes
  to SYST_CVR will be discarded when not enabled, but rather that the counting
  will only begin (and the value in SYST_RVR loaded or reloaded) when
  SYST_CSR.ENABLE becomes set.

  While QEMU's behavior does not match that of the hardware (and is therefore
  at some level malfunctioning), there is additional behavior that is very
  clearly incorrect: once SYST_CSR.ENABLE is set, not only will SYST_CVR
  continue to return 0 (that is, the counter will not be enabled),
  SYST_CSR.COUNTFLAG will become set when the *old* value of SYST_RVR ticks
  have elapsed!  This is wrong in both regards: if SYST_CVR is not counting
  down, SYST_CSR.COUNTFLAG should never become set -- and it certainly
  shouldn't match a value of SYST_RVR that has been overwritten in the
  interim!

  In terms of fixing this, it's helpful to understand the
  <a
  href="https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01217.html">context
  around this change</a>:

  > Consider the following pseudo code to configure SYSTICK (The
  > recommended programming sequence from "the definitive guide to the
  > arm cortex-m3"):
  >    SYSTICK Reload Value Register = 0xffff
  >    SYSTICK Current Value Register = 0
  >    SYSTICK Control and Status Register = 0x7
  >
  > The pseudo code "SYSTICK Current Value Register = 0" leads to invoking
  > systick_reload(). As a consequence, the systick.tick member is updated
  > and the systick timer starts to count down when the ENABLE bit of
  > SYSTICK Control and Status Register is cleared.
  >
  > The worst case is that: during the system initialization, the reset
  > value of the SYSTICK Control and Status Register is 0x00000000. 
  > When the code "SYSTICK Current Value Register = 0" is executed, the
  > systick.tick member is accumulated with "(s->systick.reload + 1) *
  > systick_scale(s)". The systick_scale() gets the external_ref_clock
  > scale because the CLKSOURCE bit of the SYSTICK Control and Status
  > Register is cleared. This is the incorrect behavior because of the
  > code "SYSTICK Control and Status Register = 0x7". Actually, we want
  > the processor clock instead of the external reference clock.
  >
  > This incorrect behavior defers the generation of the first interrupt. 
  >
  > The patch fixes the above-mentioned issue by setting the systick.tick
  > member and modifying the systick timer only if the ENABLE bit of
  > the SYSTICK Control and Status Register is set.
  >
  > In addition, the Cortex-M3 Devices Generic User Guide mentioned that
  > "When ENABLE is set to 1, the counter loads the RELOAD value from the
  > SYST RVR register and then counts down". This patch adheres to the
  > statement of the user guide.

  This fix is simply incorrect -- or at the least, incomplete:
  a write to SYST_CVR must clear any cached state
  such that a subsequent write to SYST_CSR.ENABLE will correctly cause
  a reload.  Here is a diff that solves the problem without re-introducing
  the behavior that the original fix was trying to correct:

  ```diff
  diff --git a/hw/timer/armv7m_systick.c b/hw/timer/armv7m_systick.c
  index 74c58bcf24..3f7b267c2d 100644
  --- a/hw/timer/armv7m_systick.c
  +++ b/hw/timer/armv7m_systick.c
  @@ -181,6 +181,15 @@ static MemTxResult systick_write(void *opaque, hwaddr addr,
           break;
       case 0x8: /* SysTick Current Value.  Writes reload the timer.  */
           systick_reload(s, 1);
  +
  +        if ((s->control & SYSTICK_ENABLE) == 0) {
  +            /*
  +             * If we're not enabled, explicitly clear our tick value to
  +             * assure that when we do become enabled, we correctly reload.
  +             */
  +            s->tick = 0;
  +        }
  +
           s->control &= ~SYSTICK_COUNTFLAG;
           break;
       default:
  ```

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [Bug 1872237] Re: SysTick reload behavior emulated incorrectly
  2020-04-11 19:59 [Bug 1872237] [NEW] SysTick reload behavior emulated incorrectly Bryan Cantrill
@ 2020-04-11 21:05 ` Peter Maydell
  2020-04-13 17:14 ` Bryan Cantrill
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Peter Maydell @ 2020-04-11 21:05 UTC (permalink / raw)
  To: qemu-devel

Yeah, our systick implementation is broken; I've known about this for
ages but never got round to trying to work through what the right way to
implement the behaviour is. I do have some more time to work on
M-profile stuff coming up at some point so I might get round to this if
nobody else does first.

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

Title:
  SysTick reload behavior emulated incorrectly

Status in QEMU:
  New

Bug description:
  QEMU's emuation of SysTick on ARM is incorrect with respect to reload
  behavior.  This issue is described here, and also in a repository
  dedicated to the issue:

    https://github.com/oxidecomputer/qemu-systick-bug

  
  (What follows is in Markdown, which I understand that Launchpad does
  not support; see the repository linked above for a rendering of it.)

  Take this Rust program:

  ```rust
  #![no_std]
  #![no_main]

  extern crate panic_semihosting;

  use cortex_m_rt::entry;
  use cortex_m_semihosting::hprintln;
  use cortex_m::peripheral::syst::SystClkSource;
  use cortex_m::peripheral::SYST;

  fn delay(syst: &mut cortex_m::peripheral::SYST, ms: u32)
  {
      /*
       * Configured for the LM3S6965, which has a default CPU clock of 12 Mhz
       */
      let reload = 12_000 * ms;

      syst.set_reload(reload);
      syst.clear_current();
      syst.enable_counter();

      hprintln!("waiting for {} ms (SYST_CVR={}) ...",
          ms, SYST::get_current()
      ).unwrap();

      while !syst.has_wrapped() {}

      hprintln!("  ... done (SYST_CVR={})\n",
  SYST::get_current()).unwrap();

      syst.disable_counter();
  }

  #[entry]
  fn main() -> ! {
      let p = cortex_m::Peripherals::take().unwrap();
      let mut syst = p.SYST;

      syst.set_clock_source(SystClkSource::Core);

      loop {
          delay(&mut syst, 1000);
          delay(&mut syst, 100);
      }
  }
  ```

  This program should oscillate between waiting for one second and waiting
  for 100 milliseconds.  Under hardware, this is more or less what it does
  (depending on core clock frequency); e.g., from an STM32F4107 (connected via
  OCD and with semi-hosting enabled):

  ```
  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999902)

  waiting for 100 ms (SYST_CVR=1199949) ...
    ... done (SYST_CVR=1199897)

  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999885)

  waiting for 100 ms (SYST_CVR=1199949) ...
    ... done (SYST_CVR=1199897)

  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999885)

  ```

  Under QEMU, however, its behavior is quite different:

  ```
  $ cargo run
      Finished dev [unoptimized + debuginfo] target(s) in 0.03s
       Running `qemu-system-arm -cpu cortex-m3 -machine lm3s6965evb -nographic -semihosting-config enable=on,target=native -kernel target/thumbv7m-none-eabi/debug/qemu-systick-bug`
  waiting for 1000 ms (SYST_CVR=11999658) ...
    ... done (SYST_CVR=11986226)

  waiting for 100 ms (SYST_CVR=0) ...
    ... done (SYST_CVR=1186560)

  waiting for 1000 ms (SYST_CVR=1185996) ...
    ... done (SYST_CVR=11997350)

  waiting for 100 ms (SYST_CVR=0) ...
    ... done (SYST_CVR=1186581)
  ```

  In addition to the values being strangely wrong, the behavior is wrong:
  the first wait correctly waits for 1000 ms -- but the subsequent wait
  (which should be for 100 ms) is in fact 1000 ms, and the next wait (which
  should be for 1000 ms) is in fact 100 ms.  (That is, it appears as if
  the periods of the two delays have been switched.)

  The problems is that the QEMU ARM emulation code does not reload SYST_CVR from
  SYST_RVR if SYST_CSR.ENABLE is not set -- and moreover, that SYST_CVR is
  not reloaded from SYST_RVR even when SYST_CSR.ENABLE becomes set.  This is
  very explicit; from
  <a
  href="https://github.com/qemu/qemu/blob/8bac3ba57eecc466b7e73dabf7d19328a59f684e/hw/timer/armv7m_systick.c#L42-L60">hw/timer/armv7m_systick.c</a>:

  ```c
  static void systick_reload(SysTickState *s, int reset)
  {
      /* The Cortex-M3 Devices Generic User Guide says that "When the
       * ENABLE bit is set to 1, the counter loads the RELOAD value from the
       * SYST RVR register and then counts down". So, we need to check the
       * ENABLE bit before reloading the value.
       */
      trace_systick_reload();

      if ((s->control & SYSTICK_ENABLE) == 0) {
          return;
      }

      if (reset) {
          s->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
      }
      s->tick += (s->reload + 1) * systick_scale(s);
      timer_mod(s->timer, s->tick);
  }
  ```

  The statement in the code is stronger than the statement in the
  <a href="https://static.docs.arm.com/ddi0403/eb/DDI0403E_B_armv7m_arm.pdf">ARMv7-M Architecture Reference Manual</a> (sec B3.3.1):

  > Writing to SYST_CVR clears both the register and the COUNTFLAG status
  > bit to zero. This causes the SysTick logic to reload SYST_CVR from SYST_RVR
  > on the next timer clock. A write to SYST_CVR does not trigger the
  > SysTick exception logic.

  Note that this does not mention the behavior on a write to SYST_CVR when
  SYST_CSR.ENABLE is not set -- and in particular, does *not* say that writes to
  SYST_CVR will be ignored if SYST_CSR.ENABLE is not set.

  Section 3.3.1 does go on to say:

  > The SYST_CVR value is UNKNOWN on reset. Before enabling the SysTick counter,u
  > software must write the required counter value to SYST_RVR, and then write
  > to SYST_CVR. This clears SYST_CVR to zero. When enabled, the counter 
  > reloads the value from SYST_RVR, and counts down from that value, rather]
  > than from an arbitrary value.

  (This is more or less what has been quoted in the implementation of
  `systick_reload`, above.)  This note does **not** say, however, that writes
  to SYST_CVR will be discarded when not enabled, but rather that the counting
  will only begin (and the value in SYST_RVR loaded or reloaded) when
  SYST_CSR.ENABLE becomes set.

  While QEMU's behavior does not match that of the hardware (and is therefore
  at some level malfunctioning), there is additional behavior that is very
  clearly incorrect: once SYST_CSR.ENABLE is set, not only will SYST_CVR
  continue to return 0 (that is, the counter will not be enabled),
  SYST_CSR.COUNTFLAG will become set when the *old* value of SYST_RVR ticks
  have elapsed!  This is wrong in both regards: if SYST_CVR is not counting
  down, SYST_CSR.COUNTFLAG should never become set -- and it certainly
  shouldn't match a value of SYST_RVR that has been overwritten in the
  interim!

  In terms of fixing this, it's helpful to understand the
  <a
  href="https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01217.html">context
  around this change</a>:

  > Consider the following pseudo code to configure SYSTICK (The
  > recommended programming sequence from "the definitive guide to the
  > arm cortex-m3"):
  >    SYSTICK Reload Value Register = 0xffff
  >    SYSTICK Current Value Register = 0
  >    SYSTICK Control and Status Register = 0x7
  >
  > The pseudo code "SYSTICK Current Value Register = 0" leads to invoking
  > systick_reload(). As a consequence, the systick.tick member is updated
  > and the systick timer starts to count down when the ENABLE bit of
  > SYSTICK Control and Status Register is cleared.
  >
  > The worst case is that: during the system initialization, the reset
  > value of the SYSTICK Control and Status Register is 0x00000000. 
  > When the code "SYSTICK Current Value Register = 0" is executed, the
  > systick.tick member is accumulated with "(s->systick.reload + 1) *
  > systick_scale(s)". The systick_scale() gets the external_ref_clock
  > scale because the CLKSOURCE bit of the SYSTICK Control and Status
  > Register is cleared. This is the incorrect behavior because of the
  > code "SYSTICK Control and Status Register = 0x7". Actually, we want
  > the processor clock instead of the external reference clock.
  >
  > This incorrect behavior defers the generation of the first interrupt. 
  >
  > The patch fixes the above-mentioned issue by setting the systick.tick
  > member and modifying the systick timer only if the ENABLE bit of
  > the SYSTICK Control and Status Register is set.
  >
  > In addition, the Cortex-M3 Devices Generic User Guide mentioned that
  > "When ENABLE is set to 1, the counter loads the RELOAD value from the
  > SYST RVR register and then counts down". This patch adheres to the
  > statement of the user guide.

  This fix is simply incorrect -- or at the least, incomplete:
  a write to SYST_CVR must clear any cached state
  such that a subsequent write to SYST_CSR.ENABLE will correctly cause
  a reload.  Here is a diff that solves the problem without re-introducing
  the behavior that the original fix was trying to correct:

  ```diff
  diff --git a/hw/timer/armv7m_systick.c b/hw/timer/armv7m_systick.c
  index 74c58bcf24..3f7b267c2d 100644
  --- a/hw/timer/armv7m_systick.c
  +++ b/hw/timer/armv7m_systick.c
  @@ -181,6 +181,15 @@ static MemTxResult systick_write(void *opaque, hwaddr addr,
           break;
       case 0x8: /* SysTick Current Value.  Writes reload the timer.  */
           systick_reload(s, 1);
  +
  +        if ((s->control & SYSTICK_ENABLE) == 0) {
  +            /*
  +             * If we're not enabled, explicitly clear our tick value to
  +             * assure that when we do become enabled, we correctly reload.
  +             */
  +            s->tick = 0;
  +        }
  +
           s->control &= ~SYSTICK_COUNTFLAG;
           break;
       default:
  ```

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Bug 1872237] Re: SysTick reload behavior emulated incorrectly
  2020-04-11 19:59 [Bug 1872237] [NEW] SysTick reload behavior emulated incorrectly Bryan Cantrill
  2020-04-11 21:05 ` [Bug 1872237] " Peter Maydell
@ 2020-04-13 17:14 ` Bryan Cantrill
  2020-04-13 18:14 ` Peter Maydell
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Bryan Cantrill @ 2020-04-13 17:14 UTC (permalink / raw)
  To: qemu-devel

@pmaydell: Thanks for the quick response!  For whatever it's worth, I think
that there's definitely a bunch of interest in the M-profile work:  in the
embedded Rust space (for example) Cortex-M is very much the reference 
platform.  Viz. the Embedded Rust Book:

  https://rust-embedded.github.io/book/

For this, we have been using an xPack QEMU fork:

  https://github.com/xpack-dev-tools/qemu-arm-xpack

Having a QEMU that had first-class support for a widely-available development
platform (e.g., the STM32F Discovery) would be of great interest to this
community -- and I'm sure many others!

To prevent filing of issues that are already known:  is there a list of
known issues with the M-profile with QEMU?

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

Title:
  SysTick reload behavior emulated incorrectly

Status in QEMU:
  New

Bug description:
  QEMU's emuation of SysTick on ARM is incorrect with respect to reload
  behavior.  This issue is described here, and also in a repository
  dedicated to the issue:

    https://github.com/oxidecomputer/qemu-systick-bug

  
  (What follows is in Markdown, which I understand that Launchpad does
  not support; see the repository linked above for a rendering of it.)

  Take this Rust program:

  ```rust
  #![no_std]
  #![no_main]

  extern crate panic_semihosting;

  use cortex_m_rt::entry;
  use cortex_m_semihosting::hprintln;
  use cortex_m::peripheral::syst::SystClkSource;
  use cortex_m::peripheral::SYST;

  fn delay(syst: &mut cortex_m::peripheral::SYST, ms: u32)
  {
      /*
       * Configured for the LM3S6965, which has a default CPU clock of 12 Mhz
       */
      let reload = 12_000 * ms;

      syst.set_reload(reload);
      syst.clear_current();
      syst.enable_counter();

      hprintln!("waiting for {} ms (SYST_CVR={}) ...",
          ms, SYST::get_current()
      ).unwrap();

      while !syst.has_wrapped() {}

      hprintln!("  ... done (SYST_CVR={})\n",
  SYST::get_current()).unwrap();

      syst.disable_counter();
  }

  #[entry]
  fn main() -> ! {
      let p = cortex_m::Peripherals::take().unwrap();
      let mut syst = p.SYST;

      syst.set_clock_source(SystClkSource::Core);

      loop {
          delay(&mut syst, 1000);
          delay(&mut syst, 100);
      }
  }
  ```

  This program should oscillate between waiting for one second and waiting
  for 100 milliseconds.  Under hardware, this is more or less what it does
  (depending on core clock frequency); e.g., from an STM32F4107 (connected via
  OCD and with semi-hosting enabled):

  ```
  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999902)

  waiting for 100 ms (SYST_CVR=1199949) ...
    ... done (SYST_CVR=1199897)

  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999885)

  waiting for 100 ms (SYST_CVR=1199949) ...
    ... done (SYST_CVR=1199897)

  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999885)

  ```

  Under QEMU, however, its behavior is quite different:

  ```
  $ cargo run
      Finished dev [unoptimized + debuginfo] target(s) in 0.03s
       Running `qemu-system-arm -cpu cortex-m3 -machine lm3s6965evb -nographic -semihosting-config enable=on,target=native -kernel target/thumbv7m-none-eabi/debug/qemu-systick-bug`
  waiting for 1000 ms (SYST_CVR=11999658) ...
    ... done (SYST_CVR=11986226)

  waiting for 100 ms (SYST_CVR=0) ...
    ... done (SYST_CVR=1186560)

  waiting for 1000 ms (SYST_CVR=1185996) ...
    ... done (SYST_CVR=11997350)

  waiting for 100 ms (SYST_CVR=0) ...
    ... done (SYST_CVR=1186581)
  ```

  In addition to the values being strangely wrong, the behavior is wrong:
  the first wait correctly waits for 1000 ms -- but the subsequent wait
  (which should be for 100 ms) is in fact 1000 ms, and the next wait (which
  should be for 1000 ms) is in fact 100 ms.  (That is, it appears as if
  the periods of the two delays have been switched.)

  The problems is that the QEMU ARM emulation code does not reload SYST_CVR from
  SYST_RVR if SYST_CSR.ENABLE is not set -- and moreover, that SYST_CVR is
  not reloaded from SYST_RVR even when SYST_CSR.ENABLE becomes set.  This is
  very explicit; from
  <a
  href="https://github.com/qemu/qemu/blob/8bac3ba57eecc466b7e73dabf7d19328a59f684e/hw/timer/armv7m_systick.c#L42-L60">hw/timer/armv7m_systick.c</a>:

  ```c
  static void systick_reload(SysTickState *s, int reset)
  {
      /* The Cortex-M3 Devices Generic User Guide says that "When the
       * ENABLE bit is set to 1, the counter loads the RELOAD value from the
       * SYST RVR register and then counts down". So, we need to check the
       * ENABLE bit before reloading the value.
       */
      trace_systick_reload();

      if ((s->control & SYSTICK_ENABLE) == 0) {
          return;
      }

      if (reset) {
          s->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
      }
      s->tick += (s->reload + 1) * systick_scale(s);
      timer_mod(s->timer, s->tick);
  }
  ```

  The statement in the code is stronger than the statement in the
  <a href="https://static.docs.arm.com/ddi0403/eb/DDI0403E_B_armv7m_arm.pdf">ARMv7-M Architecture Reference Manual</a> (sec B3.3.1):

  > Writing to SYST_CVR clears both the register and the COUNTFLAG status
  > bit to zero. This causes the SysTick logic to reload SYST_CVR from SYST_RVR
  > on the next timer clock. A write to SYST_CVR does not trigger the
  > SysTick exception logic.

  Note that this does not mention the behavior on a write to SYST_CVR when
  SYST_CSR.ENABLE is not set -- and in particular, does *not* say that writes to
  SYST_CVR will be ignored if SYST_CSR.ENABLE is not set.

  Section 3.3.1 does go on to say:

  > The SYST_CVR value is UNKNOWN on reset. Before enabling the SysTick counter,u
  > software must write the required counter value to SYST_RVR, and then write
  > to SYST_CVR. This clears SYST_CVR to zero. When enabled, the counter 
  > reloads the value from SYST_RVR, and counts down from that value, rather]
  > than from an arbitrary value.

  (This is more or less what has been quoted in the implementation of
  `systick_reload`, above.)  This note does **not** say, however, that writes
  to SYST_CVR will be discarded when not enabled, but rather that the counting
  will only begin (and the value in SYST_RVR loaded or reloaded) when
  SYST_CSR.ENABLE becomes set.

  While QEMU's behavior does not match that of the hardware (and is therefore
  at some level malfunctioning), there is additional behavior that is very
  clearly incorrect: once SYST_CSR.ENABLE is set, not only will SYST_CVR
  continue to return 0 (that is, the counter will not be enabled),
  SYST_CSR.COUNTFLAG will become set when the *old* value of SYST_RVR ticks
  have elapsed!  This is wrong in both regards: if SYST_CVR is not counting
  down, SYST_CSR.COUNTFLAG should never become set -- and it certainly
  shouldn't match a value of SYST_RVR that has been overwritten in the
  interim!

  In terms of fixing this, it's helpful to understand the
  <a
  href="https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01217.html">context
  around this change</a>:

  > Consider the following pseudo code to configure SYSTICK (The
  > recommended programming sequence from "the definitive guide to the
  > arm cortex-m3"):
  >    SYSTICK Reload Value Register = 0xffff
  >    SYSTICK Current Value Register = 0
  >    SYSTICK Control and Status Register = 0x7
  >
  > The pseudo code "SYSTICK Current Value Register = 0" leads to invoking
  > systick_reload(). As a consequence, the systick.tick member is updated
  > and the systick timer starts to count down when the ENABLE bit of
  > SYSTICK Control and Status Register is cleared.
  >
  > The worst case is that: during the system initialization, the reset
  > value of the SYSTICK Control and Status Register is 0x00000000. 
  > When the code "SYSTICK Current Value Register = 0" is executed, the
  > systick.tick member is accumulated with "(s->systick.reload + 1) *
  > systick_scale(s)". The systick_scale() gets the external_ref_clock
  > scale because the CLKSOURCE bit of the SYSTICK Control and Status
  > Register is cleared. This is the incorrect behavior because of the
  > code "SYSTICK Control and Status Register = 0x7". Actually, we want
  > the processor clock instead of the external reference clock.
  >
  > This incorrect behavior defers the generation of the first interrupt. 
  >
  > The patch fixes the above-mentioned issue by setting the systick.tick
  > member and modifying the systick timer only if the ENABLE bit of
  > the SYSTICK Control and Status Register is set.
  >
  > In addition, the Cortex-M3 Devices Generic User Guide mentioned that
  > "When ENABLE is set to 1, the counter loads the RELOAD value from the
  > SYST RVR register and then counts down". This patch adheres to the
  > statement of the user guide.

  This fix is simply incorrect -- or at the least, incomplete:
  a write to SYST_CVR must clear any cached state
  such that a subsequent write to SYST_CSR.ENABLE will correctly cause
  a reload.  Here is a diff that solves the problem without re-introducing
  the behavior that the original fix was trying to correct:

  ```diff
  diff --git a/hw/timer/armv7m_systick.c b/hw/timer/armv7m_systick.c
  index 74c58bcf24..3f7b267c2d 100644
  --- a/hw/timer/armv7m_systick.c
  +++ b/hw/timer/armv7m_systick.c
  @@ -181,6 +181,15 @@ static MemTxResult systick_write(void *opaque, hwaddr addr,
           break;
       case 0x8: /* SysTick Current Value.  Writes reload the timer.  */
           systick_reload(s, 1);
  +
  +        if ((s->control & SYSTICK_ENABLE) == 0) {
  +            /*
  +             * If we're not enabled, explicitly clear our tick value to
  +             * assure that when we do become enabled, we correctly reload.
  +             */
  +            s->tick = 0;
  +        }
  +
           s->control &= ~SYSTICK_COUNTFLAG;
           break;
       default:
  ```

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Bug 1872237] Re: SysTick reload behavior emulated incorrectly
  2020-04-11 19:59 [Bug 1872237] [NEW] SysTick reload behavior emulated incorrectly Bryan Cantrill
  2020-04-11 21:05 ` [Bug 1872237] " Peter Maydell
  2020-04-13 17:14 ` Bryan Cantrill
@ 2020-04-13 18:14 ` Peter Maydell
  2020-10-15 15:23 ` Peter Maydell
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Peter Maydell @ 2020-04-13 18:14 UTC (permalink / raw)
  To: qemu-devel

Other than the systick issue, I think the core M-profile emulation
should be pretty solid (bugs are always possible, of course). We have
support for v6M (cortex-m0), v7M (cortex-m3, m4) and v8M (cortex-m33,
including the security extension) and at least some board models for all
of those. No v8.1M yet (that is next on my todo list). Board and device
support in QEMU is in general more likely to have missing features than
the core cpu emulation, but I can't think of anything specifically
annoyingly missing offhand. Of the upstream board models, the various
MPS2 boards are I think fairly solid, as are the Musca board models. We
also have a Netduino 2 and a Netduino Plus 2 model which I think ought
to be OK. The Stellaris boards (lm3s6965evb and lm3s811evb) are rather
older and I dunno that I'd recommend them unless you have no other
choice.

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

Title:
  SysTick reload behavior emulated incorrectly

Status in QEMU:
  New

Bug description:
  QEMU's emuation of SysTick on ARM is incorrect with respect to reload
  behavior.  This issue is described here, and also in a repository
  dedicated to the issue:

    https://github.com/oxidecomputer/qemu-systick-bug

  
  (What follows is in Markdown, which I understand that Launchpad does
  not support; see the repository linked above for a rendering of it.)

  Take this Rust program:

  ```rust
  #![no_std]
  #![no_main]

  extern crate panic_semihosting;

  use cortex_m_rt::entry;
  use cortex_m_semihosting::hprintln;
  use cortex_m::peripheral::syst::SystClkSource;
  use cortex_m::peripheral::SYST;

  fn delay(syst: &mut cortex_m::peripheral::SYST, ms: u32)
  {
      /*
       * Configured for the LM3S6965, which has a default CPU clock of 12 Mhz
       */
      let reload = 12_000 * ms;

      syst.set_reload(reload);
      syst.clear_current();
      syst.enable_counter();

      hprintln!("waiting for {} ms (SYST_CVR={}) ...",
          ms, SYST::get_current()
      ).unwrap();

      while !syst.has_wrapped() {}

      hprintln!("  ... done (SYST_CVR={})\n",
  SYST::get_current()).unwrap();

      syst.disable_counter();
  }

  #[entry]
  fn main() -> ! {
      let p = cortex_m::Peripherals::take().unwrap();
      let mut syst = p.SYST;

      syst.set_clock_source(SystClkSource::Core);

      loop {
          delay(&mut syst, 1000);
          delay(&mut syst, 100);
      }
  }
  ```

  This program should oscillate between waiting for one second and waiting
  for 100 milliseconds.  Under hardware, this is more or less what it does
  (depending on core clock frequency); e.g., from an STM32F4107 (connected via
  OCD and with semi-hosting enabled):

  ```
  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999902)

  waiting for 100 ms (SYST_CVR=1199949) ...
    ... done (SYST_CVR=1199897)

  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999885)

  waiting for 100 ms (SYST_CVR=1199949) ...
    ... done (SYST_CVR=1199897)

  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999885)

  ```

  Under QEMU, however, its behavior is quite different:

  ```
  $ cargo run
      Finished dev [unoptimized + debuginfo] target(s) in 0.03s
       Running `qemu-system-arm -cpu cortex-m3 -machine lm3s6965evb -nographic -semihosting-config enable=on,target=native -kernel target/thumbv7m-none-eabi/debug/qemu-systick-bug`
  waiting for 1000 ms (SYST_CVR=11999658) ...
    ... done (SYST_CVR=11986226)

  waiting for 100 ms (SYST_CVR=0) ...
    ... done (SYST_CVR=1186560)

  waiting for 1000 ms (SYST_CVR=1185996) ...
    ... done (SYST_CVR=11997350)

  waiting for 100 ms (SYST_CVR=0) ...
    ... done (SYST_CVR=1186581)
  ```

  In addition to the values being strangely wrong, the behavior is wrong:
  the first wait correctly waits for 1000 ms -- but the subsequent wait
  (which should be for 100 ms) is in fact 1000 ms, and the next wait (which
  should be for 1000 ms) is in fact 100 ms.  (That is, it appears as if
  the periods of the two delays have been switched.)

  The problems is that the QEMU ARM emulation code does not reload SYST_CVR from
  SYST_RVR if SYST_CSR.ENABLE is not set -- and moreover, that SYST_CVR is
  not reloaded from SYST_RVR even when SYST_CSR.ENABLE becomes set.  This is
  very explicit; from
  <a
  href="https://github.com/qemu/qemu/blob/8bac3ba57eecc466b7e73dabf7d19328a59f684e/hw/timer/armv7m_systick.c#L42-L60">hw/timer/armv7m_systick.c</a>:

  ```c
  static void systick_reload(SysTickState *s, int reset)
  {
      /* The Cortex-M3 Devices Generic User Guide says that "When the
       * ENABLE bit is set to 1, the counter loads the RELOAD value from the
       * SYST RVR register and then counts down". So, we need to check the
       * ENABLE bit before reloading the value.
       */
      trace_systick_reload();

      if ((s->control & SYSTICK_ENABLE) == 0) {
          return;
      }

      if (reset) {
          s->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
      }
      s->tick += (s->reload + 1) * systick_scale(s);
      timer_mod(s->timer, s->tick);
  }
  ```

  The statement in the code is stronger than the statement in the
  <a href="https://static.docs.arm.com/ddi0403/eb/DDI0403E_B_armv7m_arm.pdf">ARMv7-M Architecture Reference Manual</a> (sec B3.3.1):

  > Writing to SYST_CVR clears both the register and the COUNTFLAG status
  > bit to zero. This causes the SysTick logic to reload SYST_CVR from SYST_RVR
  > on the next timer clock. A write to SYST_CVR does not trigger the
  > SysTick exception logic.

  Note that this does not mention the behavior on a write to SYST_CVR when
  SYST_CSR.ENABLE is not set -- and in particular, does *not* say that writes to
  SYST_CVR will be ignored if SYST_CSR.ENABLE is not set.

  Section 3.3.1 does go on to say:

  > The SYST_CVR value is UNKNOWN on reset. Before enabling the SysTick counter,u
  > software must write the required counter value to SYST_RVR, and then write
  > to SYST_CVR. This clears SYST_CVR to zero. When enabled, the counter 
  > reloads the value from SYST_RVR, and counts down from that value, rather]
  > than from an arbitrary value.

  (This is more or less what has been quoted in the implementation of
  `systick_reload`, above.)  This note does **not** say, however, that writes
  to SYST_CVR will be discarded when not enabled, but rather that the counting
  will only begin (and the value in SYST_RVR loaded or reloaded) when
  SYST_CSR.ENABLE becomes set.

  While QEMU's behavior does not match that of the hardware (and is therefore
  at some level malfunctioning), there is additional behavior that is very
  clearly incorrect: once SYST_CSR.ENABLE is set, not only will SYST_CVR
  continue to return 0 (that is, the counter will not be enabled),
  SYST_CSR.COUNTFLAG will become set when the *old* value of SYST_RVR ticks
  have elapsed!  This is wrong in both regards: if SYST_CVR is not counting
  down, SYST_CSR.COUNTFLAG should never become set -- and it certainly
  shouldn't match a value of SYST_RVR that has been overwritten in the
  interim!

  In terms of fixing this, it's helpful to understand the
  <a
  href="https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01217.html">context
  around this change</a>:

  > Consider the following pseudo code to configure SYSTICK (The
  > recommended programming sequence from "the definitive guide to the
  > arm cortex-m3"):
  >    SYSTICK Reload Value Register = 0xffff
  >    SYSTICK Current Value Register = 0
  >    SYSTICK Control and Status Register = 0x7
  >
  > The pseudo code "SYSTICK Current Value Register = 0" leads to invoking
  > systick_reload(). As a consequence, the systick.tick member is updated
  > and the systick timer starts to count down when the ENABLE bit of
  > SYSTICK Control and Status Register is cleared.
  >
  > The worst case is that: during the system initialization, the reset
  > value of the SYSTICK Control and Status Register is 0x00000000. 
  > When the code "SYSTICK Current Value Register = 0" is executed, the
  > systick.tick member is accumulated with "(s->systick.reload + 1) *
  > systick_scale(s)". The systick_scale() gets the external_ref_clock
  > scale because the CLKSOURCE bit of the SYSTICK Control and Status
  > Register is cleared. This is the incorrect behavior because of the
  > code "SYSTICK Control and Status Register = 0x7". Actually, we want
  > the processor clock instead of the external reference clock.
  >
  > This incorrect behavior defers the generation of the first interrupt. 
  >
  > The patch fixes the above-mentioned issue by setting the systick.tick
  > member and modifying the systick timer only if the ENABLE bit of
  > the SYSTICK Control and Status Register is set.
  >
  > In addition, the Cortex-M3 Devices Generic User Guide mentioned that
  > "When ENABLE is set to 1, the counter loads the RELOAD value from the
  > SYST RVR register and then counts down". This patch adheres to the
  > statement of the user guide.

  This fix is simply incorrect -- or at the least, incomplete:
  a write to SYST_CVR must clear any cached state
  such that a subsequent write to SYST_CSR.ENABLE will correctly cause
  a reload.  Here is a diff that solves the problem without re-introducing
  the behavior that the original fix was trying to correct:

  ```diff
  diff --git a/hw/timer/armv7m_systick.c b/hw/timer/armv7m_systick.c
  index 74c58bcf24..3f7b267c2d 100644
  --- a/hw/timer/armv7m_systick.c
  +++ b/hw/timer/armv7m_systick.c
  @@ -181,6 +181,15 @@ static MemTxResult systick_write(void *opaque, hwaddr addr,
           break;
       case 0x8: /* SysTick Current Value.  Writes reload the timer.  */
           systick_reload(s, 1);
  +
  +        if ((s->control & SYSTICK_ENABLE) == 0) {
  +            /*
  +             * If we're not enabled, explicitly clear our tick value to
  +             * assure that when we do become enabled, we correctly reload.
  +             */
  +            s->tick = 0;
  +        }
  +
           s->control &= ~SYSTICK_COUNTFLAG;
           break;
       default:
  ```

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Bug 1872237] Re: SysTick reload behavior emulated incorrectly
  2020-04-11 19:59 [Bug 1872237] [NEW] SysTick reload behavior emulated incorrectly Bryan Cantrill
                   ` (2 preceding siblings ...)
  2020-04-13 18:14 ` Peter Maydell
@ 2020-10-15 15:23 ` Peter Maydell
  2020-11-04 23:17 ` John Snow
  2020-12-10  8:54 ` Thomas Huth
  5 siblings, 0 replies; 7+ messages in thread
From: Peter Maydell @ 2020-10-15 15:23 UTC (permalink / raw)
  To: qemu-devel

I believe this bug should be fixed by this patchset, which rewrites the systick implementation to use a 'ptimer' down-counter instead of rolling its own buggy version:
https://patchew.org/QEMU/20201015151829.14656-1-peter.maydell@linaro.org/


** Changed in: qemu
       Status: New => In Progress

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

Title:
  SysTick reload behavior emulated incorrectly

Status in QEMU:
  In Progress

Bug description:
  QEMU's emuation of SysTick on ARM is incorrect with respect to reload
  behavior.  This issue is described here, and also in a repository
  dedicated to the issue:

    https://github.com/oxidecomputer/qemu-systick-bug

  
  (What follows is in Markdown, which I understand that Launchpad does
  not support; see the repository linked above for a rendering of it.)

  Take this Rust program:

  ```rust
  #![no_std]
  #![no_main]

  extern crate panic_semihosting;

  use cortex_m_rt::entry;
  use cortex_m_semihosting::hprintln;
  use cortex_m::peripheral::syst::SystClkSource;
  use cortex_m::peripheral::SYST;

  fn delay(syst: &mut cortex_m::peripheral::SYST, ms: u32)
  {
      /*
       * Configured for the LM3S6965, which has a default CPU clock of 12 Mhz
       */
      let reload = 12_000 * ms;

      syst.set_reload(reload);
      syst.clear_current();
      syst.enable_counter();

      hprintln!("waiting for {} ms (SYST_CVR={}) ...",
          ms, SYST::get_current()
      ).unwrap();

      while !syst.has_wrapped() {}

      hprintln!("  ... done (SYST_CVR={})\n",
  SYST::get_current()).unwrap();

      syst.disable_counter();
  }

  #[entry]
  fn main() -> ! {
      let p = cortex_m::Peripherals::take().unwrap();
      let mut syst = p.SYST;

      syst.set_clock_source(SystClkSource::Core);

      loop {
          delay(&mut syst, 1000);
          delay(&mut syst, 100);
      }
  }
  ```

  This program should oscillate between waiting for one second and waiting
  for 100 milliseconds.  Under hardware, this is more or less what it does
  (depending on core clock frequency); e.g., from an STM32F4107 (connected via
  OCD and with semi-hosting enabled):

  ```
  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999902)

  waiting for 100 ms (SYST_CVR=1199949) ...
    ... done (SYST_CVR=1199897)

  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999885)

  waiting for 100 ms (SYST_CVR=1199949) ...
    ... done (SYST_CVR=1199897)

  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999885)

  ```

  Under QEMU, however, its behavior is quite different:

  ```
  $ cargo run
      Finished dev [unoptimized + debuginfo] target(s) in 0.03s
       Running `qemu-system-arm -cpu cortex-m3 -machine lm3s6965evb -nographic -semihosting-config enable=on,target=native -kernel target/thumbv7m-none-eabi/debug/qemu-systick-bug`
  waiting for 1000 ms (SYST_CVR=11999658) ...
    ... done (SYST_CVR=11986226)

  waiting for 100 ms (SYST_CVR=0) ...
    ... done (SYST_CVR=1186560)

  waiting for 1000 ms (SYST_CVR=1185996) ...
    ... done (SYST_CVR=11997350)

  waiting for 100 ms (SYST_CVR=0) ...
    ... done (SYST_CVR=1186581)
  ```

  In addition to the values being strangely wrong, the behavior is wrong:
  the first wait correctly waits for 1000 ms -- but the subsequent wait
  (which should be for 100 ms) is in fact 1000 ms, and the next wait (which
  should be for 1000 ms) is in fact 100 ms.  (That is, it appears as if
  the periods of the two delays have been switched.)

  The problems is that the QEMU ARM emulation code does not reload SYST_CVR from
  SYST_RVR if SYST_CSR.ENABLE is not set -- and moreover, that SYST_CVR is
  not reloaded from SYST_RVR even when SYST_CSR.ENABLE becomes set.  This is
  very explicit; from
  <a
  href="https://github.com/qemu/qemu/blob/8bac3ba57eecc466b7e73dabf7d19328a59f684e/hw/timer/armv7m_systick.c#L42-L60">hw/timer/armv7m_systick.c</a>:

  ```c
  static void systick_reload(SysTickState *s, int reset)
  {
      /* The Cortex-M3 Devices Generic User Guide says that "When the
       * ENABLE bit is set to 1, the counter loads the RELOAD value from the
       * SYST RVR register and then counts down". So, we need to check the
       * ENABLE bit before reloading the value.
       */
      trace_systick_reload();

      if ((s->control & SYSTICK_ENABLE) == 0) {
          return;
      }

      if (reset) {
          s->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
      }
      s->tick += (s->reload + 1) * systick_scale(s);
      timer_mod(s->timer, s->tick);
  }
  ```

  The statement in the code is stronger than the statement in the
  <a href="https://static.docs.arm.com/ddi0403/eb/DDI0403E_B_armv7m_arm.pdf">ARMv7-M Architecture Reference Manual</a> (sec B3.3.1):

  > Writing to SYST_CVR clears both the register and the COUNTFLAG status
  > bit to zero. This causes the SysTick logic to reload SYST_CVR from SYST_RVR
  > on the next timer clock. A write to SYST_CVR does not trigger the
  > SysTick exception logic.

  Note that this does not mention the behavior on a write to SYST_CVR when
  SYST_CSR.ENABLE is not set -- and in particular, does *not* say that writes to
  SYST_CVR will be ignored if SYST_CSR.ENABLE is not set.

  Section 3.3.1 does go on to say:

  > The SYST_CVR value is UNKNOWN on reset. Before enabling the SysTick counter,u
  > software must write the required counter value to SYST_RVR, and then write
  > to SYST_CVR. This clears SYST_CVR to zero. When enabled, the counter 
  > reloads the value from SYST_RVR, and counts down from that value, rather]
  > than from an arbitrary value.

  (This is more or less what has been quoted in the implementation of
  `systick_reload`, above.)  This note does **not** say, however, that writes
  to SYST_CVR will be discarded when not enabled, but rather that the counting
  will only begin (and the value in SYST_RVR loaded or reloaded) when
  SYST_CSR.ENABLE becomes set.

  While QEMU's behavior does not match that of the hardware (and is therefore
  at some level malfunctioning), there is additional behavior that is very
  clearly incorrect: once SYST_CSR.ENABLE is set, not only will SYST_CVR
  continue to return 0 (that is, the counter will not be enabled),
  SYST_CSR.COUNTFLAG will become set when the *old* value of SYST_RVR ticks
  have elapsed!  This is wrong in both regards: if SYST_CVR is not counting
  down, SYST_CSR.COUNTFLAG should never become set -- and it certainly
  shouldn't match a value of SYST_RVR that has been overwritten in the
  interim!

  In terms of fixing this, it's helpful to understand the
  <a
  href="https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01217.html">context
  around this change</a>:

  > Consider the following pseudo code to configure SYSTICK (The
  > recommended programming sequence from "the definitive guide to the
  > arm cortex-m3"):
  >    SYSTICK Reload Value Register = 0xffff
  >    SYSTICK Current Value Register = 0
  >    SYSTICK Control and Status Register = 0x7
  >
  > The pseudo code "SYSTICK Current Value Register = 0" leads to invoking
  > systick_reload(). As a consequence, the systick.tick member is updated
  > and the systick timer starts to count down when the ENABLE bit of
  > SYSTICK Control and Status Register is cleared.
  >
  > The worst case is that: during the system initialization, the reset
  > value of the SYSTICK Control and Status Register is 0x00000000. 
  > When the code "SYSTICK Current Value Register = 0" is executed, the
  > systick.tick member is accumulated with "(s->systick.reload + 1) *
  > systick_scale(s)". The systick_scale() gets the external_ref_clock
  > scale because the CLKSOURCE bit of the SYSTICK Control and Status
  > Register is cleared. This is the incorrect behavior because of the
  > code "SYSTICK Control and Status Register = 0x7". Actually, we want
  > the processor clock instead of the external reference clock.
  >
  > This incorrect behavior defers the generation of the first interrupt. 
  >
  > The patch fixes the above-mentioned issue by setting the systick.tick
  > member and modifying the systick timer only if the ENABLE bit of
  > the SYSTICK Control and Status Register is set.
  >
  > In addition, the Cortex-M3 Devices Generic User Guide mentioned that
  > "When ENABLE is set to 1, the counter loads the RELOAD value from the
  > SYST RVR register and then counts down". This patch adheres to the
  > statement of the user guide.

  This fix is simply incorrect -- or at the least, incomplete:
  a write to SYST_CVR must clear any cached state
  such that a subsequent write to SYST_CSR.ENABLE will correctly cause
  a reload.  Here is a diff that solves the problem without re-introducing
  the behavior that the original fix was trying to correct:

  ```diff
  diff --git a/hw/timer/armv7m_systick.c b/hw/timer/armv7m_systick.c
  index 74c58bcf24..3f7b267c2d 100644
  --- a/hw/timer/armv7m_systick.c
  +++ b/hw/timer/armv7m_systick.c
  @@ -181,6 +181,15 @@ static MemTxResult systick_write(void *opaque, hwaddr addr,
           break;
       case 0x8: /* SysTick Current Value.  Writes reload the timer.  */
           systick_reload(s, 1);
  +
  +        if ((s->control & SYSTICK_ENABLE) == 0) {
  +            /*
  +             * If we're not enabled, explicitly clear our tick value to
  +             * assure that when we do become enabled, we correctly reload.
  +             */
  +            s->tick = 0;
  +        }
  +
           s->control &= ~SYSTICK_COUNTFLAG;
           break;
       default:
  ```

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Bug 1872237] Re: SysTick reload behavior emulated incorrectly
  2020-04-11 19:59 [Bug 1872237] [NEW] SysTick reload behavior emulated incorrectly Bryan Cantrill
                   ` (3 preceding siblings ...)
  2020-10-15 15:23 ` Peter Maydell
@ 2020-11-04 23:17 ` John Snow
  2020-12-10  8:54 ` Thomas Huth
  5 siblings, 0 replies; 7+ messages in thread
From: John Snow @ 2020-11-04 23:17 UTC (permalink / raw)
  To: qemu-devel

Merged:
https://patchew.org/QEMU/20201015151829.14656-1-peter.maydell@linaro.org/

Commits: 
https://gitlab.com/qemu-project/qemu/-/commit/68d59c6d8d85ae176d3cb2cd20a48d6a090ba288
https://gitlab.com/qemu-project/qemu/-/commit/32bd322a0134ed89db00f2b9b3894982db3dedcb

** Changed in: qemu
       Status: In Progress => Fix Committed

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

Title:
  SysTick reload behavior emulated incorrectly

Status in QEMU:
  Fix Committed

Bug description:
  QEMU's emuation of SysTick on ARM is incorrect with respect to reload
  behavior.  This issue is described here, and also in a repository
  dedicated to the issue:

    https://github.com/oxidecomputer/qemu-systick-bug

  
  (What follows is in Markdown, which I understand that Launchpad does
  not support; see the repository linked above for a rendering of it.)

  Take this Rust program:

  ```rust
  #![no_std]
  #![no_main]

  extern crate panic_semihosting;

  use cortex_m_rt::entry;
  use cortex_m_semihosting::hprintln;
  use cortex_m::peripheral::syst::SystClkSource;
  use cortex_m::peripheral::SYST;

  fn delay(syst: &mut cortex_m::peripheral::SYST, ms: u32)
  {
      /*
       * Configured for the LM3S6965, which has a default CPU clock of 12 Mhz
       */
      let reload = 12_000 * ms;

      syst.set_reload(reload);
      syst.clear_current();
      syst.enable_counter();

      hprintln!("waiting for {} ms (SYST_CVR={}) ...",
          ms, SYST::get_current()
      ).unwrap();

      while !syst.has_wrapped() {}

      hprintln!("  ... done (SYST_CVR={})\n",
  SYST::get_current()).unwrap();

      syst.disable_counter();
  }

  #[entry]
  fn main() -> ! {
      let p = cortex_m::Peripherals::take().unwrap();
      let mut syst = p.SYST;

      syst.set_clock_source(SystClkSource::Core);

      loop {
          delay(&mut syst, 1000);
          delay(&mut syst, 100);
      }
  }
  ```

  This program should oscillate between waiting for one second and waiting
  for 100 milliseconds.  Under hardware, this is more or less what it does
  (depending on core clock frequency); e.g., from an STM32F4107 (connected via
  OCD and with semi-hosting enabled):

  ```
  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999902)

  waiting for 100 ms (SYST_CVR=1199949) ...
    ... done (SYST_CVR=1199897)

  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999885)

  waiting for 100 ms (SYST_CVR=1199949) ...
    ... done (SYST_CVR=1199897)

  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999885)

  ```

  Under QEMU, however, its behavior is quite different:

  ```
  $ cargo run
      Finished dev [unoptimized + debuginfo] target(s) in 0.03s
       Running `qemu-system-arm -cpu cortex-m3 -machine lm3s6965evb -nographic -semihosting-config enable=on,target=native -kernel target/thumbv7m-none-eabi/debug/qemu-systick-bug`
  waiting for 1000 ms (SYST_CVR=11999658) ...
    ... done (SYST_CVR=11986226)

  waiting for 100 ms (SYST_CVR=0) ...
    ... done (SYST_CVR=1186560)

  waiting for 1000 ms (SYST_CVR=1185996) ...
    ... done (SYST_CVR=11997350)

  waiting for 100 ms (SYST_CVR=0) ...
    ... done (SYST_CVR=1186581)
  ```

  In addition to the values being strangely wrong, the behavior is wrong:
  the first wait correctly waits for 1000 ms -- but the subsequent wait
  (which should be for 100 ms) is in fact 1000 ms, and the next wait (which
  should be for 1000 ms) is in fact 100 ms.  (That is, it appears as if
  the periods of the two delays have been switched.)

  The problems is that the QEMU ARM emulation code does not reload SYST_CVR from
  SYST_RVR if SYST_CSR.ENABLE is not set -- and moreover, that SYST_CVR is
  not reloaded from SYST_RVR even when SYST_CSR.ENABLE becomes set.  This is
  very explicit; from
  <a
  href="https://github.com/qemu/qemu/blob/8bac3ba57eecc466b7e73dabf7d19328a59f684e/hw/timer/armv7m_systick.c#L42-L60">hw/timer/armv7m_systick.c</a>:

  ```c
  static void systick_reload(SysTickState *s, int reset)
  {
      /* The Cortex-M3 Devices Generic User Guide says that "When the
       * ENABLE bit is set to 1, the counter loads the RELOAD value from the
       * SYST RVR register and then counts down". So, we need to check the
       * ENABLE bit before reloading the value.
       */
      trace_systick_reload();

      if ((s->control & SYSTICK_ENABLE) == 0) {
          return;
      }

      if (reset) {
          s->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
      }
      s->tick += (s->reload + 1) * systick_scale(s);
      timer_mod(s->timer, s->tick);
  }
  ```

  The statement in the code is stronger than the statement in the
  <a href="https://static.docs.arm.com/ddi0403/eb/DDI0403E_B_armv7m_arm.pdf">ARMv7-M Architecture Reference Manual</a> (sec B3.3.1):

  > Writing to SYST_CVR clears both the register and the COUNTFLAG status
  > bit to zero. This causes the SysTick logic to reload SYST_CVR from SYST_RVR
  > on the next timer clock. A write to SYST_CVR does not trigger the
  > SysTick exception logic.

  Note that this does not mention the behavior on a write to SYST_CVR when
  SYST_CSR.ENABLE is not set -- and in particular, does *not* say that writes to
  SYST_CVR will be ignored if SYST_CSR.ENABLE is not set.

  Section 3.3.1 does go on to say:

  > The SYST_CVR value is UNKNOWN on reset. Before enabling the SysTick counter,u
  > software must write the required counter value to SYST_RVR, and then write
  > to SYST_CVR. This clears SYST_CVR to zero. When enabled, the counter 
  > reloads the value from SYST_RVR, and counts down from that value, rather]
  > than from an arbitrary value.

  (This is more or less what has been quoted in the implementation of
  `systick_reload`, above.)  This note does **not** say, however, that writes
  to SYST_CVR will be discarded when not enabled, but rather that the counting
  will only begin (and the value in SYST_RVR loaded or reloaded) when
  SYST_CSR.ENABLE becomes set.

  While QEMU's behavior does not match that of the hardware (and is therefore
  at some level malfunctioning), there is additional behavior that is very
  clearly incorrect: once SYST_CSR.ENABLE is set, not only will SYST_CVR
  continue to return 0 (that is, the counter will not be enabled),
  SYST_CSR.COUNTFLAG will become set when the *old* value of SYST_RVR ticks
  have elapsed!  This is wrong in both regards: if SYST_CVR is not counting
  down, SYST_CSR.COUNTFLAG should never become set -- and it certainly
  shouldn't match a value of SYST_RVR that has been overwritten in the
  interim!

  In terms of fixing this, it's helpful to understand the
  <a
  href="https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01217.html">context
  around this change</a>:

  > Consider the following pseudo code to configure SYSTICK (The
  > recommended programming sequence from "the definitive guide to the
  > arm cortex-m3"):
  >    SYSTICK Reload Value Register = 0xffff
  >    SYSTICK Current Value Register = 0
  >    SYSTICK Control and Status Register = 0x7
  >
  > The pseudo code "SYSTICK Current Value Register = 0" leads to invoking
  > systick_reload(). As a consequence, the systick.tick member is updated
  > and the systick timer starts to count down when the ENABLE bit of
  > SYSTICK Control and Status Register is cleared.
  >
  > The worst case is that: during the system initialization, the reset
  > value of the SYSTICK Control and Status Register is 0x00000000. 
  > When the code "SYSTICK Current Value Register = 0" is executed, the
  > systick.tick member is accumulated with "(s->systick.reload + 1) *
  > systick_scale(s)". The systick_scale() gets the external_ref_clock
  > scale because the CLKSOURCE bit of the SYSTICK Control and Status
  > Register is cleared. This is the incorrect behavior because of the
  > code "SYSTICK Control and Status Register = 0x7". Actually, we want
  > the processor clock instead of the external reference clock.
  >
  > This incorrect behavior defers the generation of the first interrupt. 
  >
  > The patch fixes the above-mentioned issue by setting the systick.tick
  > member and modifying the systick timer only if the ENABLE bit of
  > the SYSTICK Control and Status Register is set.
  >
  > In addition, the Cortex-M3 Devices Generic User Guide mentioned that
  > "When ENABLE is set to 1, the counter loads the RELOAD value from the
  > SYST RVR register and then counts down". This patch adheres to the
  > statement of the user guide.

  This fix is simply incorrect -- or at the least, incomplete:
  a write to SYST_CVR must clear any cached state
  such that a subsequent write to SYST_CSR.ENABLE will correctly cause
  a reload.  Here is a diff that solves the problem without re-introducing
  the behavior that the original fix was trying to correct:

  ```diff
  diff --git a/hw/timer/armv7m_systick.c b/hw/timer/armv7m_systick.c
  index 74c58bcf24..3f7b267c2d 100644
  --- a/hw/timer/armv7m_systick.c
  +++ b/hw/timer/armv7m_systick.c
  @@ -181,6 +181,15 @@ static MemTxResult systick_write(void *opaque, hwaddr addr,
           break;
       case 0x8: /* SysTick Current Value.  Writes reload the timer.  */
           systick_reload(s, 1);
  +
  +        if ((s->control & SYSTICK_ENABLE) == 0) {
  +            /*
  +             * If we're not enabled, explicitly clear our tick value to
  +             * assure that when we do become enabled, we correctly reload.
  +             */
  +            s->tick = 0;
  +        }
  +
           s->control &= ~SYSTICK_COUNTFLAG;
           break;
       default:
  ```

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [Bug 1872237] Re: SysTick reload behavior emulated incorrectly
  2020-04-11 19:59 [Bug 1872237] [NEW] SysTick reload behavior emulated incorrectly Bryan Cantrill
                   ` (4 preceding siblings ...)
  2020-11-04 23:17 ` John Snow
@ 2020-12-10  8:54 ` Thomas Huth
  5 siblings, 0 replies; 7+ messages in thread
From: Thomas Huth @ 2020-12-10  8:54 UTC (permalink / raw)
  To: qemu-devel

Released with QEMU v5.2.0.

** Changed in: qemu
       Status: Fix Committed => Fix Released

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

Title:
  SysTick reload behavior emulated incorrectly

Status in QEMU:
  Fix Released

Bug description:
  QEMU's emuation of SysTick on ARM is incorrect with respect to reload
  behavior.  This issue is described here, and also in a repository
  dedicated to the issue:

    https://github.com/oxidecomputer/qemu-systick-bug

  
  (What follows is in Markdown, which I understand that Launchpad does
  not support; see the repository linked above for a rendering of it.)

  Take this Rust program:

  ```rust
  #![no_std]
  #![no_main]

  extern crate panic_semihosting;

  use cortex_m_rt::entry;
  use cortex_m_semihosting::hprintln;
  use cortex_m::peripheral::syst::SystClkSource;
  use cortex_m::peripheral::SYST;

  fn delay(syst: &mut cortex_m::peripheral::SYST, ms: u32)
  {
      /*
       * Configured for the LM3S6965, which has a default CPU clock of 12 Mhz
       */
      let reload = 12_000 * ms;

      syst.set_reload(reload);
      syst.clear_current();
      syst.enable_counter();

      hprintln!("waiting for {} ms (SYST_CVR={}) ...",
          ms, SYST::get_current()
      ).unwrap();

      while !syst.has_wrapped() {}

      hprintln!("  ... done (SYST_CVR={})\n",
  SYST::get_current()).unwrap();

      syst.disable_counter();
  }

  #[entry]
  fn main() -> ! {
      let p = cortex_m::Peripherals::take().unwrap();
      let mut syst = p.SYST;

      syst.set_clock_source(SystClkSource::Core);

      loop {
          delay(&mut syst, 1000);
          delay(&mut syst, 100);
      }
  }
  ```

  This program should oscillate between waiting for one second and waiting
  for 100 milliseconds.  Under hardware, this is more or less what it does
  (depending on core clock frequency); e.g., from an STM32F4107 (connected via
  OCD and with semi-hosting enabled):

  ```
  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999902)

  waiting for 100 ms (SYST_CVR=1199949) ...
    ... done (SYST_CVR=1199897)

  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999885)

  waiting for 100 ms (SYST_CVR=1199949) ...
    ... done (SYST_CVR=1199897)

  waiting for 1000 ms (SYST_CVR=11999949) ...
    ... done (SYST_CVR=11999885)

  ```

  Under QEMU, however, its behavior is quite different:

  ```
  $ cargo run
      Finished dev [unoptimized + debuginfo] target(s) in 0.03s
       Running `qemu-system-arm -cpu cortex-m3 -machine lm3s6965evb -nographic -semihosting-config enable=on,target=native -kernel target/thumbv7m-none-eabi/debug/qemu-systick-bug`
  waiting for 1000 ms (SYST_CVR=11999658) ...
    ... done (SYST_CVR=11986226)

  waiting for 100 ms (SYST_CVR=0) ...
    ... done (SYST_CVR=1186560)

  waiting for 1000 ms (SYST_CVR=1185996) ...
    ... done (SYST_CVR=11997350)

  waiting for 100 ms (SYST_CVR=0) ...
    ... done (SYST_CVR=1186581)
  ```

  In addition to the values being strangely wrong, the behavior is wrong:
  the first wait correctly waits for 1000 ms -- but the subsequent wait
  (which should be for 100 ms) is in fact 1000 ms, and the next wait (which
  should be for 1000 ms) is in fact 100 ms.  (That is, it appears as if
  the periods of the two delays have been switched.)

  The problems is that the QEMU ARM emulation code does not reload SYST_CVR from
  SYST_RVR if SYST_CSR.ENABLE is not set -- and moreover, that SYST_CVR is
  not reloaded from SYST_RVR even when SYST_CSR.ENABLE becomes set.  This is
  very explicit; from
  <a
  href="https://github.com/qemu/qemu/blob/8bac3ba57eecc466b7e73dabf7d19328a59f684e/hw/timer/armv7m_systick.c#L42-L60">hw/timer/armv7m_systick.c</a>:

  ```c
  static void systick_reload(SysTickState *s, int reset)
  {
      /* The Cortex-M3 Devices Generic User Guide says that "When the
       * ENABLE bit is set to 1, the counter loads the RELOAD value from the
       * SYST RVR register and then counts down". So, we need to check the
       * ENABLE bit before reloading the value.
       */
      trace_systick_reload();

      if ((s->control & SYSTICK_ENABLE) == 0) {
          return;
      }

      if (reset) {
          s->tick = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
      }
      s->tick += (s->reload + 1) * systick_scale(s);
      timer_mod(s->timer, s->tick);
  }
  ```

  The statement in the code is stronger than the statement in the
  <a href="https://static.docs.arm.com/ddi0403/eb/DDI0403E_B_armv7m_arm.pdf">ARMv7-M Architecture Reference Manual</a> (sec B3.3.1):

  > Writing to SYST_CVR clears both the register and the COUNTFLAG status
  > bit to zero. This causes the SysTick logic to reload SYST_CVR from SYST_RVR
  > on the next timer clock. A write to SYST_CVR does not trigger the
  > SysTick exception logic.

  Note that this does not mention the behavior on a write to SYST_CVR when
  SYST_CSR.ENABLE is not set -- and in particular, does *not* say that writes to
  SYST_CVR will be ignored if SYST_CSR.ENABLE is not set.

  Section 3.3.1 does go on to say:

  > The SYST_CVR value is UNKNOWN on reset. Before enabling the SysTick counter,u
  > software must write the required counter value to SYST_RVR, and then write
  > to SYST_CVR. This clears SYST_CVR to zero. When enabled, the counter 
  > reloads the value from SYST_RVR, and counts down from that value, rather]
  > than from an arbitrary value.

  (This is more or less what has been quoted in the implementation of
  `systick_reload`, above.)  This note does **not** say, however, that writes
  to SYST_CVR will be discarded when not enabled, but rather that the counting
  will only begin (and the value in SYST_RVR loaded or reloaded) when
  SYST_CSR.ENABLE becomes set.

  While QEMU's behavior does not match that of the hardware (and is therefore
  at some level malfunctioning), there is additional behavior that is very
  clearly incorrect: once SYST_CSR.ENABLE is set, not only will SYST_CVR
  continue to return 0 (that is, the counter will not be enabled),
  SYST_CSR.COUNTFLAG will become set when the *old* value of SYST_RVR ticks
  have elapsed!  This is wrong in both regards: if SYST_CVR is not counting
  down, SYST_CSR.COUNTFLAG should never become set -- and it certainly
  shouldn't match a value of SYST_RVR that has been overwritten in the
  interim!

  In terms of fixing this, it's helpful to understand the
  <a
  href="https://lists.gnu.org/archive/html/qemu-devel/2015-05/msg01217.html">context
  around this change</a>:

  > Consider the following pseudo code to configure SYSTICK (The
  > recommended programming sequence from "the definitive guide to the
  > arm cortex-m3"):
  >    SYSTICK Reload Value Register = 0xffff
  >    SYSTICK Current Value Register = 0
  >    SYSTICK Control and Status Register = 0x7
  >
  > The pseudo code "SYSTICK Current Value Register = 0" leads to invoking
  > systick_reload(). As a consequence, the systick.tick member is updated
  > and the systick timer starts to count down when the ENABLE bit of
  > SYSTICK Control and Status Register is cleared.
  >
  > The worst case is that: during the system initialization, the reset
  > value of the SYSTICK Control and Status Register is 0x00000000. 
  > When the code "SYSTICK Current Value Register = 0" is executed, the
  > systick.tick member is accumulated with "(s->systick.reload + 1) *
  > systick_scale(s)". The systick_scale() gets the external_ref_clock
  > scale because the CLKSOURCE bit of the SYSTICK Control and Status
  > Register is cleared. This is the incorrect behavior because of the
  > code "SYSTICK Control and Status Register = 0x7". Actually, we want
  > the processor clock instead of the external reference clock.
  >
  > This incorrect behavior defers the generation of the first interrupt. 
  >
  > The patch fixes the above-mentioned issue by setting the systick.tick
  > member and modifying the systick timer only if the ENABLE bit of
  > the SYSTICK Control and Status Register is set.
  >
  > In addition, the Cortex-M3 Devices Generic User Guide mentioned that
  > "When ENABLE is set to 1, the counter loads the RELOAD value from the
  > SYST RVR register and then counts down". This patch adheres to the
  > statement of the user guide.

  This fix is simply incorrect -- or at the least, incomplete:
  a write to SYST_CVR must clear any cached state
  such that a subsequent write to SYST_CSR.ENABLE will correctly cause
  a reload.  Here is a diff that solves the problem without re-introducing
  the behavior that the original fix was trying to correct:

  ```diff
  diff --git a/hw/timer/armv7m_systick.c b/hw/timer/armv7m_systick.c
  index 74c58bcf24..3f7b267c2d 100644
  --- a/hw/timer/armv7m_systick.c
  +++ b/hw/timer/armv7m_systick.c
  @@ -181,6 +181,15 @@ static MemTxResult systick_write(void *opaque, hwaddr addr,
           break;
       case 0x8: /* SysTick Current Value.  Writes reload the timer.  */
           systick_reload(s, 1);
  +
  +        if ((s->control & SYSTICK_ENABLE) == 0) {
  +            /*
  +             * If we're not enabled, explicitly clear our tick value to
  +             * assure that when we do become enabled, we correctly reload.
  +             */
  +            s->tick = 0;
  +        }
  +
           s->control &= ~SYSTICK_COUNTFLAG;
           break;
       default:
  ```

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-12-10  9:06 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-11 19:59 [Bug 1872237] [NEW] SysTick reload behavior emulated incorrectly Bryan Cantrill
2020-04-11 21:05 ` [Bug 1872237] " Peter Maydell
2020-04-13 17:14 ` Bryan Cantrill
2020-04-13 18:14 ` Peter Maydell
2020-10-15 15:23 ` Peter Maydell
2020-11-04 23:17 ` John Snow
2020-12-10  8:54 ` Thomas Huth

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.