| Message ID | 20240825170824.107467-5-jamestiotio@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: sbi: Add support to test HSM extension | expand |
On Mon, Aug 26, 2024 at 01:08:24AM GMT, James Raphael Tiovalen wrote: > Add some tests for all of the HSM extension functions. These tests > ensure that the HSM extension functions follow the behavior as described > in the SBI specification. > > Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com> > --- > riscv/Makefile | 7 +- > riscv/sbi-asm.S | 79 ++++++++++ > riscv/sbi.c | 382 ++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 465 insertions(+), 3 deletions(-) > create mode 100644 riscv/sbi-asm.S > > diff --git a/riscv/Makefile b/riscv/Makefile > index 179a373d..548041ad 100644 > --- a/riscv/Makefile > +++ b/riscv/Makefile > @@ -43,6 +43,7 @@ cflatobjs += lib/riscv/timer.o > ifeq ($(ARCH),riscv32) > cflatobjs += lib/ldiv32.o > endif > +cflatobjs += riscv/sbi-asm.o > > ######################################## > > @@ -99,7 +100,7 @@ cflatobjs += lib/efi.o > .PRECIOUS: %.so > > %.so: EFI_LDFLAGS += -defsym=EFI_SUBSYSTEM=0xa --no-undefined > -%.so: %.o $(FLATLIBS) $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds $(cstart.o) %.aux.o > +%.so: %.o $(FLATLIBS) $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds $(cstart.o) $(sbi-asm.o) %.aux.o Something left over from the previous version? We don't have $(sbi-asm.o) anymore. > $(LD) $(EFI_LDFLAGS) -o $@ -T $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds \ > $(filter %.o, $^) $(FLATLIBS) $(EFI_LIBS) > > @@ -115,7 +116,7 @@ cflatobjs += lib/efi.o > -O binary $^ $@ > else > %.elf: LDFLAGS += -pie -n -z notext > -%.elf: %.o $(FLATLIBS) $(SRCDIR)/riscv/flat.lds $(cstart.o) %.aux.o > +%.elf: %.o $(FLATLIBS) $(SRCDIR)/riscv/flat.lds $(cstart.o) $(sbi-asm.o) %.aux.o same > $(LD) $(LDFLAGS) -o $@ -T $(SRCDIR)/riscv/flat.lds \ > $(filter %.o, $^) $(FLATLIBS) > @chmod a-x $@ > @@ -127,7 +128,7 @@ else > endif > > generated-files = $(asm-offsets) > -$(tests:.$(exe)=.o) $(cstart.o) $(cflatobjs): $(generated-files) > +$(tests:.$(exe)=.o) $(cstart.o) $(sbi-asm.o) $(cflatobjs): $(generated-files) same > > arch_clean: asm_offsets_clean > $(RM) $(TEST_DIR)/*.{o,flat,elf,so,efi,debug} \ > diff --git a/riscv/sbi-asm.S b/riscv/sbi-asm.S > new file mode 100644 > index 00000000..f31bc096 > --- /dev/null > +++ b/riscv/sbi-asm.S > @@ -0,0 +1,79 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Helper assembly code routines for RISC-V SBI extension tests. > + * > + * Copyright (C) 2024, James Raphael Tiovalen <jamestiotio@gmail.com> > + */ > +#define __ASSEMBLY__ > +#include <config.h> > +#include <asm/csr.h> > +#include <asm/page.h> > + > +#define SBI_HSM_TEST_DONE (1 << 0) > +#define SBI_HSM_TEST_SATP (1 << 1) > +#define SBI_HSM_TEST_SIE (1 << 2) > +#define SBI_HSM_TEST_HARTID_A1 (1 << 3) > + > +.section .text > +.balign 4 > +.global sbi_hsm_check_hart_start > +sbi_hsm_check_hart_start: > + li t0, 0 I see this is a faithful copy of the code I suggested in the last review, but this is a pointless load of zero to t0, it's not used and then gets overwritten a few lines down. > + csrr t1, CSR_SATP > + bnez t1, 1f > + li t0, SBI_HSM_TEST_SATP > +1: csrr t1, CSR_SSTATUS > + andi t1, t1, SR_SIE > + bnez t1, 2f > + ori t0, t0, SBI_HSM_TEST_SIE > +2: bne a0, a1, 3f > + ori t0, t0, SBI_HSM_TEST_HARTID_A1 > +3: ori t0, t0, SBI_HSM_TEST_DONE > + la t1, sbi_hsm_hart_start_checks > + add t1, t1, a0 > + sb t0, 0(t1) > +4: la t0, sbi_hsm_stop_hart > + add t0, t0, a0 > + lb t0, 0(t0) And no need to repeat the address construction, just use another register la t0, sbi_hsm_stop_hart add t1, t0, a0 4: lb t0, 0(t1) > + pause > + beqz t0, 4b > + li a7, 0x48534d /* SBI_EXT_HSM */ > + li a6, 1 /* SBI_EXT_HSM_HART_STOP */ > + ecall > + j halt > + > +.balign 4 > +.global sbi_hsm_check_non_retentive_suspend > +sbi_hsm_check_non_retentive_suspend: > + li t0, 0 > + csrr t1, CSR_SATP > + bnez t1, 1f > + li t0, SBI_HSM_TEST_SATP > +1: csrr t1, CSR_SSTATUS > + andi t1, t1, SR_SIE > + bnez t1, 2f > + ori t0, t0, SBI_HSM_TEST_SIE > +2: bne a0, a1, 3f > + ori t0, t0, SBI_HSM_TEST_HARTID_A1 > +3: ori t0, t0, SBI_HSM_TEST_DONE > + la t1, sbi_hsm_non_retentive_hart_suspend_checks > + add t1, t1, a0 > + sb t0, 0(t1) > +4: la t0, sbi_hsm_stop_hart > + add t0, t0, a0 > + lb t0, 0(t0) > + pause > + beqz t0, 4b > + li a7, 0x48534d /* SBI_EXT_HSM */ > + li a6, 1 /* SBI_EXT_HSM_HART_STOP */ > + ecall > + j halt This is identical, except the address to load for the results, to the function above. Just make a static helper function that puts that address in a register, e.g. t6, and then call it from both /* t6 is the address of the results array */ sbi_hsm_check: ... .global sbi_hsm_check_hart_start sbi_hsm_check_non_retentive_suspend: la t6, sbi_hsm_non_retentive_hart_suspend_checks j sbi_hsm_check .global sbi_hsm_check_non_retentive_suspend sbi_hsm_check_non_retentive_suspend: la t6, sbi_hsm_non_retentive_hart_suspend_checks j sbi_hsm_check > + > +.section .data > +.balign PAGE_SIZE > +.global sbi_hsm_hart_start_checks > +sbi_hsm_hart_start_checks: .space CONFIG_NR_CPUS > +.global sbi_hsm_non_retentive_hart_suspend_checks > +sbi_hsm_non_retentive_hart_suspend_checks: .space CONFIG_NR_CPUS > +.global sbi_hsm_stop_hart > +sbi_hsm_stop_hart: .space CONFIG_NR_CPUS I don't think it should be necessary to create these arrays in assembly. We should be able to make global arrays in C in riscv/sbi.c and still access them from the assembly as you've done. CONFIG_NR_CPUS will support all possible cpuids, but hartids have their own range and the code above is indexing these arrays by hartid. Since we should be able to define the arrays in C, then we could also either 1) assert that max_hartid + 1 <= NR_CPUS 2) dynamically allocate the arrays using max_hartid + 1 for the size and then assign global variables the physical addresses of those allocated regions to use in the assembly (1) is probably good enough > diff --git a/riscv/sbi.c b/riscv/sbi.c > index 6469304b..25fc2e81 100644 > --- a/riscv/sbi.c > +++ b/riscv/sbi.c > @@ -6,6 +6,8 @@ > */ > #include <libcflat.h> > #include <alloc_page.h> > +#include <cpumask.h> > +#include <on-cpus.h> > #include <stdlib.h> > #include <string.h> > #include <limits.h> > @@ -17,8 +19,10 @@ > #include <asm/io.h> > #include <asm/isa.h> > #include <asm/mmu.h> > +#include <asm/page.h> > #include <asm/processor.h> > #include <asm/sbi.h> > +#include <asm/setup.h> > #include <asm/smp.h> > #include <asm/timer.h> > > @@ -425,6 +429,383 @@ static void check_dbcn(void) > report_prefix_pop(); > } > > +#define SBI_HSM_TEST_DONE (1 << 0) > +#define SBI_HSM_TEST_SATP (1 << 1) > +#define SBI_HSM_TEST_SIE (1 << 2) > +#define SBI_HSM_TEST_HARTID_A1 (1 << 3) We should create a header for these devices, riscv/sbi.h, and share them with the asm file rather than duplicate them. > + > +static cpumask_t cpus_alive_after_start; > +static cpumask_t cpus_alive_after_retentive_suspend; > +extern void sbi_hsm_check_hart_start(void); > +extern void sbi_hsm_check_non_retentive_suspend(void); > +extern unsigned char sbi_hsm_stop_hart[NR_CPUS]; > +extern unsigned char sbi_hsm_hart_start_checks[NR_CPUS]; > +extern unsigned char sbi_hsm_non_retentive_hart_suspend_checks[NR_CPUS]; need blank line here > +static void on_secondary_cpus_async(void (*func)(void *data), void *data) > +{ > + int cpu, me = smp_processor_id(); > + > + for_each_present_cpu(cpu) { > + if (cpu == me) > + continue; > + on_cpu_async(cpu, func, data); > + } > +} Seems on-cpus is missing an API for "all, but ...". How about, as a separate patch, adding void on_cpus_async(cpumask_t *mask, void (*func)(void *data), void *data) to lib/on-cpus.c Then here you'd copy the present mask to your own mask and clear 'me' from it for an 'all present, but me' mask. > + > +static bool cpumask_test_secondary_cpus(cpumask_t *mask) > +{ > + int cpu, me = smp_processor_id(); > + > + for_each_present_cpu(cpu) { > + if (cpu == me) > + continue; > + > + if (!cpumask_test_cpu(cpu, mask)) > + return false; > + } > + > + return true; > +} This is just cpumask_weight(mask) == nr_cpus - 1 or !cpumask_test_cpu(me, mask) && cpumask_weight(mask) == nr_cpus - 1 if there's concern for 'me' being set in the mask erroneously. > + > +static void hart_execute(void *data) > +{ > + int me = smp_processor_id(); > + > + cpumask_set_cpu(me, &cpus_alive_after_start); > +} > + > +static void hart_retentive_suspend(void *data) > +{ > + int me = smp_processor_id(); > + unsigned long hartid = current_thread_info()->hartid; > + struct sbiret ret = sbi_hart_suspend(SBI_EXT_HSM_HART_SUSPEND_RETENTIVE, __pa(NULL), __pa(NULL)); __pa(NULL) is just 0 and I don't think __pa(NULL) tells the reader anything useful. > + > + if (ret.error) > + report_fail("failed to retentive suspend hart %ld", hartid); Should output ret.error too > + else > + cpumask_set_cpu(me, &cpus_alive_after_retentive_suspend); This mask is redundant with the cpu_idle_mask. > +} > + > +static void hart_non_retentive_suspend(void *data) > +{ > + unsigned long hartid = current_thread_info()->hartid; > + Put a comment here explaining why opaque must be hartid, i.e. for the a0==a1 test which ensures a0 is hartid and a1 is opaque per the spec. > + struct sbiret ret = sbi_hart_suspend(SBI_EXT_HSM_HART_SUSPEND_NON_RETENTIVE, > + virt_to_phys(&sbi_hsm_check_non_retentive_suspend), hartid); > + > + if (ret.error) Drop the 'if'. Any return from a non-retentive suspend is a failure. > + report_fail("failed to non-retentive suspend hart %ld", hartid); Should output ret.error too > +} > + > +static void hart_wait_on_status(unsigned long hartid, enum sbi_ext_hsm_sid status) > +{ > + struct sbiret ret = sbi_hart_get_status(hartid); > + > + while (!ret.error && ret.value == status) > + ret = sbi_hart_get_status(hartid); Let's throw a cpu_relax() in the loop too. > + > + if (ret.error) > + report_fail("got %ld while waiting on status %u for hart %lx\n", ret.error, status, hartid); > +} > + > +static void check_hsm(void) > +{ > + struct sbiret ret; > + unsigned long hartid; > + unsigned char per_hart_start_checks, per_hart_non_retentive_suspend_checks; > + unsigned long hart_mask[NR_CPUS / BITS_PER_LONG] = {0}; Use a real cpumask. When passing a single ulong to an SBI call you can simply use cpumask_bits(mask)[] > + bool ipi_failed = false; > + int cpu, me = smp_processor_id(); > + > + report_prefix_push("hsm"); > + > + if (!sbi_probe(SBI_EXT_HSM)) { > + report_skip("hsm extension not available"); > + report_prefix_pop(); > + return; > + } > + > + report_prefix_push("hart_get_status"); > + > + hartid = current_thread_info()->hartid; > + ret = sbi_hart_get_status(hartid); > + > + if (ret.error == SBI_ERR_INVALID_PARAM) { Any error is a failure, so this should be if (ret.error) and we should output the error in the fail message. > + report_fail("current hartid is invalid"); > + report_prefix_popn(2); > + return; > + } else if (ret.value != SBI_EXT_HSM_STARTED) { > + report_fail("current hart is not started"); need to output ret.value > + report_prefix_popn(2); > + return; > + } > + > + report_pass("status of current hart is started"); > + > + report_prefix_pop(); > + > + report_prefix_push("hart_start"); > + > + ret = sbi_hart_start(hartid, virt_to_phys(&hart_execute), __pa(NULL)); > + report(ret.error == SBI_ERR_ALREADY_AVAILABLE, "boot hart is already started"); If we don't get the expected error of 'already started', then the test could hang here. A test like this is best to do on a secondary, passing the results back to the primary through a global. > + > + ret = sbi_hart_start(ULONG_MAX, virt_to_phys(&hart_execute), __pa(NULL)); > + report(ret.error == SBI_ERR_INVALID_PARAM, "invalid hartid check"); > + > + if (nr_cpus < 2) { > + report_skip("no other cpus to run the remaining hsm tests on"); > + report_prefix_popn(2); > + return; > + } > + > + for_each_present_cpu(cpu) { > + if (cpu == me) > + continue; > + > + hartid = cpus[cpu].hartid; Need the same why opaque must be hartid comment here as above. > + ret = sbi_hart_start(hartid, virt_to_phys(&sbi_hsm_check_hart_start), hartid); > + remove this blank line > + if (ret.error) { > + report_fail("failed to start test hart %ld", hartid); output ret.error > + report_prefix_popn(2); > + return; > + } > + } > + > + for_each_present_cpu(cpu) { > + if (cpu == me) > + continue; > + > + hartid = cpus[cpu].hartid; > + > + hart_wait_on_status(hartid, SBI_EXT_HSM_STOPPED); > + hart_wait_on_status(hartid, SBI_EXT_HSM_START_PENDING); > + ret = sbi_hart_get_status(hartid); > + report(!ret.error && ret.value == SBI_EXT_HSM_STARTED, > + "test hart with hartid %ld successfully started", hartid); We should split the reporting to check ret.error and ret.value separately. > + } > + > + for_each_present_cpu(cpu) { > + if (cpu == me) > + continue; > + > + hartid = cpus[cpu].hartid; > + > + while (!((per_hart_start_checks = READ_ONCE(sbi_hsm_hart_start_checks[hartid])) > + & SBI_HSM_TEST_DONE)) This while condition would be a bit less ugly if we dropped per_hart_start_checks and then just used hartid to index the array in the three checks below. > + cpu_relax(); > + > + report(per_hart_start_checks & SBI_HSM_TEST_SATP, > + "satp is zero for test hart %ld", hartid); > + report(per_hart_start_checks & SBI_HSM_TEST_SIE, > + "sstatus.SIE is zero for test hart %ld", hartid); > + report(per_hart_start_checks & SBI_HSM_TEST_HARTID_A1, > + "a0 and a1 are hartid for test hart %ld", hartid); > + } > + > + report_prefix_pop(); > + > + report_prefix_push("hart_stop"); > + > + for_each_present_cpu(cpu) { > + if (cpu == me) > + continue; > + > + hartid = cpus[cpu].hartid; > + WRITE_ONCE(sbi_hsm_stop_hart[hartid], true); instead of the loop we could do barrier(); memset(...); barrier(); if we don't mind setting the 'me' hart too. > + } > + > + for_each_present_cpu(cpu) { > + if (cpu == me) > + continue; > + > + hartid = cpus[cpu].hartid; > + hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED); > + hart_wait_on_status(hartid, SBI_EXT_HSM_STOP_PENDING); > + ret = sbi_hart_get_status(hartid); > + report(!ret.error && (ret.value == SBI_EXT_HSM_STOPPED), unnecessary (), anyway we should split the check for ret.error and ret.value and output ret.error/value. See the DBCN test for how we do it. For example, the write_byte test has report(ret.error == SBI_SUCCESS, "write success (error=%ld)", ret.error); report(ret.value == 0, "expected ret.value (%ld)", ret.value); > + "test hart %ld successfully stopped", hartid); > + } > + > + /* Reset the stop flags so that we can reuse them after suspension tests */ > + for_each_present_cpu(cpu) { > + if (cpu == me) > + continue; > + > + hartid = cpus[cpu].hartid; > + WRITE_ONCE(sbi_hsm_stop_hart[hartid], false); instead of the loop we could do barrier(); memset(...); barrier(); since we definitely we don't mind setting the 'me' hart too as it's getting set to zero. > + } > + > + report_prefix_pop(); > + > + report_prefix_push("hart_start"); > + > + on_secondary_cpus_async(hart_execute, NULL); > + > + for_each_present_cpu(cpu) { > + if (cpu == me) > + continue; > + > + hartid = cpus[cpu].hartid; > + hart_wait_on_status(hartid, SBI_EXT_HSM_STOPPED); > + hart_wait_on_status(hartid, SBI_EXT_HSM_START_PENDING); > + ret = sbi_hart_get_status(hartid); > + report(!ret.error && (ret.value == SBI_EXT_HSM_STARTED), > + "new hart with hartid %ld successfully started", hartid); split error/value tests > + } > + > + while (!cpumask_test_secondary_cpus(&cpu_idle_mask)) while (cpumask_weight(&cpu_idle_mask) != nr_cpus - 1) > + cpu_relax(); We could get stuck in this loop forever if the cpus don't come up, so you may want to set a timeout with timer_start(), then either handler the failure due to the timeout handler firing or stop the timer, so timer_start(long_enough_duration); while (cpumask_weight(&cpu_idle_mask) != nr_cpus - 1) cpu_relax(); timer_stop(); if (timer_fired) report_fail(...) > + > + report(cpumask_full(&cpu_online_mask), "all cpus online"); > + report(cpumask_test_secondary_cpus(&cpus_alive_after_start), > + "all secondary harts successfully executed code after start"); This test is covered by the idle mask check above. hart_execute() could just be an empty function > + > + report_prefix_pop(); > + > + report_prefix_push("hart_suspend"); > + > + if (sbi_probe(SBI_EXT_IPI)) { > + on_secondary_cpus_async(hart_retentive_suspend, NULL); > + > + for_each_present_cpu(cpu) { > + if (cpu == me) > + continue; > + > + hartid = cpus[cpu].hartid; > + hart_mask[hartid / BITS_PER_LONG] |= 1UL << hartid; > + hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED); > + hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPEND_PENDING); > + ret = sbi_hart_get_status(hartid); > + report(!ret.error && (ret.value == SBI_EXT_HSM_SUSPENDED), split report > + "hart %ld successfully retentive suspended", hartid); > + } > + > + for (int i = 0; i < NR_CPUS / BITS_PER_LONG; ++i) { > + if (hart_mask[i]) { using a real mask for hart_mask would allow using for_each_cpu(i, &hart_mask) > + ret = sbi_send_ipi(hart_mask[i], i * BITS_PER_LONG); but, instead, let's provide the following (untested) function in lib/riscv/sbi.c struct sbiret sbi_send_ipi_cpumask(cpumask_t *mask) { struct sbiret ret; for (int i = 0; i < CPUMASK_NR_LONGS; ++i) { if (cpumask_bits(mask)[i]) { ret = sbi_send_ipi(cpumask_bits(mask)[i], i * BITS_PER_LONG); if (ret.error) break; } } return ret; } then you can drop your loop and just call the new API. > + if (ret.error) { > + ipi_failed = true; > + report_fail("got %ld when sending ipi to retentive suspended harts", > + ret.error); > + break; > + } > + } > + } > + > + if (!ipi_failed) { > + for_each_present_cpu(cpu) { > + if (cpu == me) > + continue; > + > + hartid = cpus[cpu].hartid; > + hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPENDED); > + hart_wait_on_status(hartid, SBI_EXT_HSM_RESUME_PENDING); > + ret = sbi_hart_get_status(hartid); > + report(!ret.error && (ret.value == SBI_EXT_HSM_STARTED), split report > + "hart %ld successfully retentive resumed", hartid); > + } > + > + while (!cpumask_test_secondary_cpus(&cpu_idle_mask)) > + cpu_relax(); Timed wait and use cpumask_weight > + > + report(cpumask_full(&cpu_online_mask), "all cpus online"); > + report(cpumask_test_secondary_cpus(&cpus_alive_after_retentive_suspend), > + "all secondary harts successfully executed code after retentive suspend"); Already checked with the wait on idle. > + } > + > + /* Reset the ipi_failed flag so that we can reuse it for non-retentive suspension tests */ > + ipi_failed = false; > + > + on_secondary_cpus_async(hart_non_retentive_suspend, NULL); > + > + for_each_present_cpu(cpu) { > + if (cpu == me) > + continue; > + > + hartid = cpus[cpu].hartid; > + hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED); > + hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPEND_PENDING); > + ret = sbi_hart_get_status(hartid); > + report(!ret.error && (ret.value == SBI_EXT_HSM_SUSPENDED), split errors > + "hart %ld successfully non-retentive suspended", hartid); > + } > + > + for (int i = 0; i < NR_CPUS / BITS_PER_LONG; ++i) { > + if (hart_mask[i]) { use real mask > + ret = sbi_send_ipi(hart_mask[i], i * BITS_PER_LONG); > + if (ret.error) { > + ipi_failed = true; > + report_fail("got %ld when sending ipi to non-retentive suspended harts", > + ret.error); > + break; > + } > + } > + } > + > + if (!ipi_failed) { > + for_each_present_cpu(cpu) { > + if (cpu == me) > + continue; > + > + hartid = cpus[cpu].hartid; > + hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPENDED); > + hart_wait_on_status(hartid, SBI_EXT_HSM_RESUME_PENDING); > + ret = sbi_hart_get_status(hartid); > + report(!ret.error && (ret.value == SBI_EXT_HSM_STARTED), split errors > + "hart %ld successfully non-retentive resumed", hartid); > + } > + > + for_each_present_cpu(cpu) { > + if (cpu == me) > + continue; > + > + hartid = cpus[cpu].hartid; > + > + while (!((per_hart_non_retentive_suspend_checks = > + READ_ONCE(sbi_hsm_non_retentive_hart_suspend_checks[hartid])) > + & SBI_HSM_TEST_DONE)) Could drop the local variable and index the array below. > + cpu_relax(); > + > + report(per_hart_non_retentive_suspend_checks & SBI_HSM_TEST_SATP, > + "satp is zero for test hart %ld", hartid); > + report(per_hart_non_retentive_suspend_checks & SBI_HSM_TEST_SIE, > + "sstatus.SIE is zero for test hart %ld", hartid); > + report(per_hart_non_retentive_suspend_checks & SBI_HSM_TEST_HARTID_A1, > + "a0 and a1 are hartid for test hart %ld", hartid); > + } > + > + report_prefix_pop(); > + > + report_prefix_push("hart_stop"); > + > + for_each_present_cpu(cpu) { > + if (cpu == me) > + continue; > + > + hartid = cpus[cpu].hartid; > + WRITE_ONCE(sbi_hsm_stop_hart[hartid], true); barrier, memset, barrier? > + } > + > + for_each_present_cpu(cpu) { > + if (cpu == me) > + continue; > + > + hartid = cpus[cpu].hartid; > + hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED); > + hart_wait_on_status(hartid, SBI_EXT_HSM_STOP_PENDING); > + ret = sbi_hart_get_status(hartid); > + report(!ret.error && (ret.value == SBI_EXT_HSM_STOPPED), split errors > + "test hart %ld successfully stopped", hartid); > + } > + } We can avoid having to add a level of indention to all the code that depends on SBI_EXT_IPI by either putting it all in a function and then calling that when IPI is available or by doing if (!sbi_probe(SBI_EXT_IPI)) { report_skip("skipping suspension tests since ipi extension is unavailable"); return; } since all these tests are currently the last in this function. > + } else { > + report_skip("skipping suspension tests since ipi extension is unavailable"); > + } > + > + report_prefix_popn(2); > +} > + > int main(int argc, char **argv) > { > if (argc > 1 && !strcmp(argv[1], "-h")) { > @@ -435,6 +816,7 @@ int main(int argc, char **argv) > report_prefix_push("sbi"); > check_base(); > check_time(); > + check_hsm(); > check_dbcn(); > > return report_summary(); > -- > 2.43.0 > Thanks, drew
On Thu, Aug 29, 2024 at 03:30:26PM GMT, Andrew Jones wrote: > On Mon, Aug 26, 2024 at 01:08:24AM GMT, James Raphael Tiovalen wrote: ... > > +.section .data > > +.balign PAGE_SIZE > > +.global sbi_hsm_hart_start_checks > > +sbi_hsm_hart_start_checks: .space CONFIG_NR_CPUS > > +.global sbi_hsm_non_retentive_hart_suspend_checks > > +sbi_hsm_non_retentive_hart_suspend_checks: .space CONFIG_NR_CPUS > > +.global sbi_hsm_stop_hart > > +sbi_hsm_stop_hart: .space CONFIG_NR_CPUS > > I don't think it should be necessary to create these arrays in assembly. > We should be able to make global arrays in C in riscv/sbi.c and still > access them from the assembly as you've done. > > CONFIG_NR_CPUS will support all possible cpuids, but hartids have their > own range and the code above is indexing these arrays by hartid. Since > we should be able to define the arrays in C, then we could also either > > 1) assert that max_hartid + 1 <= NR_CPUS > 2) dynamically allocate the arrays using max_hartid + 1 for the size > and then assign global variables the physical addresses of those > allocated regions to use in the assembly > > (1) is probably good enough Actually we have to do (1) unless we want to open a big can of worms because we're currently shoehorning hartids into cpumasks, but cpumasks are based on NR_CPUS for size. To do it right, we should have hartmasks, but they may be very large and/or sparse. > > Seems on-cpus is missing an API for "all, but ...". How about, as a > separate patch, adding > > void on_cpus_async(cpumask_t *mask, void (*func)(void *data), void *data) > > to lib/on-cpus.c > > Then here you'd copy the present mask to your own mask and clear 'me' from > it for an 'all present, but me' mask. I'll write a patch for on_cpus_async() now. > but, instead, let's provide the following (untested) function in lib/riscv/sbi.c > > struct sbiret sbi_send_ipi_cpumask(cpumask_t *mask) > { > struct sbiret ret; > > for (int i = 0; i < CPUMASK_NR_LONGS; ++i) { > if (cpumask_bits(mask)[i]) { > ret = sbi_send_ipi(cpumask_bits(mask)[i], i * BITS_PER_LONG); > if (ret.error) > break; > } > } > > return ret; > } > I'll write a patch adding this now too. Thanks, drew
diff --git a/riscv/Makefile b/riscv/Makefile index 179a373d..548041ad 100644 --- a/riscv/Makefile +++ b/riscv/Makefile @@ -43,6 +43,7 @@ cflatobjs += lib/riscv/timer.o ifeq ($(ARCH),riscv32) cflatobjs += lib/ldiv32.o endif +cflatobjs += riscv/sbi-asm.o ######################################## @@ -99,7 +100,7 @@ cflatobjs += lib/efi.o .PRECIOUS: %.so %.so: EFI_LDFLAGS += -defsym=EFI_SUBSYSTEM=0xa --no-undefined -%.so: %.o $(FLATLIBS) $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds $(cstart.o) %.aux.o +%.so: %.o $(FLATLIBS) $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds $(cstart.o) $(sbi-asm.o) %.aux.o $(LD) $(EFI_LDFLAGS) -o $@ -T $(SRCDIR)/riscv/efi/elf_riscv64_efi.lds \ $(filter %.o, $^) $(FLATLIBS) $(EFI_LIBS) @@ -115,7 +116,7 @@ cflatobjs += lib/efi.o -O binary $^ $@ else %.elf: LDFLAGS += -pie -n -z notext -%.elf: %.o $(FLATLIBS) $(SRCDIR)/riscv/flat.lds $(cstart.o) %.aux.o +%.elf: %.o $(FLATLIBS) $(SRCDIR)/riscv/flat.lds $(cstart.o) $(sbi-asm.o) %.aux.o $(LD) $(LDFLAGS) -o $@ -T $(SRCDIR)/riscv/flat.lds \ $(filter %.o, $^) $(FLATLIBS) @chmod a-x $@ @@ -127,7 +128,7 @@ else endif generated-files = $(asm-offsets) -$(tests:.$(exe)=.o) $(cstart.o) $(cflatobjs): $(generated-files) +$(tests:.$(exe)=.o) $(cstart.o) $(sbi-asm.o) $(cflatobjs): $(generated-files) arch_clean: asm_offsets_clean $(RM) $(TEST_DIR)/*.{o,flat,elf,so,efi,debug} \ diff --git a/riscv/sbi-asm.S b/riscv/sbi-asm.S new file mode 100644 index 00000000..f31bc096 --- /dev/null +++ b/riscv/sbi-asm.S @@ -0,0 +1,79 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Helper assembly code routines for RISC-V SBI extension tests. + * + * Copyright (C) 2024, James Raphael Tiovalen <jamestiotio@gmail.com> + */ +#define __ASSEMBLY__ +#include <config.h> +#include <asm/csr.h> +#include <asm/page.h> + +#define SBI_HSM_TEST_DONE (1 << 0) +#define SBI_HSM_TEST_SATP (1 << 1) +#define SBI_HSM_TEST_SIE (1 << 2) +#define SBI_HSM_TEST_HARTID_A1 (1 << 3) + +.section .text +.balign 4 +.global sbi_hsm_check_hart_start +sbi_hsm_check_hart_start: + li t0, 0 + csrr t1, CSR_SATP + bnez t1, 1f + li t0, SBI_HSM_TEST_SATP +1: csrr t1, CSR_SSTATUS + andi t1, t1, SR_SIE + bnez t1, 2f + ori t0, t0, SBI_HSM_TEST_SIE +2: bne a0, a1, 3f + ori t0, t0, SBI_HSM_TEST_HARTID_A1 +3: ori t0, t0, SBI_HSM_TEST_DONE + la t1, sbi_hsm_hart_start_checks + add t1, t1, a0 + sb t0, 0(t1) +4: la t0, sbi_hsm_stop_hart + add t0, t0, a0 + lb t0, 0(t0) + pause + beqz t0, 4b + li a7, 0x48534d /* SBI_EXT_HSM */ + li a6, 1 /* SBI_EXT_HSM_HART_STOP */ + ecall + j halt + +.balign 4 +.global sbi_hsm_check_non_retentive_suspend +sbi_hsm_check_non_retentive_suspend: + li t0, 0 + csrr t1, CSR_SATP + bnez t1, 1f + li t0, SBI_HSM_TEST_SATP +1: csrr t1, CSR_SSTATUS + andi t1, t1, SR_SIE + bnez t1, 2f + ori t0, t0, SBI_HSM_TEST_SIE +2: bne a0, a1, 3f + ori t0, t0, SBI_HSM_TEST_HARTID_A1 +3: ori t0, t0, SBI_HSM_TEST_DONE + la t1, sbi_hsm_non_retentive_hart_suspend_checks + add t1, t1, a0 + sb t0, 0(t1) +4: la t0, sbi_hsm_stop_hart + add t0, t0, a0 + lb t0, 0(t0) + pause + beqz t0, 4b + li a7, 0x48534d /* SBI_EXT_HSM */ + li a6, 1 /* SBI_EXT_HSM_HART_STOP */ + ecall + j halt + +.section .data +.balign PAGE_SIZE +.global sbi_hsm_hart_start_checks +sbi_hsm_hart_start_checks: .space CONFIG_NR_CPUS +.global sbi_hsm_non_retentive_hart_suspend_checks +sbi_hsm_non_retentive_hart_suspend_checks: .space CONFIG_NR_CPUS +.global sbi_hsm_stop_hart +sbi_hsm_stop_hart: .space CONFIG_NR_CPUS diff --git a/riscv/sbi.c b/riscv/sbi.c index 6469304b..25fc2e81 100644 --- a/riscv/sbi.c +++ b/riscv/sbi.c @@ -6,6 +6,8 @@ */ #include <libcflat.h> #include <alloc_page.h> +#include <cpumask.h> +#include <on-cpus.h> #include <stdlib.h> #include <string.h> #include <limits.h> @@ -17,8 +19,10 @@ #include <asm/io.h> #include <asm/isa.h> #include <asm/mmu.h> +#include <asm/page.h> #include <asm/processor.h> #include <asm/sbi.h> +#include <asm/setup.h> #include <asm/smp.h> #include <asm/timer.h> @@ -425,6 +429,383 @@ static void check_dbcn(void) report_prefix_pop(); } +#define SBI_HSM_TEST_DONE (1 << 0) +#define SBI_HSM_TEST_SATP (1 << 1) +#define SBI_HSM_TEST_SIE (1 << 2) +#define SBI_HSM_TEST_HARTID_A1 (1 << 3) + +static cpumask_t cpus_alive_after_start; +static cpumask_t cpus_alive_after_retentive_suspend; +extern void sbi_hsm_check_hart_start(void); +extern void sbi_hsm_check_non_retentive_suspend(void); +extern unsigned char sbi_hsm_stop_hart[NR_CPUS]; +extern unsigned char sbi_hsm_hart_start_checks[NR_CPUS]; +extern unsigned char sbi_hsm_non_retentive_hart_suspend_checks[NR_CPUS]; +static void on_secondary_cpus_async(void (*func)(void *data), void *data) +{ + int cpu, me = smp_processor_id(); + + for_each_present_cpu(cpu) { + if (cpu == me) + continue; + on_cpu_async(cpu, func, data); + } +} + +static bool cpumask_test_secondary_cpus(cpumask_t *mask) +{ + int cpu, me = smp_processor_id(); + + for_each_present_cpu(cpu) { + if (cpu == me) + continue; + + if (!cpumask_test_cpu(cpu, mask)) + return false; + } + + return true; +} + +static void hart_execute(void *data) +{ + int me = smp_processor_id(); + + cpumask_set_cpu(me, &cpus_alive_after_start); +} + +static void hart_retentive_suspend(void *data) +{ + int me = smp_processor_id(); + unsigned long hartid = current_thread_info()->hartid; + struct sbiret ret = sbi_hart_suspend(SBI_EXT_HSM_HART_SUSPEND_RETENTIVE, __pa(NULL), __pa(NULL)); + + if (ret.error) + report_fail("failed to retentive suspend hart %ld", hartid); + else + cpumask_set_cpu(me, &cpus_alive_after_retentive_suspend); +} + +static void hart_non_retentive_suspend(void *data) +{ + unsigned long hartid = current_thread_info()->hartid; + + struct sbiret ret = sbi_hart_suspend(SBI_EXT_HSM_HART_SUSPEND_NON_RETENTIVE, + virt_to_phys(&sbi_hsm_check_non_retentive_suspend), hartid); + + if (ret.error) + report_fail("failed to non-retentive suspend hart %ld", hartid); +} + +static void hart_wait_on_status(unsigned long hartid, enum sbi_ext_hsm_sid status) +{ + struct sbiret ret = sbi_hart_get_status(hartid); + + while (!ret.error && ret.value == status) + ret = sbi_hart_get_status(hartid); + + if (ret.error) + report_fail("got %ld while waiting on status %u for hart %lx\n", ret.error, status, hartid); +} + +static void check_hsm(void) +{ + struct sbiret ret; + unsigned long hartid; + unsigned char per_hart_start_checks, per_hart_non_retentive_suspend_checks; + unsigned long hart_mask[NR_CPUS / BITS_PER_LONG] = {0}; + bool ipi_failed = false; + int cpu, me = smp_processor_id(); + + report_prefix_push("hsm"); + + if (!sbi_probe(SBI_EXT_HSM)) { + report_skip("hsm extension not available"); + report_prefix_pop(); + return; + } + + report_prefix_push("hart_get_status"); + + hartid = current_thread_info()->hartid; + ret = sbi_hart_get_status(hartid); + + if (ret.error == SBI_ERR_INVALID_PARAM) { + report_fail("current hartid is invalid"); + report_prefix_popn(2); + return; + } else if (ret.value != SBI_EXT_HSM_STARTED) { + report_fail("current hart is not started"); + report_prefix_popn(2); + return; + } + + report_pass("status of current hart is started"); + + report_prefix_pop(); + + report_prefix_push("hart_start"); + + ret = sbi_hart_start(hartid, virt_to_phys(&hart_execute), __pa(NULL)); + report(ret.error == SBI_ERR_ALREADY_AVAILABLE, "boot hart is already started"); + + ret = sbi_hart_start(ULONG_MAX, virt_to_phys(&hart_execute), __pa(NULL)); + report(ret.error == SBI_ERR_INVALID_PARAM, "invalid hartid check"); + + if (nr_cpus < 2) { + report_skip("no other cpus to run the remaining hsm tests on"); + report_prefix_popn(2); + return; + } + + for_each_present_cpu(cpu) { + if (cpu == me) + continue; + + hartid = cpus[cpu].hartid; + ret = sbi_hart_start(hartid, virt_to_phys(&sbi_hsm_check_hart_start), hartid); + + if (ret.error) { + report_fail("failed to start test hart %ld", hartid); + report_prefix_popn(2); + return; + } + } + + for_each_present_cpu(cpu) { + if (cpu == me) + continue; + + hartid = cpus[cpu].hartid; + + hart_wait_on_status(hartid, SBI_EXT_HSM_STOPPED); + hart_wait_on_status(hartid, SBI_EXT_HSM_START_PENDING); + ret = sbi_hart_get_status(hartid); + report(!ret.error && ret.value == SBI_EXT_HSM_STARTED, + "test hart with hartid %ld successfully started", hartid); + } + + for_each_present_cpu(cpu) { + if (cpu == me) + continue; + + hartid = cpus[cpu].hartid; + + while (!((per_hart_start_checks = READ_ONCE(sbi_hsm_hart_start_checks[hartid])) + & SBI_HSM_TEST_DONE)) + cpu_relax(); + + report(per_hart_start_checks & SBI_HSM_TEST_SATP, + "satp is zero for test hart %ld", hartid); + report(per_hart_start_checks & SBI_HSM_TEST_SIE, + "sstatus.SIE is zero for test hart %ld", hartid); + report(per_hart_start_checks & SBI_HSM_TEST_HARTID_A1, + "a0 and a1 are hartid for test hart %ld", hartid); + } + + report_prefix_pop(); + + report_prefix_push("hart_stop"); + + for_each_present_cpu(cpu) { + if (cpu == me) + continue; + + hartid = cpus[cpu].hartid; + WRITE_ONCE(sbi_hsm_stop_hart[hartid], true); + } + + for_each_present_cpu(cpu) { + if (cpu == me) + continue; + + hartid = cpus[cpu].hartid; + hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED); + hart_wait_on_status(hartid, SBI_EXT_HSM_STOP_PENDING); + ret = sbi_hart_get_status(hartid); + report(!ret.error && (ret.value == SBI_EXT_HSM_STOPPED), + "test hart %ld successfully stopped", hartid); + } + + /* Reset the stop flags so that we can reuse them after suspension tests */ + for_each_present_cpu(cpu) { + if (cpu == me) + continue; + + hartid = cpus[cpu].hartid; + WRITE_ONCE(sbi_hsm_stop_hart[hartid], false); + } + + report_prefix_pop(); + + report_prefix_push("hart_start"); + + on_secondary_cpus_async(hart_execute, NULL); + + for_each_present_cpu(cpu) { + if (cpu == me) + continue; + + hartid = cpus[cpu].hartid; + hart_wait_on_status(hartid, SBI_EXT_HSM_STOPPED); + hart_wait_on_status(hartid, SBI_EXT_HSM_START_PENDING); + ret = sbi_hart_get_status(hartid); + report(!ret.error && (ret.value == SBI_EXT_HSM_STARTED), + "new hart with hartid %ld successfully started", hartid); + } + + while (!cpumask_test_secondary_cpus(&cpu_idle_mask)) + cpu_relax(); + + report(cpumask_full(&cpu_online_mask), "all cpus online"); + report(cpumask_test_secondary_cpus(&cpus_alive_after_start), + "all secondary harts successfully executed code after start"); + + report_prefix_pop(); + + report_prefix_push("hart_suspend"); + + if (sbi_probe(SBI_EXT_IPI)) { + on_secondary_cpus_async(hart_retentive_suspend, NULL); + + for_each_present_cpu(cpu) { + if (cpu == me) + continue; + + hartid = cpus[cpu].hartid; + hart_mask[hartid / BITS_PER_LONG] |= 1UL << hartid; + hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED); + hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPEND_PENDING); + ret = sbi_hart_get_status(hartid); + report(!ret.error && (ret.value == SBI_EXT_HSM_SUSPENDED), + "hart %ld successfully retentive suspended", hartid); + } + + for (int i = 0; i < NR_CPUS / BITS_PER_LONG; ++i) { + if (hart_mask[i]) { + ret = sbi_send_ipi(hart_mask[i], i * BITS_PER_LONG); + if (ret.error) { + ipi_failed = true; + report_fail("got %ld when sending ipi to retentive suspended harts", + ret.error); + break; + } + } + } + + if (!ipi_failed) { + for_each_present_cpu(cpu) { + if (cpu == me) + continue; + + hartid = cpus[cpu].hartid; + hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPENDED); + hart_wait_on_status(hartid, SBI_EXT_HSM_RESUME_PENDING); + ret = sbi_hart_get_status(hartid); + report(!ret.error && (ret.value == SBI_EXT_HSM_STARTED), + "hart %ld successfully retentive resumed", hartid); + } + + while (!cpumask_test_secondary_cpus(&cpu_idle_mask)) + cpu_relax(); + + report(cpumask_full(&cpu_online_mask), "all cpus online"); + report(cpumask_test_secondary_cpus(&cpus_alive_after_retentive_suspend), + "all secondary harts successfully executed code after retentive suspend"); + } + + /* Reset the ipi_failed flag so that we can reuse it for non-retentive suspension tests */ + ipi_failed = false; + + on_secondary_cpus_async(hart_non_retentive_suspend, NULL); + + for_each_present_cpu(cpu) { + if (cpu == me) + continue; + + hartid = cpus[cpu].hartid; + hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED); + hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPEND_PENDING); + ret = sbi_hart_get_status(hartid); + report(!ret.error && (ret.value == SBI_EXT_HSM_SUSPENDED), + "hart %ld successfully non-retentive suspended", hartid); + } + + for (int i = 0; i < NR_CPUS / BITS_PER_LONG; ++i) { + if (hart_mask[i]) { + ret = sbi_send_ipi(hart_mask[i], i * BITS_PER_LONG); + if (ret.error) { + ipi_failed = true; + report_fail("got %ld when sending ipi to non-retentive suspended harts", + ret.error); + break; + } + } + } + + if (!ipi_failed) { + for_each_present_cpu(cpu) { + if (cpu == me) + continue; + + hartid = cpus[cpu].hartid; + hart_wait_on_status(hartid, SBI_EXT_HSM_SUSPENDED); + hart_wait_on_status(hartid, SBI_EXT_HSM_RESUME_PENDING); + ret = sbi_hart_get_status(hartid); + report(!ret.error && (ret.value == SBI_EXT_HSM_STARTED), + "hart %ld successfully non-retentive resumed", hartid); + } + + for_each_present_cpu(cpu) { + if (cpu == me) + continue; + + hartid = cpus[cpu].hartid; + + while (!((per_hart_non_retentive_suspend_checks = + READ_ONCE(sbi_hsm_non_retentive_hart_suspend_checks[hartid])) + & SBI_HSM_TEST_DONE)) + cpu_relax(); + + report(per_hart_non_retentive_suspend_checks & SBI_HSM_TEST_SATP, + "satp is zero for test hart %ld", hartid); + report(per_hart_non_retentive_suspend_checks & SBI_HSM_TEST_SIE, + "sstatus.SIE is zero for test hart %ld", hartid); + report(per_hart_non_retentive_suspend_checks & SBI_HSM_TEST_HARTID_A1, + "a0 and a1 are hartid for test hart %ld", hartid); + } + + report_prefix_pop(); + + report_prefix_push("hart_stop"); + + for_each_present_cpu(cpu) { + if (cpu == me) + continue; + + hartid = cpus[cpu].hartid; + WRITE_ONCE(sbi_hsm_stop_hart[hartid], true); + } + + for_each_present_cpu(cpu) { + if (cpu == me) + continue; + + hartid = cpus[cpu].hartid; + hart_wait_on_status(hartid, SBI_EXT_HSM_STARTED); + hart_wait_on_status(hartid, SBI_EXT_HSM_STOP_PENDING); + ret = sbi_hart_get_status(hartid); + report(!ret.error && (ret.value == SBI_EXT_HSM_STOPPED), + "test hart %ld successfully stopped", hartid); + } + } + } else { + report_skip("skipping suspension tests since ipi extension is unavailable"); + } + + report_prefix_popn(2); +} + int main(int argc, char **argv) { if (argc > 1 && !strcmp(argv[1], "-h")) { @@ -435,6 +816,7 @@ int main(int argc, char **argv) report_prefix_push("sbi"); check_base(); check_time(); + check_hsm(); check_dbcn(); return report_summary();
Add some tests for all of the HSM extension functions. These tests ensure that the HSM extension functions follow the behavior as described in the SBI specification. Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com> --- riscv/Makefile | 7 +- riscv/sbi-asm.S | 79 ++++++++++ riscv/sbi.c | 382 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 465 insertions(+), 3 deletions(-) create mode 100644 riscv/sbi-asm.S