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

[kvm-unit-tests PATCH 0/4] 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 3 patches add
infrastructural support for handling interrupts, while the last patch
adds the actual test for the timer extension.

James Raphael Tiovalen (4):
  riscv: Extend exception handling support for interrupts
  riscv: Update exception cause list
  riscv: Add methods to toggle interrupt enable bits
  riscv: sbi: Add test for timer extension

 riscv/Makefile            |  1 +
 lib/riscv/asm/csr.h       | 30 +++++++++++---
 lib/riscv/asm/interrupt.h | 12 ++++++
 lib/riscv/asm/processor.h | 15 ++++++-
 lib/riscv/asm/sbi.h       |  5 +++
 lib/riscv/interrupt.c     | 39 ++++++++++++++++++
 lib/riscv/processor.c     | 27 ++++++++++--
 riscv/sbi.c               | 87 +++++++++++++++++++++++++++++++++++++++
 8 files changed, 205 insertions(+), 11 deletions(-)
 create mode 100644 lib/riscv/asm/interrupt.h
 create mode 100644 lib/riscv/interrupt.c

--
2.43.0

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

[James Raphael Tiovalen]

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..d5879d2a 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 SSTATUS_SIE		(_AC(1, UL) << 1)
+
 /* 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..767b1caa 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, SSTATUS_SIE);
+}
+
+static inline void local_irq_disable(void)
+{
+	csr_clear(CSR_SSTATUS, SSTATUS_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 2/4] 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).

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

diff --git a/lib/riscv/asm/csr.h b/lib/riscv/asm/csr.h
index d5879d2a..c1777744 100644
--- a/lib/riscv/asm/csr.h
+++ b/lib/riscv/asm/csr.h
@@ -26,15 +26,18 @@
 #define EXC_STORE_MISALIGNED	6
 #define EXC_STORE_ACCESS	7
 #define EXC_SYSCALL		8
-#define EXC_HYPERVISOR_SYSCALL	9
-#define EXC_SUPERVISOR_SYSCALL	10
+#define EXC_SUPERVISOR_SYSCALL	9
 #define EXC_INST_PAGE_FAULT	12
 #define EXC_LOAD_PAGE_FAULT	13
 #define EXC_STORE_PAGE_FAULT	15
-#define EXC_INST_GUEST_PAGE_FAULT	20
-#define EXC_LOAD_GUEST_PAGE_FAULT	21
-#define EXC_VIRTUAL_INST_FAULT		22
-#define EXC_STORE_GUEST_PAGE_FAULT	23
+#define EXC_SOFTWARE_CHECK	18
+#define EXC_HARDWARE_ERROR	19
+
+/* Interrupt causes */
+#define IRQ_SUPERVISOR_SOFTWARE	1
+#define IRQ_SUPERVISOR_TIMER	5
+#define IRQ_SUPERVISOR_EXTERNAL	9
+#define IRQ_COUNTER_OVERFLOW	13
 
 #ifndef __ASSEMBLY__
 
diff --git a/lib/riscv/asm/processor.h b/lib/riscv/asm/processor.h
index 767b1caa..5942ed2e 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	64
 #define INTERRUPT_CAUSE_MAX	16
 
 typedef void (*exception_fn)(struct pt_regs *);
-- 
2.43.0

[kvm-unit-tests PATCH 3/4] riscv: Add methods to toggle interrupt enable bits

[James Raphael Tiovalen]

Add some helper methods to toggle the interrupt enable bits in the SIE
register.

Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
---
 riscv/Makefile            |  1 +
 lib/riscv/asm/csr.h       |  7 +++++++
 lib/riscv/asm/interrupt.h | 12 ++++++++++++
 lib/riscv/interrupt.c     | 39 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 59 insertions(+)
 create mode 100644 lib/riscv/asm/interrupt.h
 create mode 100644 lib/riscv/interrupt.c

diff --git a/riscv/Makefile b/riscv/Makefile
index 919a3ebb..108d4481 100644
--- a/riscv/Makefile
+++ b/riscv/Makefile
@@ -30,6 +30,7 @@ cflatobjs += lib/memregions.o
 cflatobjs += lib/on-cpus.o
 cflatobjs += lib/vmalloc.o
 cflatobjs += lib/riscv/bitops.o
+cflatobjs += lib/riscv/interrupt.o
 cflatobjs += lib/riscv/io.o
 cflatobjs += lib/riscv/isa.o
 cflatobjs += lib/riscv/mmu.o
diff --git a/lib/riscv/asm/csr.h b/lib/riscv/asm/csr.h
index c1777744..da58b0ce 100644
--- a/lib/riscv/asm/csr.h
+++ b/lib/riscv/asm/csr.h
@@ -4,15 +4,22 @@
 #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 SSTATUS_SIE		(_AC(1, UL) << 1)
 
+#define SIE_SSIE		(_AC(1, UL) << 1)
+#define SIE_STIE		(_AC(1, UL) << 5)
+#define SIE_SEIE		(_AC(1, UL) << 9)
+#define SIE_LCOFIE		(_AC(1, UL) << 13)
+
 /* 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/interrupt.h b/lib/riscv/asm/interrupt.h
new file mode 100644
index 00000000..b760afbb
--- /dev/null
+++ b/lib/riscv/asm/interrupt.h
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef _ASMRISCV_INTERRUPT_H_
+#define _ASMRISCV_INTERRUPT_H_
+
+#include <stdbool.h>
+
+void toggle_software_interrupt(bool enable);
+void toggle_timer_interrupt(bool enable);
+void toggle_external_interrupt(bool enable);
+void toggle_local_cof_interrupt(bool enable);
+
+#endif /* _ASMRISCV_INTERRUPT_H_ */
diff --git a/lib/riscv/interrupt.c b/lib/riscv/interrupt.c
new file mode 100644
index 00000000..bc0e16f1
--- /dev/null
+++ b/lib/riscv/interrupt.c
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2024, James Raphael Tiovalen <jamestiotio@gmail.com>
+ */
+#include <libcflat.h>
+#include <asm/csr.h>
+#include <asm/interrupt.h>
+
+void toggle_software_interrupt(bool enable)
+{
+	if (enable)
+		csr_set(CSR_SIE, SIE_SSIE);
+	else
+		csr_clear(CSR_SIE, SIE_SSIE);
+}
+
+void toggle_timer_interrupt(bool enable)
+{
+	if (enable)
+		csr_set(CSR_SIE, SIE_STIE);
+	else
+		csr_clear(CSR_SIE, SIE_STIE);
+}
+
+void toggle_external_interrupt(bool enable)
+{
+	if (enable)
+		csr_set(CSR_SIE, SIE_SEIE);
+	else
+		csr_clear(CSR_SIE, SIE_SEIE);
+}
+
+void toggle_local_cof_interrupt(bool enable)
+{
+	if (enable)
+		csr_set(CSR_SIE, SIE_LCOFIE);
+	else
+		csr_clear(CSR_SIE, SIE_LCOFIE);
+}
-- 
2.43.0

[kvm-unit-tests PATCH 4/4] 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 frame

The timer interrupt delay can be set using the TIMER_DELAY environment
variable. If the variable is not set, the default delay value is
1000000. The time interval used to validate the timer interrupt is
between the specified delay and double the delay. Because of this, the
test might fail if the delay is too short. Hence, we set the default
delay value as the minimum value.

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 |  6 ++++
 lib/riscv/asm/sbi.h |  5 +++
 riscv/sbi.c         | 87 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 98 insertions(+)

diff --git a/lib/riscv/asm/csr.h b/lib/riscv/asm/csr.h
index da58b0ce..c4435650 100644
--- a/lib/riscv/asm/csr.h
+++ b/lib/riscv/asm/csr.h
@@ -12,6 +12,7 @@
 #define CSR_STVAL		0x143
 #define CSR_SIP			0x144
 #define CSR_SATP		0x180
+#define CSR_TIME		0xc01
 
 #define SSTATUS_SIE		(_AC(1, UL) << 1)
 
@@ -108,5 +109,10 @@
 				: "memory");			\
 })
 
+#define wfi()							\
+({								\
+	__asm__ __volatile__("wfi" ::: "memory");		\
+})
+
 #endif /* !__ASSEMBLY__ */
 #endif /* _ASMRISCV_CSR_H_ */
diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h
index d82a384d..eb4c77ef 100644
--- a/lib/riscv/asm/sbi.h
+++ b/lib/riscv/asm/sbi.h
@@ -18,6 +18,7 @@ enum sbi_ext_id {
 	SBI_EXT_BASE = 0x10,
 	SBI_EXT_HSM = 0x48534d,
 	SBI_EXT_SRST = 0x53525354,
+	SBI_EXT_TIME = 0x54494D45,
 };
 
 enum sbi_ext_base_fid {
@@ -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/riscv/sbi.c b/riscv/sbi.c
index 762e9711..6ad1dff6 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -6,8 +6,13 @@
  */
 #include <libcflat.h>
 #include <stdlib.h>
+#include <asm/csr.h>
+#include <asm/interrupt.h>
+#include <asm/processor.h>
 #include <asm/sbi.h>
 
+static bool timer_work;
+
 static void help(void)
 {
 	puts("Test SBI\n");
@@ -19,6 +24,18 @@ 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(int fid, unsigned long arg0)
+{
+	return sbi_ecall(SBI_EXT_TIME, fid, arg0, 0, 0, 0, 0, 0);
+}
+
+static void timer_interrupt_handler(struct pt_regs *regs)
+{
+	timer_work = true;
+	toggle_timer_interrupt(false);
+	local_irq_disable();
+}
+
 static bool env_or_skip(const char *env)
 {
 	if (!getenv(env)) {
@@ -112,6 +129,75 @@ static void check_base(void)
 	report_prefix_pop();
 }
 
+static void check_time(void)
+{
+	struct sbiret ret;
+	unsigned long begin, end, duration;
+	const unsigned long default_delay = 1000000;
+	unsigned long delay = getenv("TIMER_DELAY")
+				? MAX(strtol(getenv("TIMER_DELAY"), NULL, 0), default_delay)
+				: default_delay;
+
+	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 = csr_read(CSR_TIME);
+	end = csr_read(CSR_TIME);
+	if (begin >= end) {
+		report_fail("time counter has decreased");
+		report_prefix_pop();
+		return;
+	}
+
+	report_prefix_push("set_timer");
+
+	install_irq_handler(IRQ_SUPERVISOR_TIMER, timer_interrupt_handler);
+	local_irq_enable();
+
+	begin = csr_read(CSR_TIME);
+	ret = __time_sbi_ecall(SBI_EXT_TIME_SET_TIMER, csr_read(CSR_TIME) + delay);
+
+	if (ret.error) {
+		report_fail("setting timer failed");
+		install_irq_handler(IRQ_SUPERVISOR_TIMER, NULL);
+		report_prefix_pop();
+		report_prefix_pop();
+		return;
+	}
+
+	toggle_timer_interrupt(true);
+
+	while ((!timer_work) && (csr_read(CSR_TIME) <= (begin + delay)))
+		wfi();
+
+	end = csr_read(CSR_TIME);
+	report(timer_work, "timer interrupt received");
+
+	install_irq_handler(IRQ_SUPERVISOR_TIMER, NULL);
+	__time_sbi_ecall(SBI_EXT_TIME_SET_TIMER,  -1);
+
+	duration = end - begin;
+	if (timer_work)
+		report((duration >= delay) && (duration <= (delay + delay)), "timer delay honored");
+
+	report_prefix_pop();
+
+	report_prefix_pop();
+}
+
 int main(int argc, char **argv)
 {
 
@@ -122,6 +208,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 2/4] riscv: Update exception cause list

[Andrew Jones]

On Wed, Jun 19, 2024 at 01:30:51AM GMT, James Raphael Tiovalen wrote:
> Update the list of exception and interrupt causes to follow the latest
> RISC-V privileged ISA specification (version 20240411).
> 
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> ---
>  lib/riscv/asm/csr.h       | 15 +++++++++------
>  lib/riscv/asm/processor.h |  2 +-
>  2 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/riscv/asm/csr.h b/lib/riscv/asm/csr.h
> index d5879d2a..c1777744 100644
> --- a/lib/riscv/asm/csr.h
> +++ b/lib/riscv/asm/csr.h
> @@ -26,15 +26,18 @@
>  #define EXC_STORE_MISALIGNED	6
>  #define EXC_STORE_ACCESS	7
>  #define EXC_SYSCALL		8
> -#define EXC_HYPERVISOR_SYSCALL	9
> -#define EXC_SUPERVISOR_SYSCALL	10
> +#define EXC_SUPERVISOR_SYSCALL	9
>  #define EXC_INST_PAGE_FAULT	12
>  #define EXC_LOAD_PAGE_FAULT	13
>  #define EXC_STORE_PAGE_FAULT	15
> -#define EXC_INST_GUEST_PAGE_FAULT	20
> -#define EXC_LOAD_GUEST_PAGE_FAULT	21
> -#define EXC_VIRTUAL_INST_FAULT		22
> -#define EXC_STORE_GUEST_PAGE_FAULT	23
> +#define EXC_SOFTWARE_CHECK	18
> +#define EXC_HARDWARE_ERROR	19

The above changes don't update the exception cause list to the latest
spec, they drop the defines supporting the hypervisor extension's
augmentations (see Section 18.6.1 of the 20240411 priv spec).

> +
> +/* Interrupt causes */
> +#define IRQ_SUPERVISOR_SOFTWARE	1
> +#define IRQ_SUPERVISOR_TIMER	5
> +#define IRQ_SUPERVISOR_EXTERNAL	9
> +#define IRQ_COUNTER_OVERFLOW	13

These are fine, but we could also add the defines for the hypervisor
extension's augmentations. I also usually just copy+paste the defines
from Linux since I prefer name consistency.

>  
>  #ifndef __ASSEMBLY__
>  
> diff --git a/lib/riscv/asm/processor.h b/lib/riscv/asm/processor.h
> index 767b1caa..5942ed2e 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	64

If we want to test the H extension, then we'll want 20-23, but everything
else is custom or reserved, so we don't need to allocate handler pointer
space all the way up to 64 as they'll never be used.

Thanks,
drew

>  #define INTERRUPT_CAUSE_MAX	16
>  
>  typedef void (*exception_fn)(struct pt_regs *);
> -- 
> 2.43.0
> 
> 
> -- 
> kvm-riscv mailing list
> kvm-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv

Re: [kvm-unit-tests PATCH 3/4] riscv: Add methods to toggle interrupt enable bits

[Andrew Jones]

On Wed, Jun 19, 2024 at 01:30:52AM GMT, James Raphael Tiovalen wrote:
> Add some helper methods to toggle the interrupt enable bits in the SIE
> register.
> 
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> ---
>  riscv/Makefile            |  1 +
>  lib/riscv/asm/csr.h       |  7 +++++++
>  lib/riscv/asm/interrupt.h | 12 ++++++++++++
>  lib/riscv/interrupt.c     | 39 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 59 insertions(+)
>  create mode 100644 lib/riscv/asm/interrupt.h
>  create mode 100644 lib/riscv/interrupt.c
> 
> diff --git a/riscv/Makefile b/riscv/Makefile
> index 919a3ebb..108d4481 100644
> --- a/riscv/Makefile
> +++ b/riscv/Makefile
> @@ -30,6 +30,7 @@ cflatobjs += lib/memregions.o
>  cflatobjs += lib/on-cpus.o
>  cflatobjs += lib/vmalloc.o
>  cflatobjs += lib/riscv/bitops.o
> +cflatobjs += lib/riscv/interrupt.o
>  cflatobjs += lib/riscv/io.o
>  cflatobjs += lib/riscv/isa.o
>  cflatobjs += lib/riscv/mmu.o
> diff --git a/lib/riscv/asm/csr.h b/lib/riscv/asm/csr.h
> index c1777744..da58b0ce 100644
> --- a/lib/riscv/asm/csr.h
> +++ b/lib/riscv/asm/csr.h
> @@ -4,15 +4,22 @@
>  #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 SSTATUS_SIE		(_AC(1, UL) << 1)
>  
> +#define SIE_SSIE		(_AC(1, UL) << 1)
> +#define SIE_STIE		(_AC(1, UL) << 5)
> +#define SIE_SEIE		(_AC(1, UL) << 9)
> +#define SIE_LCOFIE		(_AC(1, UL) << 13)
> +
>  /* 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/interrupt.h b/lib/riscv/asm/interrupt.h
> new file mode 100644
> index 00000000..b760afbb
> --- /dev/null
> +++ b/lib/riscv/asm/interrupt.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef _ASMRISCV_INTERRUPT_H_
> +#define _ASMRISCV_INTERRUPT_H_
> +
> +#include <stdbool.h>
> +
> +void toggle_software_interrupt(bool enable);
> +void toggle_timer_interrupt(bool enable);
> +void toggle_external_interrupt(bool enable);
> +void toggle_local_cof_interrupt(bool enable);
> +
> +#endif /* _ASMRISCV_INTERRUPT_H_ */
> diff --git a/lib/riscv/interrupt.c b/lib/riscv/interrupt.c
> new file mode 100644
> index 00000000..bc0e16f1
> --- /dev/null
> +++ b/lib/riscv/interrupt.c
> @@ -0,0 +1,39 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2024, James Raphael Tiovalen <jamestiotio@gmail.com>
> + */
> +#include <libcflat.h>
> +#include <asm/csr.h>
> +#include <asm/interrupt.h>
> +
> +void toggle_software_interrupt(bool enable)
> +{
> +	if (enable)
> +		csr_set(CSR_SIE, SIE_SSIE);
> +	else
> +		csr_clear(CSR_SIE, SIE_SSIE);
> +}
> +
> +void toggle_timer_interrupt(bool enable)
> +{
> +	if (enable)
> +		csr_set(CSR_SIE, SIE_STIE);
> +	else
> +		csr_clear(CSR_SIE, SIE_STIE);
> +}
> +
> +void toggle_external_interrupt(bool enable)
> +{
> +	if (enable)
> +		csr_set(CSR_SIE, SIE_SEIE);
> +	else
> +		csr_clear(CSR_SIE, SIE_SEIE);
> +}
> +
> +void toggle_local_cof_interrupt(bool enable)
> +{
> +	if (enable)
> +		csr_set(CSR_SIE, SIE_LCOFIE);
> +	else
> +		csr_clear(CSR_SIE, SIE_LCOFIE);
> +}
> -- 
> 2.43.0
>

Most of this patch seems premature since the series only needs
toggle_timer_interrupt(). Also, I think lib/riscv/interrupt.c
is premature because something like toggle_timer_interrupt()
can be a static inline in a new lib/riscv/asm/timer.h file.

And please provide two functions rather than a toggle with
a parameter, i.e.

  timer_interrupt_enable() / timer_interrupt_disable()

Thanks,
drew

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

[James R T]

On Wed, Jun 19, 2024 at 4:31 PM Andrew Jones <andrew.jones@linux.dev> wrote:
>
> On Wed, Jun 19, 2024 at 01:30:51AM GMT, James Raphael Tiovalen wrote:
> > Update the list of exception and interrupt causes to follow the latest
> > RISC-V privileged ISA specification (version 20240411).
> >
> > Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> > ---
> >  lib/riscv/asm/csr.h       | 15 +++++++++------
> >  lib/riscv/asm/processor.h |  2 +-
> >  2 files changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/riscv/asm/csr.h b/lib/riscv/asm/csr.h
> > index d5879d2a..c1777744 100644
> > --- a/lib/riscv/asm/csr.h
> > +++ b/lib/riscv/asm/csr.h
> > @@ -26,15 +26,18 @@
> >  #define EXC_STORE_MISALIGNED 6
> >  #define EXC_STORE_ACCESS     7
> >  #define EXC_SYSCALL          8
> > -#define EXC_HYPERVISOR_SYSCALL       9
> > -#define EXC_SUPERVISOR_SYSCALL       10
> > +#define EXC_SUPERVISOR_SYSCALL       9
> >  #define EXC_INST_PAGE_FAULT  12
> >  #define EXC_LOAD_PAGE_FAULT  13
> >  #define EXC_STORE_PAGE_FAULT 15
> > -#define EXC_INST_GUEST_PAGE_FAULT    20
> > -#define EXC_LOAD_GUEST_PAGE_FAULT    21
> > -#define EXC_VIRTUAL_INST_FAULT               22
> > -#define EXC_STORE_GUEST_PAGE_FAULT   23
> > +#define EXC_SOFTWARE_CHECK   18
> > +#define EXC_HARDWARE_ERROR   19
>
> The above changes don't update the exception cause list to the latest
> spec, they drop the defines supporting the hypervisor extension's
> augmentations (see Section 18.6.1 of the 20240411 priv spec).
>

Right, I missed that section. I only checked Section 10.1.8. I will
update this list accordingly.

> > +
> > +/* Interrupt causes */
> > +#define IRQ_SUPERVISOR_SOFTWARE      1
> > +#define IRQ_SUPERVISOR_TIMER 5
> > +#define IRQ_SUPERVISOR_EXTERNAL      9
> > +#define IRQ_COUNTER_OVERFLOW 13
>
> These are fine, but we could also add the defines for the hypervisor
> extension's augmentations. I also usually just copy+paste the defines
> from Linux since I prefer name consistency.
>

Sure, I will do that.

> >
> >  #ifndef __ASSEMBLY__
> >
> > diff --git a/lib/riscv/asm/processor.h b/lib/riscv/asm/processor.h
> > index 767b1caa..5942ed2e 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  64
>
> If we want to test the H extension, then we'll want 20-23, but everything
> else is custom or reserved, so we don't need to allocate handler pointer
> space all the way up to 64 as they'll never be used.
>

In that case, I will keep it to 24 then.

> Thanks,
> drew
>
> >  #define INTERRUPT_CAUSE_MAX  16
> >
> >  typedef void (*exception_fn)(struct pt_regs *);
> > --
> > 2.43.0
> >
> >
> > --
> > kvm-riscv mailing list
> > kvm-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kvm-riscv

Best regards,
James Raphael Tiovalen

Re: [kvm-unit-tests PATCH 3/4] riscv: Add methods to toggle interrupt enable bits

[James R T]

On Wed, Jun 19, 2024 at 4:39 PM Andrew Jones <andrew.jones@linux.dev> wrote:
>
> On Wed, Jun 19, 2024 at 01:30:52AM GMT, James Raphael Tiovalen wrote:
> > Add some helper methods to toggle the interrupt enable bits in the SIE
> > register.
> >
> > Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> > ---
> >  riscv/Makefile            |  1 +
> >  lib/riscv/asm/csr.h       |  7 +++++++
> >  lib/riscv/asm/interrupt.h | 12 ++++++++++++
> >  lib/riscv/interrupt.c     | 39 +++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 59 insertions(+)
> >  create mode 100644 lib/riscv/asm/interrupt.h
> >  create mode 100644 lib/riscv/interrupt.c
> >
> > diff --git a/riscv/Makefile b/riscv/Makefile
> > index 919a3ebb..108d4481 100644
> > --- a/riscv/Makefile
> > +++ b/riscv/Makefile
> > @@ -30,6 +30,7 @@ cflatobjs += lib/memregions.o
> >  cflatobjs += lib/on-cpus.o
> >  cflatobjs += lib/vmalloc.o
> >  cflatobjs += lib/riscv/bitops.o
> > +cflatobjs += lib/riscv/interrupt.o
> >  cflatobjs += lib/riscv/io.o
> >  cflatobjs += lib/riscv/isa.o
> >  cflatobjs += lib/riscv/mmu.o
> > diff --git a/lib/riscv/asm/csr.h b/lib/riscv/asm/csr.h
> > index c1777744..da58b0ce 100644
> > --- a/lib/riscv/asm/csr.h
> > +++ b/lib/riscv/asm/csr.h
> > @@ -4,15 +4,22 @@
> >  #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 SSTATUS_SIE          (_AC(1, UL) << 1)
> >
> > +#define SIE_SSIE             (_AC(1, UL) << 1)
> > +#define SIE_STIE             (_AC(1, UL) << 5)
> > +#define SIE_SEIE             (_AC(1, UL) << 9)
> > +#define SIE_LCOFIE           (_AC(1, UL) << 13)
> > +
> >  /* 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/interrupt.h b/lib/riscv/asm/interrupt.h
> > new file mode 100644
> > index 00000000..b760afbb
> > --- /dev/null
> > +++ b/lib/riscv/asm/interrupt.h
> > @@ -0,0 +1,12 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef _ASMRISCV_INTERRUPT_H_
> > +#define _ASMRISCV_INTERRUPT_H_
> > +
> > +#include <stdbool.h>
> > +
> > +void toggle_software_interrupt(bool enable);
> > +void toggle_timer_interrupt(bool enable);
> > +void toggle_external_interrupt(bool enable);
> > +void toggle_local_cof_interrupt(bool enable);
> > +
> > +#endif /* _ASMRISCV_INTERRUPT_H_ */
> > diff --git a/lib/riscv/interrupt.c b/lib/riscv/interrupt.c
> > new file mode 100644
> > index 00000000..bc0e16f1
> > --- /dev/null
> > +++ b/lib/riscv/interrupt.c
> > @@ -0,0 +1,39 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (C) 2024, James Raphael Tiovalen <jamestiotio@gmail.com>
> > + */
> > +#include <libcflat.h>
> > +#include <asm/csr.h>
> > +#include <asm/interrupt.h>
> > +
> > +void toggle_software_interrupt(bool enable)
> > +{
> > +     if (enable)
> > +             csr_set(CSR_SIE, SIE_SSIE);
> > +     else
> > +             csr_clear(CSR_SIE, SIE_SSIE);
> > +}
> > +
> > +void toggle_timer_interrupt(bool enable)
> > +{
> > +     if (enable)
> > +             csr_set(CSR_SIE, SIE_STIE);
> > +     else
> > +             csr_clear(CSR_SIE, SIE_STIE);
> > +}
> > +
> > +void toggle_external_interrupt(bool enable)
> > +{
> > +     if (enable)
> > +             csr_set(CSR_SIE, SIE_SEIE);
> > +     else
> > +             csr_clear(CSR_SIE, SIE_SEIE);
> > +}
> > +
> > +void toggle_local_cof_interrupt(bool enable)
> > +{
> > +     if (enable)
> > +             csr_set(CSR_SIE, SIE_LCOFIE);
> > +     else
> > +             csr_clear(CSR_SIE, SIE_LCOFIE);
> > +}
> > --
> > 2.43.0
> >
>
> Most of this patch seems premature since the series only needs
> toggle_timer_interrupt(). Also, I think lib/riscv/interrupt.c
> is premature because something like toggle_timer_interrupt()
> can be a static inline in a new lib/riscv/asm/timer.h file.
>

Got it. In that case, I will combine the changes with the actual test
since we will be adding only the timer interrupt code.

> And please provide two functions rather than a toggle with
> a parameter, i.e.
>
>   timer_interrupt_enable() / timer_interrupt_disable()
>

Sure, will do that.

> Thanks,
> drew

Best regards,
James Raphael Tiovalen

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

[Andrew Jones]

On Wed, Jun 19, 2024 at 01:30:53AM 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 frame
> 
> The timer interrupt delay can be set using the TIMER_DELAY environment
> variable. If the variable is not set, the default delay value is
> 1000000. The time interval used to validate the timer interrupt is
> between the specified delay and double the delay. Because of this, the
> test might fail if the delay is too short. Hence, we set the default
> delay value as the minimum value.
> 
> 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 |  6 ++++
>  lib/riscv/asm/sbi.h |  5 +++
>  riscv/sbi.c         | 87 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 98 insertions(+)
> 
> diff --git a/lib/riscv/asm/csr.h b/lib/riscv/asm/csr.h
> index da58b0ce..c4435650 100644
> --- a/lib/riscv/asm/csr.h
> +++ b/lib/riscv/asm/csr.h
> @@ -12,6 +12,7 @@
>  #define CSR_STVAL		0x143
>  #define CSR_SIP			0x144
>  #define CSR_SATP		0x180
> +#define CSR_TIME		0xc01
>  
>  #define SSTATUS_SIE		(_AC(1, UL) << 1)
>  
> @@ -108,5 +109,10 @@
>  				: "memory");			\
>  })
>  
> +#define wfi()							\
> +({								\
> +	__asm__ __volatile__("wfi" ::: "memory");		\
> +})

This belongs in lib/riscv/asm/barrier.h (but actually we don't need it at
all, see below.)

> +
>  #endif /* !__ASSEMBLY__ */
>  #endif /* _ASMRISCV_CSR_H_ */
> diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h
> index d82a384d..eb4c77ef 100644
> --- a/lib/riscv/asm/sbi.h
> +++ b/lib/riscv/asm/sbi.h
> @@ -18,6 +18,7 @@ enum sbi_ext_id {
>  	SBI_EXT_BASE = 0x10,
>  	SBI_EXT_HSM = 0x48534d,
>  	SBI_EXT_SRST = 0x53525354,
> +	SBI_EXT_TIME = 0x54494D45,

Let's list these in chapter order, so TIME comes after BASE.

>  };
>  
>  enum sbi_ext_base_fid {
> @@ -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/riscv/sbi.c b/riscv/sbi.c
> index 762e9711..6ad1dff6 100644
> --- a/riscv/sbi.c
> +++ b/riscv/sbi.c
> @@ -6,8 +6,13 @@
>   */
>  #include <libcflat.h>
>  #include <stdlib.h>
> +#include <asm/csr.h>
> +#include <asm/interrupt.h>
> +#include <asm/processor.h>
>  #include <asm/sbi.h>
>  
> +static bool timer_work;

timer_works

> +
>  static void help(void)
>  {
>  	puts("Test SBI\n");
> @@ -19,6 +24,18 @@ 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(int fid, unsigned long arg0)

Since this is the time extension specific wrapper function then we should
use time extension specific parameter names, i.e. s/arg0/stime_value/ and
we don't need to take fid as a parameter since there's only a single
function ID which can just be put directly in the sbi_ecall() call.

> +{
> +	return sbi_ecall(SBI_EXT_TIME, fid, arg0, 0, 0, 0, 0, 0);
> +}
> +
> +static void timer_interrupt_handler(struct pt_regs *regs)
> +{
> +	timer_work = true;
> +	toggle_timer_interrupt(false);
> +	local_irq_disable();

Just masking the timer interrupt should be enough. We don't want to over
disable interrupts because we want to ensure the minimally specified
disabling is sufficient in order to properly test the SBI impl. Also,
we probably don't want to mask the timer interrupt here since we need
to also provide a test case which only sets the next timer interrupt to
be "infinitely far into the future" as specified by SBI as an alternative
for "clearing" the timer interrupt.

> +}
> +
>  static bool env_or_skip(const char *env)
>  {
>  	if (!getenv(env)) {
> @@ -112,6 +129,75 @@ static void check_base(void)
>  	report_prefix_pop();
>  }
>  
> +static void check_time(void)
> +{
> +	struct sbiret ret;
> +	unsigned long begin, end, duration;
> +	const unsigned long default_delay = 1000000;
> +	unsigned long delay = getenv("TIMER_DELAY")
> +				? MAX(strtol(getenv("TIMER_DELAY"), NULL, 0), default_delay)

Is there a reason we can't have a shorter delay than 1000000? Using MAX()
will silently force 1000000 even when the user provided an environment
variable with something smaller.

> +				: default_delay;
> +
> +	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 = csr_read(CSR_TIME);
> +	end = csr_read(CSR_TIME);

It doesn't hurt to have this sanity check, but I'd probably make it
an assert since things are really nuts if the timer isn't working.

 begin = csr_read(CSR_TIME);
 // Add delay here based on the timer frequency to ensure at
 // least one tick of the timer occurs
 assert(begin < csr_read(CSR_TIME));

> +	if (begin >= end) {
> +		report_fail("time counter has decreased");
> +		report_prefix_pop();
> +		return;
> +	}
> +
> +	report_prefix_push("set_timer");
> +
> +	install_irq_handler(IRQ_SUPERVISOR_TIMER, timer_interrupt_handler);
> +	local_irq_enable();
> +
> +	begin = csr_read(CSR_TIME);
> +	ret = __time_sbi_ecall(SBI_EXT_TIME_SET_TIMER, csr_read(CSR_TIME) + delay);
> +
> +	if (ret.error) {
> +		report_fail("setting timer failed");
> +		install_irq_handler(IRQ_SUPERVISOR_TIMER, NULL);
> +		report_prefix_pop();
> +		report_prefix_pop();
> +		return;

We don't necessarily need to abort the rest of the tests. Sometimes yes,
if we know that when this test fails nothing else can work, but if there
are other tests, such as negative tests, that we could still try, then
we should.

> +	}
> +
> +	toggle_timer_interrupt(true);
> +
> +	while ((!timer_work) && (csr_read(CSR_TIME) <= (begin + delay)))

nit: Unnecessary () on both sides of the &&

> +		wfi();


wfi() means we expect some interrupt, sometime, but if the timer setting
isn't working then we may never get any interrupt. We should use
cpu_relax(). Unit tests can't make any assumptions, but that's OK, since
they don't need to be efficient. (Anything about the environment we would
like to assume should be checked with asserts or at least be documented.)

> +
> +	end = csr_read(CSR_TIME);

duration will be slightly more accurate if we write the while loop like

 while ((end = csr_read(CSR_TIME)) < begin + delay)
     cpu_relax();

> +	report(timer_work, "timer interrupt received");
> +
> +	install_irq_handler(IRQ_SUPERVISOR_TIMER, NULL);
> +	__time_sbi_ecall(SBI_EXT_TIME_SET_TIMER,  -1);
> +
> +	duration = end - begin;
> +	if (timer_work)
> +		report((duration >= delay) && (duration <= (delay + delay)), "timer delay honored");

The <= delay + delay check implies that the delay has been selected for
two purposes, how long to wait for the interrupt and how much time we
allow the interrupt to be late. We should have two separate variables
for those two purposes, both configurable.

> +
> +	report_prefix_pop();
> +

nit: drop this extra blank line.

> +	report_prefix_pop();

(After seeing all these repeated _pop() lines I'm actually thinking we
need another report API call, something like report_prefix_popn(int n)
which pops n number times.)

> +}
> +
>  int main(int argc, char **argv)
>  {
>  
> @@ -122,6 +208,7 @@ int main(int argc, char **argv)
>  
>  	report_prefix_push("sbi");
>  	check_base();
> +	check_time();
>  
>  	return report_summary();
>  }
> -- 
> 2.43.0
>

The spec says "This function must clear the pending timer interrupt bit as
well." so I think we should have something in the test that checks that
too.

Thanks,
drew