From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 07D94C433F5 for ; Thu, 29 Sep 2022 16:19:35 +0000 (UTC) Received: from localhost ([::1]:56954 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1odwG5-0007lX-VB for qemu-devel@archiver.kernel.org; Thu, 29 Sep 2022 12:19:34 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:55170) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1odvzi-0004le-I1 for qemu-devel@nongnu.org; Thu, 29 Sep 2022 12:02:38 -0400 Received: from mail-pf1-x431.google.com ([2607:f8b0:4864:20::431]:40654) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1odvzR-0005ok-PH for qemu-devel@nongnu.org; Thu, 29 Sep 2022 12:02:38 -0400 Received: by mail-pf1-x431.google.com with SMTP id b75so1837034pfb.7 for ; Thu, 29 Sep 2022 09:02:20 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20210112.gappssmtp.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=jHX3NbtZXPGNYrYyPj8aLVIYT2qUT1TOB4w0M9gxP7o=; b=4woqZK3CA1vAXxvN9tuP9rHR3TAzXAAo1wobzgaFcAbJ2r6MkQ8plrszEXSWmV0KwY wBqWSIxmGJ5Sbz6wHqc/QlVo2rG7CWCmNUOxa/tFK9xGWma8ODPAAu35mB221/nqhZVg NUNU/GLSFyZ0r9js+eDrc/litbu5KVAlst/Ok0XJiPYsBhQF1RZYcAO2pqjQU0Vl6XC6 MrosCTi4JewP8oivsFfWxXeEPYCy00iLHLbV9VumTBX1JEEf8l2z1V/VvHlFtjayalQV 1+Ua6d+PQV9bN0Fm4kEEvKKgAKi+KcP/WtbDMicYvXDM7y239CYCyIdAPhzSBVJFkBme Xvhw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=jHX3NbtZXPGNYrYyPj8aLVIYT2qUT1TOB4w0M9gxP7o=; b=iR3LzOIy++R1TLmgKBHhy01g4h+DzqHorSMiMo7S1WeDd+fin+zAtRVC7FgDVSYfrL ZRTUxJOnEdg9CWo2vI3+no16X8ayrovpJ+wkpiAfCrstEMWGm+2B5AOxMF/bJ2OEwc4C UjqYyFOZ+hVxJB3f01Jgm5hDeJu+Y+FP0FJKOg10o18TM0v2B0q8xVNR9eAY8rqmQjKK HsRho+VfPMwZl1BhFLbGFokndoZ9/M23c+7Jth3M0azqrAI2nkLq5Z8xYfRCS/lz8uLx rIl8Xvm2Rro9cY637I5JOaHU+AtAzG/tJH4xUsVKj3Nmqv8xfF9qU2xukBs9tjTuMHND VYKw== X-Gm-Message-State: ACrzQf0/ZZFKZq0lXvlyViptHGuTeXs9bYEfnI0ZMQkXVQL4G83jOQI+ AVO1x7/nQfYVnv1ZIj2D1AhsGO6aB4US6za+dq3HkQ== X-Google-Smtp-Source: AMsMyM7qtCsuIORHhgI3eMqQHV8557iqp07TBL3bqDBAdW9gaPyJT58+LIiS9sJuB3RVzPb4n/MCIGoXzvAGd8T1ZxE= X-Received: by 2002:a63:1e10:0:b0:439:3c93:25ab with SMTP id e16-20020a631e10000000b004393c9325abmr3558956pge.317.1664467339775; Thu, 29 Sep 2022 09:02:19 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Tyler Ng Date: Thu, 29 Sep 2022 09:02:08 -0700 Message-ID: Subject: Re: [PATCH v2 3/3] hw/timer: ibex_timer.c: Add support for writes to mtime 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 Content-Type: multipart/alternative; boundary="00000000000087297905e9d300a0" Received-SPF: pass client-ip=2607:f8b0:4864:20::431; envelope-from=tkng@rivosinc.com; helo=mail-pf1-x431.google.com X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, HTML_MESSAGE=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=unavailable autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --00000000000087297905e9d300a0 Content-Type: text/plain; charset="UTF-8" On Wed, Sep 28, 2022 at 4:00 PM Dong, Eddie wrote: > > > From: Tyler Ng > Sent: Monday, September 26, 2022 4:38 PM > To: Dong, Eddie > Cc: open list:RISC-V ; qemu-devel@nongnu.org > Developers ; Alistair Francis < > Alistair.Francis@wdc.com>; Meng, Bin ; Thomas > Huth ; Paolo Bonzini ; Palmer > Dabbelt ; Laurent Vivier > 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 > wrote: > > > > -----Original Message----- > > From: Qemu-devel intel.com@nongnu.org> > > On Behalf Of Tyler Ng > > Sent: Thursday, September 22, 2022 8:59 AM > > To: open list:RISC-V ; mailto: > qemu-devel@nongnu.org > > Developers > > Cc: Alistair Francis ; Meng, Bin > > ; Thomas Huth ; > Paolo > > Bonzini ; Palmer Dabbelt palmer@dabbelt.com>; > > Laurent Vivier > > 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 > > --- > > 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. */ > > + > > /* */ > > 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 > --00000000000087297905e9d300a0 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Wed, Sep 28, 2022 at 4:00 PM Dong,= Eddie <eddie.dong@intel.com= > wrote:


From: Tyler Ng <t= kng@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&= gt;; Alistair Francis <Alistair.Francis@wdc.com>; Meng, Bin <bin.meng@windriver.com>= ;; Thomas Huth <th= uth@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=3Dmailto: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_lo= wer0.
>
> 2. Changes the read and write functions to use the mtime fields.
>
> 3. Updates the value of mtime in update_mtime() by extrapolating the t= ime
> 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>
> ---
>=C2=A0 hw/timer/ibex_timer.c=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 9= 8 +++++++++++++------
>=C2=A0 include/hw/timer/ibex_timer.h |=C2=A0 =C2=A06 ++
>=C2=A0 tests/qtest/ibex-timer-test.c | 178 ++++++++++++++++++++++++++++= ++++++
>=C2=A0 tests/qtest/meson.build=C2=A0 =C2=A0 =C2=A0 =C2=A0|=C2=A0 =C2=A0= 3 +-
>=C2=A0 4 files changed, 256 insertions(+), 29 deletions(-)=C2=A0 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)=C2=A0 REG32(INTR_TEST, > 0x11C)
>=C2=A0 =C2=A0 =C2=A0 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)
>=C2=A0 {
> -=C2=A0 =C2=A0 return muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),<= br> > -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= timebase_freq, NANOSECONDS_PER_SECOND);
> +=C2=A0 =C2=A0 IbexTimerState *s =3D opaque;
> +=C2=A0 =C2=A0 return (s->timer_lower0) | ((uint64_t) s->timer_u= pper0 << 32);
>=C2=A0 }
>
> -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)
>=C2=A0 {
> -=C2=A0 =C2=A0 uint64_t value =3D s->timer_compare_lower0 |
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0((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:
+=C2=A0 =C2=A0 uint32_t timer_lower0;
+=C2=A0 =C2=A0 uint32_t timer_upper0;
=C2=A0 =C2=A0 I'm not too sure what a C union would do for us here?
=C2=A0
I think what the hardware really implement is a 64 bits register, with 32 b= its interface to access.
struct IbexTimerState actually defines both of them:=C2=A0
=C2=A0 uint64_t mtimecmp;
=C2=A0 uint32_t timer_compare_lower0;
=C2=A0 uint32_t timer_compare_upper0;


It seems that the 64-bit mtimecmp fiel= d was added between patch versions... Upon closer inspection I believe it d= uplicates functionality of the existing 32-bit fields. It probably should b= e removed.
=C2=A0
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 th= ink 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 t= he components, is that what you mean? That is pretty handy, thanks.

-Tyler


> -=C2=A0 =C2=A0 uint64_t next, diff;
> -=C2=A0 =C2=A0 uint64_t now =3D cpu_riscv_read_rtc(s->timebase_freq= );
> -
>=C2=A0 =C2=A0 =C2=A0 if (!(s->timer_ctrl & R_CTRL_ACTIVE_MASK)) = {
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Timer isn't active */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 return false;
> +=C2=A0 =C2=A0 }
> +=C2=A0 =C2=A0 /* Get the time then extrapolate the number of ticks th= at have elapsed */
> +=C2=A0 =C2=A0 uint64_t mtime =3D get_mtime(s);
> +=C2=A0 =C2=A0 int64_t now =3D qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);<= br> > +=C2=A0 =C2=A0 int64_t elapsed =3D now - s->timer_last_update;
> +=C2=A0 =C2=A0 if (elapsed < 0) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* We jumped back in time. */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 mtime -=3D muldiv64((uint64_t)(-elapsed),= s->timebase_freq,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0NANOSECONDS_PER_SECOND);
> +=C2=A0 =C2=A0 } else {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 mtime +=3D muldiv64(elapsed, s->timeba= se_freq,
> NANOSECONDS_PER_SECOND);
> +=C2=A0 =C2=A0 }
> +=C2=A0 =C2=A0 s->timer_lower0 =3D mtime & 0xffffffff;
> +=C2=A0 =C2=A0 s->timer_upper0 =3D (mtime >> 32) & 0xffff= ffff;
> +=C2=A0 =C2=A0 /* update last-checkpoint timestamp */
> +=C2=A0 =C2=A0 s->timer_last_update =3D now;
> +=C2=A0 =C2=A0 return true;
> +}
> +
> +static void ibex_timer_update_irqs(IbexTimerState *s) {
> +=C2=A0 =C2=A0 if (!update_mtime(s)) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Timer is not enabled? */
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return;
>=C2=A0 =C2=A0 =C2=A0 }
> +=C2=A0 =C2=A0 uint64_t mtimecmp =3D s->timer_compare_lower0 |
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0((uint64_t)s->timer_compare_upper0 << 32); > +=C2=A0 =C2=A0 uint64_t mtime =3D get_mtime(s);
>
>=C2=A0 =C2=A0 =C2=A0 /* Update the CPUs mtimecmp */
> -=C2=A0 =C2=A0 s->mtimecmp =3D value;
> +=C2=A0 =C2=A0 s->mtimecmp =3D mtimecmp;
>
> -=C2=A0 =C2=A0 if (s->mtimecmp <=3D now) {
> +=C2=A0 =C2=A0 if (s->mtimecmp <=3D mtime) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /*
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* If the mtimecmp was in the p= ast raise the interrupt now.
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
> @@ -84,18 +112,17 @@ static void ibex_timer_update_irqs(IbexTimerState=
> *s)
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return;
>=C2=A0 =C2=A0 =C2=A0 }
> -
> -=C2=A0 =C2=A0 /* Setup a timer to trigger the interrupt in the future= */
> +=C2=A0 =C2=A0 /* Update timers: setup a timer to trigger the interrup= t in the
> + future */
>=C2=A0 =C2=A0 =C2=A0 qemu_irq_lower(s->m_timer_irq);
>=C2=A0 =C2=A0 =C2=A0 qemu_set_irq(s->irq, false);
> -
> -=C2=A0 =C2=A0 diff =3D s->mtimecmp - now;
> -=C2=A0 =C2=A0 next =3D qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0muldiv64(diff,
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 NANOSECONDS_PER_SECOND,
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 s->timebase_freq);
> -
> -=C2=A0 =C2=A0 if (next < qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)) {<= br> > +=C2=A0 =C2=A0 /* Compute the difference, and set a timer for the next= update. */
> +=C2=A0 =C2=A0 const uint64_t diff =3D mtimecmp - mtime;
> +=C2=A0 =C2=A0 const int64_t now =3D qemu_clock_get_ns(QEMU_CLOCK_VIRT= UAL);
> +=C2=A0 =C2=A0 const uint64_t towait =3D muldiv64(diff, NANOSECONDS_PE= R_SECOND,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0s->timeba= se_freq);
> +=C2=A0 =C2=A0 /* timer_mod takes in a int64_t, not uint64_t! Need to = saturate it */
> +=C2=A0 =C2=A0 const int64_t next =3D now + towait;
> +=C2=A0 =C2=A0 if (next < now) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* We overflowed the timer, just set= it as large as we can */
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 timer_mod(s->mtimer, 0x7FFFFFFFFF= FFFFFF);
>=C2=A0 =C2=A0 =C2=A0 } else {
> @@ -124,11 +151,13 @@ static void ibex_timer_reset(DeviceState *dev) >
>=C2=A0 =C2=A0 =C2=A0 s->timer_ctrl =3D 0x00000000;
>=C2=A0 =C2=A0 =C2=A0 s->timer_cfg0 =3D 0x00010000;
> +=C2=A0 =C2=A0 s->timer_lower0 =3D 0x00000000;
> +=C2=A0 =C2=A0 s->timer_upper0 =3D 0x00000000;
>=C2=A0 =C2=A0 =C2=A0 s->timer_compare_lower0 =3D 0xFFFFFFFF;
>=C2=A0 =C2=A0 =C2=A0 s->timer_compare_upper0 =3D 0xFFFFFFFF;
>=C2=A0 =C2=A0 =C2=A0 s->timer_intr_enable =3D 0x00000000;
>=C2=A0 =C2=A0 =C2=A0 s->timer_intr_state =3D 0x00000000;
> -
> +=C2=A0 =C2=A0 s->timer_last_update =3D 0x00000000;
>=C2=A0 =C2=A0 =C2=A0 ibex_timer_update_irqs(s);
>=C2=A0 }
>
> @@ -136,7 +165,6 @@ static uint64_t ibex_timer_read(void *opaque,
> hwaddr addr,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0unsigned int size)=C2=A0 {
>=C2=A0 =C2=A0 =C2=A0 IbexTimerState *s =3D opaque;
> -=C2=A0 =C2=A0 uint64_t now =3D cpu_riscv_read_rtc(s->timebase_freq= );
>=C2=A0 =C2=A0 =C2=A0 uint64_t retvalue =3D 0;
>
>=C2=A0 =C2=A0 =C2=A0 switch (addr >> 2) {
> @@ -151,10 +179,12 @@ static uint64_t ibex_timer_read(void *opaque, > hwaddr addr,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 retvalue =3D s->timer_cfg0;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
>=C2=A0 =C2=A0 =C2=A0 case R_LOWER0:
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 retvalue =3D now;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 update_mtime(s);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 retvalue =3D s->timer_lower0;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
>=C2=A0 =C2=A0 =C2=A0 case R_UPPER0:
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 retvalue =3D now >> 32;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 update_mtime(s);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 retvalue =3D s->timer_upper0;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
>=C2=A0 =C2=A0 =C2=A0 case R_COMPARE_LOWER0:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 retvalue =3D s->timer_compare_low= er0; @@ -192,17 +222,26 @@ static
> void ibex_timer_write(void *opaque, hwaddr addr,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_log_mask(LOG_UNIMP, "Alert= triggering not supported");
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
>=C2=A0 =C2=A0 =C2=A0 case R_CTRL:
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!(s->timer_ctrl & R_CTRL_ACTIV= E_MASK)) {
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 s->timer_last_update =3D= qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 s->timer_ctrl =3D val;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* We must update IRQs, because the QEMU = timer gets updated here.
> */
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 ibex_timer_update_irqs(s);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
>=C2=A0 =C2=A0 =C2=A0 case R_CFG0:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_log_mask(LOG_UNIMP, "Chang= ing prescale or step not
> supported");
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 s->timer_cfg0 =3D val;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
>=C2=A0 =C2=A0 =C2=A0 case R_LOWER0:
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_log_mask(LOG_UNIMP, "Changing t= imer value is not supported");
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 s->timer_lower0 =3D val;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 s->timer_last_update =3D qemu_clock_ge= t_ns(QEMU_CLOCK_VIRTUAL);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 ibex_timer_update_irqs(s);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
>=C2=A0 =C2=A0 =C2=A0 case R_UPPER0:
> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_log_mask(LOG_UNIMP, "Changing t= imer value is not supported");
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 s->timer_upper0 =3D val;
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 s->timer_last_update =3D qemu_clock_ge= t_ns(QEMU_CLOCK_VIRTUAL);
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 ibex_timer_update_irqs(s);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 break;
>=C2=A0 =C2=A0 =C2=A0 case R_COMPARE_LOWER0:
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 s->timer_compare_lower0 =3D val;<= br> > @@ -259,6 +298,9 @@ static const VMStateDescription vmstate_ibex_timer=
> =3D {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 VMSTATE_UINT32(timer_compare_upper0,= IbexTimerState),
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 VMSTATE_UINT32(timer_intr_enable, Ib= exTimerState),
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 VMSTATE_UINT32(timer_intr_state, Ibe= xTimerState),
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 VMSTATE_INT64(timer_last_update, IbexTime= rState),
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 VMSTATE_UINT32(timer_lower0, IbexTimerSta= te),
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 VMSTATE_UINT32(timer_upper0, IbexTimerSta= te),
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 VMSTATE_END_OF_LIST()
>=C2=A0 =C2=A0 =C2=A0 }
>=C2=A0 };
> diff --git a/include/hw/timer/ibex_timer.h b/include/hw/timer/ibex_tim= er.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 {
>=C2=A0 =C2=A0 =C2=A0 uint64_t mtimecmp;
>=C2=A0 =C2=A0 =C2=A0 QEMUTimer *mtimer; /* Internal timer for M-mode in= terrupt */
>
> +=C2=A0 =C2=A0 int64_t timer_last_update; /* Used for extrapolating mt= ime. */
> +
>=C2=A0 =C2=A0 =C2=A0 /* <public> */
>=C2=A0 =C2=A0 =C2=A0 MemoryRegion mmio;
>
>=C2=A0 =C2=A0 =C2=A0 uint32_t timer_ctrl;
>=C2=A0 =C2=A0 =C2=A0 uint32_t timer_cfg0;
> +
> +
> +=C2=A0 =C2=A0 uint32_t timer_lower0;
> +=C2=A0 =C2=A0 uint32_t timer_upper0;
>=C2=A0 =C2=A0 =C2=A0 uint32_t timer_compare_lower0;
>=C2=A0 =C2=A0 =C2=A0 uint32_t timer_compare_upper0;
>=C2=A0 =C2=A0 =C2=A0 uint32_t timer_intr_enable;
> diff --git a/tests/qtest/ibex-timer-test.c b/tests/qtest/ibex-timer-te= st.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<= br> > + 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 AN= Y 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)
> +{
> +=C2=A0 =C2=A0 QTestState *test =3D qtest_init("-M opentitan"= ;);
> +=C2=A0 =C2=A0 qtest_irq_intercept_in(test, "/machine/soc/plic&qu= ot;);
> +=C2=A0 =C2=A0 /* Regs are reset; skip WO regs */
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CTRL)),= =3D=3D, 0);
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_EN= ABLE)), =3D=3D, 0);
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_ST= ATE)), =3D=3D, 0);
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CFG0)),= =3D=3D, 0x10000);
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0= )), =3D=3D, 0);
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0= )), =3D=3D, 0);
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE= _LOWER0)),
> =3D=3D,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0UINT32_MAX);
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE= _UPPER0)),
> =3D=3D,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0UINT32_MAX);
> +=C2=A0 =C2=A0 g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> +=C2=A0 =C2=A0 qtest_quit(test);
> +}
> +
> +/*
> + * Test that writes worked.
> + */
> +static void test_writes(void)
> +{
> +=C2=A0 =C2=A0 QTestState *test =3D qtest_init("-M opentitan"= ;);
> +
> +=C2=A0 =C2=A0 qtest_irq_intercept_in(test, "/machine/soc/plic&qu= ot;);
> +
> +=C2=A0 =C2=A0 /* Special regs that may/may not need to be tested yet = */
> +/*
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_ALERT_T= EST)), =3D=3D, 0);
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_TE= ST)), =3D=3D, 0);
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_ST= ATE)), =3D=3D,
> +0); */
> +=C2=A0 =C2=A0 /* Write to ctrl */
> +=C2=A0 =C2=A0 qtest_writel(test, TIMER_ADDR(A_CTRL), 0x1);
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CTRL)),= =3D=3D, 0x1);
> +=C2=A0 =C2=A0 /* Write to intr_enable */
> +=C2=A0 =C2=A0 qtest_writel(test, TIMER_ADDR(A_INTR_ENABLE), 0x1);
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_INTR_EN= ABLE)), =3D=3D,
> +0x1);
> +
> +=C2=A0 =C2=A0 /* Writes to config? Though none of it is supported */<= br> > +=C2=A0 =C2=A0 qtest_writel(test, TIMER_ADDR(A_CFG0), 0x20001);
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_CFG0)),= =3D=3D,
> + 0x20001);
> +
> +=C2=A0 =C2=A0 /* Writes to mtime */
> +=C2=A0 =C2=A0 qtest_writel(test, TIMER_ADDR(A_LOWER_0), 0xdeaddad); > +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0= )), =3D=3D,
> 0xdeaddad);
> +=C2=A0 =C2=A0 qtest_writel(test, TIMER_ADDR(A_UPPER_0), 0xdeaddad); > +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0= )), =3D=3D,
> + 0xdeaddad);
> +
> +=C2=A0 =C2=A0 /* Writes to mtimecmp */
> +=C2=A0 =C2=A0 qtest_writel(test, TIMER_ADDR(A_COMPARE_LOWER0), 0xdead= dad);
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE= _LOWER0)),
> =3D=3D,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A00xdeaddad);
> +=C2=A0 =C2=A0 qtest_writel(test, TIMER_ADDR(A_COMPARE_UPPER0), 0xdead= dad);
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_COMPARE= _UPPER0)),
> =3D=3D,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A00xdeaddad);
> +=C2=A0 =C2=A0 qtest_quit(test);
> +}
> +
> +/*
> + * Test the standard operation of the timer.
> + */
> +static void test_operation(void)
> +{
> +=C2=A0 =C2=A0 /* A frequency of 1000000 Hz*/
> +=C2=A0 =C2=A0 QTestState *test =3D qtest_init(
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "-M opentitan "
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 "-global driver=3Dibex-timer,propert= y=3Dtimebase-freq,value=3D1000000"
> +=C2=A0 =C2=A0 );
> +=C2=A0 =C2=A0 qtest_irq_intercept_in(test, "/machine/soc/plic&qu= ot;);
> +=C2=A0 =C2=A0 /* Set mtimecmp; approx 1 second. */
> +=C2=A0 =C2=A0 qtest_writel(test, TIMER_ADDR(A_COMPARE_LOWER0), 100000= 0);
> +=C2=A0 =C2=A0 qtest_writel(test, TIMER_ADDR(A_COMPARE_UPPER0), 0); > +
> +=C2=A0 =C2=A0 /* Wait for some amount of time. Timer should not tick.= */
> +=C2=A0 =C2=A0 qtest_clock_step(test, NANOS_PER_SECOND * 30);
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0= )), =3D=3D, 0);
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0= )), =3D=3D, 0);
> +
> +=C2=A0 =C2=A0 /* First, enable the timer. */
> +=C2=A0 =C2=A0 qtest_writel(test, TIMER_ADDR(A_CTRL), 0x1);
> +=C2=A0 =C2=A0 qtest_writel(test, TIMER_ADDR(A_INTR_ENABLE), 0x1);
> +
> +=C2=A0 =C2=A0 /* The counter should remain at 0, and no interrupts. *= /
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0= )), =3D=3D, 0);
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0= )), =3D=3D, 0);
> +=C2=A0 =C2=A0 g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> +
> +=C2=A0 =C2=A0 /* Let it run for half a second. No interrupts. */
> +=C2=A0 =C2=A0 qtest_clock_step(test, NANOS_PER_SECOND / 2);
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0= )), <,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)));
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0= )), <=3D,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)));
> +=C2=A0 =C2=A0 g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> +
> +=C2=A0 =C2=A0 /* Let it run for half a second again. Interrupt. */ > +=C2=A0 =C2=A0 qtest_clock_step(test, NANOS_PER_SECOND / 2);
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0= )), =3D=3D,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)));
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0= )), =3D=3D,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)));
> +=C2=A0 =C2=A0 g_assert(qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> +
> +=C2=A0 =C2=A0 /* Handle interrupt, no more interrupt after. */
> +=C2=A0 =C2=A0 qtest_writel(test, TIMER_ADDR(A_LOWER_0), 0x0);
> +=C2=A0 =C2=A0 qtest_writel(test, TIMER_ADDR(A_UPPER_0), 0x0);
> +=C2=A0 =C2=A0 qtest_writel(test, TIMER_ADDR(A_INTR_STATE), 0x1);
> +=C2=A0 =C2=A0 g_assert(!qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> +
> +=C2=A0 =C2=A0 /* One more second. */
> +=C2=A0 =C2=A0 qtest_clock_step(test, NANOS_PER_SECOND);
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_LOWER_0= )), =3D=3D,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0qtest_readl(test, TIMER_ADDR(A_COMPARE_LOWER0)));
> +=C2=A0 =C2=A0 g_assert_cmpuint(qtest_readl(test, TIMER_ADDR(A_UPPER_0= )), =3D=3D,
> +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0qtest_readl(test, TIMER_ADDR(A_COMPARE_UPPER0)));
> +=C2=A0 =C2=A0 g_assert(qtest_get_irq(test, TIMER_EXPIRED_IRQ));
> +
> +=C2=A0 =C2=A0 qtest_quit(test);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +=C2=A0 =C2=A0 g_test_init(&argc, &argv, NULL);
> +=C2=A0 =C2=A0 qtest_add_func("ibex-timer/reset", test_reset= );
> +=C2=A0 =C2=A0 qtest_add_func("ibex-timer/writes", test_writ= es);
> +=C2=A0 =C2=A0 qtest_add_func("ibex-timer/op", test_operatio= n);
> +=C2=A0 =C2=A0 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 =3D \
>=C2=A0 =C2=A0 =C2=A0'migration-test']
>
>=C2=A0 qtests_riscv32 =3D \
> -=C2=A0 ['ibex-aon-timer-test']
> +=C2=A0 ['ibex-aon-timer-test',
> +=C2=A0 =C2=A0'ibex-timer-test']
>
>=C2=A0 qos_test_ss =3D ss.source_set()
>=C2=A0 qos_test_ss.add(
> --
> 2.34.1
-Tyler
--00000000000087297905e9d300a0--