* [PATCH v2 3/3] hw/timer: ibex_timer.c: Add support for writes to mtime
@ 2022-09-22 15:58 Tyler Ng
2022-09-22 21:45 ` Philippe Mathieu-Daudé
2022-09-26 21:05 ` Dong, Eddie
0 siblings, 2 replies; 9+ messages in thread
From: Tyler Ng @ 2022-09-22 15:58 UTC (permalink / raw)
To: open list:RISC-V, qemu-devel@nongnu.org Developers
Cc: Alistair Francis, Bin Meng, Thomas Huth, Paolo Bonzini,
Palmer Dabbelt, Laurent Vivier
1. Adds fields to hold the value of mtime in timer_upper0 and timer_lower0.
2. Changes the read and write functions to use the mtime fields.
3. Updates the value of mtime in update_mtime() by extrapolating the
time elapsed. This will need to change if/when the prescalar is
implemented.
4. Adds a qtest for the ibex timer.
Signed-off-by: Tyler Ng <tkng@rivosinc.com>
---
hw/timer/ibex_timer.c | 98 +++++++++++++------
include/hw/timer/ibex_timer.h | 6 ++
tests/qtest/ibex-timer-test.c | 178 ++++++++++++++++++++++++++++++++++
tests/qtest/meson.build | 3 +-
4 files changed, 256 insertions(+), 29 deletions(-)
create mode 100644 tests/qtest/ibex-timer-test.c
diff --git a/hw/timer/ibex_timer.c b/hw/timer/ibex_timer.c
index d8b8e4e1f6..4230d60e85 100644
--- a/hw/timer/ibex_timer.c
+++ b/hw/timer/ibex_timer.c
@@ -52,28 +52,56 @@ REG32(INTR_STATE, 0x118)
REG32(INTR_TEST, 0x11C)
FIELD(INTR_TEST, T_0, 0, 1)
-static uint64_t cpu_riscv_read_rtc(uint32_t timebase_freq)
+static inline uint64_t get_mtime(void *opaque)
{
- return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
- timebase_freq, NANOSECONDS_PER_SECOND);
+ IbexTimerState *s = opaque;
+ return (s->timer_lower0) | ((uint64_t) s->timer_upper0 << 32);
}
-static void ibex_timer_update_irqs(IbexTimerState *s)
+/*
+ * The goal of this function is to:
+ * 1. Check if the timer is enabled. If not, return false,
+ * 2. Calculate the amount of time that has passed since.
+ * 3. Extrapolate the number of ticks that have passed, and add it to `mtime`.
+ * 4. Return true.
+ */
+static bool update_mtime(IbexTimerState *s)
{
- uint64_t value = s->timer_compare_lower0 |
- ((uint64_t)s->timer_compare_upper0 << 32);
- uint64_t next, diff;
- uint64_t now = cpu_riscv_read_rtc(s->timebase_freq);
-
if (!(s->timer_ctrl & R_CTRL_ACTIVE_MASK)) {
- /* Timer isn't active */
+ return false;
+ }
+ /* Get the time then extrapolate the number of ticks that have elapsed */
+ uint64_t mtime = get_mtime(s);
+ int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+ int64_t elapsed = now - s->timer_last_update;
+ if (elapsed < 0) {
+ /* We jumped back in time. */
+ mtime -= muldiv64((uint64_t)(-elapsed), s->timebase_freq,
+ NANOSECONDS_PER_SECOND);
+ } else {
+ mtime += muldiv64(elapsed, s->timebase_freq, NANOSECONDS_PER_SECOND);
+ }
+ s->timer_lower0 = mtime & 0xffffffff;
+ s->timer_upper0 = (mtime >> 32) & 0xffffffff;
+ /* update last-checkpoint timestamp */
+ s->timer_last_update = now;
+ return true;
+}
+
+static void ibex_timer_update_irqs(IbexTimerState *s)
+{
+ if (!update_mtime(s)) {
+ /* Timer is not enabled? */
return;
}
+ uint64_t mtimecmp = s->timer_compare_lower0 |
+ ((uint64_t)s->timer_compare_upper0 << 32);
+ uint64_t mtime = get_mtime(s);
/* Update the CPUs mtimecmp */
- s->mtimecmp = value;
+ s->mtimecmp = mtimecmp;
- if (s->mtimecmp <= now) {
+ if (s->mtimecmp <= mtime) {
/*
* If the mtimecmp was in the past raise the interrupt now.
*/
@@ -84,18 +112,17 @@ static void ibex_timer_update_irqs(IbexTimerState *s)
}
return;
}
-
- /* Setup a timer to trigger the interrupt in the future */
+ /* Update timers: setup a timer to trigger the interrupt in the future */
qemu_irq_lower(s->m_timer_irq);
qemu_set_irq(s->irq, false);
-
- diff = s->mtimecmp - now;
- next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
- muldiv64(diff,
- NANOSECONDS_PER_SECOND,
- s->timebase_freq);
-
- if (next < qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)) {
+ /* Compute the difference, and set a timer for the next update. */
+ const uint64_t diff = mtimecmp - mtime;
+ const int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+ const uint64_t towait = muldiv64(diff, NANOSECONDS_PER_SECOND,
+ s->timebase_freq);
+ /* timer_mod takes in a int64_t, not uint64_t! Need to saturate it */
+ const int64_t next = now + towait;
+ if (next < now) {
/* We overflowed the timer, just set it as large as we can */
timer_mod(s->mtimer, 0x7FFFFFFFFFFFFFFF);
} else {
@@ -124,11 +151,13 @@ static void ibex_timer_reset(DeviceState *dev)
s->timer_ctrl = 0x00000000;
s->timer_cfg0 = 0x00010000;
+ s->timer_lower0 = 0x00000000;
+ s->timer_upper0 = 0x00000000;
s->timer_compare_lower0 = 0xFFFFFFFF;
s->timer_compare_upper0 = 0xFFFFFFFF;
s->timer_intr_enable = 0x00000000;
s->timer_intr_state = 0x00000000;
-
+ s->timer_last_update = 0x00000000;
ibex_timer_update_irqs(s);
}
@@ -136,7 +165,6 @@ static uint64_t ibex_timer_read(void *opaque, hwaddr addr,
unsigned int size)
{
IbexTimerState *s = opaque;
- uint64_t now = cpu_riscv_read_rtc(s->timebase_freq);
uint64_t retvalue = 0;
switch (addr >> 2) {
@@ -151,10 +179,12 @@ static uint64_t ibex_timer_read(void *opaque, hwaddr addr,
retvalue = s->timer_cfg0;
break;
case R_LOWER0:
- retvalue = now;
+ update_mtime(s);
+ retvalue = s->timer_lower0;
break;
case R_UPPER0:
- retvalue = now >> 32;
+ update_mtime(s);
+ retvalue = s->timer_upper0;
break;
case R_COMPARE_LOWER0:
retvalue = s->timer_compare_lower0;
@@ -192,17 +222,26 @@ static void ibex_timer_write(void *opaque, hwaddr addr,
qemu_log_mask(LOG_UNIMP, "Alert triggering not supported");
break;
case R_CTRL:
+ if (!(s->timer_ctrl & R_CTRL_ACTIVE_MASK)) {
+ s->timer_last_update = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+ }
s->timer_ctrl = val;
+ /* We must update IRQs, because the QEMU timer gets updated here. */
+ ibex_timer_update_irqs(s);
break;
case R_CFG0:
qemu_log_mask(LOG_UNIMP, "Changing prescale or step not supported");
s->timer_cfg0 = val;
break;
case R_LOWER0:
- qemu_log_mask(LOG_UNIMP, "Changing timer value is not supported");
+ s->timer_lower0 = val;
+ s->timer_last_update = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+ ibex_timer_update_irqs(s);
break;
case R_UPPER0:
- qemu_log_mask(LOG_UNIMP, "Changing timer value is not supported");
+ s->timer_upper0 = val;
+ s->timer_last_update = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+ ibex_timer_update_irqs(s);
break;
case R_COMPARE_LOWER0:
s->timer_compare_lower0 = val;
@@ -259,6 +298,9 @@ static const VMStateDescription vmstate_ibex_timer = {
VMSTATE_UINT32(timer_compare_upper0, IbexTimerState),
VMSTATE_UINT32(timer_intr_enable, IbexTimerState),
VMSTATE_UINT32(timer_intr_state, IbexTimerState),
+ VMSTATE_INT64(timer_last_update, IbexTimerState),
+ VMSTATE_UINT32(timer_lower0, IbexTimerState),
+ VMSTATE_UINT32(timer_upper0, IbexTimerState),
VMSTATE_END_OF_LIST()
}
};
diff --git a/include/hw/timer/ibex_timer.h b/include/hw/timer/ibex_timer.h
index 41f5c82a92..15c16035a8 100644
--- a/include/hw/timer/ibex_timer.h
+++ b/include/hw/timer/ibex_timer.h
@@ -36,11 +36,17 @@ struct IbexTimerState {
uint64_t mtimecmp;
QEMUTimer *mtimer; /* Internal timer for M-mode interrupt */
+ int64_t timer_last_update; /* Used for extrapolating mtime. */
+
/* <public> */
MemoryRegion mmio;
uint32_t timer_ctrl;
uint32_t timer_cfg0;
+
+
+ uint32_t timer_lower0;
+ uint32_t timer_upper0;
uint32_t timer_compare_lower0;
uint32_t timer_compare_upper0;
uint32_t timer_intr_enable;
diff --git a/tests/qtest/ibex-timer-test.c b/tests/qtest/ibex-timer-test.c
new file mode 100644
index 0000000000..1c2dfb0b6c
--- /dev/null
+++ b/tests/qtest/ibex-timer-test.c
@@ -0,0 +1,178 @@
+/*
+ * Testing the Ibex Timer
+ *
+ * Copyright (c) 2022 Rivos Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the
"Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include "qemu/osdep.h"
+#include "libqtest.h"
+#include "qapi/qmp/qdict.h"
+
+#define TIMER_BASE_ADDR 0x40100000UL
+#define TIMER_ADDR(addr) (TIMER_BASE_ADDR + addr)
+#define TIMER_EXPIRED_IRQ 127
+#define NANOS_PER_SECOND 1000000000LL
+
+#define A_ALERT_TEST 0x0
+#define A_CTRL 0x4
+#define A_CFG0 0x100
+#define A_LOWER_0 0x104
+#define A_UPPER_0 0x108
+#define A_COMPARE_LOWER0 0x10C
+#define A_COMPARE_UPPER0 0x110
+#define A_INTR_ENABLE 0x114
+#define A_INTR_STATE 0x118
+#define A_INTR_TEST 0x11C
+
+/*
+ * Tests that regs get reset properly.
+ */
+static void test_reset(void)
+{
+ QTestState *test = qtest_init("-M opentitan");
+ qtest_irq_intercept_in(test, "/machine/soc/plic");
+ /* Regs are reset; skip WO regs */
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CTRL)), ==, 0);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_ENABLE)), ==, 0);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_STATE)), ==, 0);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CFG0)), ==, 0x10000);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==, 0);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==, 0);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)), ==,
+ UINT32_MAX);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)), ==,
+ UINT32_MAX);
+ g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
+ qtest_quit(test);
+}
+
+/*
+ * Test that writes worked.
+ */
+static void test_writes(void)
+{
+ QTestState *test = qtest_init("-M opentitan");
+
+ qtest_irq_intercept_in(test, "/machine/soc/plic");
+
+ /* Special regs that may/may not need to be tested yet */
+/*
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_ALERT_TEST)), ==, 0);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_TEST)), ==, 0);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_STATE)), ==, 0);
+*/
+ /* Write to ctrl */
+ qtest_writel(test, TIMER_ADDR(A_CTRL), 0x1);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CTRL)), ==, 0x1);
+ /* Write to intr_enable */
+ qtest_writel(test, TIMER_ADDR(A_INTR_ENABLE), 0x1);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_ENABLE)), ==, 0x1);
+
+ /* Writes to config? Though none of it is supported */
+ qtest_writel(test, TIMER_ADDR(A_CFG0), 0x20001);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CFG0)), ==, 0x20001);
+
+ /* Writes to mtime */
+ qtest_writel(test, TIMER_ADDR(A_LOWER_0), 0xdeaddad);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==, 0xdeaddad);
+ qtest_writel(test, TIMER_ADDR(A_UPPER_0), 0xdeaddad);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==, 0xdeaddad);
+
+ /* Writes to mtimecmp */
+ qtest_writel(test, TIMER_ADDR(A_COMPARE_LOWER0), 0xdeaddad);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)), ==,
+ 0xdeaddad);
+ qtest_writel(test, TIMER_ADDR(A_COMPARE_UPPER0), 0xdeaddad);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)), ==,
+ 0xdeaddad);
+ qtest_quit(test);
+}
+
+/*
+ * Test the standard operation of the timer.
+ */
+static void test_operation(void)
+{
+ /* A frequency of 1000000 Hz*/
+ QTestState *test = qtest_init(
+ "-M opentitan "
+ "-global driver=ibex-timer,property=timebase-freq,value=1000000"
+ );
+ qtest_irq_intercept_in(test, "/machine/soc/plic");
+ /* Set mtimecmp; approx 1 second. */
+ qtest_writel(test, TIMER_ADDR(A_COMPARE_LOWER0), 1000000);
+ qtest_writel(test, TIMER_ADDR(A_COMPARE_UPPER0), 0);
+
+ /* Wait for some amount of time. Timer should not tick. */
+ qtest_clock_step(test, NANOS_PER_SECOND * 30);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==, 0);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==, 0);
+
+ /* First, enable the timer. */
+ qtest_writel(test, TIMER_ADDR(A_CTRL), 0x1);
+ qtest_writel(test, TIMER_ADDR(A_INTR_ENABLE), 0x1);
+
+ /* The counter should remain at 0, and no interrupts. */
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==, 0);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==, 0);
+ g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
+
+ /* Let it run for half a second. No interrupts. */
+ qtest_clock_step(test, NANOS_PER_SECOND / 2);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), <,
+ qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)));
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), <=,
+ qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)));
+ g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
+
+ /* Let it run for half a second again. Interrupt. */
+ qtest_clock_step(test, NANOS_PER_SECOND / 2);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==,
+ qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)));
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==,
+ qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)));
+ g_assert(qtest_get_irq(test, TIMER_EXPIRED_IRQ));
+
+ /* Handle interrupt, no more interrupt after. */
+ qtest_writel(test, TIMER_ADDR(A_LOWER_0), 0x0);
+ qtest_writel(test, TIMER_ADDR(A_UPPER_0), 0x0);
+ qtest_writel(test, TIMER_ADDR(A_INTR_STATE), 0x1);
+ g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
+
+ /* One more second. */
+ qtest_clock_step(test, NANOS_PER_SECOND);
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==,
+ qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)));
+ g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==,
+ qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)));
+ g_assert(qtest_get_irq(test, TIMER_EXPIRED_IRQ));
+
+ qtest_quit(test);
+}
+
+int main(int argc, char **argv)
+{
+ g_test_init(&argc, &argv, NULL);
+ qtest_add_func("ibex-timer/reset", test_reset);
+ qtest_add_func("ibex-timer/writes", test_writes);
+ qtest_add_func("ibex-timer/op", test_operation);
+ return g_test_run();
+}
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index fb63b8d3fa..7a769a79c5 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -235,7 +235,8 @@ qtests_s390x = \
'migration-test']
qtests_riscv32 = \
- ['ibex-aon-timer-test']
+ ['ibex-aon-timer-test',
+ 'ibex-timer-test']
qos_test_ss = ss.source_set()
qos_test_ss.add(
--
2.34.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/3] hw/timer: ibex_timer.c: Add support for writes to mtime
2022-09-22 15:58 [PATCH v2 3/3] hw/timer: ibex_timer.c: Add support for writes to mtime Tyler Ng
@ 2022-09-22 21:45 ` Philippe Mathieu-Daudé
2022-09-26 21:05 ` Dong, Eddie
1 sibling, 0 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé via @ 2022-09-22 21:45 UTC (permalink / raw)
To: Tyler Ng, open list:RISC-V, qemu-devel@nongnu.org Developers
Cc: Alistair Francis, Bin Meng, Thomas Huth, Paolo Bonzini,
Palmer Dabbelt, Laurent Vivier
On 22/9/22 17:58, Tyler Ng wrote:
> 1. Adds fields to hold the value of mtime in timer_upper0 and timer_lower0.
>
> 2. Changes the read and write functions to use the mtime fields.
>
> 3. Updates the value of mtime in update_mtime() by extrapolating the
> time elapsed. This will need to change if/when the prescalar is
> implemented.
>
> 4. Adds a qtest for the ibex timer.
>
> Signed-off-by: Tyler Ng <tkng@rivosinc.com>
> ---
> hw/timer/ibex_timer.c | 98 +++++++++++++------
> include/hw/timer/ibex_timer.h | 6 ++
> tests/qtest/ibex-timer-test.c | 178 ++++++++++++++++++++++++++++++++++
> tests/qtest/meson.build | 3 +-
> 4 files changed, 256 insertions(+), 29 deletions(-)
> create mode 100644 tests/qtest/ibex-timer-test.c
> -static void ibex_timer_update_irqs(IbexTimerState *s)
> +/*
> + * The goal of this function is to:
> + * 1. Check if the timer is enabled. If not, return false,
> + * 2. Calculate the amount of time that has passed since.
> + * 3. Extrapolate the number of ticks that have passed, and add it to `mtime`.
> + * 4. Return true.
> + */
> +static bool update_mtime(IbexTimerState *s)
> {
> - uint64_t value = s->timer_compare_lower0 |
> - ((uint64_t)s->timer_compare_upper0 << 32);
> - uint64_t next, diff;
> - uint64_t now = cpu_riscv_read_rtc(s->timebase_freq);
> -
> if (!(s->timer_ctrl & R_CTRL_ACTIVE_MASK)) {
> - /* Timer isn't active */
> + return false;
> + }
> + /* Get the time then extrapolate the number of ticks that have elapsed */
> + uint64_t mtime = get_mtime(s);
> + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + int64_t elapsed = now - s->timer_last_update;
> + if (elapsed < 0) {
> + /* We jumped back in time. */
> + mtime -= muldiv64((uint64_t)(-elapsed), s->timebase_freq,
> + NANOSECONDS_PER_SECOND);
> + } else {
> + mtime += muldiv64(elapsed, s->timebase_freq, NANOSECONDS_PER_SECOND);
> + }
> + s->timer_lower0 = mtime & 0xffffffff;
> + s->timer_upper0 = (mtime >> 32) & 0xffffffff;
Could use extract64(mtime, 0, 32) and extract64(mtime, 32, 32);
> + /* update last-checkpoint timestamp */
> + s->timer_last_update = now;
> + return true;
> +}
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/3] hw/timer: ibex_timer.c: Add support for writes to mtime
@ 2022-09-22 21:45 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 9+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-09-22 21:45 UTC (permalink / raw)
To: Tyler Ng, open list:RISC-V, qemu-devel@nongnu.org Developers
Cc: Alistair Francis, Bin Meng, Thomas Huth, Paolo Bonzini,
Palmer Dabbelt, Laurent Vivier
On 22/9/22 17:58, Tyler Ng wrote:
> 1. Adds fields to hold the value of mtime in timer_upper0 and timer_lower0.
>
> 2. Changes the read and write functions to use the mtime fields.
>
> 3. Updates the value of mtime in update_mtime() by extrapolating the
> time elapsed. This will need to change if/when the prescalar is
> implemented.
>
> 4. Adds a qtest for the ibex timer.
>
> Signed-off-by: Tyler Ng <tkng@rivosinc.com>
> ---
> hw/timer/ibex_timer.c | 98 +++++++++++++------
> include/hw/timer/ibex_timer.h | 6 ++
> tests/qtest/ibex-timer-test.c | 178 ++++++++++++++++++++++++++++++++++
> tests/qtest/meson.build | 3 +-
> 4 files changed, 256 insertions(+), 29 deletions(-)
> create mode 100644 tests/qtest/ibex-timer-test.c
> -static void ibex_timer_update_irqs(IbexTimerState *s)
> +/*
> + * The goal of this function is to:
> + * 1. Check if the timer is enabled. If not, return false,
> + * 2. Calculate the amount of time that has passed since.
> + * 3. Extrapolate the number of ticks that have passed, and add it to `mtime`.
> + * 4. Return true.
> + */
> +static bool update_mtime(IbexTimerState *s)
> {
> - uint64_t value = s->timer_compare_lower0 |
> - ((uint64_t)s->timer_compare_upper0 << 32);
> - uint64_t next, diff;
> - uint64_t now = cpu_riscv_read_rtc(s->timebase_freq);
> -
> if (!(s->timer_ctrl & R_CTRL_ACTIVE_MASK)) {
> - /* Timer isn't active */
> + return false;
> + }
> + /* Get the time then extrapolate the number of ticks that have elapsed */
> + uint64_t mtime = get_mtime(s);
> + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + int64_t elapsed = now - s->timer_last_update;
> + if (elapsed < 0) {
> + /* We jumped back in time. */
> + mtime -= muldiv64((uint64_t)(-elapsed), s->timebase_freq,
> + NANOSECONDS_PER_SECOND);
> + } else {
> + mtime += muldiv64(elapsed, s->timebase_freq, NANOSECONDS_PER_SECOND);
> + }
> + s->timer_lower0 = mtime & 0xffffffff;
> + s->timer_upper0 = (mtime >> 32) & 0xffffffff;
Could use extract64(mtime, 0, 32) and extract64(mtime, 32, 32);
> + /* update last-checkpoint timestamp */
> + s->timer_last_update = now;
> + return true;
> +}
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v2 3/3] hw/timer: ibex_timer.c: Add support for writes to mtime
2022-09-22 15:58 [PATCH v2 3/3] hw/timer: ibex_timer.c: Add support for writes to mtime Tyler Ng
2022-09-22 21:45 ` Philippe Mathieu-Daudé
@ 2022-09-26 21:05 ` Dong, Eddie
2022-09-26 23:38 ` Tyler Ng
1 sibling, 1 reply; 9+ messages in thread
From: Dong, Eddie @ 2022-09-26 21:05 UTC (permalink / raw)
To: Tyler Ng, open list:RISC-V, qemu-devel@nongnu.org Developers
Cc: Alistair Francis, Meng, Bin, Thomas Huth, Paolo Bonzini,
Palmer Dabbelt, Laurent Vivier
> -----Original Message-----
> From: Qemu-devel <qemu-devel-bounces+eddie.dong=intel.com@nongnu.org>
> On Behalf Of Tyler Ng
> Sent: Thursday, September 22, 2022 8:59 AM
> To: open list:RISC-V <qemu-riscv@nongnu.org>; qemu-devel@nongnu.org
> Developers <qemu-devel@nongnu.org>
> Cc: Alistair Francis <Alistair.Francis@wdc.com>; Meng, Bin
> <bin.meng@windriver.com>; Thomas Huth <thuth@redhat.com>; Paolo
> Bonzini <pbonzini@redhat.com>; Palmer Dabbelt <palmer@dabbelt.com>;
> Laurent Vivier <lvivier@redhat.com>
> Subject: [PATCH v2 3/3] hw/timer: ibex_timer.c: Add support for writes to
> mtime
>
> 1. Adds fields to hold the value of mtime in timer_upper0 and timer_lower0.
>
> 2. Changes the read and write functions to use the mtime fields.
>
> 3. Updates the value of mtime in update_mtime() by extrapolating the time
> elapsed. This will need to change if/when the prescalar is implemented.
>
> 4. Adds a qtest for the ibex timer.
>
> Signed-off-by: Tyler Ng <tkng@rivosinc.com>
> ---
> hw/timer/ibex_timer.c | 98 +++++++++++++------
> include/hw/timer/ibex_timer.h | 6 ++
> tests/qtest/ibex-timer-test.c | 178 ++++++++++++++++++++++++++++++++++
> tests/qtest/meson.build | 3 +-
> 4 files changed, 256 insertions(+), 29 deletions(-) create mode 100644
> tests/qtest/ibex-timer-test.c
>
> diff --git a/hw/timer/ibex_timer.c b/hw/timer/ibex_timer.c index
> d8b8e4e1f6..4230d60e85 100644
> --- a/hw/timer/ibex_timer.c
> +++ b/hw/timer/ibex_timer.c
> @@ -52,28 +52,56 @@ REG32(INTR_STATE, 0x118) REG32(INTR_TEST,
> 0x11C)
> FIELD(INTR_TEST, T_0, 0, 1)
>
> -static uint64_t cpu_riscv_read_rtc(uint32_t timebase_freq)
> +static inline uint64_t get_mtime(void *opaque)
> {
> - return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> - timebase_freq, NANOSECONDS_PER_SECOND);
> + IbexTimerState *s = opaque;
> + return (s->timer_lower0) | ((uint64_t) s->timer_upper0 << 32);
> }
>
> -static void ibex_timer_update_irqs(IbexTimerState *s)
> +/*
> + * The goal of this function is to:
> + * 1. Check if the timer is enabled. If not, return false,
> + * 2. Calculate the amount of time that has passed since.
> + * 3. Extrapolate the number of ticks that have passed, and add it to `mtime`.
> + * 4. Return true.
> + */
> +static bool update_mtime(IbexTimerState *s)
> {
> - uint64_t value = s->timer_compare_lower0 |
> - ((uint64_t)s->timer_compare_upper0 << 32);
So the hardware actually implements 64 bits register (used in 32 bits CPU), why not use an union for this?
Same for:
+ uint32_t timer_lower0;
+ uint32_t timer_upper0;
> - uint64_t next, diff;
> - uint64_t now = cpu_riscv_read_rtc(s->timebase_freq);
> -
> if (!(s->timer_ctrl & R_CTRL_ACTIVE_MASK)) {
> - /* Timer isn't active */
> + return false;
> + }
> + /* Get the time then extrapolate the number of ticks that have elapsed */
> + uint64_t mtime = get_mtime(s);
> + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + int64_t elapsed = now - s->timer_last_update;
> + if (elapsed < 0) {
> + /* We jumped back in time. */
> + mtime -= muldiv64((uint64_t)(-elapsed), s->timebase_freq,
> + NANOSECONDS_PER_SECOND);
> + } else {
> + mtime += muldiv64(elapsed, s->timebase_freq,
> NANOSECONDS_PER_SECOND);
> + }
> + s->timer_lower0 = mtime & 0xffffffff;
> + s->timer_upper0 = (mtime >> 32) & 0xffffffff;
> + /* update last-checkpoint timestamp */
> + s->timer_last_update = now;
> + return true;
> +}
> +
> +static void ibex_timer_update_irqs(IbexTimerState *s) {
> + if (!update_mtime(s)) {
> + /* Timer is not enabled? */
> return;
> }
> + uint64_t mtimecmp = s->timer_compare_lower0 |
> + ((uint64_t)s->timer_compare_upper0 << 32);
> + uint64_t mtime = get_mtime(s);
>
> /* Update the CPUs mtimecmp */
> - s->mtimecmp = value;
> + s->mtimecmp = mtimecmp;
>
> - if (s->mtimecmp <= now) {
> + if (s->mtimecmp <= mtime) {
> /*
> * If the mtimecmp was in the past raise the interrupt now.
> */
> @@ -84,18 +112,17 @@ static void ibex_timer_update_irqs(IbexTimerState
> *s)
> }
> return;
> }
> -
> - /* Setup a timer to trigger the interrupt in the future */
> + /* Update timers: setup a timer to trigger the interrupt in the
> + future */
> qemu_irq_lower(s->m_timer_irq);
> qemu_set_irq(s->irq, false);
> -
> - diff = s->mtimecmp - now;
> - next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> - muldiv64(diff,
> - NANOSECONDS_PER_SECOND,
> - s->timebase_freq);
> -
> - if (next < qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)) {
> + /* Compute the difference, and set a timer for the next update. */
> + const uint64_t diff = mtimecmp - mtime;
> + const int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + const uint64_t towait = muldiv64(diff, NANOSECONDS_PER_SECOND,
> + s->timebase_freq);
> + /* timer_mod takes in a int64_t, not uint64_t! Need to saturate it */
> + const int64_t next = now + towait;
> + if (next < now) {
> /* We overflowed the timer, just set it as large as we can */
> timer_mod(s->mtimer, 0x7FFFFFFFFFFFFFFF);
> } else {
> @@ -124,11 +151,13 @@ static void ibex_timer_reset(DeviceState *dev)
>
> s->timer_ctrl = 0x00000000;
> s->timer_cfg0 = 0x00010000;
> + s->timer_lower0 = 0x00000000;
> + s->timer_upper0 = 0x00000000;
> s->timer_compare_lower0 = 0xFFFFFFFF;
> s->timer_compare_upper0 = 0xFFFFFFFF;
> s->timer_intr_enable = 0x00000000;
> s->timer_intr_state = 0x00000000;
> -
> + s->timer_last_update = 0x00000000;
> ibex_timer_update_irqs(s);
> }
>
> @@ -136,7 +165,6 @@ static uint64_t ibex_timer_read(void *opaque,
> hwaddr addr,
> unsigned int size) {
> IbexTimerState *s = opaque;
> - uint64_t now = cpu_riscv_read_rtc(s->timebase_freq);
> uint64_t retvalue = 0;
>
> switch (addr >> 2) {
> @@ -151,10 +179,12 @@ static uint64_t ibex_timer_read(void *opaque,
> hwaddr addr,
> retvalue = s->timer_cfg0;
> break;
> case R_LOWER0:
> - retvalue = now;
> + update_mtime(s);
> + retvalue = s->timer_lower0;
> break;
> case R_UPPER0:
> - retvalue = now >> 32;
> + update_mtime(s);
> + retvalue = s->timer_upper0;
> break;
> case R_COMPARE_LOWER0:
> retvalue = s->timer_compare_lower0; @@ -192,17 +222,26 @@ static
> void ibex_timer_write(void *opaque, hwaddr addr,
> qemu_log_mask(LOG_UNIMP, "Alert triggering not supported");
> break;
> case R_CTRL:
> + if (!(s->timer_ctrl & R_CTRL_ACTIVE_MASK)) {
> + s->timer_last_update = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + }
> s->timer_ctrl = val;
> + /* We must update IRQs, because the QEMU timer gets updated here.
> */
> + ibex_timer_update_irqs(s);
> break;
> case R_CFG0:
> qemu_log_mask(LOG_UNIMP, "Changing prescale or step not
> supported");
> s->timer_cfg0 = val;
> break;
> case R_LOWER0:
> - qemu_log_mask(LOG_UNIMP, "Changing timer value is not supported");
> + s->timer_lower0 = val;
> + s->timer_last_update = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + ibex_timer_update_irqs(s);
> break;
> case R_UPPER0:
> - qemu_log_mask(LOG_UNIMP, "Changing timer value is not supported");
> + s->timer_upper0 = val;
> + s->timer_last_update = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + ibex_timer_update_irqs(s);
> break;
> case R_COMPARE_LOWER0:
> s->timer_compare_lower0 = val;
> @@ -259,6 +298,9 @@ static const VMStateDescription vmstate_ibex_timer
> = {
> VMSTATE_UINT32(timer_compare_upper0, IbexTimerState),
> VMSTATE_UINT32(timer_intr_enable, IbexTimerState),
> VMSTATE_UINT32(timer_intr_state, IbexTimerState),
> + VMSTATE_INT64(timer_last_update, IbexTimerState),
> + VMSTATE_UINT32(timer_lower0, IbexTimerState),
> + VMSTATE_UINT32(timer_upper0, IbexTimerState),
> VMSTATE_END_OF_LIST()
> }
> };
> diff --git a/include/hw/timer/ibex_timer.h b/include/hw/timer/ibex_timer.h
> index 41f5c82a92..15c16035a8 100644
> --- a/include/hw/timer/ibex_timer.h
> +++ b/include/hw/timer/ibex_timer.h
> @@ -36,11 +36,17 @@ struct IbexTimerState {
> uint64_t mtimecmp;
> QEMUTimer *mtimer; /* Internal timer for M-mode interrupt */
>
> + int64_t timer_last_update; /* Used for extrapolating mtime. */
> +
> /* <public> */
> MemoryRegion mmio;
>
> uint32_t timer_ctrl;
> uint32_t timer_cfg0;
> +
> +
> + uint32_t timer_lower0;
> + uint32_t timer_upper0;
> uint32_t timer_compare_lower0;
> uint32_t timer_compare_upper0;
> uint32_t timer_intr_enable;
> diff --git a/tests/qtest/ibex-timer-test.c b/tests/qtest/ibex-timer-test.c new
> file mode 100644 index 0000000000..1c2dfb0b6c
> --- /dev/null
> +++ b/tests/qtest/ibex-timer-test.c
> @@ -0,0 +1,178 @@
> +/*
> + * Testing the Ibex Timer
> + *
> + * Copyright (c) 2022 Rivos Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person
> +obtaining a copy
> + * of this software and associated documentation files (the
> "Software"), to deal
> + * in the Software without restriction, including without limitation
> + the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or
> + sell
> + * copies of the Software, and to permit persons to whom the Software
> + is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> EVENT
> + SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> DAMAGES OR
> + OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +#include "qapi/qmp/qdict.h"
> +
> +#define TIMER_BASE_ADDR 0x40100000UL
> +#define TIMER_ADDR(addr) (TIMER_BASE_ADDR + addr) #define
> +TIMER_EXPIRED_IRQ 127 #define NANOS_PER_SECOND 1000000000LL
> +
> +#define A_ALERT_TEST 0x0
> +#define A_CTRL 0x4
> +#define A_CFG0 0x100
> +#define A_LOWER_0 0x104
> +#define A_UPPER_0 0x108
> +#define A_COMPARE_LOWER0 0x10C
> +#define A_COMPARE_UPPER0 0x110
> +#define A_INTR_ENABLE 0x114
> +#define A_INTR_STATE 0x118
> +#define A_INTR_TEST 0x11C
> +
> +/*
> + * Tests that regs get reset properly.
> + */
> +static void test_reset(void)
> +{
> + QTestState *test = qtest_init("-M opentitan");
> + qtest_irq_intercept_in(test, "/machine/soc/plic");
> + /* Regs are reset; skip WO regs */
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CTRL)), ==, 0);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_ENABLE)), ==, 0);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_STATE)), ==, 0);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CFG0)), ==, 0x10000);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==, 0);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==, 0);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)),
> ==,
> + UINT32_MAX);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)),
> ==,
> + UINT32_MAX);
> + g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> + qtest_quit(test);
> +}
> +
> +/*
> + * Test that writes worked.
> + */
> +static void test_writes(void)
> +{
> + QTestState *test = qtest_init("-M opentitan");
> +
> + qtest_irq_intercept_in(test, "/machine/soc/plic");
> +
> + /* Special regs that may/may not need to be tested yet */
> +/*
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_ALERT_TEST)), ==, 0);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_TEST)), ==, 0);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_STATE)), ==,
> +0); */
> + /* Write to ctrl */
> + qtest_writel(test, TIMER_ADDR(A_CTRL), 0x1);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CTRL)), ==, 0x1);
> + /* Write to intr_enable */
> + qtest_writel(test, TIMER_ADDR(A_INTR_ENABLE), 0x1);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_ENABLE)), ==,
> +0x1);
> +
> + /* Writes to config? Though none of it is supported */
> + qtest_writel(test, TIMER_ADDR(A_CFG0), 0x20001);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CFG0)), ==,
> + 0x20001);
> +
> + /* Writes to mtime */
> + qtest_writel(test, TIMER_ADDR(A_LOWER_0), 0xdeaddad);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==,
> 0xdeaddad);
> + qtest_writel(test, TIMER_ADDR(A_UPPER_0), 0xdeaddad);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==,
> + 0xdeaddad);
> +
> + /* Writes to mtimecmp */
> + qtest_writel(test, TIMER_ADDR(A_COMPARE_LOWER0), 0xdeaddad);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)),
> ==,
> + 0xdeaddad);
> + qtest_writel(test, TIMER_ADDR(A_COMPARE_UPPER0), 0xdeaddad);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)),
> ==,
> + 0xdeaddad);
> + qtest_quit(test);
> +}
> +
> +/*
> + * Test the standard operation of the timer.
> + */
> +static void test_operation(void)
> +{
> + /* A frequency of 1000000 Hz*/
> + QTestState *test = qtest_init(
> + "-M opentitan "
> + "-global driver=ibex-timer,property=timebase-freq,value=1000000"
> + );
> + qtest_irq_intercept_in(test, "/machine/soc/plic");
> + /* Set mtimecmp; approx 1 second. */
> + qtest_writel(test, TIMER_ADDR(A_COMPARE_LOWER0), 1000000);
> + qtest_writel(test, TIMER_ADDR(A_COMPARE_UPPER0), 0);
> +
> + /* Wait for some amount of time. Timer should not tick. */
> + qtest_clock_step(test, NANOS_PER_SECOND * 30);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==, 0);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==, 0);
> +
> + /* First, enable the timer. */
> + qtest_writel(test, TIMER_ADDR(A_CTRL), 0x1);
> + qtest_writel(test, TIMER_ADDR(A_INTR_ENABLE), 0x1);
> +
> + /* The counter should remain at 0, and no interrupts. */
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==, 0);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==, 0);
> + g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> +
> + /* Let it run for half a second. No interrupts. */
> + qtest_clock_step(test, NANOS_PER_SECOND / 2);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), <,
> + qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)));
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), <=,
> + qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)));
> + g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> +
> + /* Let it run for half a second again. Interrupt. */
> + qtest_clock_step(test, NANOS_PER_SECOND / 2);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==,
> + qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)));
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==,
> + qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)));
> + g_assert(qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> +
> + /* Handle interrupt, no more interrupt after. */
> + qtest_writel(test, TIMER_ADDR(A_LOWER_0), 0x0);
> + qtest_writel(test, TIMER_ADDR(A_UPPER_0), 0x0);
> + qtest_writel(test, TIMER_ADDR(A_INTR_STATE), 0x1);
> + g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> +
> + /* One more second. */
> + qtest_clock_step(test, NANOS_PER_SECOND);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==,
> + qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)));
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==,
> + qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)));
> + g_assert(qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> +
> + qtest_quit(test);
> +}
> +
> +int main(int argc, char **argv)
> +{
> + g_test_init(&argc, &argv, NULL);
> + qtest_add_func("ibex-timer/reset", test_reset);
> + qtest_add_func("ibex-timer/writes", test_writes);
> + qtest_add_func("ibex-timer/op", test_operation);
> + return g_test_run();
> +}
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build index
> fb63b8d3fa..7a769a79c5 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -235,7 +235,8 @@ qtests_s390x = \
> 'migration-test']
>
> qtests_riscv32 = \
> - ['ibex-aon-timer-test']
> + ['ibex-aon-timer-test',
> + 'ibex-timer-test']
>
> qos_test_ss = ss.source_set()
> qos_test_ss.add(
> --
> 2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/3] hw/timer: ibex_timer.c: Add support for writes to mtime
2022-09-22 21:45 ` Philippe Mathieu-Daudé
(?)
@ 2022-09-26 23:37 ` Tyler Ng
-1 siblings, 0 replies; 9+ messages in thread
From: Tyler Ng @ 2022-09-26 23:37 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: open list:RISC-V, qemu-devel@nongnu.org Developers,
Alistair Francis, Bin Meng, Thomas Huth, Paolo Bonzini,
Palmer Dabbelt, Laurent Vivier
[-- Attachment #1: Type: text/plain, Size: 2607 bytes --]
On Thu, Sep 22, 2022 at 2:45 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:
> On 22/9/22 17:58, Tyler Ng wrote:
> > 1. Adds fields to hold the value of mtime in timer_upper0 and
> timer_lower0.
> >
> > 2. Changes the read and write functions to use the mtime fields.
> >
> > 3. Updates the value of mtime in update_mtime() by extrapolating the
> > time elapsed. This will need to change if/when the prescalar is
> > implemented.
> >
> > 4. Adds a qtest for the ibex timer.
> >
> > Signed-off-by: Tyler Ng <tkng@rivosinc.com>
> > ---
> > hw/timer/ibex_timer.c | 98 +++++++++++++------
> > include/hw/timer/ibex_timer.h | 6 ++
> > tests/qtest/ibex-timer-test.c | 178 ++++++++++++++++++++++++++++++++++
> > tests/qtest/meson.build | 3 +-
> > 4 files changed, 256 insertions(+), 29 deletions(-)
> > create mode 100644 tests/qtest/ibex-timer-test.c
>
> > -static void ibex_timer_update_irqs(IbexTimerState *s)
> > +/*
> > + * The goal of this function is to:
> > + * 1. Check if the timer is enabled. If not, return false,
> > + * 2. Calculate the amount of time that has passed since.
> > + * 3. Extrapolate the number of ticks that have passed, and add it to
> `mtime`.
> > + * 4. Return true.
> > + */
> > +static bool update_mtime(IbexTimerState *s)
> > {
> > - uint64_t value = s->timer_compare_lower0 |
> > - ((uint64_t)s->timer_compare_upper0 << 32);
> > - uint64_t next, diff;
> > - uint64_t now = cpu_riscv_read_rtc(s->timebase_freq);
> > -
> > if (!(s->timer_ctrl & R_CTRL_ACTIVE_MASK)) {
> > - /* Timer isn't active */
> > + return false;
> > + }
> > + /* Get the time then extrapolate the number of ticks that have
> elapsed */
> > + uint64_t mtime = get_mtime(s);
> > + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > + int64_t elapsed = now - s->timer_last_update;
> > + if (elapsed < 0) {
> > + /* We jumped back in time. */
> > + mtime -= muldiv64((uint64_t)(-elapsed), s->timebase_freq,
> > + NANOSECONDS_PER_SECOND);
> > + } else {
> > + mtime += muldiv64(elapsed, s->timebase_freq,
> NANOSECONDS_PER_SECOND);
> > + }
> > + s->timer_lower0 = mtime & 0xffffffff;
> > + s->timer_upper0 = (mtime >> 32) & 0xffffffff;
>
> Could use extract64(mtime, 0, 32) and extract64(mtime, 32, 32);
>
>
Didn't know about this function, thanks.
> > + /* update last-checkpoint timestamp */
> > + s->timer_last_update = now;
> > + return true;
> > +}
>
[-- Attachment #2: Type: text/html, Size: 3581 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/3] hw/timer: ibex_timer.c: Add support for writes to mtime
2022-09-26 21:05 ` Dong, Eddie
@ 2022-09-26 23:38 ` Tyler Ng
2022-09-28 22:59 ` Dong, Eddie
0 siblings, 1 reply; 9+ messages in thread
From: Tyler Ng @ 2022-09-26 23:38 UTC (permalink / raw)
To: Dong, Eddie
Cc: open list:RISC-V, qemu-devel@nongnu.org Developers,
Alistair Francis, Meng, Bin, Thomas Huth, Paolo Bonzini,
Palmer Dabbelt, Laurent Vivier
[-- Attachment #1: Type: text/plain, Size: 18687 bytes --]
Hi Eddie,
On Mon, Sep 26, 2022 at 2:06 PM Dong, Eddie <eddie.dong@intel.com> wrote:
>
>
> > -----Original Message-----
> > From: Qemu-devel <qemu-devel-bounces+eddie.dong=intel.com@nongnu.org>
> > On Behalf Of Tyler Ng
> > Sent: Thursday, September 22, 2022 8:59 AM
> > To: open list:RISC-V <qemu-riscv@nongnu.org>; qemu-devel@nongnu.org
> > Developers <qemu-devel@nongnu.org>
> > Cc: Alistair Francis <Alistair.Francis@wdc.com>; Meng, Bin
> > <bin.meng@windriver.com>; Thomas Huth <thuth@redhat.com>; Paolo
> > Bonzini <pbonzini@redhat.com>; Palmer Dabbelt <palmer@dabbelt.com>;
> > Laurent Vivier <lvivier@redhat.com>
> > Subject: [PATCH v2 3/3] hw/timer: ibex_timer.c: Add support for writes to
> > mtime
> >
> > 1. Adds fields to hold the value of mtime in timer_upper0 and
> timer_lower0.
> >
> > 2. Changes the read and write functions to use the mtime fields.
> >
> > 3. Updates the value of mtime in update_mtime() by extrapolating the time
> > elapsed. This will need to change if/when the prescalar is implemented.
> >
> > 4. Adds a qtest for the ibex timer.
> >
> > Signed-off-by: Tyler Ng <tkng@rivosinc.com>
> > ---
> > hw/timer/ibex_timer.c | 98 +++++++++++++------
> > include/hw/timer/ibex_timer.h | 6 ++
> > tests/qtest/ibex-timer-test.c | 178 ++++++++++++++++++++++++++++++++++
> > tests/qtest/meson.build | 3 +-
> > 4 files changed, 256 insertions(+), 29 deletions(-) create mode 100644
> > tests/qtest/ibex-timer-test.c
> >
> > diff --git a/hw/timer/ibex_timer.c b/hw/timer/ibex_timer.c index
> > d8b8e4e1f6..4230d60e85 100644
> > --- a/hw/timer/ibex_timer.c
> > +++ b/hw/timer/ibex_timer.c
> > @@ -52,28 +52,56 @@ REG32(INTR_STATE, 0x118) REG32(INTR_TEST,
> > 0x11C)
> > FIELD(INTR_TEST, T_0, 0, 1)
> >
> > -static uint64_t cpu_riscv_read_rtc(uint32_t timebase_freq)
> > +static inline uint64_t get_mtime(void *opaque)
> > {
> > - return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> > - timebase_freq, NANOSECONDS_PER_SECOND);
> > + IbexTimerState *s = opaque;
> > + return (s->timer_lower0) | ((uint64_t) s->timer_upper0 << 32);
> > }
> >
> > -static void ibex_timer_update_irqs(IbexTimerState *s)
> > +/*
> > + * The goal of this function is to:
> > + * 1. Check if the timer is enabled. If not, return false,
> > + * 2. Calculate the amount of time that has passed since.
> > + * 3. Extrapolate the number of ticks that have passed, and add it to
> `mtime`.
> > + * 4. Return true.
> > + */
> > +static bool update_mtime(IbexTimerState *s)
> > {
> > - uint64_t value = s->timer_compare_lower0 |
> > - ((uint64_t)s->timer_compare_upper0 << 32);
> So the hardware actually implements 64 bits register (used in 32 bits
> CPU), why not use an union for this?
> Same for:
> + uint32_t timer_lower0;
> + uint32_t timer_upper0;
>
> I'm not too sure what a C union would do for us here?
> > - uint64_t next, diff;
> > - uint64_t now = cpu_riscv_read_rtc(s->timebase_freq);
> > -
> > if (!(s->timer_ctrl & R_CTRL_ACTIVE_MASK)) {
> > - /* Timer isn't active */
> > + return false;
> > + }
> > + /* Get the time then extrapolate the number of ticks that have
> elapsed */
> > + uint64_t mtime = get_mtime(s);
> > + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > + int64_t elapsed = now - s->timer_last_update;
> > + if (elapsed < 0) {
> > + /* We jumped back in time. */
> > + mtime -= muldiv64((uint64_t)(-elapsed), s->timebase_freq,
> > + NANOSECONDS_PER_SECOND);
> > + } else {
> > + mtime += muldiv64(elapsed, s->timebase_freq,
> > NANOSECONDS_PER_SECOND);
> > + }
> > + s->timer_lower0 = mtime & 0xffffffff;
> > + s->timer_upper0 = (mtime >> 32) & 0xffffffff;
> > + /* update last-checkpoint timestamp */
> > + s->timer_last_update = now;
> > + return true;
> > +}
> > +
> > +static void ibex_timer_update_irqs(IbexTimerState *s) {
> > + if (!update_mtime(s)) {
> > + /* Timer is not enabled? */
> > return;
> > }
> > + uint64_t mtimecmp = s->timer_compare_lower0 |
> > + ((uint64_t)s->timer_compare_upper0 << 32);
> > + uint64_t mtime = get_mtime(s);
> >
> > /* Update the CPUs mtimecmp */
> > - s->mtimecmp = value;
> > + s->mtimecmp = mtimecmp;
> >
> > - if (s->mtimecmp <= now) {
> > + if (s->mtimecmp <= mtime) {
> > /*
> > * If the mtimecmp was in the past raise the interrupt now.
> > */
> > @@ -84,18 +112,17 @@ static void ibex_timer_update_irqs(IbexTimerState
> > *s)
> > }
> > return;
> > }
> > -
> > - /* Setup a timer to trigger the interrupt in the future */
> > + /* Update timers: setup a timer to trigger the interrupt in the
> > + future */
> > qemu_irq_lower(s->m_timer_irq);
> > qemu_set_irq(s->irq, false);
> > -
> > - diff = s->mtimecmp - now;
> > - next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> > - muldiv64(diff,
> > - NANOSECONDS_PER_SECOND,
> > - s->timebase_freq);
> > -
> > - if (next < qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)) {
> > + /* Compute the difference, and set a timer for the next update. */
> > + const uint64_t diff = mtimecmp - mtime;
> > + const int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > + const uint64_t towait = muldiv64(diff, NANOSECONDS_PER_SECOND,
> > + s->timebase_freq);
> > + /* timer_mod takes in a int64_t, not uint64_t! Need to saturate it
> */
> > + const int64_t next = now + towait;
> > + if (next < now) {
> > /* We overflowed the timer, just set it as large as we can */
> > timer_mod(s->mtimer, 0x7FFFFFFFFFFFFFFF);
> > } else {
> > @@ -124,11 +151,13 @@ static void ibex_timer_reset(DeviceState *dev)
> >
> > s->timer_ctrl = 0x00000000;
> > s->timer_cfg0 = 0x00010000;
> > + s->timer_lower0 = 0x00000000;
> > + s->timer_upper0 = 0x00000000;
> > s->timer_compare_lower0 = 0xFFFFFFFF;
> > s->timer_compare_upper0 = 0xFFFFFFFF;
> > s->timer_intr_enable = 0x00000000;
> > s->timer_intr_state = 0x00000000;
> > -
> > + s->timer_last_update = 0x00000000;
> > ibex_timer_update_irqs(s);
> > }
> >
> > @@ -136,7 +165,6 @@ static uint64_t ibex_timer_read(void *opaque,
> > hwaddr addr,
> > unsigned int size) {
> > IbexTimerState *s = opaque;
> > - uint64_t now = cpu_riscv_read_rtc(s->timebase_freq);
> > uint64_t retvalue = 0;
> >
> > switch (addr >> 2) {
> > @@ -151,10 +179,12 @@ static uint64_t ibex_timer_read(void *opaque,
> > hwaddr addr,
> > retvalue = s->timer_cfg0;
> > break;
> > case R_LOWER0:
> > - retvalue = now;
> > + update_mtime(s);
> > + retvalue = s->timer_lower0;
> > break;
> > case R_UPPER0:
> > - retvalue = now >> 32;
> > + update_mtime(s);
> > + retvalue = s->timer_upper0;
> > break;
> > case R_COMPARE_LOWER0:
> > retvalue = s->timer_compare_lower0; @@ -192,17 +222,26 @@ static
> > void ibex_timer_write(void *opaque, hwaddr addr,
> > qemu_log_mask(LOG_UNIMP, "Alert triggering not supported");
> > break;
> > case R_CTRL:
> > + if (!(s->timer_ctrl & R_CTRL_ACTIVE_MASK)) {
> > + s->timer_last_update =
> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > + }
> > s->timer_ctrl = val;
> > + /* We must update IRQs, because the QEMU timer gets updated
> here.
> > */
> > + ibex_timer_update_irqs(s);
> > break;
> > case R_CFG0:
> > qemu_log_mask(LOG_UNIMP, "Changing prescale or step not
> > supported");
> > s->timer_cfg0 = val;
> > break;
> > case R_LOWER0:
> > - qemu_log_mask(LOG_UNIMP, "Changing timer value is not
> supported");
> > + s->timer_lower0 = val;
> > + s->timer_last_update = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > + ibex_timer_update_irqs(s);
> > break;
> > case R_UPPER0:
> > - qemu_log_mask(LOG_UNIMP, "Changing timer value is not
> supported");
> > + s->timer_upper0 = val;
> > + s->timer_last_update = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > + ibex_timer_update_irqs(s);
> > break;
> > case R_COMPARE_LOWER0:
> > s->timer_compare_lower0 = val;
> > @@ -259,6 +298,9 @@ static const VMStateDescription vmstate_ibex_timer
> > = {
> > VMSTATE_UINT32(timer_compare_upper0, IbexTimerState),
> > VMSTATE_UINT32(timer_intr_enable, IbexTimerState),
> > VMSTATE_UINT32(timer_intr_state, IbexTimerState),
> > + VMSTATE_INT64(timer_last_update, IbexTimerState),
> > + VMSTATE_UINT32(timer_lower0, IbexTimerState),
> > + VMSTATE_UINT32(timer_upper0, IbexTimerState),
> > VMSTATE_END_OF_LIST()
> > }
> > };
> > diff --git a/include/hw/timer/ibex_timer.h
> b/include/hw/timer/ibex_timer.h
> > index 41f5c82a92..15c16035a8 100644
> > --- a/include/hw/timer/ibex_timer.h
> > +++ b/include/hw/timer/ibex_timer.h
> > @@ -36,11 +36,17 @@ struct IbexTimerState {
> > uint64_t mtimecmp;
> > QEMUTimer *mtimer; /* Internal timer for M-mode interrupt */
> >
> > + int64_t timer_last_update; /* Used for extrapolating mtime. */
> > +
> > /* <public> */
> > MemoryRegion mmio;
> >
> > uint32_t timer_ctrl;
> > uint32_t timer_cfg0;
> > +
> > +
> > + uint32_t timer_lower0;
> > + uint32_t timer_upper0;
> > uint32_t timer_compare_lower0;
> > uint32_t timer_compare_upper0;
> > uint32_t timer_intr_enable;
> > diff --git a/tests/qtest/ibex-timer-test.c
> b/tests/qtest/ibex-timer-test.c new
> > file mode 100644 index 0000000000..1c2dfb0b6c
> > --- /dev/null
> > +++ b/tests/qtest/ibex-timer-test.c
> > @@ -0,0 +1,178 @@
> > +/*
> > + * Testing the Ibex Timer
> > + *
> > + * Copyright (c) 2022 Rivos Inc.
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > +obtaining a copy
> > + * of this software and associated documentation files (the
> > "Software"), to deal
> > + * in the Software without restriction, including without limitation
> > + the rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or
> > + sell
> > + * copies of the Software, and to permit persons to whom the Software
> > + is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be
> > + included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > + EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > + MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> > EVENT
> > + SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> > DAMAGES OR
> > + OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > ARISING FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > + DEALINGS IN
> > + * THE SOFTWARE.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "libqtest.h"
> > +#include "qapi/qmp/qdict.h"
> > +
> > +#define TIMER_BASE_ADDR 0x40100000UL
> > +#define TIMER_ADDR(addr) (TIMER_BASE_ADDR + addr) #define
> > +TIMER_EXPIRED_IRQ 127 #define NANOS_PER_SECOND 1000000000LL
> > +
> > +#define A_ALERT_TEST 0x0
> > +#define A_CTRL 0x4
> > +#define A_CFG0 0x100
> > +#define A_LOWER_0 0x104
> > +#define A_UPPER_0 0x108
> > +#define A_COMPARE_LOWER0 0x10C
> > +#define A_COMPARE_UPPER0 0x110
> > +#define A_INTR_ENABLE 0x114
> > +#define A_INTR_STATE 0x118
> > +#define A_INTR_TEST 0x11C
> > +
> > +/*
> > + * Tests that regs get reset properly.
> > + */
> > +static void test_reset(void)
> > +{
> > + QTestState *test = qtest_init("-M opentitan");
> > + qtest_irq_intercept_in(test, "/machine/soc/plic");
> > + /* Regs are reset; skip WO regs */
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CTRL)), ==, 0);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_ENABLE)), ==,
> 0);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_STATE)), ==,
> 0);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CFG0)), ==,
> 0x10000);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==, 0);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==, 0);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)),
> > ==,
> > + UINT32_MAX);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)),
> > ==,
> > + UINT32_MAX);
> > + g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> > + qtest_quit(test);
> > +}
> > +
> > +/*
> > + * Test that writes worked.
> > + */
> > +static void test_writes(void)
> > +{
> > + QTestState *test = qtest_init("-M opentitan");
> > +
> > + qtest_irq_intercept_in(test, "/machine/soc/plic");
> > +
> > + /* Special regs that may/may not need to be tested yet */
> > +/*
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_ALERT_TEST)), ==,
> 0);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_TEST)), ==, 0);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_STATE)), ==,
> > +0); */
> > + /* Write to ctrl */
> > + qtest_writel(test, TIMER_ADDR(A_CTRL), 0x1);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CTRL)), ==, 0x1);
> > + /* Write to intr_enable */
> > + qtest_writel(test, TIMER_ADDR(A_INTR_ENABLE), 0x1);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_ENABLE)), ==,
> > +0x1);
> > +
> > + /* Writes to config? Though none of it is supported */
> > + qtest_writel(test, TIMER_ADDR(A_CFG0), 0x20001);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CFG0)), ==,
> > + 0x20001);
> > +
> > + /* Writes to mtime */
> > + qtest_writel(test, TIMER_ADDR(A_LOWER_0), 0xdeaddad);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==,
> > 0xdeaddad);
> > + qtest_writel(test, TIMER_ADDR(A_UPPER_0), 0xdeaddad);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==,
> > + 0xdeaddad);
> > +
> > + /* Writes to mtimecmp */
> > + qtest_writel(test, TIMER_ADDR(A_COMPARE_LOWER0), 0xdeaddad);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)),
> > ==,
> > + 0xdeaddad);
> > + qtest_writel(test, TIMER_ADDR(A_COMPARE_UPPER0), 0xdeaddad);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)),
> > ==,
> > + 0xdeaddad);
> > + qtest_quit(test);
> > +}
> > +
> > +/*
> > + * Test the standard operation of the timer.
> > + */
> > +static void test_operation(void)
> > +{
> > + /* A frequency of 1000000 Hz*/
> > + QTestState *test = qtest_init(
> > + "-M opentitan "
> > + "-global driver=ibex-timer,property=timebase-freq,value=1000000"
> > + );
> > + qtest_irq_intercept_in(test, "/machine/soc/plic");
> > + /* Set mtimecmp; approx 1 second. */
> > + qtest_writel(test, TIMER_ADDR(A_COMPARE_LOWER0), 1000000);
> > + qtest_writel(test, TIMER_ADDR(A_COMPARE_UPPER0), 0);
> > +
> > + /* Wait for some amount of time. Timer should not tick. */
> > + qtest_clock_step(test, NANOS_PER_SECOND * 30);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==, 0);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==, 0);
> > +
> > + /* First, enable the timer. */
> > + qtest_writel(test, TIMER_ADDR(A_CTRL), 0x1);
> > + qtest_writel(test, TIMER_ADDR(A_INTR_ENABLE), 0x1);
> > +
> > + /* The counter should remain at 0, and no interrupts. */
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==, 0);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==, 0);
> > + g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> > +
> > + /* Let it run for half a second. No interrupts. */
> > + qtest_clock_step(test, NANOS_PER_SECOND / 2);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), <,
> > + qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)));
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), <=,
> > + qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)));
> > + g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> > +
> > + /* Let it run for half a second again. Interrupt. */
> > + qtest_clock_step(test, NANOS_PER_SECOND / 2);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==,
> > + qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)));
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==,
> > + qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)));
> > + g_assert(qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> > +
> > + /* Handle interrupt, no more interrupt after. */
> > + qtest_writel(test, TIMER_ADDR(A_LOWER_0), 0x0);
> > + qtest_writel(test, TIMER_ADDR(A_UPPER_0), 0x0);
> > + qtest_writel(test, TIMER_ADDR(A_INTR_STATE), 0x1);
> > + g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> > +
> > + /* One more second. */
> > + qtest_clock_step(test, NANOS_PER_SECOND);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==,
> > + qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)));
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==,
> > + qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)));
> > + g_assert(qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> > +
> > + qtest_quit(test);
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > + g_test_init(&argc, &argv, NULL);
> > + qtest_add_func("ibex-timer/reset", test_reset);
> > + qtest_add_func("ibex-timer/writes", test_writes);
> > + qtest_add_func("ibex-timer/op", test_operation);
> > + return g_test_run();
> > +}
> > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build index
> > fb63b8d3fa..7a769a79c5 100644
> > --- a/tests/qtest/meson.build
> > +++ b/tests/qtest/meson.build
> > @@ -235,7 +235,8 @@ qtests_s390x = \
> > 'migration-test']
> >
> > qtests_riscv32 = \
> > - ['ibex-aon-timer-test']
> > + ['ibex-aon-timer-test',
> > + 'ibex-timer-test']
> >
> > qos_test_ss = ss.source_set()
> > qos_test_ss.add(
> > --
> > 2.34.1
>
> -Tyler
[-- Attachment #2: Type: text/html, Size: 23997 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v2 3/3] hw/timer: ibex_timer.c: Add support for writes to mtime
2022-09-26 23:38 ` Tyler Ng
@ 2022-09-28 22:59 ` Dong, Eddie
2022-09-29 16:02 ` Tyler Ng
0 siblings, 1 reply; 9+ messages in thread
From: Dong, Eddie @ 2022-09-28 22:59 UTC (permalink / raw)
To: Tyler Ng
Cc: open list:RISC-V, qemu-devel@nongnu.org Developers,
Alistair Francis, Meng, Bin, Thomas Huth, Paolo Bonzini,
Palmer Dabbelt, Laurent Vivier
From: Tyler Ng <tkng@rivosinc.com>
Sent: Monday, September 26, 2022 4:38 PM
To: Dong, Eddie <eddie.dong@intel.com>
Cc: open list:RISC-V <qemu-riscv@nongnu.org>; qemu-devel@nongnu.org Developers <qemu-devel@nongnu.org>; Alistair Francis <Alistair.Francis@wdc.com>; Meng, Bin <bin.meng@windriver.com>; Thomas Huth <thuth@redhat.com>; Paolo Bonzini <pbonzini@redhat.com>; Palmer Dabbelt <palmer@dabbelt.com>; Laurent Vivier <lvivier@redhat.com>
Subject: Re: [PATCH v2 3/3] hw/timer: ibex_timer.c: Add support for writes to mtime
Hi Eddie,
On Mon, Sep 26, 2022 at 2:06 PM Dong, Eddie <mailto:eddie.dong@intel.com> wrote:
> -----Original Message-----
> From: Qemu-devel <qemu-devel-bounces+eddie.dong=mailto:intel.com@nongnu.org>
> On Behalf Of Tyler Ng
> Sent: Thursday, September 22, 2022 8:59 AM
> To: open list:RISC-V <mailto:qemu-riscv@nongnu.org>; mailto:qemu-devel@nongnu.org
> Developers <mailto:qemu-devel@nongnu.org>
> Cc: Alistair Francis <mailto:Alistair.Francis@wdc.com>; Meng, Bin
> <mailto:bin.meng@windriver.com>; Thomas Huth <mailto:thuth@redhat.com>; Paolo
> Bonzini <mailto:pbonzini@redhat.com>; Palmer Dabbelt <mailto:palmer@dabbelt.com>;
> Laurent Vivier <mailto:lvivier@redhat.com>
> Subject: [PATCH v2 3/3] hw/timer: ibex_timer.c: Add support for writes to
> mtime
>
> 1. Adds fields to hold the value of mtime in timer_upper0 and timer_lower0.
>
> 2. Changes the read and write functions to use the mtime fields.
>
> 3. Updates the value of mtime in update_mtime() by extrapolating the time
> elapsed. This will need to change if/when the prescalar is implemented.
>
> 4. Adds a qtest for the ibex timer.
>
> Signed-off-by: Tyler Ng <mailto:tkng@rivosinc.com>
> ---
> hw/timer/ibex_timer.c | 98 +++++++++++++------
> include/hw/timer/ibex_timer.h | 6 ++
> tests/qtest/ibex-timer-test.c | 178 ++++++++++++++++++++++++++++++++++
> tests/qtest/meson.build | 3 +-
> 4 files changed, 256 insertions(+), 29 deletions(-) create mode 100644
> tests/qtest/ibex-timer-test.c
>
> diff --git a/hw/timer/ibex_timer.c b/hw/timer/ibex_timer.c index
> d8b8e4e1f6..4230d60e85 100644
> --- a/hw/timer/ibex_timer.c
> +++ b/hw/timer/ibex_timer.c
> @@ -52,28 +52,56 @@ REG32(INTR_STATE, 0x118) REG32(INTR_TEST,
> 0x11C)
> FIELD(INTR_TEST, T_0, 0, 1)
>
> -static uint64_t cpu_riscv_read_rtc(uint32_t timebase_freq)
> +static inline uint64_t get_mtime(void *opaque)
> {
> - return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> - timebase_freq, NANOSECONDS_PER_SECOND);
> + IbexTimerState *s = opaque;
> + return (s->timer_lower0) | ((uint64_t) s->timer_upper0 << 32);
> }
>
> -static void ibex_timer_update_irqs(IbexTimerState *s)
> +/*
> + * The goal of this function is to:
> + * 1. Check if the timer is enabled. If not, return false,
> + * 2. Calculate the amount of time that has passed since.
> + * 3. Extrapolate the number of ticks that have passed, and add it to `mtime`.
> + * 4. Return true.
> + */
> +static bool update_mtime(IbexTimerState *s)
> {
> - uint64_t value = s->timer_compare_lower0 |
> - ((uint64_t)s->timer_compare_upper0 << 32);
So the hardware actually implements 64 bits register (used in 32 bits CPU), why not use an union for this?
Same for:
+ uint32_t timer_lower0;
+ uint32_t timer_upper0;
I'm not too sure what a C union would do for us here?
I think what the hardware really implement is a 64 bits register, with 32 bits interface to access.
struct IbexTimerState actually defines both of them:
uint64_t mtimecmp;
uint32_t timer_compare_lower0;
uint32_t timer_compare_upper0;
Using a union can better reflect this. Also, it can avoid the convert from 2 32-bits register to 64 bits, like the above code does.
ibex_timer_update_irqs() also does this conversion.
> - uint64_t next, diff;
> - uint64_t now = cpu_riscv_read_rtc(s->timebase_freq);
> -
> if (!(s->timer_ctrl & R_CTRL_ACTIVE_MASK)) {
> - /* Timer isn't active */
> + return false;
> + }
> + /* Get the time then extrapolate the number of ticks that have elapsed */
> + uint64_t mtime = get_mtime(s);
> + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + int64_t elapsed = now - s->timer_last_update;
> + if (elapsed < 0) {
> + /* We jumped back in time. */
> + mtime -= muldiv64((uint64_t)(-elapsed), s->timebase_freq,
> + NANOSECONDS_PER_SECOND);
> + } else {
> + mtime += muldiv64(elapsed, s->timebase_freq,
> NANOSECONDS_PER_SECOND);
> + }
> + s->timer_lower0 = mtime & 0xffffffff;
> + s->timer_upper0 = (mtime >> 32) & 0xffffffff;
> + /* update last-checkpoint timestamp */
> + s->timer_last_update = now;
> + return true;
> +}
> +
> +static void ibex_timer_update_irqs(IbexTimerState *s) {
> + if (!update_mtime(s)) {
> + /* Timer is not enabled? */
> return;
> }
> + uint64_t mtimecmp = s->timer_compare_lower0 |
> + ((uint64_t)s->timer_compare_upper0 << 32);
> + uint64_t mtime = get_mtime(s);
>
> /* Update the CPUs mtimecmp */
> - s->mtimecmp = value;
> + s->mtimecmp = mtimecmp;
>
> - if (s->mtimecmp <= now) {
> + if (s->mtimecmp <= mtime) {
> /*
> * If the mtimecmp was in the past raise the interrupt now.
> */
> @@ -84,18 +112,17 @@ static void ibex_timer_update_irqs(IbexTimerState
> *s)
> }
> return;
> }
> -
> - /* Setup a timer to trigger the interrupt in the future */
> + /* Update timers: setup a timer to trigger the interrupt in the
> + future */
> qemu_irq_lower(s->m_timer_irq);
> qemu_set_irq(s->irq, false);
> -
> - diff = s->mtimecmp - now;
> - next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> - muldiv64(diff,
> - NANOSECONDS_PER_SECOND,
> - s->timebase_freq);
> -
> - if (next < qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)) {
> + /* Compute the difference, and set a timer for the next update. */
> + const uint64_t diff = mtimecmp - mtime;
> + const int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + const uint64_t towait = muldiv64(diff, NANOSECONDS_PER_SECOND,
> + s->timebase_freq);
> + /* timer_mod takes in a int64_t, not uint64_t! Need to saturate it */
> + const int64_t next = now + towait;
> + if (next < now) {
> /* We overflowed the timer, just set it as large as we can */
> timer_mod(s->mtimer, 0x7FFFFFFFFFFFFFFF);
> } else {
> @@ -124,11 +151,13 @@ static void ibex_timer_reset(DeviceState *dev)
>
> s->timer_ctrl = 0x00000000;
> s->timer_cfg0 = 0x00010000;
> + s->timer_lower0 = 0x00000000;
> + s->timer_upper0 = 0x00000000;
> s->timer_compare_lower0 = 0xFFFFFFFF;
> s->timer_compare_upper0 = 0xFFFFFFFF;
> s->timer_intr_enable = 0x00000000;
> s->timer_intr_state = 0x00000000;
> -
> + s->timer_last_update = 0x00000000;
> ibex_timer_update_irqs(s);
> }
>
> @@ -136,7 +165,6 @@ static uint64_t ibex_timer_read(void *opaque,
> hwaddr addr,
> unsigned int size) {
> IbexTimerState *s = opaque;
> - uint64_t now = cpu_riscv_read_rtc(s->timebase_freq);
> uint64_t retvalue = 0;
>
> switch (addr >> 2) {
> @@ -151,10 +179,12 @@ static uint64_t ibex_timer_read(void *opaque,
> hwaddr addr,
> retvalue = s->timer_cfg0;
> break;
> case R_LOWER0:
> - retvalue = now;
> + update_mtime(s);
> + retvalue = s->timer_lower0;
> break;
> case R_UPPER0:
> - retvalue = now >> 32;
> + update_mtime(s);
> + retvalue = s->timer_upper0;
> break;
> case R_COMPARE_LOWER0:
> retvalue = s->timer_compare_lower0; @@ -192,17 +222,26 @@ static
> void ibex_timer_write(void *opaque, hwaddr addr,
> qemu_log_mask(LOG_UNIMP, "Alert triggering not supported");
> break;
> case R_CTRL:
> + if (!(s->timer_ctrl & R_CTRL_ACTIVE_MASK)) {
> + s->timer_last_update = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + }
> s->timer_ctrl = val;
> + /* We must update IRQs, because the QEMU timer gets updated here.
> */
> + ibex_timer_update_irqs(s);
> break;
> case R_CFG0:
> qemu_log_mask(LOG_UNIMP, "Changing prescale or step not
> supported");
> s->timer_cfg0 = val;
> break;
> case R_LOWER0:
> - qemu_log_mask(LOG_UNIMP, "Changing timer value is not supported");
> + s->timer_lower0 = val;
> + s->timer_last_update = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + ibex_timer_update_irqs(s);
> break;
> case R_UPPER0:
> - qemu_log_mask(LOG_UNIMP, "Changing timer value is not supported");
> + s->timer_upper0 = val;
> + s->timer_last_update = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> + ibex_timer_update_irqs(s);
> break;
> case R_COMPARE_LOWER0:
> s->timer_compare_lower0 = val;
> @@ -259,6 +298,9 @@ static const VMStateDescription vmstate_ibex_timer
> = {
> VMSTATE_UINT32(timer_compare_upper0, IbexTimerState),
> VMSTATE_UINT32(timer_intr_enable, IbexTimerState),
> VMSTATE_UINT32(timer_intr_state, IbexTimerState),
> + VMSTATE_INT64(timer_last_update, IbexTimerState),
> + VMSTATE_UINT32(timer_lower0, IbexTimerState),
> + VMSTATE_UINT32(timer_upper0, IbexTimerState),
> VMSTATE_END_OF_LIST()
> }
> };
> diff --git a/include/hw/timer/ibex_timer.h b/include/hw/timer/ibex_timer.h
> index 41f5c82a92..15c16035a8 100644
> --- a/include/hw/timer/ibex_timer.h
> +++ b/include/hw/timer/ibex_timer.h
> @@ -36,11 +36,17 @@ struct IbexTimerState {
> uint64_t mtimecmp;
> QEMUTimer *mtimer; /* Internal timer for M-mode interrupt */
>
> + int64_t timer_last_update; /* Used for extrapolating mtime. */
> +
> /* <public> */
> MemoryRegion mmio;
>
> uint32_t timer_ctrl;
> uint32_t timer_cfg0;
> +
> +
> + uint32_t timer_lower0;
> + uint32_t timer_upper0;
> uint32_t timer_compare_lower0;
> uint32_t timer_compare_upper0;
> uint32_t timer_intr_enable;
> diff --git a/tests/qtest/ibex-timer-test.c b/tests/qtest/ibex-timer-test.c new
> file mode 100644 index 0000000000..1c2dfb0b6c
> --- /dev/null
> +++ b/tests/qtest/ibex-timer-test.c
> @@ -0,0 +1,178 @@
> +/*
> + * Testing the Ibex Timer
> + *
> + * Copyright (c) 2022 Rivos Inc.
> + *
> + * Permission is hereby granted, free of charge, to any person
> +obtaining a copy
> + * of this software and associated documentation files (the
> "Software"), to deal
> + * in the Software without restriction, including without limitation
> + the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or
> + sell
> + * copies of the Software, and to permit persons to whom the Software
> + is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be
> + included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> EVENT
> + SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> DAMAGES OR
> + OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "libqtest.h"
> +#include "qapi/qmp/qdict.h"
> +
> +#define TIMER_BASE_ADDR 0x40100000UL
> +#define TIMER_ADDR(addr) (TIMER_BASE_ADDR + addr) #define
> +TIMER_EXPIRED_IRQ 127 #define NANOS_PER_SECOND 1000000000LL
> +
> +#define A_ALERT_TEST 0x0
> +#define A_CTRL 0x4
> +#define A_CFG0 0x100
> +#define A_LOWER_0 0x104
> +#define A_UPPER_0 0x108
> +#define A_COMPARE_LOWER0 0x10C
> +#define A_COMPARE_UPPER0 0x110
> +#define A_INTR_ENABLE 0x114
> +#define A_INTR_STATE 0x118
> +#define A_INTR_TEST 0x11C
> +
> +/*
> + * Tests that regs get reset properly.
> + */
> +static void test_reset(void)
> +{
> + QTestState *test = qtest_init("-M opentitan");
> + qtest_irq_intercept_in(test, "/machine/soc/plic");
> + /* Regs are reset; skip WO regs */
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CTRL)), ==, 0);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_ENABLE)), ==, 0);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_STATE)), ==, 0);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CFG0)), ==, 0x10000);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==, 0);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==, 0);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)),
> ==,
> + UINT32_MAX);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)),
> ==,
> + UINT32_MAX);
> + g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> + qtest_quit(test);
> +}
> +
> +/*
> + * Test that writes worked.
> + */
> +static void test_writes(void)
> +{
> + QTestState *test = qtest_init("-M opentitan");
> +
> + qtest_irq_intercept_in(test, "/machine/soc/plic");
> +
> + /* Special regs that may/may not need to be tested yet */
> +/*
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_ALERT_TEST)), ==, 0);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_TEST)), ==, 0);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_STATE)), ==,
> +0); */
> + /* Write to ctrl */
> + qtest_writel(test, TIMER_ADDR(A_CTRL), 0x1);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CTRL)), ==, 0x1);
> + /* Write to intr_enable */
> + qtest_writel(test, TIMER_ADDR(A_INTR_ENABLE), 0x1);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_ENABLE)), ==,
> +0x1);
> +
> + /* Writes to config? Though none of it is supported */
> + qtest_writel(test, TIMER_ADDR(A_CFG0), 0x20001);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CFG0)), ==,
> + 0x20001);
> +
> + /* Writes to mtime */
> + qtest_writel(test, TIMER_ADDR(A_LOWER_0), 0xdeaddad);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==,
> 0xdeaddad);
> + qtest_writel(test, TIMER_ADDR(A_UPPER_0), 0xdeaddad);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==,
> + 0xdeaddad);
> +
> + /* Writes to mtimecmp */
> + qtest_writel(test, TIMER_ADDR(A_COMPARE_LOWER0), 0xdeaddad);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)),
> ==,
> + 0xdeaddad);
> + qtest_writel(test, TIMER_ADDR(A_COMPARE_UPPER0), 0xdeaddad);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)),
> ==,
> + 0xdeaddad);
> + qtest_quit(test);
> +}
> +
> +/*
> + * Test the standard operation of the timer.
> + */
> +static void test_operation(void)
> +{
> + /* A frequency of 1000000 Hz*/
> + QTestState *test = qtest_init(
> + "-M opentitan "
> + "-global driver=ibex-timer,property=timebase-freq,value=1000000"
> + );
> + qtest_irq_intercept_in(test, "/machine/soc/plic");
> + /* Set mtimecmp; approx 1 second. */
> + qtest_writel(test, TIMER_ADDR(A_COMPARE_LOWER0), 1000000);
> + qtest_writel(test, TIMER_ADDR(A_COMPARE_UPPER0), 0);
> +
> + /* Wait for some amount of time. Timer should not tick. */
> + qtest_clock_step(test, NANOS_PER_SECOND * 30);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==, 0);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==, 0);
> +
> + /* First, enable the timer. */
> + qtest_writel(test, TIMER_ADDR(A_CTRL), 0x1);
> + qtest_writel(test, TIMER_ADDR(A_INTR_ENABLE), 0x1);
> +
> + /* The counter should remain at 0, and no interrupts. */
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==, 0);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==, 0);
> + g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> +
> + /* Let it run for half a second. No interrupts. */
> + qtest_clock_step(test, NANOS_PER_SECOND / 2);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), <,
> + qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)));
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), <=,
> + qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)));
> + g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> +
> + /* Let it run for half a second again. Interrupt. */
> + qtest_clock_step(test, NANOS_PER_SECOND / 2);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==,
> + qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)));
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==,
> + qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)));
> + g_assert(qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> +
> + /* Handle interrupt, no more interrupt after. */
> + qtest_writel(test, TIMER_ADDR(A_LOWER_0), 0x0);
> + qtest_writel(test, TIMER_ADDR(A_UPPER_0), 0x0);
> + qtest_writel(test, TIMER_ADDR(A_INTR_STATE), 0x1);
> + g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> +
> + /* One more second. */
> + qtest_clock_step(test, NANOS_PER_SECOND);
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==,
> + qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)));
> + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==,
> + qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)));
> + g_assert(qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> +
> + qtest_quit(test);
> +}
> +
> +int main(int argc, char **argv)
> +{
> + g_test_init(&argc, &argv, NULL);
> + qtest_add_func("ibex-timer/reset", test_reset);
> + qtest_add_func("ibex-timer/writes", test_writes);
> + qtest_add_func("ibex-timer/op", test_operation);
> + return g_test_run();
> +}
> diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build index
> fb63b8d3fa..7a769a79c5 100644
> --- a/tests/qtest/meson.build
> +++ b/tests/qtest/meson.build
> @@ -235,7 +235,8 @@ qtests_s390x = \
> 'migration-test']
>
> qtests_riscv32 = \
> - ['ibex-aon-timer-test']
> + ['ibex-aon-timer-test',
> + 'ibex-timer-test']
>
> qos_test_ss = ss.source_set()
> qos_test_ss.add(
> --
> 2.34.1
-Tyler
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 3/3] hw/timer: ibex_timer.c: Add support for writes to mtime
2022-09-28 22:59 ` Dong, Eddie
@ 2022-09-29 16:02 ` Tyler Ng
2022-09-29 18:11 ` Dong, Eddie
0 siblings, 1 reply; 9+ messages in thread
From: Tyler Ng @ 2022-09-29 16:02 UTC (permalink / raw)
To: Dong, Eddie
Cc: open list:RISC-V, qemu-devel@nongnu.org Developers,
Alistair Francis, Meng, Bin, Thomas Huth, Paolo Bonzini,
Palmer Dabbelt, Laurent Vivier
[-- Attachment #1: Type: text/plain, Size: 20331 bytes --]
On Wed, Sep 28, 2022 at 4:00 PM Dong, Eddie <eddie.dong@intel.com> wrote:
>
>
> From: Tyler Ng <tkng@rivosinc.com>
> Sent: Monday, September 26, 2022 4:38 PM
> To: Dong, Eddie <eddie.dong@intel.com>
> Cc: open list:RISC-V <qemu-riscv@nongnu.org>; qemu-devel@nongnu.org
> Developers <qemu-devel@nongnu.org>; Alistair Francis <
> Alistair.Francis@wdc.com>; Meng, Bin <bin.meng@windriver.com>; Thomas
> Huth <thuth@redhat.com>; Paolo Bonzini <pbonzini@redhat.com>; Palmer
> Dabbelt <palmer@dabbelt.com>; Laurent Vivier <lvivier@redhat.com>
> Subject: Re: [PATCH v2 3/3] hw/timer: ibex_timer.c: Add support for writes
> to mtime
>
> Hi Eddie,
>
> On Mon, Sep 26, 2022 at 2:06 PM Dong, Eddie <mailto:eddie.dong@intel.com>
> wrote:
>
>
> > -----Original Message-----
> > From: Qemu-devel <qemu-devel-bounces+eddie.dong=mailto:
> intel.com@nongnu.org>
> > On Behalf Of Tyler Ng
> > Sent: Thursday, September 22, 2022 8:59 AM
> > To: open list:RISC-V <mailto:qemu-riscv@nongnu.org>; mailto:
> qemu-devel@nongnu.org
> > Developers <mailto:qemu-devel@nongnu.org>
> > Cc: Alistair Francis <mailto:Alistair.Francis@wdc.com>; Meng, Bin
> > <mailto:bin.meng@windriver.com>; Thomas Huth <mailto:thuth@redhat.com>;
> Paolo
> > Bonzini <mailto:pbonzini@redhat.com>; Palmer Dabbelt <mailto:
> palmer@dabbelt.com>;
> > Laurent Vivier <mailto:lvivier@redhat.com>
> > Subject: [PATCH v2 3/3] hw/timer: ibex_timer.c: Add support for writes to
> > mtime
> >
> > 1. Adds fields to hold the value of mtime in timer_upper0 and
> timer_lower0.
> >
> > 2. Changes the read and write functions to use the mtime fields.
> >
> > 3. Updates the value of mtime in update_mtime() by extrapolating the time
> > elapsed. This will need to change if/when the prescalar is implemented.
> >
> > 4. Adds a qtest for the ibex timer.
> >
> > Signed-off-by: Tyler Ng <mailto:tkng@rivosinc.com>
> > ---
> > hw/timer/ibex_timer.c | 98 +++++++++++++------
> > include/hw/timer/ibex_timer.h | 6 ++
> > tests/qtest/ibex-timer-test.c | 178 ++++++++++++++++++++++++++++++++++
> > tests/qtest/meson.build | 3 +-
> > 4 files changed, 256 insertions(+), 29 deletions(-) create mode 100644
> > tests/qtest/ibex-timer-test.c
> >
> > diff --git a/hw/timer/ibex_timer.c b/hw/timer/ibex_timer.c index
> > d8b8e4e1f6..4230d60e85 100644
> > --- a/hw/timer/ibex_timer.c
> > +++ b/hw/timer/ibex_timer.c
> > @@ -52,28 +52,56 @@ REG32(INTR_STATE, 0x118) REG32(INTR_TEST,
> > 0x11C)
> > FIELD(INTR_TEST, T_0, 0, 1)
> >
> > -static uint64_t cpu_riscv_read_rtc(uint32_t timebase_freq)
> > +static inline uint64_t get_mtime(void *opaque)
> > {
> > - return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> > - timebase_freq, NANOSECONDS_PER_SECOND);
> > + IbexTimerState *s = opaque;
> > + return (s->timer_lower0) | ((uint64_t) s->timer_upper0 << 32);
> > }
> >
> > -static void ibex_timer_update_irqs(IbexTimerState *s)
> > +/*
> > + * The goal of this function is to:
> > + * 1. Check if the timer is enabled. If not, return false,
> > + * 2. Calculate the amount of time that has passed since.
> > + * 3. Extrapolate the number of ticks that have passed, and add it to
> `mtime`.
> > + * 4. Return true.
> > + */
> > +static bool update_mtime(IbexTimerState *s)
> > {
> > - uint64_t value = s->timer_compare_lower0 |
> > - ((uint64_t)s->timer_compare_upper0 << 32);
> So the hardware actually implements 64 bits register (used in 32 bits
> CPU), why not use an union for this?
> Same for:
> + uint32_t timer_lower0;
> + uint32_t timer_upper0;
> I'm not too sure what a C union would do for us here?
>
> I think what the hardware really implement is a 64 bits register, with 32
> bits interface to access.
> struct IbexTimerState actually defines both of them:
> uint64_t mtimecmp;
> uint32_t timer_compare_lower0;
> uint32_t timer_compare_upper0;
>
>
It seems that the 64-bit mtimecmp field was added between patch versions...
Upon closer inspection I believe it duplicates functionality of the
existing 32-bit fields. It probably should be removed.
> Using a union can better reflect this. Also, it can avoid the convert from
> 2 32-bits register to 64 bits, like the above code does.
> ibex_timer_update_irqs() also does this conversion.
>
>
It took me a bit of time, but now I think I understand what you mean: a
union of 2 uint32_t's (perhaps packed into a struct or an array) and a
uint64_t would make it easier to access the components, is that what you
mean? That is pretty handy, thanks.
-Tyler
> > - uint64_t next, diff;
> > - uint64_t now = cpu_riscv_read_rtc(s->timebase_freq);
> > -
> > if (!(s->timer_ctrl & R_CTRL_ACTIVE_MASK)) {
> > - /* Timer isn't active */
> > + return false;
> > + }
> > + /* Get the time then extrapolate the number of ticks that have
> elapsed */
> > + uint64_t mtime = get_mtime(s);
> > + int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > + int64_t elapsed = now - s->timer_last_update;
> > + if (elapsed < 0) {
> > + /* We jumped back in time. */
> > + mtime -= muldiv64((uint64_t)(-elapsed), s->timebase_freq,
> > + NANOSECONDS_PER_SECOND);
> > + } else {
> > + mtime += muldiv64(elapsed, s->timebase_freq,
> > NANOSECONDS_PER_SECOND);
> > + }
> > + s->timer_lower0 = mtime & 0xffffffff;
> > + s->timer_upper0 = (mtime >> 32) & 0xffffffff;
> > + /* update last-checkpoint timestamp */
> > + s->timer_last_update = now;
> > + return true;
> > +}
> > +
> > +static void ibex_timer_update_irqs(IbexTimerState *s) {
> > + if (!update_mtime(s)) {
> > + /* Timer is not enabled? */
> > return;
> > }
> > + uint64_t mtimecmp = s->timer_compare_lower0 |
> > + ((uint64_t)s->timer_compare_upper0 << 32);
> > + uint64_t mtime = get_mtime(s);
> >
> > /* Update the CPUs mtimecmp */
> > - s->mtimecmp = value;
> > + s->mtimecmp = mtimecmp;
> >
> > - if (s->mtimecmp <= now) {
> > + if (s->mtimecmp <= mtime) {
> > /*
> > * If the mtimecmp was in the past raise the interrupt now.
> > */
> > @@ -84,18 +112,17 @@ static void ibex_timer_update_irqs(IbexTimerState
> > *s)
> > }
> > return;
> > }
> > -
> > - /* Setup a timer to trigger the interrupt in the future */
> > + /* Update timers: setup a timer to trigger the interrupt in the
> > + future */
> > qemu_irq_lower(s->m_timer_irq);
> > qemu_set_irq(s->irq, false);
> > -
> > - diff = s->mtimecmp - now;
> > - next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> > - muldiv64(diff,
> > - NANOSECONDS_PER_SECOND,
> > - s->timebase_freq);
> > -
> > - if (next < qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)) {
> > + /* Compute the difference, and set a timer for the next update. */
> > + const uint64_t diff = mtimecmp - mtime;
> > + const int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > + const uint64_t towait = muldiv64(diff, NANOSECONDS_PER_SECOND,
> > + s->timebase_freq);
> > + /* timer_mod takes in a int64_t, not uint64_t! Need to saturate it
> */
> > + const int64_t next = now + towait;
> > + if (next < now) {
> > /* We overflowed the timer, just set it as large as we can */
> > timer_mod(s->mtimer, 0x7FFFFFFFFFFFFFFF);
> > } else {
> > @@ -124,11 +151,13 @@ static void ibex_timer_reset(DeviceState *dev)
> >
> > s->timer_ctrl = 0x00000000;
> > s->timer_cfg0 = 0x00010000;
> > + s->timer_lower0 = 0x00000000;
> > + s->timer_upper0 = 0x00000000;
> > s->timer_compare_lower0 = 0xFFFFFFFF;
> > s->timer_compare_upper0 = 0xFFFFFFFF;
> > s->timer_intr_enable = 0x00000000;
> > s->timer_intr_state = 0x00000000;
> > -
> > + s->timer_last_update = 0x00000000;
> > ibex_timer_update_irqs(s);
> > }
> >
> > @@ -136,7 +165,6 @@ static uint64_t ibex_timer_read(void *opaque,
> > hwaddr addr,
> > unsigned int size) {
> > IbexTimerState *s = opaque;
> > - uint64_t now = cpu_riscv_read_rtc(s->timebase_freq);
> > uint64_t retvalue = 0;
> >
> > switch (addr >> 2) {
> > @@ -151,10 +179,12 @@ static uint64_t ibex_timer_read(void *opaque,
> > hwaddr addr,
> > retvalue = s->timer_cfg0;
> > break;
> > case R_LOWER0:
> > - retvalue = now;
> > + update_mtime(s);
> > + retvalue = s->timer_lower0;
> > break;
> > case R_UPPER0:
> > - retvalue = now >> 32;
> > + update_mtime(s);
> > + retvalue = s->timer_upper0;
> > break;
> > case R_COMPARE_LOWER0:
> > retvalue = s->timer_compare_lower0; @@ -192,17 +222,26 @@ static
> > void ibex_timer_write(void *opaque, hwaddr addr,
> > qemu_log_mask(LOG_UNIMP, "Alert triggering not supported");
> > break;
> > case R_CTRL:
> > + if (!(s->timer_ctrl & R_CTRL_ACTIVE_MASK)) {
> > + s->timer_last_update =
> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > + }
> > s->timer_ctrl = val;
> > + /* We must update IRQs, because the QEMU timer gets updated
> here.
> > */
> > + ibex_timer_update_irqs(s);
> > break;
> > case R_CFG0:
> > qemu_log_mask(LOG_UNIMP, "Changing prescale or step not
> > supported");
> > s->timer_cfg0 = val;
> > break;
> > case R_LOWER0:
> > - qemu_log_mask(LOG_UNIMP, "Changing timer value is not
> supported");
> > + s->timer_lower0 = val;
> > + s->timer_last_update = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > + ibex_timer_update_irqs(s);
> > break;
> > case R_UPPER0:
> > - qemu_log_mask(LOG_UNIMP, "Changing timer value is not
> supported");
> > + s->timer_upper0 = val;
> > + s->timer_last_update = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > + ibex_timer_update_irqs(s);
> > break;
> > case R_COMPARE_LOWER0:
> > s->timer_compare_lower0 = val;
> > @@ -259,6 +298,9 @@ static const VMStateDescription vmstate_ibex_timer
> > = {
> > VMSTATE_UINT32(timer_compare_upper0, IbexTimerState),
> > VMSTATE_UINT32(timer_intr_enable, IbexTimerState),
> > VMSTATE_UINT32(timer_intr_state, IbexTimerState),
> > + VMSTATE_INT64(timer_last_update, IbexTimerState),
> > + VMSTATE_UINT32(timer_lower0, IbexTimerState),
> > + VMSTATE_UINT32(timer_upper0, IbexTimerState),
> > VMSTATE_END_OF_LIST()
> > }
> > };
> > diff --git a/include/hw/timer/ibex_timer.h
> b/include/hw/timer/ibex_timer.h
> > index 41f5c82a92..15c16035a8 100644
> > --- a/include/hw/timer/ibex_timer.h
> > +++ b/include/hw/timer/ibex_timer.h
> > @@ -36,11 +36,17 @@ struct IbexTimerState {
> > uint64_t mtimecmp;
> > QEMUTimer *mtimer; /* Internal timer for M-mode interrupt */
> >
> > + int64_t timer_last_update; /* Used for extrapolating mtime. */
> > +
> > /* <public> */
> > MemoryRegion mmio;
> >
> > uint32_t timer_ctrl;
> > uint32_t timer_cfg0;
> > +
> > +
> > + uint32_t timer_lower0;
> > + uint32_t timer_upper0;
> > uint32_t timer_compare_lower0;
> > uint32_t timer_compare_upper0;
> > uint32_t timer_intr_enable;
> > diff --git a/tests/qtest/ibex-timer-test.c
> b/tests/qtest/ibex-timer-test.c new
> > file mode 100644 index 0000000000..1c2dfb0b6c
> > --- /dev/null
> > +++ b/tests/qtest/ibex-timer-test.c
> > @@ -0,0 +1,178 @@
> > +/*
> > + * Testing the Ibex Timer
> > + *
> > + * Copyright (c) 2022 Rivos Inc.
> > + *
> > + * Permission is hereby granted, free of charge, to any person
> > +obtaining a copy
> > + * of this software and associated documentation files (the
> > "Software"), to deal
> > + * in the Software without restriction, including without limitation
> > + the rights
> > + * to use, copy, modify, merge, publish, distribute, sublicense, and/or
> > + sell
> > + * copies of the Software, and to permit persons to whom the Software
> > + is
> > + * furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be
> > + included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > + EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > + MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> > EVENT
> > + SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> > DAMAGES OR
> > + OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > ARISING FROM,
> > + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > + DEALINGS IN
> > + * THE SOFTWARE.
> > + */
> > +
> > +#include "qemu/osdep.h"
> > +#include "libqtest.h"
> > +#include "qapi/qmp/qdict.h"
> > +
> > +#define TIMER_BASE_ADDR 0x40100000UL
> > +#define TIMER_ADDR(addr) (TIMER_BASE_ADDR + addr) #define
> > +TIMER_EXPIRED_IRQ 127 #define NANOS_PER_SECOND 1000000000LL
> > +
> > +#define A_ALERT_TEST 0x0
> > +#define A_CTRL 0x4
> > +#define A_CFG0 0x100
> > +#define A_LOWER_0 0x104
> > +#define A_UPPER_0 0x108
> > +#define A_COMPARE_LOWER0 0x10C
> > +#define A_COMPARE_UPPER0 0x110
> > +#define A_INTR_ENABLE 0x114
> > +#define A_INTR_STATE 0x118
> > +#define A_INTR_TEST 0x11C
> > +
> > +/*
> > + * Tests that regs get reset properly.
> > + */
> > +static void test_reset(void)
> > +{
> > + QTestState *test = qtest_init("-M opentitan");
> > + qtest_irq_intercept_in(test, "/machine/soc/plic");
> > + /* Regs are reset; skip WO regs */
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CTRL)), ==, 0);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_ENABLE)), ==,
> 0);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_STATE)), ==,
> 0);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CFG0)), ==,
> 0x10000);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==, 0);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==, 0);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)),
> > ==,
> > + UINT32_MAX);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)),
> > ==,
> > + UINT32_MAX);
> > + g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> > + qtest_quit(test);
> > +}
> > +
> > +/*
> > + * Test that writes worked.
> > + */
> > +static void test_writes(void)
> > +{
> > + QTestState *test = qtest_init("-M opentitan");
> > +
> > + qtest_irq_intercept_in(test, "/machine/soc/plic");
> > +
> > + /* Special regs that may/may not need to be tested yet */
> > +/*
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_ALERT_TEST)), ==,
> 0);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_TEST)), ==, 0);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_STATE)), ==,
> > +0); */
> > + /* Write to ctrl */
> > + qtest_writel(test, TIMER_ADDR(A_CTRL), 0x1);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CTRL)), ==, 0x1);
> > + /* Write to intr_enable */
> > + qtest_writel(test, TIMER_ADDR(A_INTR_ENABLE), 0x1);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_ENABLE)), ==,
> > +0x1);
> > +
> > + /* Writes to config? Though none of it is supported */
> > + qtest_writel(test, TIMER_ADDR(A_CFG0), 0x20001);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CFG0)), ==,
> > + 0x20001);
> > +
> > + /* Writes to mtime */
> > + qtest_writel(test, TIMER_ADDR(A_LOWER_0), 0xdeaddad);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==,
> > 0xdeaddad);
> > + qtest_writel(test, TIMER_ADDR(A_UPPER_0), 0xdeaddad);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==,
> > + 0xdeaddad);
> > +
> > + /* Writes to mtimecmp */
> > + qtest_writel(test, TIMER_ADDR(A_COMPARE_LOWER0), 0xdeaddad);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)),
> > ==,
> > + 0xdeaddad);
> > + qtest_writel(test, TIMER_ADDR(A_COMPARE_UPPER0), 0xdeaddad);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)),
> > ==,
> > + 0xdeaddad);
> > + qtest_quit(test);
> > +}
> > +
> > +/*
> > + * Test the standard operation of the timer.
> > + */
> > +static void test_operation(void)
> > +{
> > + /* A frequency of 1000000 Hz*/
> > + QTestState *test = qtest_init(
> > + "-M opentitan "
> > + "-global driver=ibex-timer,property=timebase-freq,value=1000000"
> > + );
> > + qtest_irq_intercept_in(test, "/machine/soc/plic");
> > + /* Set mtimecmp; approx 1 second. */
> > + qtest_writel(test, TIMER_ADDR(A_COMPARE_LOWER0), 1000000);
> > + qtest_writel(test, TIMER_ADDR(A_COMPARE_UPPER0), 0);
> > +
> > + /* Wait for some amount of time. Timer should not tick. */
> > + qtest_clock_step(test, NANOS_PER_SECOND * 30);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==, 0);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==, 0);
> > +
> > + /* First, enable the timer. */
> > + qtest_writel(test, TIMER_ADDR(A_CTRL), 0x1);
> > + qtest_writel(test, TIMER_ADDR(A_INTR_ENABLE), 0x1);
> > +
> > + /* The counter should remain at 0, and no interrupts. */
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==, 0);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==, 0);
> > + g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> > +
> > + /* Let it run for half a second. No interrupts. */
> > + qtest_clock_step(test, NANOS_PER_SECOND / 2);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), <,
> > + qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)));
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), <=,
> > + qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)));
> > + g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> > +
> > + /* Let it run for half a second again. Interrupt. */
> > + qtest_clock_step(test, NANOS_PER_SECOND / 2);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==,
> > + qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)));
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==,
> > + qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)));
> > + g_assert(qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> > +
> > + /* Handle interrupt, no more interrupt after. */
> > + qtest_writel(test, TIMER_ADDR(A_LOWER_0), 0x0);
> > + qtest_writel(test, TIMER_ADDR(A_UPPER_0), 0x0);
> > + qtest_writel(test, TIMER_ADDR(A_INTR_STATE), 0x1);
> > + g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> > +
> > + /* One more second. */
> > + qtest_clock_step(test, NANOS_PER_SECOND);
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0)), ==,
> > + qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)));
> > + g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0)), ==,
> > + qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)));
> > + g_assert(qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> > +
> > + qtest_quit(test);
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > + g_test_init(&argc, &argv, NULL);
> > + qtest_add_func("ibex-timer/reset", test_reset);
> > + qtest_add_func("ibex-timer/writes", test_writes);
> > + qtest_add_func("ibex-timer/op", test_operation);
> > + return g_test_run();
> > +}
> > diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build index
> > fb63b8d3fa..7a769a79c5 100644
> > --- a/tests/qtest/meson.build
> > +++ b/tests/qtest/meson.build
> > @@ -235,7 +235,8 @@ qtests_s390x = \
> > 'migration-test']
> >
> > qtests_riscv32 = \
> > - ['ibex-aon-timer-test']
> > + ['ibex-aon-timer-test',
> > + 'ibex-timer-test']
> >
> > qos_test_ss = ss.source_set()
> > qos_test_ss.add(
> > --
> > 2.34.1
> -Tyler
>
[-- Attachment #2: Type: text/html, Size: 26660 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v2 3/3] hw/timer: ibex_timer.c: Add support for writes to mtime
2022-09-29 16:02 ` Tyler Ng
@ 2022-09-29 18:11 ` Dong, Eddie
0 siblings, 0 replies; 9+ messages in thread
From: Dong, Eddie @ 2022-09-29 18:11 UTC (permalink / raw)
To: Tyler Ng
Cc: open list:RISC-V, qemu-devel@nongnu.org Developers,
Alistair Francis, Meng, Bin, Thomas Huth, Paolo Bonzini,
Palmer Dabbelt, Laurent Vivier
Using a union can better reflect this. Also, it can avoid the convert from 2 32-bits register to 64 bits, like the above code does.
ibex_timer_update_irqs() also does this conversion.
It took me a bit of time, but now I think I understand what you mean: a union of 2 uint32_t's (perhaps packed into a struct or an array) and a uint64_t would make it easier to access the components, is that what you mean? That is pretty handy, thanks.
YES. You decide😊
Thanks Eddie
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-09-29 18:14 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-22 15:58 [PATCH v2 3/3] hw/timer: ibex_timer.c: Add support for writes to mtime Tyler Ng
2022-09-22 21:45 ` Philippe Mathieu-Daudé via
2022-09-22 21:45 ` Philippe Mathieu-Daudé
2022-09-26 23:37 ` Tyler Ng
2022-09-26 21:05 ` Dong, Eddie
2022-09-26 23:38 ` Tyler Ng
2022-09-28 22:59 ` Dong, Eddie
2022-09-29 16:02 ` Tyler Ng
2022-09-29 18:11 ` Dong, Eddie
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.