[PATCH] arm:Add PSCI_CPU_OFF testscase to arm/psci testsuite.

[PATCH] arm:Add PSCI_CPU_OFF testscase to arm/psci testsuite.

[Nikita Venkatesh]

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.

Signed-off-by: Nikita Venkatesh <Nikita.Venkatesh@arm.com>
---
 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)
+{
+	int cpu = smp_processor_id();
+	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)
+			continue;
+		smp_boot_secondary(cpu, cpu_off_secondary_test);
+	}
+
+	cpumask_set_cpu(0, &cpu_off_done);
+
+	report_info("PSCI OFF Test");
+
+	/* 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();
+	}
 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

Re: [PATCH] arm:Add PSCI_CPU_OFF testscase to arm/psci testsuite.

[Alexandru Elisei]

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@arm.com/

> 
> Signed-off-by: Nikita Venkatesh <Nikita.Venkatesh@arm.com>
> ---
>  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