[kvm-unit-tests PATCH 0/4] riscv: A few SMP fixes

[kvm-unit-tests PATCH 0/4] riscv: A few SMP fixes

[Andrew Jones]

tl;dr - these patches are improvement to the riscv framework to better
support the SBI HSM tests.

The first patch is a simpler alternative to [1] and [2] which doesn't
require us to decide how best to make the number configurable. The
second patch just adds sanity checking to make sure we can expect the
SBI implementation to accept all hartids mapped from the present mask.
The third patch was already posted once before[3] with a slightly
different summary. It and the last patch improve smp_boot_secondary()
since the SBI HSM tests were attempting to make workarounds for odd
behaviors.

[1] https://lore.kernel.org/all/20240820170150.377580-2-andrew.jones@linux.dev/
[2] https://lore.kernel.org/all/20240903143946.834864-6-andrew.jones@linux.dev/
[3] https://lore.kernel.org/all/20240904120812.1798715-2-andrew.jones@linux.dev/

Andrew Jones (4):
  riscv: Bump NR_CPUS to 256
  riscv: Filter unmanaged harts from present mask
  riscv: Fix secondary_entry
  riscv: Rework smp_boot_secondary

 lib/riscv/asm/processor.h |  1 +
 lib/riscv/asm/setup.h     |  2 +-
 lib/riscv/setup.c         | 11 ++++++---
 lib/riscv/smp.c           | 49 +++++++++++++++++++++++++++------------
 riscv/cstart.S            |  7 +++---
 5 files changed, 48 insertions(+), 22 deletions(-)

-- 
2.47.0

[kvm-unit-tests PATCH 1/4] riscv: Bump NR_CPUS to 256

[Andrew Jones]

Besides a bit more memory used for the .bss section, where there are
NR_CPUS sized arrays, and a tiny bit more stack used by functions
with cpumasks on their stacks, then there's no harm in bumping
NR_CPUS. Bump it to 256, which should cover us for quite a while.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/riscv/asm/setup.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/riscv/asm/setup.h b/lib/riscv/asm/setup.h
index a13159bfe395..a031ebe7d762 100644
--- a/lib/riscv/asm/setup.h
+++ b/lib/riscv/asm/setup.h
@@ -4,7 +4,7 @@
 #include <libcflat.h>
 #include <asm/processor.h>
 
-#define NR_CPUS 16
+#define NR_CPUS 256
 extern struct thread_info cpus[NR_CPUS];
 extern int nr_cpus;
 extern uint64_t timebase_frequency;
-- 
2.47.0

[kvm-unit-tests PATCH 2/4] riscv: Filter unmanaged harts from present mask

[Andrew Jones]

We use SBI to manage harts and SBI may have a different idea of which
harts it should manage than our hardware description. Filter out all
harts which fail an SBI HSM status call from the present mask to
ensure we don't try to use them.

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

diff --git a/lib/riscv/setup.c b/lib/riscv/setup.c
index f347ad6352d7..211945448b0f 100644
--- a/lib/riscv/setup.c
+++ b/lib/riscv/setup.c
@@ -19,6 +19,7 @@
 #include <asm/mmu.h>
 #include <asm/page.h>
 #include <asm/processor.h>
+#include <asm/sbi.h>
 #include <asm/setup.h>
 #include <asm/timer.h>
 
@@ -51,7 +52,9 @@ static void cpu_set_fdt(int fdtnode __unused, u64 regval, void *info __unused)
 
 	cpus[cpu].cpu = cpu;
 	cpus[cpu].hartid = regval;
-	set_cpu_present(cpu, true);
+
+	if (!sbi_hart_get_status(cpus[cpu].hartid).error)
+		set_cpu_present(cpu, true);
 }
 
 static void cpu_init_acpi(void)
@@ -61,7 +64,7 @@ static void cpu_init_acpi(void)
 
 static void cpu_init(void)
 {
-	int ret;
+	int ret, me;
 
 	nr_cpus = 0;
 	if (dt_available()) {
@@ -71,7 +74,9 @@ static void cpu_init(void)
 		cpu_init_acpi();
 	}
 
-	set_cpu_online(hartid_to_cpu(csr_read(CSR_SSCRATCH)), true);
+	me = hartid_to_cpu(csr_read(CSR_SSCRATCH));
+	assert(cpu_present(me));
+	set_cpu_online(me, true);
 	cpu0_calls_idle = true;
 }
 
-- 
2.47.0

[kvm-unit-tests PATCH 3/4] riscv: Fix secondary_entry

[Andrew Jones]

The last few instructions of secondary_entry had the right concept,
but were the totally wrong implementation. Without setting ra, then,
when the boot function doesn't stay in an infinite loop, like
do_idle() would, we'd go off into the weeds when trying to return
from it. Make sure we set ra to come back to where we can then call
do_idle() instead. The bug was found by inspection since nobody is
calling smp_boot_secondary() with anything other than do_idle() at
this time.

Fixes: 9c92b28e6b7b ("riscv: Add SMP support")
Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 riscv/cstart.S | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/riscv/cstart.S b/riscv/cstart.S
index bae1d2f5b4d5..687173706d83 100644
--- a/riscv/cstart.S
+++ b/riscv/cstart.S
@@ -154,9 +154,9 @@ secondary_entry:
 	mv	a0, sp
 	call	secondary_cinit
 	addi	sp, sp, SECONDARY_DATA_SIZE
-	jr	a0
-	la	a0, do_idle
-	jr	a0
+	jalr	ra, a0
+	call	do_idle
+	j	.	/* unreachable */
 
 /*
  * Save context to address in a0.
-- 
2.47.0

[kvm-unit-tests PATCH 4/4] riscv: Rework smp_boot_secondary

[Andrew Jones]

Use HSM status to determine when a secondary should be started.
Also save the stack pointer so a secondary may be stopped and
started again without leaking old stacks.

Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
---
 lib/riscv/asm/processor.h |  1 +
 lib/riscv/smp.c           | 49 +++++++++++++++++++++++++++------------
 riscv/cstart.S            |  1 +
 3 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/lib/riscv/asm/processor.h b/lib/riscv/asm/processor.h
index 4063255a3475..401042724cee 100644
--- a/lib/riscv/asm/processor.h
+++ b/lib/riscv/asm/processor.h
@@ -13,6 +13,7 @@ struct thread_info {
 	int cpu;
 	unsigned long hartid;
 	unsigned long isa[1];
+	unsigned long sp;
 	exception_fn exception_handlers[EXCEPTION_CAUSE_MAX];
 	exception_fn interrupt_handlers[INTERRUPT_CAUSE_MAX];
 };
diff --git a/lib/riscv/smp.c b/lib/riscv/smp.c
index eb7061abfe7f..e92f83e1310d 100644
--- a/lib/riscv/smp.c
+++ b/lib/riscv/smp.c
@@ -19,8 +19,6 @@ cpumask_t cpu_present_mask;
 cpumask_t cpu_online_mask;
 cpumask_t cpu_idle_mask;
 
-static cpumask_t cpu_started;
-
 secondary_func_t secondary_cinit(struct secondary_data *data)
 {
 	struct thread_info *info;
@@ -37,27 +35,40 @@ secondary_func_t secondary_cinit(struct secondary_data *data)
 
 static void __smp_boot_secondary(int cpu, secondary_func_t func)
 {
-	struct secondary_data *sp = alloc_pages(1) + SZ_8K - 16;
-	phys_addr_t sp_phys;
+	void *sp_mem = (void *)cpus[cpu].sp;
+	struct secondary_data *data;
 	struct sbiret ret;
 
-	sp -= sizeof(struct secondary_data);
-	sp->satp = csr_read(CSR_SATP);
-	sp->stvec = csr_read(CSR_STVEC);
-	sp->func = func;
+	if (!sp_mem) {
+		phys_addr_t sp_phys;
+
+		sp_mem = alloc_pages(1) + SZ_8K - 16;
+		sp_phys = virt_to_phys(sp_mem);
+		cpus[cpu].sp = __pa(sp_phys);
 
-	sp_phys = virt_to_phys(sp);
-	assert(sp_phys == __pa(sp_phys));
+		assert(sp_phys == cpus[cpu].sp);
+	}
 
-	ret = sbi_hart_start(cpus[cpu].hartid, (unsigned long)&secondary_entry, __pa(sp_phys));
+	sp_mem -= sizeof(struct secondary_data);
+	data = (struct secondary_data *)sp_mem;
+	data->satp = csr_read(CSR_SATP);
+	data->stvec = csr_read(CSR_STVEC);
+	data->func = func;
+
+	ret = sbi_hart_start(cpus[cpu].hartid, (unsigned long)&secondary_entry, cpus[cpu].sp);
 	assert(ret.error == SBI_SUCCESS);
 }
 
 void smp_boot_secondary(int cpu, void (*func)(void))
 {
-	int ret = cpumask_test_and_set_cpu(cpu, &cpu_started);
+	struct sbiret ret;
 
-	assert_msg(!ret, "CPU%d already boot once", cpu);
+	do {
+		ret = sbi_hart_get_status(cpus[cpu].hartid);
+		assert(!ret.error);
+	} while (ret.value == SBI_EXT_HSM_STOP_PENDING);
+
+	assert_msg(ret.value == SBI_EXT_HSM_STOPPED, "CPU%d is not stopped", cpu);
 	__smp_boot_secondary(cpu, func);
 
 	while (!cpu_online(cpu))
@@ -66,10 +77,18 @@ void smp_boot_secondary(int cpu, void (*func)(void))
 
 void smp_boot_secondary_nofail(int cpu, void (*func)(void))
 {
-	int ret = cpumask_test_and_set_cpu(cpu, &cpu_started);
+	struct sbiret ret;
+
+	do {
+		ret = sbi_hart_get_status(cpus[cpu].hartid);
+		assert(!ret.error);
+	} while (ret.value == SBI_EXT_HSM_STOP_PENDING);
 
-	if (!ret)
+	if (ret.value == SBI_EXT_HSM_STOPPED)
 		__smp_boot_secondary(cpu, func);
+	else
+		assert_msg(ret.value == SBI_EXT_HSM_START_PENDING || ret.value == SBI_EXT_HSM_STARTED,
+			   "CPU%d is in an unexpected state %ld", cpu, ret.value);
 
 	while (!cpu_online(cpu))
 		smp_wait_for_event();
diff --git a/riscv/cstart.S b/riscv/cstart.S
index 687173706d83..b7ee9b9c96b3 100644
--- a/riscv/cstart.S
+++ b/riscv/cstart.S
@@ -149,6 +149,7 @@ secondary_entry:
 	csrw	CSR_SSCRATCH, a0
 	mv	sp, a1
 	mv	fp, zero
+	addi	sp, sp, -SECONDARY_DATA_SIZE
 	REG_L	a0, SECONDARY_STVEC(sp)
 	csrw	CSR_STVEC, a0
 	mv	a0, sp
-- 
2.47.0

Re: [kvm-unit-tests PATCH 0/4] riscv: A few SMP fixes

[Andrew Jones]

On Wed, Oct 23, 2024 at 03:21:31PM +0200, Andrew Jones wrote:
> tl;dr - these patches are improvement to the riscv framework to better
> support the SBI HSM tests.
> 
> The first patch is a simpler alternative to [1] and [2] which doesn't
> require us to decide how best to make the number configurable. The
> second patch just adds sanity checking to make sure we can expect the
> SBI implementation to accept all hartids mapped from the present mask.
> The third patch was already posted once before[3] with a slightly
> different summary. It and the last patch improve smp_boot_secondary()
> since the SBI HSM tests were attempting to make workarounds for odd
> behaviors.
> 
> [1] https://lore.kernel.org/all/20240820170150.377580-2-andrew.jones@linux.dev/
> [2] https://lore.kernel.org/all/20240903143946.834864-6-andrew.jones@linux.dev/
> [3] https://lore.kernel.org/all/20240904120812.1798715-2-andrew.jones@linux.dev/
> 
> Andrew Jones (4):
>   riscv: Bump NR_CPUS to 256
>   riscv: Filter unmanaged harts from present mask
>   riscv: Fix secondary_entry
>   riscv: Rework smp_boot_secondary
> 
>  lib/riscv/asm/processor.h |  1 +
>  lib/riscv/asm/setup.h     |  2 +-
>  lib/riscv/setup.c         | 11 ++++++---
>  lib/riscv/smp.c           | 49 +++++++++++++++++++++++++++------------
>  riscv/cstart.S            |  7 +++---
>  5 files changed, 48 insertions(+), 22 deletions(-)
> 
> -- 
> 2.47.0

Merged to master through riscv/queue.

Thanks,
drew