Hi, The patch doesn't apply on top of master (commit 7362976db651 ("x86/pmu: Run the "emulation" test iff forced emulation is available")). I think it doesn't apply correctly because you've used an email client which converts tabs to spaces. Another thing that I spotted while trying to figure out what is wrong is that the encoding is iso-8859-1 (Content-Type: text/plain; charset="iso-8859-1") instead of us-ascii or utf-8 as per the submitting patches documentation in the Linux kernel [1]. That's also a good guide to make your email client play nicely with the mailing lit. Also, the subject should be in this format: [kvm-unit-tests PATCH] arm: Add PSCI_CPU_OFF testcase to arm/psci testsuite Notice the "kvm-unit-tests" prefix (and the missing and added spaces, and the removed period "." at the end). The prefix should be there as per the README file. Not a big deal, but it helps with sorting the emails. And it definitely helps with getting people to notice the patch. [1] https://github.com/torvalds/linux/blob/master/Documentation/process/email-clients.rst On Fri, Aug 05, 2022 at 02:26:11PM +0100, Nikita Venkatesh wrote: > The test uses the following method. > > The primary CPU brings up all the secondary CPUs, which are held in a wait > loop. Once the primary releases the CPUs, each of the secondary CPUs > proceed to issue PSCI_CPU_OFF. This is indicated by a cpumask and also > the status of the call is updated by the secondary CPU in cpu_off_done[]. > > The primary CPU waits for all the secondary CPUs to update the cpumask > and then proceeds to check for the status of the individual CPU CPU_OFF > request. There is a chance that some CPUs might fail at the CPU_OFF > request and come back and update the status once the primary CPU has > finished the scan. There is no fool proof method to handle this. As of > now, we add a 1sec delay between the cpumask check and the scan for the > status. > > The test can be triggered by "cpu-off" command line argument. The logic of the test looks correct to me, and the comments below are all cosmetic. What looks rather awkward is that we do the CPU_OFF test separate from the CPU_ON test. The CPU_ON test already brings online all of the CPUs, doing that again and as a separate test feels awkward. I've sent a patch [2] which leaves all the secondary CPUs in do_idle(), ready to execute another function. You can rework this patch on top of that one, and have the regular PSCI test do CPU_OFF for all CPUs after the existing CPU_ON test, which makes more sense to me. I'm talking about something like this (some parts omitted for brevity): [..] +static void secondary_entry_stub (void *unused) +{ +} + static bool psci_cpu_off_test(void) { bool failed = false; @@ -168,7 +172,7 @@ static bool psci_cpu_off_test(void) for_each_present_cpu(cpu) { if (cpu < 1) continue; - smp_boot_secondary(cpu, cpu_off_secondary_test); + on_cpu_async(cpu, cpu_off_secondary_test, NULL); } cpumask_set_cpu(0, &cpu_off_done); [..] @@ -221,15 +213,18 @@ int main(int argc, char **argv) } report_info("PSCI version %d.%d", PSCI_VERSION_MAJOR(ver), - PSCI_VERSION_MINOR(ver)); - if (argc < 2) { - run_default_psci_tests(); - } else if (strcmp(argv[1], "cpu-off") == 0) { - report(psci_cpu_off_test(), "cpu-off"); + PSCI_VERSION_MINOR(ver)); + + report(psci_invalid_function(), "invalid-function"); + report(psci_affinity_info_on(), "affinity-info-on"); + report(psci_affinity_info_off(), "affinity-info-off"); + if (ERRATA(6c7a5dce22b3)){ + report(psci_cpu_on_test(), "cpu-on"); } else { - printf("Unknown subtest\n"); - abort(); + report_skip("Skipping unsafe cpu-on test. Set ERRATA_6c7a5dce22b3=y to enable."); + on_cpus(secondary_entry_stub, NULL); } + report(psci_cpu_off_test(), "cpu-off"); done: #if 0 [2] https://lore.kernel.org/kvm/20220909142925.52198-1-alexandru.elisei@xxxxxxx/ > > Signed-off-by: Nikita Venkatesh <Nikita.Venkatesh@xxxxxxx> > --- > arm/psci.c | 87 +++++++++++++++++++++++++++++++++++++++++------ > arm/unittests.cfg | 6 ++++ > 2 files changed, 83 insertions(+), 10 deletions(-) > > diff --git a/arm/psci.c b/arm/psci.c > index efa0722..5485718 100644 > --- a/arm/psci.c > +++ b/arm/psci.c > @@ -12,6 +12,9 @@ > #include <asm/processor.h> > #include <asm/smp.h> > #include <asm/psci.h> > +#include <asm/delay.h> > + > +#define CPU_OFF_TEST_WAIT_TIME 1000 > > static bool invalid_function_exception; > > @@ -69,8 +72,10 @@ static bool psci_affinity_info_off(void) > } > > static int cpu_on_ret[NR_CPUS]; > -static cpumask_t cpu_on_ready, cpu_on_done; > +static bool cpu_off_success[NR_CPUS]; > +static cpumask_t cpu_on_ready, cpu_on_done, cpu_off_done; > static volatile int cpu_on_start; > +static volatile int cpu_off_start; > > static void cpu_on_secondary_entry(void) > { > @@ -83,6 +88,19 @@ static void cpu_on_secondary_entry(void) > cpumask_set_cpu(cpu, &cpu_on_done); > } > > +static void cpu_off_secondary_test(void) cpu_off_secondary_entry? > +{ > + int cpu = smp_processor_id(); The coding style followed by this file has a newline after local variable declaration(s). > + while (!cpu_off_start) > + cpu_relax(); > + /* On to the CPU off test */ > + cpu_off_success[cpu] = true; > + cpumask_set_cpu(cpu, &cpu_off_done); > + cpu_psci_cpu_die(); > + /* The CPU shouldn't execute the next steps. */ > + cpu_off_success[cpu] = false; > +} > + > static bool psci_cpu_on_test(void) > { > bool failed = false; > @@ -130,7 +148,56 @@ static bool psci_cpu_on_test(void) > return !failed; > } > > -int main(void) > +static bool psci_cpu_off_test(void) > +{ > + bool failed = false; > + int cpu; > + > + for_each_present_cpu(cpu) { > + if (cpu < 1) I would prefer the test to be if (cpu == 0), as it makes it clear that the boot CPU is expected to be CPU 0. It also matches the way you express this test below, in the for loop. > + continue; > + smp_boot_secondary(cpu, cpu_off_secondary_test); > + } > + > + cpumask_set_cpu(0, &cpu_off_done); > + > + report_info("PSCI OFF Test"); This is not strictly necessary, the psci_cpu_on_tests doesn't have it and it clutters the output by displaying redundant information. I guess it can be helpful because the test is rather long (at least 1 second because of the delay) and we don't want the user to get the impression that the hanged. How about changing it to report_info("starting CPU_OFF test..."), which looks similar to what kvm-unit-tests does for the timer test, which has also has a similar delay? Up to you really, as this is a matter of taste. > + > + /* Release the CPUs */ > + cpu_off_start = 1; > + > + /* Wait until all are done */ > + while (!cpumask_full(&cpu_off_done)) > + cpu_relax(); > + > + /* Allow all the other CPUs to complete the operation */ > + mdelay(CPU_OFF_TEST_WAIT_TIME); > + > + for_each_present_cpu(cpu) { > + if (cpu == 0) > + continue; > + > + if (!cpu_off_success[cpu]) { > + report_info("CPU%d could not be turned off", cpu); > + failed = true; > + } > + } > + return !failed; > +} > + > +static void run_default_psci_tests(void) > +{ > + report(psci_invalid_function(), "invalid-function"); > + report(psci_affinity_info_on(), "affinity-info-on"); > + report(psci_affinity_info_off(), "affinity-info-off"); > + if (ERRATA(6c7a5dce22b3)){ > + report(psci_cpu_on_test(), "cpu-on"); > + } else { > + report_skip("Skipping unsafe cpu-on test. Set ERRATA_6c7a5dce22b3=y to enable."); > + } > +} > + > +int main(int argc, char **argv) > { > int ver = psci_invoke(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0); > > @@ -143,15 +210,15 @@ int main(void) > > report_info("PSCI version %d.%d", PSCI_VERSION_MAJOR(ver), > PSCI_VERSION_MINOR(ver)); > - report(psci_invalid_function(), "invalid-function"); > - report(psci_affinity_info_on(), "affinity-info-on"); > - report(psci_affinity_info_off(), "affinity-info-off"); > - > - if (ERRATA(6c7a5dce22b3)) > - report(psci_cpu_on_test(), "cpu-on"); > - else > - report_skip("Skipping unsafe cpu-on test. Set ERRATA_6c7a5dce22b3=y to enable."); > > + if (argc < 2) { > + run_default_psci_tests(); > + } else if (strcmp(argv[1], "cpu-off") == 0) { > + report(psci_cpu_off_test(), "cpu-off"); > + } else { > + printf("Unknown subtest\n"); > + abort(); > + } There should be a newline here, as it previously was. Thanks, Alex > done: > #if 0 > report_summary(); > diff --git a/arm/unittests.cfg b/arm/unittests.cfg > index 5e67b55..02ffbcd 100644 > --- a/arm/unittests.cfg > +++ b/arm/unittests.cfg > @@ -218,6 +218,12 @@ file = psci.flat > smp = $MAX_SMP > groups = psci > > +[psci-cpu-off] > +file = psci.flat > +groups = psci > +smp = $MAX_SMP > +extra_params = -append 'cpu-off' > + > # Timer tests > [timer] > file = timer.flat > -- > 2.25.1