On Wed, Jan 18, 2023 at 02:49:11PM +0000, Alexandru Elisei wrote:
> For the PSCI CPU_ON function test, all other CPUs perform a CPU_ON call
> that target CPU 1. The test is considered a success if CPU_ON returns PSCI
> SUCCESS exactly once, and for the rest of the calls PSCI ALREADY_ON.
>
> Enhance the test by checking that CPU 1 is actually online and able to
> execute code. Also make the test more robust by checking that the CPU_ON
> call returns, instead of assuming that it will always succeed and
> hanging indefinitely if it doesn't.
>
> Since the CPU 1 thread is now being set up properly by kvm-unit-tests
> when being brought online, it becomes possible to add other tests in the
> future that require all CPUs.
>
> The include header order in arm/psci.c has been changed to be in
> alphabetic order. This means moving the errata.h include before
> libcflat.h, which causes compilation to fail because of missing includes
> in errata.h. Fix that also by including the needed header in errata.h.
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
> ---
> arm/psci.c | 60 ++++++++++++++++++++++++++++++++++++-----------
> lib/arm/asm/smp.h | 1 +
> lib/arm/smp.c | 12 +++++++---
> lib/errata.h | 2 ++
> 4 files changed, 58 insertions(+), 17 deletions(-)
>
> diff --git a/arm/psci.c b/arm/psci.c
> index efa0722c0566..e96be941953b 100644
> --- a/arm/psci.c
> +++ b/arm/psci.c
> @@ -7,11 +7,13 @@
> *
> * This work is licensed under the terms of the GNU LGPL, version 2.
> */
> -#include <libcflat.h>
> #include <errata.h>
> +#include <libcflat.h>
> +
> +#include <asm/delay.h>
> #include <asm/processor.h>
> -#include <asm/smp.h>
> #include <asm/psci.h>
> +#include <asm/smp.h>
>
> static bool invalid_function_exception;
>
> @@ -72,14 +74,23 @@ static int cpu_on_ret[NR_CPUS];
> static cpumask_t cpu_on_ready, cpu_on_done;
> static volatile int cpu_on_start;
>
> -static void cpu_on_secondary_entry(void)
> +extern void secondary_entry(void);
> +static void cpu_on_do_wake_target(void)
> {
> int cpu = smp_processor_id();
>
> cpumask_set_cpu(cpu, &cpu_on_ready);
> while (!cpu_on_start)
> cpu_relax();
> - cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(halt));
> + cpu_on_ret[cpu] = psci_cpu_on(cpus[1], __pa(secondary_entry));
> + cpumask_set_cpu(cpu, &cpu_on_done);
> +}
> +
> +static void cpu_on_target(void)
> +{
> + int cpu = smp_processor_id();
> +
> + cpu_on_ret[cpu] = PSCI_RET_ALREADY_ON;
I'm not sure this is better than just skipping cpu1 in the check loop, as
is done now, but OK.
> cpumask_set_cpu(cpu, &cpu_on_done);
> }
>
> @@ -87,33 +98,53 @@ static bool psci_cpu_on_test(void)
> {
> bool failed = false;
> int ret_success = 0;
> - int cpu;
> -
> - cpumask_set_cpu(1, &cpu_on_ready);
> - cpumask_set_cpu(1, &cpu_on_done);
> + int i, cpu;
>
> for_each_present_cpu(cpu) {
> if (cpu < 2)
> continue;
> - smp_boot_secondary(cpu, cpu_on_secondary_entry);
> + smp_boot_secondary(cpu, cpu_on_do_wake_target);
> }
>
> cpumask_set_cpu(0, &cpu_on_ready);
> + cpumask_set_cpu(1, &cpu_on_ready);
> while (!cpumask_full(&cpu_on_ready))
> cpu_relax();
>
> + /*
> + * Wait for all other CPUs to be online before configuring the thread
> + * for the target CPU, as all secondaries are set up using the same
> + * global variable.
> + */
This comment says "Wait", so I'd move the while loop under it.
> + smp_prepare_secondary(1, cpu_on_target);
This new smp_prepare_secondary() function should be local to this unit
test, please see my justification below.
> +
> cpu_on_start = 1;
> smp_mb();
>
> - cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(halt));
> + cpu_on_ret[0] = psci_cpu_on(cpus[1], __pa(secondary_entry));
> cpumask_set_cpu(0, &cpu_on_done);
>
> - while (!cpumask_full(&cpu_on_done))
> - cpu_relax();
> + report_info("waiting for CPU1 to come online...");
> + for (i = 0; i < 10; i++) {
> + mdelay(100);
> + if (cpumask_full(&cpu_on_done))
> + break;
> + }
> +
> + if (!cpumask_full(&cpu_on_done)) {
> + for_each_present_cpu(cpu) {
> + if (!cpumask_test_cpu(cpu, &cpu_on_done)) {
> + if (cpu == 1)
> + report_info("CPU1 failed to come online");
> + else
> + report_info("CPU%d failed to online CPU1", cpu);
> + }
> + }
> + failed = true;
> + goto out;
We could still run the other checks below, perhaps guarded with
cpumask_test_cpu(cpu, &cpu_on_done), rather than bail early. I'm
also OK with bailing early though. But, for that, I'd just return
false right here rather than introduce the goto.
> + }
>
> for_each_present_cpu(cpu) {
> - if (cpu == 1)
> - continue;
> if (cpu_on_ret[cpu] == PSCI_RET_SUCCESS) {
> ret_success++;
> } else if (cpu_on_ret[cpu] != PSCI_RET_ALREADY_ON) {
> @@ -127,6 +158,7 @@ static bool psci_cpu_on_test(void)
> failed = true;
> }
>
> +out:
> return !failed;
> }
>
> diff --git a/lib/arm/asm/smp.h b/lib/arm/asm/smp.h
> index 077afde85520..ff2ef8f88247 100644
> --- a/lib/arm/asm/smp.h
> +++ b/lib/arm/asm/smp.h
> @@ -49,6 +49,7 @@ static inline void set_cpu_idle(int cpu, bool idle)
> }
>
> typedef void (*secondary_entry_fn)(void);
> +extern void smp_prepare_secondary(int cpu, secondary_entry_fn entry);
> extern void smp_boot_secondary(int cpu, secondary_entry_fn entry);
> extern void on_cpu_async(int cpu, void (*func)(void *data), void *data);
> extern void on_cpu(int cpu, void (*func)(void *data), void *data);
> diff --git a/lib/arm/smp.c b/lib/arm/smp.c
> index 98a5054e039b..947f417f4aea 100644
> --- a/lib/arm/smp.c
> +++ b/lib/arm/smp.c
> @@ -58,13 +58,19 @@ secondary_entry_fn secondary_cinit(void)
> return entry;
> }
>
> -static void __smp_boot_secondary(int cpu, secondary_entry_fn entry)
> +void smp_prepare_secondary(int cpu, secondary_entry_fn entry)
I'd rather not create an unsafe library function, especially one named
without the leading underscores. It's OK for a unit test to duplicate
__smp_boot_secondary() (secondary_data is available), but then the unit
test also needs to ensure it does its own synchronization, as you do
with the wait on cpu_on_ready already.
> {
> - int ret;
> -
> secondary_data.stack = thread_stack_alloc();
> secondary_data.entry = entry;
> mmu_mark_disabled(cpu);
> +}
> +
> +static void __smp_boot_secondary(int cpu, secondary_entry_fn entry)
> +{
> + int ret;
> +
> + smp_prepare_secondary(cpu, entry);
> +
> ret = cpu_psci_cpu_boot(cpu);
> assert(ret == 0);
>
> diff --git a/lib/errata.h b/lib/errata.h
> index 5af0eb3bf8e2..de8205d8b370 100644
> --- a/lib/errata.h
> +++ b/lib/errata.h
> @@ -6,6 +6,8 @@
> */
> #ifndef _ERRATA_H_
> #define _ERRATA_H_
> +#include <libcflat.h>
> +
> #include "config.h"
>
> #ifndef CONFIG_ERRATA_FORCE
> --
> 2.25.1
>
Thanks,
drew
