[kvm-unit-tests PATCH 0/3] riscv: Timer support

[kvm-unit-tests PATCH 0/3] riscv: Timer support

[Andrew Jones]

We already have some timer / delay support but we leave a lot to
the unit test authors for deciding what timer to use (SBI vs. Sstc)
and setting it. Provide an API that prefers Sstc and falls back to
SBI.

This found a bug in QEMU's Sstc that I'll try to find time to fix.
If we start a timer with a long delay and then stop it before it has
expired, QEMU still delivers the interrupt. The only way to avoid
getting it on QEMU is to disable the timer irq. The BPI, which also
has Sstc, behaves as expected though, i.e. even with the timer irq
always enabled we won't get a timer irq if we stop it before it had
a chance to expire. Disabling Sstc on QEMU, which falls back to SBI
TIME, also behaves as expected.

Andrew Jones (3):
  riscv: Introduce local_timer_init
  riscv: Share sbi_time_ecall with framework
  riscv: Provide timer_start and timer_stop

 lib/riscv/asm/sbi.h   |  1 +
 lib/riscv/asm/timer.h |  3 +++
 lib/riscv/sbi.c       |  5 +++++
 lib/riscv/setup.c     |  2 ++
 lib/riscv/smp.c       |  2 ++
 lib/riscv/timer.c     | 46 +++++++++++++++++++++++++++++++++++++++++++
 riscv/sbi.c           | 18 ++++-------------
 7 files changed, 63 insertions(+), 14 deletions(-)

-- 
2.45.2

[kvm-unit-tests PATCH 1/3] riscv: Introduce local_timer_init

[Andrew Jones]

When Sstc is available make sure that even if we enable timer
interrupts nothing will happen. This is necessary for cases where
the unit tests actually intend to use the SBI TIME extension and
aren't thinking about Sstc at all, like the SBI TIME test in
riscv/sbi where we can now remove the initialization.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/riscv/asm/timer.h |  1 +
 lib/riscv/setup.c     |  2 ++
 lib/riscv/smp.c       |  2 ++
 lib/riscv/timer.c     | 13 +++++++++++++
 riscv/sbi.c           |  5 -----
 5 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/lib/riscv/asm/timer.h b/lib/riscv/asm/timer.h
index b3514d3f6a78..fd12251a3a6b 100644
--- a/lib/riscv/asm/timer.h
+++ b/lib/riscv/asm/timer.h
@@ -5,6 +5,7 @@
 #include <asm/csr.h>
 
 extern void timer_get_frequency(void);
+extern void local_timer_init(void);
 
 static inline uint64_t timer_get_cycles(void)
 {
diff --git a/lib/riscv/setup.c b/lib/riscv/setup.c
index 9a16f00093d7..7c4321b1c30f 100644
--- a/lib/riscv/setup.c
+++ b/lib/riscv/setup.c
@@ -210,6 +210,7 @@ void setup(const void *fdt, phys_addr_t freemem_start)
 	cpu_init();
 	timer_get_frequency();
 	thread_info_init();
+	local_timer_init();
 	io_init();
 
 	ret = dt_get_bootargs(&bootargs);
@@ -276,6 +277,7 @@ efi_status_t setup_efi(efi_bootinfo_t *efi_bootinfo)
 	cpu_init();
 	timer_get_frequency();
 	thread_info_init();
+	local_timer_init();
 	io_init();
 	initrd_setup();
 
diff --git a/lib/riscv/smp.c b/lib/riscv/smp.c
index 4d373e0a29a8..18d0393c0cc2 100644
--- a/lib/riscv/smp.c
+++ b/lib/riscv/smp.c
@@ -14,6 +14,7 @@
 #include <asm/processor.h>
 #include <asm/sbi.h>
 #include <asm/smp.h>
+#include <asm/timer.h>
 
 cpumask_t cpu_present_mask;
 cpumask_t cpu_online_mask;
@@ -27,6 +28,7 @@ secondary_func_t secondary_cinit(struct secondary_data *data)
 
 	__mmu_enable(data->satp);
 	thread_info_init();
+	local_timer_init();
 	info = current_thread_info();
 	set_cpu_online(info->cpu, true);
 	smp_send_event();
diff --git a/lib/riscv/timer.c b/lib/riscv/timer.c
index d78d254c8eca..92826d6ec3fe 100644
--- a/lib/riscv/timer.c
+++ b/lib/riscv/timer.c
@@ -4,7 +4,11 @@
  */
 #include <libcflat.h>
 #include <devicetree.h>
+#include <limits.h>
+#include <asm/csr.h>
+#include <asm/isa.h>
 #include <asm/setup.h>
+#include <asm/smp.h>
 #include <asm/timer.h>
 
 void timer_get_frequency(void)
@@ -26,3 +30,12 @@ void timer_get_frequency(void)
 	data = (u32 *)prop->data;
 	timebase_frequency = fdt32_to_cpu(*data);
 }
+
+void local_timer_init(void)
+{
+	if (cpu_has_extension(smp_processor_id(), ISA_SSTC)) {
+		csr_write(CSR_STIMECMP, ULONG_MAX);
+		if (__riscv_xlen == 32)
+			csr_write(CSR_STIMECMPH, ULONG_MAX);
+	}
+}
diff --git a/riscv/sbi.c b/riscv/sbi.c
index 01697aed3457..e8598fe721a6 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -258,11 +258,6 @@ static void check_time(void)
 
 	install_irq_handler(IRQ_S_TIMER, timer_irq_handler);
 	local_irq_enable();
-	if (cpu_has_extension(smp_processor_id(), ISA_SSTC)) {
-		csr_write(CSR_STIMECMP, ULONG_MAX);
-		if (__riscv_xlen == 32)
-			csr_write(CSR_STIMECMPH, ULONG_MAX);
-	}
 	timer_irq_enable();
 
 	timer_check_set_timer(false);
-- 
2.45.2

Re: [kvm-unit-tests PATCH 1/3] riscv: Introduce local_timer_init

[Andrew Jones]

On Wed, Aug 28, 2024 at 06:22:02PM GMT, Andrew Jones wrote:
> When Sstc is available make sure that even if we enable timer
> interrupts nothing will happen. This is necessary for cases where
> the unit tests actually intend to use the SBI TIME extension and
> aren't thinking about Sstc at all, like the SBI TIME test in
> riscv/sbi where we can now remove the initialization.
> 
> Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> ---
>  lib/riscv/asm/timer.h |  1 +
>  lib/riscv/setup.c     |  2 ++
>  lib/riscv/smp.c       |  2 ++
>  lib/riscv/timer.c     | 13 +++++++++++++
>  riscv/sbi.c           |  5 -----
>  5 files changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/riscv/asm/timer.h b/lib/riscv/asm/timer.h
> index b3514d3f6a78..fd12251a3a6b 100644
> --- a/lib/riscv/asm/timer.h
> +++ b/lib/riscv/asm/timer.h
> @@ -5,6 +5,7 @@
>  #include <asm/csr.h>
>  
>  extern void timer_get_frequency(void);
> +extern void local_timer_init(void);
>  

I've renamed the new function to local_hart_init and put it in processor.c
instead of timer.c. This is because going forward there will be other
non-timer-related CSRs that need to be set at init time and we can just
lump them all together.

Thanks,
drew

[kvm-unit-tests PATCH 2/3] riscv: Share sbi_time_ecall with framework

[Andrew Jones]

Setting timers is a useful thing to do for all types of tests. Not
every platform will have Sstc so make the SBI TIME extension
available as well.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/riscv/asm/sbi.h |  1 +
 lib/riscv/sbi.c     |  5 +++++
 riscv/sbi.c         | 13 ++++---------
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/lib/riscv/asm/sbi.h b/lib/riscv/asm/sbi.h
index a864e268437b..4a35cf38da70 100644
--- a/lib/riscv/asm/sbi.h
+++ b/lib/riscv/asm/sbi.h
@@ -67,6 +67,7 @@ struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
 void sbi_shutdown(void);
 struct sbiret sbi_hart_start(unsigned long hartid, unsigned long entry, unsigned long sp);
 struct sbiret sbi_send_ipi(unsigned long hart_mask, unsigned long hart_mask_base);
+struct sbiret sbi_set_timer(unsigned long stime_value);
 long sbi_probe(int ext);
 
 #endif /* !__ASSEMBLY__ */
diff --git a/lib/riscv/sbi.c b/lib/riscv/sbi.c
index 19d58ab73593..07660e422cbb 100644
--- a/lib/riscv/sbi.c
+++ b/lib/riscv/sbi.c
@@ -44,6 +44,11 @@ struct sbiret sbi_send_ipi(unsigned long hart_mask, unsigned long hart_mask_base
 	return sbi_ecall(SBI_EXT_IPI, SBI_EXT_IPI_SEND_IPI, hart_mask, hart_mask_base, 0, 0, 0, 0);
 }
 
+struct sbiret sbi_set_timer(unsigned long stime_value)
+{
+	return sbi_ecall(SBI_EXT_TIME, SBI_EXT_TIME_SET_TIMER, stime_value, 0, 0, 0, 0, 0);
+}
+
 long sbi_probe(int ext)
 {
 	struct sbiret ret;
diff --git a/riscv/sbi.c b/riscv/sbi.c
index e8598fe721a6..85cb7e589bdc 100644
--- a/riscv/sbi.c
+++ b/riscv/sbi.c
@@ -35,11 +35,6 @@ 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 struct sbiret __dbcn_sbi_ecall(int fid, unsigned long arg0, unsigned long arg1, unsigned long arg2)
 {
 	return sbi_ecall(SBI_EXT_DBCN, fid, arg0, arg1, arg2, 0, 0, 0);
@@ -198,7 +193,7 @@ static void timer_irq_handler(struct pt_regs *regs)
 	if (timer_info.mask_timer_irq)
 		timer_irq_disable();
 	else
-		__time_sbi_ecall(ULONG_MAX);
+		sbi_set_timer(ULONG_MAX);
 
 	if (!timer_irq_pending())
 		timer_info.timer_irq_cleared = true;
@@ -217,7 +212,7 @@ static void timer_check_set_timer(bool mask_timer_irq)
 
 	timer_info = (struct timer_info){ .mask_timer_irq = mask_timer_irq };
 	begin = timer_get_cycles();
-	ret = __time_sbi_ecall(begin + d);
+	ret = sbi_set_timer(begin + d);
 
 	report(!ret.error, "set timer%s", mask_test_str);
 	if (ret.error)
@@ -268,10 +263,10 @@ static void check_time(void)
 		report_skip("timer irq enable bit is not writable, skipping mask irq test");
 
 	timer_irq_disable();
-	__time_sbi_ecall(0);
+	sbi_set_timer(0);
 	pending = timer_irq_pending();
 	report(pending, "timer immediately pending by setting timer to 0");
-	__time_sbi_ecall(ULONG_MAX);
+	sbi_set_timer(ULONG_MAX);
 	if (pending)
 		report(!timer_irq_pending(), "pending timer cleared while masked");
 	else
-- 
2.45.2

[kvm-unit-tests PATCH 3/3] riscv: Provide timer_start and timer_stop

[Andrew Jones]

For unit tests that need a timer but don't care if they use Sstc or
SBI TIME, provide timer_start and timer_stop which will try Sstc
first and fallback to SBI TIME.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/riscv/asm/timer.h |  2 ++
 lib/riscv/timer.c     | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/lib/riscv/asm/timer.h b/lib/riscv/asm/timer.h
index fd12251a3a6b..9e790a97bd24 100644
--- a/lib/riscv/asm/timer.h
+++ b/lib/riscv/asm/timer.h
@@ -6,6 +6,8 @@
 
 extern void timer_get_frequency(void);
 extern void local_timer_init(void);
+extern void timer_start(unsigned long duration_us);
+extern void timer_stop(void);
 
 static inline uint64_t timer_get_cycles(void)
 {
diff --git a/lib/riscv/timer.c b/lib/riscv/timer.c
index 92826d6ec3fe..67fd031ab95f 100644
--- a/lib/riscv/timer.c
+++ b/lib/riscv/timer.c
@@ -6,7 +6,9 @@
 #include <devicetree.h>
 #include <limits.h>
 #include <asm/csr.h>
+#include <asm/delay.h>
 #include <asm/isa.h>
+#include <asm/sbi.h>
 #include <asm/setup.h>
 #include <asm/smp.h>
 #include <asm/timer.h>
@@ -39,3 +41,34 @@ void local_timer_init(void)
 			csr_write(CSR_STIMECMPH, ULONG_MAX);
 	}
 }
+
+void timer_start(unsigned long duration_us)
+{
+	uint64_t next = timer_get_cycles() + usec_to_cycles((uint64_t)duration_us);
+
+	if (cpu_has_extension(smp_processor_id(), ISA_SSTC)) {
+		csr_write(CSR_STIMECMP, (unsigned long)next);
+		if (__riscv_xlen == 32)
+			csr_write(CSR_STIMECMPH, (unsigned long)(next >> 32));
+	} else if (sbi_probe(SBI_EXT_TIME)) {
+		struct sbiret ret = sbi_set_timer(next);
+		assert(ret.error == SBI_SUCCESS);
+		assert(!(next >> 32));
+	} else {
+		assert_msg(false, "No timer to start!");
+	}
+}
+
+void timer_stop(void)
+{
+	if (cpu_has_extension(smp_processor_id(), ISA_SSTC)) {
+		csr_write(CSR_STIMECMP, ULONG_MAX);
+		if (__riscv_xlen == 32)
+			csr_write(CSR_STIMECMPH, ULONG_MAX);
+	} else if (sbi_probe(SBI_EXT_TIME)) {
+		struct sbiret ret = sbi_set_timer(ULONG_MAX);
+		assert(ret.error == SBI_SUCCESS);
+	} else {
+		assert_msg(false, "No timer to stop!");
+	}
+}
-- 
2.45.2

[kvm-unit-tests PATCH 4/3] riscv: QEMU Sstc timer stop workaround

[Andrew Jones]

QEMU doesn't stop a pending timer when UINT64_MAX is written to
stimecmp, see QEMU commit ae0edf2188b3 ("target/riscv: No need to
re-start QEMU timer when timecmp == UINT64_MAX"). We should probably
change that in QEMU, but we need a solution in kvm-unit-tests anyway
in order to support older QEMU versions. A bit of an ugly workaround
is to simply subtract one from UINT64_MAX, which is still a really
big number, but not the exact number QEMU is using to decide it
should skip the timer update.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/riscv/timer.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/lib/riscv/timer.c b/lib/riscv/timer.c
index 67fd031ab95f..28e1626607f7 100644
--- a/lib/riscv/timer.c
+++ b/lib/riscv/timer.c
@@ -62,9 +62,16 @@ void timer_start(unsigned long duration_us)
 void timer_stop(void)
 {
 	if (cpu_has_extension(smp_processor_id(), ISA_SSTC)) {
-		csr_write(CSR_STIMECMP, ULONG_MAX);
+		/*
+		 * Subtract one from ULONG_MAX to workaround QEMU using that
+		 * exact number to decide *not* to update the timer. IOW, if
+		 * we used ULONG_MAX, then we wouldn't stop the timer at all,
+		 * but one less is still a big number ("infinity") and it gets
+		 * QEMU to do what we want.
+		 */
+		csr_write(CSR_STIMECMP, ULONG_MAX - 1);
 		if (__riscv_xlen == 32)
-			csr_write(CSR_STIMECMPH, ULONG_MAX);
+			csr_write(CSR_STIMECMPH, ULONG_MAX - 1);
 	} else if (sbi_probe(SBI_EXT_TIME)) {
 		struct sbiret ret = sbi_set_timer(ULONG_MAX);
 		assert(ret.error == SBI_SUCCESS);
-- 
2.45.2

Re: [kvm-unit-tests PATCH 0/3] riscv: Timer support

[Andrew Jones]

On Wed, Aug 28, 2024 at 06:22:01PM GMT, Andrew Jones wrote:
> We already have some timer / delay support but we leave a lot to
> the unit test authors for deciding what timer to use (SBI vs. Sstc)
> and setting it. Provide an API that prefers Sstc and falls back to
> SBI.
> 
> This found a bug in QEMU's Sstc that I'll try to find time to fix.
> If we start a timer with a long delay and then stop it before it has
> expired, QEMU still delivers the interrupt. The only way to avoid
> getting it on QEMU is to disable the timer irq. The BPI, which also
> has Sstc, behaves as expected though, i.e. even with the timer irq
> always enabled we won't get a timer irq if we stop it before it had
> a chance to expire. Disabling Sstc on QEMU, which falls back to SBI
> TIME, also behaves as expected.
> 
> Andrew Jones (3):
>   riscv: Introduce local_timer_init
>   riscv: Share sbi_time_ecall with framework
>   riscv: Provide timer_start and timer_stop
> 
>  lib/riscv/asm/sbi.h   |  1 +
>  lib/riscv/asm/timer.h |  3 +++
>  lib/riscv/sbi.c       |  5 +++++
>  lib/riscv/setup.c     |  2 ++
>  lib/riscv/smp.c       |  2 ++
>  lib/riscv/timer.c     | 46 +++++++++++++++++++++++++++++++++++++++++++
>  riscv/sbi.c           | 18 ++++-------------
>  7 files changed, 63 insertions(+), 14 deletions(-)
> 
> -- 
> 2.45.2
>

Queued on riscv/sbi, https://gitlab.com/jones-drew/kvm-unit-tests/-/commits/riscv%2Fsbi