[kvm-unit-tests PATCH v2 0/3] riscv: sbi: Add support to test timer extension

[kvm-unit-tests PATCH v2 0/3] riscv: sbi: Add support to test timer extension

[James Raphael Tiovalen]

This patch series adds support for testing the timer extension as
defined in the RISC-V SBI specification. The first 2 patches add
infrastructural support for handling interrupts, while the last patch
adds the actual test for the timer extension.

v2:
- Addressed all of the previous comments from Andrew.
- Updated the test to get the timer frequency value from the device tree
  and allow the test parameters to be specified in microseconds instead of
  cycles.

Andrew Jones (1):
  riscv: Extend exception handling support for interrupts

James Raphael Tiovalen (2):
  riscv: Update exception cause list
  riscv: sbi: Add test for timer extension

 lib/riscv/asm/csr.h       |  23 +++++++++
 lib/riscv/asm/processor.h |  15 +++++-
 lib/riscv/asm/sbi.h       |   5 ++
 lib/riscv/asm/setup.h     |   1 +
 lib/riscv/asm/timer.h     |  30 +++++++++++
 lib/riscv/processor.c     |  32 ++++++++++--
 lib/riscv/setup.c         |  24 +++++++++
 riscv/sbi.c               | 106 ++++++++++++++++++++++++++++++++++++++
 8 files changed, 231 insertions(+), 5 deletions(-)
 create mode 100644 lib/riscv/asm/timer.h

--
2.43.0

[kvm-unit-tests PATCH v2 1/3] riscv: Extend exception handling support for interrupts

[James Raphael Tiovalen]

From: Andrew Jones <andrew.jones@linux.dev>

Add install_irq_handler() to enable tests to install interrupt handlers.
Also add local_irq_enable() and local_irq_disable() to respectively
enable and disable IRQs via the sstatus.SIE bit.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
---
 lib/riscv/asm/csr.h       |  2 ++
 lib/riscv/asm/processor.h | 13 +++++++++++++
 lib/riscv/processor.c     | 27 +++++++++++++++++++++++----
 3 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/lib/riscv/asm/csr.h b/lib/riscv/asm/csr.h
index 52608512..d6909d93 100644
--- a/lib/riscv/asm/csr.h
+++ b/lib/riscv/asm/csr.h
@@ -11,6 +11,8 @@
 #define CSR_STVAL		0x143
 #define CSR_SATP		0x180
 
+#define SR_SIE			_AC(0x00000002, UL)
+
 /* Exception cause high bit - is an interrupt if set */
 #define CAUSE_IRQ_FLAG		(_AC(1, UL) << (__riscv_xlen - 1))
 
diff --git a/lib/riscv/asm/processor.h b/lib/riscv/asm/processor.h
index 32c499d0..6451adb5 100644
--- a/lib/riscv/asm/processor.h
+++ b/lib/riscv/asm/processor.h
@@ -5,6 +5,7 @@
 #include <asm/ptrace.h>
 
 #define EXCEPTION_CAUSE_MAX	16
+#define INTERRUPT_CAUSE_MAX	16
 
 typedef void (*exception_fn)(struct pt_regs *);
 
@@ -13,6 +14,7 @@ struct thread_info {
 	unsigned long hartid;
 	unsigned long isa[1];
 	exception_fn exception_handlers[EXCEPTION_CAUSE_MAX];
+	exception_fn interrupt_handlers[INTERRUPT_CAUSE_MAX];
 };
 
 static inline struct thread_info *current_thread_info(void)
@@ -20,7 +22,18 @@ static inline struct thread_info *current_thread_info(void)
 	return (struct thread_info *)csr_read(CSR_SSCRATCH);
 }
 
+static inline void local_irq_enable(void)
+{
+	csr_set(CSR_SSTATUS, SR_SIE);
+}
+
+static inline void local_irq_disable(void)
+{
+	csr_clear(CSR_SSTATUS, SR_SIE);
+}
+
 void install_exception_handler(unsigned long cause, void (*handler)(struct pt_regs *));
+void install_irq_handler(unsigned long cause, void (*handler)(struct pt_regs *));
 void do_handle_exception(struct pt_regs *regs);
 void thread_info_init(void);
 
diff --git a/lib/riscv/processor.c b/lib/riscv/processor.c
index ece7cbff..0dffadc7 100644
--- a/lib/riscv/processor.c
+++ b/lib/riscv/processor.c
@@ -36,10 +36,21 @@ void do_handle_exception(struct pt_regs *regs)
 {
 	struct thread_info *info = current_thread_info();
 
-	assert(regs->cause < EXCEPTION_CAUSE_MAX);
-	if (info->exception_handlers[regs->cause]) {
-		info->exception_handlers[regs->cause](regs);
-		return;
+	if (regs->cause & CAUSE_IRQ_FLAG) {
+		unsigned long irq_cause = regs->cause & ~CAUSE_IRQ_FLAG;
+
+		assert(irq_cause < INTERRUPT_CAUSE_MAX);
+		if (info->interrupt_handlers[irq_cause]) {
+			info->interrupt_handlers[irq_cause](regs);
+			return;
+		}
+	} else {
+		assert(regs->cause < EXCEPTION_CAUSE_MAX);
+
+		if (info->exception_handlers[regs->cause]) {
+			info->exception_handlers[regs->cause](regs);
+			return;
+		}
 	}
 
 	show_regs(regs);
@@ -47,6 +58,14 @@ void do_handle_exception(struct pt_regs *regs)
 	abort();
 }
 
+void install_irq_handler(unsigned long cause, void (*handler)(struct pt_regs *))
+{
+	struct thread_info *info = current_thread_info();
+
+	assert(cause < INTERRUPT_CAUSE_MAX);
+	info->interrupt_handlers[cause] = handler;
+}
+
 void install_exception_handler(unsigned long cause, void (*handler)(struct pt_regs *))
 {
 	struct thread_info *info = current_thread_info();
-- 
2.43.0

[kvm-unit-tests PATCH v2 2/3] riscv: Update exception cause list

[James Raphael Tiovalen]

Update the list of exception and interrupt causes to follow the latest
RISC-V privileged ISA specification (version 20240411 section 18.6.1).

Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
---
 lib/riscv/asm/csr.h       | 14 ++++++++++++++
 lib/riscv/asm/processor.h |  2 +-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/lib/riscv/asm/csr.h b/lib/riscv/asm/csr.h
index d6909d93..b3c48e8e 100644
--- a/lib/riscv/asm/csr.h
+++ b/lib/riscv/asm/csr.h
@@ -28,6 +28,7 @@
 #define EXC_SYSCALL		8
 #define EXC_HYPERVISOR_SYSCALL	9
 #define EXC_SUPERVISOR_SYSCALL	10
+#define EXC_MACHINE_SYSCALL	11
 #define EXC_INST_PAGE_FAULT	12
 #define EXC_LOAD_PAGE_FAULT	13
 #define EXC_STORE_PAGE_FAULT	15
@@ -36,6 +37,19 @@
 #define EXC_VIRTUAL_INST_FAULT		22
 #define EXC_STORE_GUEST_PAGE_FAULT	23
 
+/* Interrupt causes */
+#define IRQ_S_SOFT		1
+#define IRQ_VS_SOFT		2
+#define IRQ_M_SOFT		3
+#define IRQ_S_TIMER		5
+#define IRQ_VS_TIMER		6
+#define IRQ_M_TIMER		7
+#define IRQ_S_EXT		9
+#define IRQ_VS_EXT		10
+#define IRQ_M_EXT		11
+#define IRQ_S_GEXT		12
+#define IRQ_PMU_OVF		13
+
 #ifndef __ASSEMBLY__
 
 #define csr_swap(csr, val)					\
diff --git a/lib/riscv/asm/processor.h b/lib/riscv/asm/processor.h
index 6451adb5..4c9ad968 100644
--- a/lib/riscv/asm/processor.h
+++ b/lib/riscv/asm/processor.h
@@ -4,7 +4,7 @@
 #include <asm/csr.h>
 #include <asm/ptrace.h>
 
-#define EXCEPTION_CAUSE_MAX	16
+#define EXCEPTION_CAUSE_MAX	24
 #define INTERRUPT_CAUSE_MAX	16
 
 typedef void (*exception_fn)(struct pt_regs *);
-- 
2.43.0

[kvm-unit-tests PATCH v2 3/3] riscv: sbi: Add test for timer extension

[James Raphael Tiovalen]

Add a test for the set_timer function of the time extension. The test
checks that:
- The time extension is available
- The time counter monotonically increases
- The installed timer interrupt handler is called
- The timer interrupt is received within a reasonable time interval
- The timer interrupt pending bit is cleared after the set_timer SBI
  call is made
- The timer interrupt can be cleared either by requesting a timer
  interrupt infinitely far into the future or by masking the timer
  interrupt

The timer interrupt delay can be set using the TIMER_DELAY environment
variable in microseconds. The default delay value is 1 second. Since the
interrupt can arrive a little later than the specified delay, allow some
margin of error. This margin of error can be specified via the
TIMER_MARGIN environment variable in microseconds. The default margin of
error is 1 second.

This test has been verified on RV32 and RV64 with OpenSBI using QEMU.

Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
---
 lib/riscv/asm/csr.h   |   7 +++
 lib/riscv/asm/sbi.h   |   5 ++
 lib/riscv/asm/setup.h |   1 +
 lib/riscv/asm/timer.h |  30 ++++++++++++
 lib/riscv/processor.c |   6 +++
 lib/riscv/setup.c     |  24 ++++++++++
 riscv/sbi.c           | 108 ++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 181 insertions(+)
 create mode 100644 lib/riscv/asm/timer.h

diff --git a/lib/riscv/asm/csr.h b/lib/riscv/asm/csr.h
index b3c48e8e..dc05bfc9 100644
--- a/lib/riscv/asm/csr.h
+++ b/lib/riscv/asm/csr.h
@@ -4,12 +4,15 @@
 #include <linux/const.h>
 
 #define CSR_SSTATUS		0x100
+#define CSR_SIE			0x104
 #define CSR_STVEC		0x105
 #define CSR_SSCRATCH		0x140
 #define CSR_SEPC		0x141
 #define CSR_SCAUSE		0x142
 #define CSR_STVAL		0x143
+#define CSR_SIP			0x144
 #define CSR_SATP		0x180
+#define CSR_TIME		0xc01
 
 #define SR_SIE			_AC(0x00000002, UL)
 
@@ -50,6 +53,10 @@
 #define IRQ_S_GEXT		12
 #define IRQ_PMU_OVF		13
 
+#define IE_TIE			(_AC(0x1, UL) << IRQ_S_TIMER)
+
+#define IP_TIP			IE_TIE
+
 #ifndef __ASSEMBLY__
 
 #define csr_swap(csr, val)					\
diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h
index d82a384d..84ce1bff 100644
--- a/lib/riscv/asm/sbi.h
+++ b/lib/riscv/asm/sbi.h
@@ -16,6 +16,7 @@
 
 enum sbi_ext_id {
 	SBI_EXT_BASE = 0x10,
+	SBI_EXT_TIME = 0x54494d45,
 	SBI_EXT_HSM = 0x48534d,
 	SBI_EXT_SRST = 0x53525354,
 };
@@ -37,6 +38,10 @@ enum sbi_ext_hsm_fid {
 	SBI_EXT_HSM_HART_SUSPEND,
 };
 
+enum sbi_ext_time_fid {
+	SBI_EXT_TIME_SET_TIMER = 0,
+};
+
 struct sbiret {
 	long error;
 	long value;
diff --git a/lib/riscv/asm/setup.h b/lib/riscv/asm/setup.h
index 7f81a705..5be252df 100644
--- a/lib/riscv/asm/setup.h
+++ b/lib/riscv/asm/setup.h
@@ -7,6 +7,7 @@
 #define NR_CPUS 16
 extern struct thread_info cpus[NR_CPUS];
 extern int nr_cpus;
+extern uint64_t tb_hz;
 int hartid_to_cpu(unsigned long hartid);
 
 void io_init(void);
diff --git a/lib/riscv/asm/timer.h b/lib/riscv/asm/timer.h
new file mode 100644
index 00000000..3eeb8344
--- /dev/null
+++ b/lib/riscv/asm/timer.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef _ASMRISCV_TIMER_H_
+#define _ASMRISCV_TIMER_H_
+
+#include <asm/csr.h>
+#include <asm/processor.h>
+
+extern uint64_t usec_to_cycles(uint64_t usec);
+
+static inline void timer_irq_enable(void)
+{
+	csr_set(CSR_SIE, IE_TIE);
+}
+
+static inline void timer_irq_disable(void)
+{
+	csr_clear(CSR_SIE, IE_TIE);
+}
+
+static inline uint64_t timer_get_cycles(void)
+{
+	return csr_read(CSR_TIME);
+}
+
+static inline bool timer_irq_pending(void)
+{
+	return csr_read(CSR_SIP) & IP_TIP;
+}
+
+#endif /* _ASMRISCV_TIMER_H_ */
diff --git a/lib/riscv/processor.c b/lib/riscv/processor.c
index 0dffadc7..082b9d80 100644
--- a/lib/riscv/processor.c
+++ b/lib/riscv/processor.c
@@ -7,6 +7,7 @@
 #include <asm/isa.h>
 #include <asm/processor.h>
 #include <asm/setup.h>
+#include <asm/timer.h>
 
 extern unsigned long ImageBase;
 
@@ -82,3 +83,8 @@ void thread_info_init(void)
 	isa_init(&cpus[cpu]);
 	csr_write(CSR_SSCRATCH, &cpus[cpu]);
 }
+
+uint64_t usec_to_cycles(uint64_t usec)
+{
+	return (tb_hz * usec) / 1000000;
+}
diff --git a/lib/riscv/setup.c b/lib/riscv/setup.c
index 50ffb0d0..b659c14e 100644
--- a/lib/riscv/setup.c
+++ b/lib/riscv/setup.c
@@ -20,6 +20,7 @@
 #include <asm/page.h>
 #include <asm/processor.h>
 #include <asm/setup.h>
+#include <asm/timer.h>
 
 #define VA_BASE			((phys_addr_t)3 * SZ_1G)
 #if __riscv_xlen == 64
@@ -38,6 +39,7 @@ u32 initrd_size;
 
 struct thread_info cpus[NR_CPUS];
 int nr_cpus;
+uint64_t tb_hz;
 
 static struct mem_region riscv_mem_regions[NR_MEM_REGIONS + 1];
 
@@ -67,6 +69,26 @@ static void cpu_init_acpi(void)
 	assert_msg(false, "ACPI not available");
 }
 
+static int cpu_init_timer(const void *fdt)
+{
+	const struct fdt_property *prop;
+	u32 *data;
+	int cpus;
+
+	cpus = fdt_path_offset(fdt, "/cpus");
+	if (cpus < 0)
+		return cpus;
+
+	prop = fdt_get_property(fdt, cpus, "timebase-frequency", NULL);
+	if (prop == NULL)
+		return -1;
+
+	data = (u32 *)prop->data;
+	tb_hz = fdt32_to_cpu(*data);
+
+	return 0;
+}
+
 static void cpu_init(void)
 {
 	int ret;
@@ -75,6 +97,8 @@ static void cpu_init(void)
 	if (dt_available()) {
 		ret = dt_for_each_cpu_node(cpu_set_fdt, NULL);
 		assert(ret == 0);
+		ret = cpu_init_timer(dt_fdt());
+		assert(ret == 0);
 	} else {
 		cpu_init_acpi();
 	}
diff --git a/riscv/sbi.c b/riscv/sbi.c
index 762e9711..a1a9ce84 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -6,7 +6,14 @@
  */
 #include <libcflat.h>
 #include <stdlib.h>
+#include <asm/barrier.h>
+#include <asm/csr.h>
+#include <asm/processor.h>
 #include <asm/sbi.h>
+#include <asm/timer.h>
+
+static bool timer_works;
+static bool mask_timer_irq;
 
 static void help(void)
 {
@@ -19,6 +26,27 @@ static struct sbiret __base_sbi_ecall(int fid, unsigned long arg0)
 	return sbi_ecall(SBI_EXT_BASE, fid, arg0, 0, 0, 0, 0, 0);
 }
 
+static struct sbiret __time_sbi_ecall(unsigned long stime_value)
+{
+	return sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value, 0, 0, 0, 0, 0);
+}
+
+static void timer_irq_handler(struct pt_regs *regs)
+{
+	if (timer_works)
+		report_abort("timer interrupt received multiple times");
+
+	timer_works = true;
+	report(timer_irq_pending(), "pending timer interrupt bit set");
+
+	if (mask_timer_irq)
+		timer_irq_disable();
+	else {
+		__time_sbi_ecall(-1);
+		report(!timer_irq_pending(), "pending timer interrupt bit cleared");
+	}
+}
+
 static bool env_or_skip(const char *env)
 {
 	if (!getenv(env)) {
@@ -112,6 +140,85 @@ static void check_base(void)
 	report_prefix_pop();
 }
 
+static void check_time(void)
+{
+	struct sbiret ret;
+	unsigned long begin, end, duration;
+	unsigned long delay = getenv("TIMER_DELAY")
+							? strtol(getenv("TIMER_DELAY"), NULL, 0)
+							: 1000000;
+	unsigned long margin = getenv("TIMER_MARGIN")
+							? strtol(getenv("TIMER_MARGIN"), NULL, 0)
+							: 1000000;
+
+	delay = usec_to_cycles(delay);
+	margin = usec_to_cycles(margin);
+
+	report_prefix_push("time");
+
+	ret = __base_sbi_ecall(SBI_EXT_BASE_PROBE_EXT, SBI_EXT_TIME);
+
+	if (ret.error) {
+		report_fail("probing for time extension failed");
+		report_prefix_pop();
+		return;
+	}
+
+	if (!ret.value) {
+		report_skip("time extension not available");
+		report_prefix_pop();
+		return;
+	}
+
+	begin = timer_get_cycles();
+	while ((end = timer_get_cycles()) <= begin)
+		cpu_relax();
+	assert(begin < end);
+
+	report_prefix_push("set_timer");
+
+	install_irq_handler(IRQ_S_TIMER, timer_irq_handler);
+	local_irq_enable();
+	timer_irq_enable();
+
+	begin = timer_get_cycles();
+	ret = __time_sbi_ecall(begin + delay);
+
+	if (ret.error)
+		report_fail("setting timer failed");
+
+	report(!timer_irq_pending(), "pending timer interrupt bit cleared");
+
+	while ((end = timer_get_cycles()) <= (begin + delay + margin) && !timer_works)
+		cpu_relax();
+
+	report(timer_works, "timer interrupt received");
+
+	if (timer_works) {
+		duration = end - begin;
+		report(duration >= delay && duration <= (delay + margin), "timer delay honored");
+	}
+
+	timer_works = false;
+	mask_timer_irq = true;
+	begin = timer_get_cycles();
+	ret = __time_sbi_ecall(begin + delay);
+
+	if (ret.error)
+		report_fail("setting timer failed");
+
+	while ((end = timer_get_cycles()) <= (begin + delay + margin) && !timer_works)
+		cpu_relax();
+
+	report(timer_works, "timer interrupt received");
+
+	local_irq_disable();
+	install_irq_handler(IRQ_S_TIMER, NULL);
+
+	report_prefix_pop();
+	report_prefix_pop();
+}
+
 int main(int argc, char **argv)
 {
 
@@ -122,6 +229,7 @@ int main(int argc, char **argv)
 
 	report_prefix_push("sbi");
 	check_base();
+	check_time();
 
 	return report_summary();
 }
-- 
2.43.0

Re: [kvm-unit-tests PATCH v2 3/3] riscv: sbi: Add test for timer extension

[James R T]

On Sun, Jul 7, 2024 at 6:11 PM James Raphael Tiovalen
<jamestiotio@gmail.com> wrote:
>
> Add a test for the set_timer function of the time extension. The test
> checks that:
> - The time extension is available
> - The time counter monotonically increases
> - The installed timer interrupt handler is called
> - The timer interrupt is received within a reasonable time interval
> - The timer interrupt pending bit is cleared after the set_timer SBI
>   call is made
> - The timer interrupt can be cleared either by requesting a timer
>   interrupt infinitely far into the future or by masking the timer
>   interrupt
>
> The timer interrupt delay can be set using the TIMER_DELAY environment
> variable in microseconds. The default delay value is 1 second. Since the
> interrupt can arrive a little later than the specified delay, allow some
> margin of error. This margin of error can be specified via the
> TIMER_MARGIN environment variable in microseconds. The default margin of
> error is 1 second.
>
> This test has been verified on RV32 and RV64 with OpenSBI using QEMU.
>
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> ---
>  lib/riscv/asm/csr.h   |   7 +++
>  lib/riscv/asm/sbi.h   |   5 ++
>  lib/riscv/asm/setup.h |   1 +
>  lib/riscv/asm/timer.h |  30 ++++++++++++
>  lib/riscv/processor.c |   6 +++
>  lib/riscv/setup.c     |  24 ++++++++++
>  riscv/sbi.c           | 108 ++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 181 insertions(+)
>  create mode 100644 lib/riscv/asm/timer.h
>
> diff --git a/lib/riscv/asm/csr.h b/lib/riscv/asm/csr.h
> index b3c48e8e..dc05bfc9 100644
> --- a/lib/riscv/asm/csr.h
> +++ b/lib/riscv/asm/csr.h
> @@ -4,12 +4,15 @@
>  #include <linux/const.h>
>
>  #define CSR_SSTATUS            0x100
> +#define CSR_SIE                        0x104
>  #define CSR_STVEC              0x105
>  #define CSR_SSCRATCH           0x140
>  #define CSR_SEPC               0x141
>  #define CSR_SCAUSE             0x142
>  #define CSR_STVAL              0x143
> +#define CSR_SIP                        0x144
>  #define CSR_SATP               0x180
> +#define CSR_TIME               0xc01
>
>  #define SR_SIE                 _AC(0x00000002, UL)
>
> @@ -50,6 +53,10 @@
>  #define IRQ_S_GEXT             12
>  #define IRQ_PMU_OVF            13
>
> +#define IE_TIE                 (_AC(0x1, UL) << IRQ_S_TIMER)
> +
> +#define IP_TIP                 IE_TIE
> +
>  #ifndef __ASSEMBLY__
>
>  #define csr_swap(csr, val)                                     \
> diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h
> index d82a384d..84ce1bff 100644
> --- a/lib/riscv/asm/sbi.h
> +++ b/lib/riscv/asm/sbi.h
> @@ -16,6 +16,7 @@
>
>  enum sbi_ext_id {
>         SBI_EXT_BASE = 0x10,
> +       SBI_EXT_TIME = 0x54494d45,
>         SBI_EXT_HSM = 0x48534d,
>         SBI_EXT_SRST = 0x53525354,
>  };
> @@ -37,6 +38,10 @@ enum sbi_ext_hsm_fid {
>         SBI_EXT_HSM_HART_SUSPEND,
>  };
>
> +enum sbi_ext_time_fid {
> +       SBI_EXT_TIME_SET_TIMER = 0,
> +};
> +
>  struct sbiret {
>         long error;
>         long value;
> diff --git a/lib/riscv/asm/setup.h b/lib/riscv/asm/setup.h
> index 7f81a705..5be252df 100644
> --- a/lib/riscv/asm/setup.h
> +++ b/lib/riscv/asm/setup.h
> @@ -7,6 +7,7 @@
>  #define NR_CPUS 16
>  extern struct thread_info cpus[NR_CPUS];
>  extern int nr_cpus;
> +extern uint64_t tb_hz;
>  int hartid_to_cpu(unsigned long hartid);
>
>  void io_init(void);
> diff --git a/lib/riscv/asm/timer.h b/lib/riscv/asm/timer.h
> new file mode 100644
> index 00000000..3eeb8344
> --- /dev/null
> +++ b/lib/riscv/asm/timer.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef _ASMRISCV_TIMER_H_
> +#define _ASMRISCV_TIMER_H_
> +
> +#include <asm/csr.h>
> +#include <asm/processor.h>
> +
> +extern uint64_t usec_to_cycles(uint64_t usec);
> +
> +static inline void timer_irq_enable(void)
> +{
> +       csr_set(CSR_SIE, IE_TIE);
> +}
> +
> +static inline void timer_irq_disable(void)
> +{
> +       csr_clear(CSR_SIE, IE_TIE);
> +}
> +
> +static inline uint64_t timer_get_cycles(void)
> +{
> +       return csr_read(CSR_TIME);
> +}
> +
> +static inline bool timer_irq_pending(void)
> +{
> +       return csr_read(CSR_SIP) & IP_TIP;
> +}
> +
> +#endif /* _ASMRISCV_TIMER_H_ */
> diff --git a/lib/riscv/processor.c b/lib/riscv/processor.c
> index 0dffadc7..082b9d80 100644
> --- a/lib/riscv/processor.c
> +++ b/lib/riscv/processor.c
> @@ -7,6 +7,7 @@
>  #include <asm/isa.h>
>  #include <asm/processor.h>
>  #include <asm/setup.h>
> +#include <asm/timer.h>
>
>  extern unsigned long ImageBase;
>
> @@ -82,3 +83,8 @@ void thread_info_init(void)
>         isa_init(&cpus[cpu]);
>         csr_write(CSR_SSCRATCH, &cpus[cpu]);
>  }
> +
> +uint64_t usec_to_cycles(uint64_t usec)
> +{
> +       return (tb_hz * usec) / 1000000;
> +}
> diff --git a/lib/riscv/setup.c b/lib/riscv/setup.c
> index 50ffb0d0..b659c14e 100644
> --- a/lib/riscv/setup.c
> +++ b/lib/riscv/setup.c
> @@ -20,6 +20,7 @@
>  #include <asm/page.h>
>  #include <asm/processor.h>
>  #include <asm/setup.h>
> +#include <asm/timer.h>
>
>  #define VA_BASE                        ((phys_addr_t)3 * SZ_1G)
>  #if __riscv_xlen == 64
> @@ -38,6 +39,7 @@ u32 initrd_size;
>
>  struct thread_info cpus[NR_CPUS];
>  int nr_cpus;
> +uint64_t tb_hz;
>
>  static struct mem_region riscv_mem_regions[NR_MEM_REGIONS + 1];
>
> @@ -67,6 +69,26 @@ static void cpu_init_acpi(void)
>         assert_msg(false, "ACPI not available");
>  }
>
> +static int cpu_init_timer(const void *fdt)
> +{
> +       const struct fdt_property *prop;
> +       u32 *data;
> +       int cpus;
> +
> +       cpus = fdt_path_offset(fdt, "/cpus");
> +       if (cpus < 0)
> +               return cpus;
> +
> +       prop = fdt_get_property(fdt, cpus, "timebase-frequency", NULL);
> +       if (prop == NULL)
> +               return -1;
> +
> +       data = (u32 *)prop->data;
> +       tb_hz = fdt32_to_cpu(*data);
> +
> +       return 0;
> +}
> +
>  static void cpu_init(void)
>  {
>         int ret;
> @@ -75,6 +97,8 @@ static void cpu_init(void)
>         if (dt_available()) {
>                 ret = dt_for_each_cpu_node(cpu_set_fdt, NULL);
>                 assert(ret == 0);
> +               ret = cpu_init_timer(dt_fdt());
> +               assert(ret == 0);
>         } else {
>                 cpu_init_acpi();
>         }
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index 762e9711..a1a9ce84 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -6,7 +6,14 @@
>   */
>  #include <libcflat.h>
>  #include <stdlib.h>
> +#include <asm/barrier.h>
> +#include <asm/csr.h>
> +#include <asm/processor.h>
>  #include <asm/sbi.h>
> +#include <asm/timer.h>
> +
> +static bool timer_works;
> +static bool mask_timer_irq;
>
>  static void help(void)
>  {
> @@ -19,6 +26,27 @@ static struct sbiret __base_sbi_ecall(int fid, unsigned long arg0)
>         return sbi_ecall(SBI_EXT_BASE, fid, arg0, 0, 0, 0, 0, 0);
>  }
>
> +static struct sbiret __time_sbi_ecall(unsigned long stime_value)
> +{
> +       return sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value, 0, 0, 0, 0, 0);
> +}
> +
> +static void timer_irq_handler(struct pt_regs *regs)
> +{
> +       if (timer_works)
> +               report_abort("timer interrupt received multiple times");
> +
> +       timer_works = true;
> +       report(timer_irq_pending(), "pending timer interrupt bit set");
> +
> +       if (mask_timer_irq)
> +               timer_irq_disable();
> +       else {
> +               __time_sbi_ecall(-1);
> +               report(!timer_irq_pending(), "pending timer interrupt bit cleared");
> +       }
> +}
> +
>  static bool env_or_skip(const char *env)
>  {
>         if (!getenv(env)) {
> @@ -112,6 +140,85 @@ static void check_base(void)
>         report_prefix_pop();
>  }
>
> +static void check_time(void)
> +{
> +       struct sbiret ret;
> +       unsigned long begin, end, duration;
> +       unsigned long delay = getenv("TIMER_DELAY")
> +                                                       ? strtol(getenv("TIMER_DELAY"), NULL, 0)
> +                                                       : 1000000;
> +       unsigned long margin = getenv("TIMER_MARGIN")
> +                                                       ? strtol(getenv("TIMER_MARGIN"), NULL, 0)
> +                                                       : 1000000;

Whoops, it looks like my editor settings showed tabs as 4 characters
instead of 8. I will fix this spacing and manually validate it in the
next patch version.

> +
> +       delay = usec_to_cycles(delay);
> +       margin = usec_to_cycles(margin);
> +
> +       report_prefix_push("time");
> +
> +       ret = __base_sbi_ecall(SBI_EXT_BASE_PROBE_EXT, SBI_EXT_TIME);
> +
> +       if (ret.error) {
> +               report_fail("probing for time extension failed");
> +               report_prefix_pop();
> +               return;
> +       }
> +
> +       if (!ret.value) {
> +               report_skip("time extension not available");
> +               report_prefix_pop();
> +               return;
> +       }
> +
> +       begin = timer_get_cycles();
> +       while ((end = timer_get_cycles()) <= begin)
> +               cpu_relax();
> +       assert(begin < end);
> +
> +       report_prefix_push("set_timer");
> +
> +       install_irq_handler(IRQ_S_TIMER, timer_irq_handler);
> +       local_irq_enable();
> +       timer_irq_enable();
> +
> +       begin = timer_get_cycles();
> +       ret = __time_sbi_ecall(begin + delay);
> +
> +       if (ret.error)
> +               report_fail("setting timer failed");
> +
> +       report(!timer_irq_pending(), "pending timer interrupt bit cleared");
> +
> +       while ((end = timer_get_cycles()) <= (begin + delay + margin) && !timer_works)
> +               cpu_relax();
> +
> +       report(timer_works, "timer interrupt received");
> +
> +       if (timer_works) {
> +               duration = end - begin;
> +               report(duration >= delay && duration <= (delay + margin), "timer delay honored");
> +       }
> +
> +       timer_works = false;
> +       mask_timer_irq = true;
> +       begin = timer_get_cycles();
> +       ret = __time_sbi_ecall(begin + delay);
> +
> +       if (ret.error)
> +               report_fail("setting timer failed");
> +
> +       while ((end = timer_get_cycles()) <= (begin + delay + margin) && !timer_works)
> +               cpu_relax();
> +
> +       report(timer_works, "timer interrupt received");
> +
> +       local_irq_disable();
> +       install_irq_handler(IRQ_S_TIMER, NULL);
> +
> +       report_prefix_pop();
> +       report_prefix_pop();
> +}
> +
>  int main(int argc, char **argv)
>  {
>
> @@ -122,6 +229,7 @@ int main(int argc, char **argv)
>
>         report_prefix_push("sbi");
>         check_base();
> +       check_time();
>
>         return report_summary();
>  }
> --
> 2.43.0
>

Best regards,
James Raphael Tiovalen

Re: [kvm-unit-tests PATCH v2 2/3] riscv: Update exception cause list

[Andrew Jones]

On Sun, Jul 07, 2024 at 06:10:51PM GMT, James Raphael Tiovalen wrote:
> Update the list of exception and interrupt causes to follow the latest
> RISC-V privileged ISA specification (version 20240411 section 18.6.1).
> 
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> ---
>  lib/riscv/asm/csr.h       | 14 ++++++++++++++
>  lib/riscv/asm/processor.h |  2 +-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/riscv/asm/csr.h b/lib/riscv/asm/csr.h
> index d6909d93..b3c48e8e 100644
> --- a/lib/riscv/asm/csr.h
> +++ b/lib/riscv/asm/csr.h
> @@ -28,6 +28,7 @@
>  #define EXC_SYSCALL		8
>  #define EXC_HYPERVISOR_SYSCALL	9
>  #define EXC_SUPERVISOR_SYSCALL	10
> +#define EXC_MACHINE_SYSCALL	11
>  #define EXC_INST_PAGE_FAULT	12
>  #define EXC_LOAD_PAGE_FAULT	13
>  #define EXC_STORE_PAGE_FAULT	15
> @@ -36,6 +37,19 @@
>  #define EXC_VIRTUAL_INST_FAULT		22
>  #define EXC_STORE_GUEST_PAGE_FAULT	23
>  
> +/* Interrupt causes */
> +#define IRQ_S_SOFT		1
> +#define IRQ_VS_SOFT		2
> +#define IRQ_M_SOFT		3
> +#define IRQ_S_TIMER		5
> +#define IRQ_VS_TIMER		6
> +#define IRQ_M_TIMER		7
> +#define IRQ_S_EXT		9
> +#define IRQ_VS_EXT		10
> +#define IRQ_M_EXT		11
> +#define IRQ_S_GEXT		12
> +#define IRQ_PMU_OVF		13
> +

While it doesn't hurt to define them, we don't need the M-mode stuff,
since we don't intend to run in M-mode.

>  #ifndef __ASSEMBLY__
>  
>  #define csr_swap(csr, val)					\
> diff --git a/lib/riscv/asm/processor.h b/lib/riscv/asm/processor.h
> index 6451adb5..4c9ad968 100644
> --- a/lib/riscv/asm/processor.h
> +++ b/lib/riscv/asm/processor.h
> @@ -4,7 +4,7 @@
>  #include <asm/csr.h>
>  #include <asm/ptrace.h>
>  
> -#define EXCEPTION_CAUSE_MAX	16
> +#define EXCEPTION_CAUSE_MAX	24
>  #define INTERRUPT_CAUSE_MAX	16
>  
>  typedef void (*exception_fn)(struct pt_regs *);
> -- 
> 2.43.0
>

Otherwise,

Reviewed-by: Andrew Jones <andrew.jones@linux.dev>

Re: [kvm-unit-tests PATCH v2 3/3] riscv: sbi: Add test for timer extension

[Andrew Jones]

On Sun, Jul 07, 2024 at 06:10:52PM GMT, James Raphael Tiovalen wrote:
> Add a test for the set_timer function of the time extension. The test
> checks that:
> - The time extension is available
> - The time counter monotonically increases
> - The installed timer interrupt handler is called
> - The timer interrupt is received within a reasonable time interval
> - The timer interrupt pending bit is cleared after the set_timer SBI
>   call is made
> - The timer interrupt can be cleared either by requesting a timer
>   interrupt infinitely far into the future or by masking the timer
>   interrupt
> 
> The timer interrupt delay can be set using the TIMER_DELAY environment
> variable in microseconds. The default delay value is 1 second. Since the
> interrupt can arrive a little later than the specified delay, allow some
> margin of error. This margin of error can be specified via the
> TIMER_MARGIN environment variable in microseconds. The default margin of
> error is 1 second.

1 second is too much since we want these tests to run really fast when
possible. Let's use 200 ms.

> 
> This test has been verified on RV32 and RV64 with OpenSBI using QEMU.
> 
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> ---
>  lib/riscv/asm/csr.h   |   7 +++
>  lib/riscv/asm/sbi.h   |   5 ++
>  lib/riscv/asm/setup.h |   1 +
>  lib/riscv/asm/timer.h |  30 ++++++++++++
>  lib/riscv/processor.c |   6 +++
>  lib/riscv/setup.c     |  24 ++++++++++
>  riscv/sbi.c           | 108 ++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 181 insertions(+)
>  create mode 100644 lib/riscv/asm/timer.h
> 
> diff --git a/lib/riscv/asm/csr.h b/lib/riscv/asm/csr.h
> index b3c48e8e..dc05bfc9 100644
> --- a/lib/riscv/asm/csr.h
> +++ b/lib/riscv/asm/csr.h
> @@ -4,12 +4,15 @@
>  #include <linux/const.h>
>  
>  #define CSR_SSTATUS		0x100
> +#define CSR_SIE			0x104
>  #define CSR_STVEC		0x105
>  #define CSR_SSCRATCH		0x140
>  #define CSR_SEPC		0x141
>  #define CSR_SCAUSE		0x142
>  #define CSR_STVAL		0x143
> +#define CSR_SIP			0x144
>  #define CSR_SATP		0x180
> +#define CSR_TIME		0xc01
>  
>  #define SR_SIE			_AC(0x00000002, UL)
>  
> @@ -50,6 +53,10 @@
>  #define IRQ_S_GEXT		12
>  #define IRQ_PMU_OVF		13
>  
> +#define IE_TIE			(_AC(0x1, UL) << IRQ_S_TIMER)
> +
> +#define IP_TIP			IE_TIE
> +
>  #ifndef __ASSEMBLY__
>  
>  #define csr_swap(csr, val)					\
> diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h
> index d82a384d..84ce1bff 100644
> --- a/lib/riscv/asm/sbi.h
> +++ b/lib/riscv/asm/sbi.h
> @@ -16,6 +16,7 @@
>  
>  enum sbi_ext_id {
>  	SBI_EXT_BASE = 0x10,
> +	SBI_EXT_TIME = 0x54494d45,
>  	SBI_EXT_HSM = 0x48534d,
>  	SBI_EXT_SRST = 0x53525354,
>  };
> @@ -37,6 +38,10 @@ enum sbi_ext_hsm_fid {
>  	SBI_EXT_HSM_HART_SUSPEND,
>  };
>  
> +enum sbi_ext_time_fid {
> +	SBI_EXT_TIME_SET_TIMER = 0,
> +};
> +
>  struct sbiret {
>  	long error;
>  	long value;
> diff --git a/lib/riscv/asm/setup.h b/lib/riscv/asm/setup.h
> index 7f81a705..5be252df 100644
> --- a/lib/riscv/asm/setup.h
> +++ b/lib/riscv/asm/setup.h
> @@ -7,6 +7,7 @@
>  #define NR_CPUS 16
>  extern struct thread_info cpus[NR_CPUS];
>  extern int nr_cpus;
> +extern uint64_t tb_hz;

Let's spell out timebase and maybe even just call it exactly
the same name as the DT node, i.e. 'timebase_frequency'

>  int hartid_to_cpu(unsigned long hartid);
>  
>  void io_init(void);
> diff --git a/lib/riscv/asm/timer.h b/lib/riscv/asm/timer.h
> new file mode 100644
> index 00000000..3eeb8344
> --- /dev/null
> +++ b/lib/riscv/asm/timer.h
> @@ -0,0 +1,30 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef _ASMRISCV_TIMER_H_
> +#define _ASMRISCV_TIMER_H_
> +
> +#include <asm/csr.h>
> +#include <asm/processor.h>
> +
> +extern uint64_t usec_to_cycles(uint64_t usec);
> +
> +static inline void timer_irq_enable(void)
> +{
> +	csr_set(CSR_SIE, IE_TIE);
> +}
> +
> +static inline void timer_irq_disable(void)
> +{
> +	csr_clear(CSR_SIE, IE_TIE);
> +}
> +
> +static inline uint64_t timer_get_cycles(void)
> +{
> +	return csr_read(CSR_TIME);
> +}
> +
> +static inline bool timer_irq_pending(void)
> +{
> +	return csr_read(CSR_SIP) & IP_TIP;

I don't think we need to add this function to asm/timer.h.
I don't expect we'll be checking SIP for TIP much other
than in its test, so this function or the open coded
check can be put directly in the test code.

> +}
> +
> +#endif /* _ASMRISCV_TIMER_H_ */
> diff --git a/lib/riscv/processor.c b/lib/riscv/processor.c
> index 0dffadc7..082b9d80 100644
> --- a/lib/riscv/processor.c
> +++ b/lib/riscv/processor.c
> @@ -7,6 +7,7 @@
>  #include <asm/isa.h>
>  #include <asm/processor.h>
>  #include <asm/setup.h>
> +#include <asm/timer.h>
>  
>  extern unsigned long ImageBase;
>  
> @@ -82,3 +83,8 @@ void thread_info_init(void)
>  	isa_init(&cpus[cpu]);
>  	csr_write(CSR_SSCRATCH, &cpus[cpu]);
>  }
> +
> +uint64_t usec_to_cycles(uint64_t usec)
> +{
> +	return (tb_hz * usec) / 1000000;

This function should be a static inline. It also seems like
it belongs in something like a 'delay' component, rather than
the 'processor' component. We should probably add the frequency
getting and usec_to_cycles() support adding in a separate patch,
possibly also introducing delay.{c,h} at the same time (see
Arm's delay.{c,h})

> +}
> diff --git a/lib/riscv/setup.c b/lib/riscv/setup.c
> index 50ffb0d0..b659c14e 100644
> --- a/lib/riscv/setup.c
> +++ b/lib/riscv/setup.c
> @@ -20,6 +20,7 @@
>  #include <asm/page.h>
>  #include <asm/processor.h>
>  #include <asm/setup.h>
> +#include <asm/timer.h>
>  
>  #define VA_BASE			((phys_addr_t)3 * SZ_1G)
>  #if __riscv_xlen == 64
> @@ -38,6 +39,7 @@ u32 initrd_size;
>  
>  struct thread_info cpus[NR_CPUS];
>  int nr_cpus;
> +uint64_t tb_hz;
>  
>  static struct mem_region riscv_mem_regions[NR_MEM_REGIONS + 1];
>  
> @@ -67,6 +69,26 @@ static void cpu_init_acpi(void)
>  	assert_msg(false, "ACPI not available");
>  }
>  
> +static int cpu_init_timer(const void *fdt)

We're not initializing per-cpu timers so the name of this function isn't
correct. It should be something like 'timer_get_frequency' and maybe it
should live in lib/riscv/timer.c rather than setup.c

> +{
> +	const struct fdt_property *prop;
> +	u32 *data;
> +	int cpus;
> +
> +	cpus = fdt_path_offset(fdt, "/cpus");
> +	if (cpus < 0)
> +		return cpus;

We intend to assert on all non-zero return values so we could just
make this function void and assert directly here. That would actually
make it easier to debug since we'd see which assert fired, i.e. what
exactly failed.

> +
> +	prop = fdt_get_property(fdt, cpus, "timebase-frequency", NULL);
> +	if (prop == NULL)
> +		return -1;
> +
> +	data = (u32 *)prop->data;
> +	tb_hz = fdt32_to_cpu(*data);
> +
> +	return 0;
> +}
> +
>  static void cpu_init(void)
>  {
>  	int ret;
> @@ -75,6 +97,8 @@ static void cpu_init(void)
>  	if (dt_available()) {
>  		ret = dt_for_each_cpu_node(cpu_set_fdt, NULL);
>  		assert(ret == 0);
> +		ret = cpu_init_timer(dt_fdt());
> +		assert(ret == 0);

I think we should call timer_get_frequency() directly from setup()
and setup_efi() (please also test with EFI) and have a

 assert_msg(!dt_available(), "ACPI not yet supported");

at the top of timer_get_frequency().

>  	} else {
>  		cpu_init_acpi();
>  	}
> diff --git a/riscv/sbi.c b/riscv/sbi.c
> index 762e9711..a1a9ce84 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -6,7 +6,14 @@
>   */
>  #include <libcflat.h>
>  #include <stdlib.h>
> +#include <asm/barrier.h>
> +#include <asm/csr.h>
> +#include <asm/processor.h>
>  #include <asm/sbi.h>
> +#include <asm/timer.h>
> +
> +static bool timer_works;
> +static bool mask_timer_irq;
>  
>  static void help(void)
>  {
> @@ -19,6 +26,27 @@ static struct sbiret __base_sbi_ecall(int fid, unsigned long arg0)
>  	return sbi_ecall(SBI_EXT_BASE, fid, arg0, 0, 0, 0, 0, 0);
>  }
>  
> +static struct sbiret __time_sbi_ecall(unsigned long stime_value)
> +{
> +	return sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value, 0, 0, 0, 0, 0);
> +}
> +
> +static void timer_irq_handler(struct pt_regs *regs)
> +{
> +	if (timer_works)
> +		report_abort("timer interrupt received multiple times");

We should generally avoid doing too much in an interrupt handler as
it can add delays to our expected interrupt arrivals, etc. This is
why we have variables like 'timer_works'. We only set the variable
in the handler and then, back in normal context, we look at the
variable to decide what we should do.

> +
> +	timer_works = true;
> +	report(timer_irq_pending(), "pending timer interrupt bit set");
> +
> +	if (mask_timer_irq)
> +		timer_irq_disable();
> +	else {
> +		__time_sbi_ecall(-1);
> +		report(!timer_irq_pending(), "pending timer interrupt bit cleared");
> +	}
> +}
> +
>  static bool env_or_skip(const char *env)
>  {
>  	if (!getenv(env)) {
> @@ -112,6 +140,85 @@ static void check_base(void)
>  	report_prefix_pop();
>  }
>  
> +static void check_time(void)
> +{
> +	struct sbiret ret;
> +	unsigned long begin, end, duration;
> +	unsigned long delay = getenv("TIMER_DELAY")
> +							? strtol(getenv("TIMER_DELAY"), NULL, 0)
> +							: 1000000;
> +	unsigned long margin = getenv("TIMER_MARGIN")
> +							? strtol(getenv("TIMER_MARGIN"), NULL, 0)
> +							: 1000000;

Put the '?' part of the ternary on the same line as the '='.

foo = bar ? x
          : y;

> +
> +	delay = usec_to_cycles(delay);
> +	margin = usec_to_cycles(margin);
> +
> +	report_prefix_push("time");
> +
> +	ret = __base_sbi_ecall(SBI_EXT_BASE_PROBE_EXT, SBI_EXT_TIME);
> +
> +	if (ret.error) {
> +		report_fail("probing for time extension failed");
> +		report_prefix_pop();
> +		return;
> +	}

See my suggestion in Cade's patch about adding sbi_probe() which asserts
on ret.error.

> +
> +	if (!ret.value) {
> +		report_skip("time extension not available");
> +		report_prefix_pop();
> +		return;
> +	}
> +
> +	begin = timer_get_cycles();
> +	while ((end = timer_get_cycles()) <= begin)
> +		cpu_relax();
> +	assert(begin < end);

This will always be true since we loop until it's true.
We should just do

 begin = timer_get_cycles();
 delay(d);
 end = timer_get_cycles();
 assert(begin + usec_to_cycles(d) <= end);

(which means we need the 'delay' component)

> +
> +	report_prefix_push("set_timer");
> +
> +	install_irq_handler(IRQ_S_TIMER, timer_irq_handler);
> +	local_irq_enable();
> +	timer_irq_enable();

Before enabling the timer irq we should make sure stimecmp is -1
when we have the Sstc extension.

> +
> +	begin = timer_get_cycles();
> +	ret = __time_sbi_ecall(begin + delay);
> +
> +	if (ret.error)
> +		report_fail("setting timer failed");

This should be something like

  report(!ret.error, "set timer");
  if (ret.error)
     report_info("set timer failed with %ld\n", ret.error);

Hmm, the spec doesn't actually say what to expect for ret.error and
ret.value. We can still test for !ret.error though, I guess... We
should probably also clarify the spec, please open a PR.

> +
> +	report(!timer_irq_pending(), "pending timer interrupt bit cleared");
> +
> +	while ((end = timer_get_cycles()) <= (begin + delay + margin) && !timer_works)
> +		cpu_relax();
> +
> +	report(timer_works, "timer interrupt received");
> +
> +	if (timer_works) {
> +		duration = end - begin;
> +		report(duration >= delay && duration <= (delay + margin), "timer delay honored");
> +	}
> +
> +	timer_works = false;
> +	mask_timer_irq = true;
> +	begin = timer_get_cycles();
> +	ret = __time_sbi_ecall(begin + delay);
> +
> +	if (ret.error)
> +		report_fail("setting timer failed");

same comment as above

> +
> +	while ((end = timer_get_cycles()) <= (begin + delay + margin) && !timer_works)
> +		cpu_relax();
> +
> +	report(timer_works, "timer interrupt received");

This should have a different text than above. Something like

 "timer interrupt received for mask irq test"

We should probably repeat the check that the interrupt arrived within the
appropriate margin too (and also ensure it's text is different from the
test above)

> +
> +	local_irq_disable();
> +	install_irq_handler(IRQ_S_TIMER, NULL);
> +
> +	report_prefix_pop();
> +	report_prefix_pop();
> +}
> +
>  int main(int argc, char **argv)
>  {
>  
> @@ -122,6 +229,7 @@ int main(int argc, char **argv)
>  
>  	report_prefix_push("sbi");
>  	check_base();
> +	check_time();
>  
>  	return report_summary();
>  }
> -- 
> 2.43.0
> 

Thanks,
drew