Re: [kvm-unit-tests PATCH v3 2/2] arm/psci: Add PSCI_CPU_OFF testscase to arm/psci testsuite

[Alexandru Elisei <alexandru.elisei@arm.com>]

Hi,

On Tue, Dec 20, 2022 at 02:31:56PM +0000, 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.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I don't think that's true anymore.

> 
> Signed-off-by: Nikita Venkatesh <Nikita.Venkatesh@arm.com>
> ---
>  arm/psci.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 84 insertions(+), 6 deletions(-)
> 
> diff --git a/arm/psci.c b/arm/psci.c
> index 0b9834c..8e664c2 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;
>  
>  extern void secondary_entry(void);
>  static void cpu_on_wake_target(void)
> @@ -92,11 +97,25 @@ static void cpu_on_target(void)
>  	cpumask_set_cpu(cpu, &cpu_on_done);
>  }
>  
> +static void cpu_off_secondary_entry(void *data)
> +{
> +	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;
>  	int ret_success = 0;
> -	int cpu;
> +	int i, cpu;
>  
>  	for_each_present_cpu(cpu) {
>  		if (cpu < 2)
> @@ -125,6 +144,25 @@ static bool psci_cpu_on_test(void)
>  	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);
> +			}
> +		}
> +		return false;
> +	}
> +

This change should be part of the previous patch.

>  	for_each_present_cpu(cpu) {
>  		if (cpu_on_ret[cpu] == PSCI_RET_SUCCESS) {
>  			ret_success++;
> @@ -142,7 +180,44 @@ 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 == 0)
> +			continue;
> +		on_cpu_async(cpu, cpu_off_secondary_entry, NULL);
> +	}
> +
> +	cpumask_set_cpu(0, &cpu_off_done);
> +
> +	report_info("starting CPU_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;
> +}
> +
> +int main(int argc, char **argv)
>  {
>  	int ver = psci_invoke(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
>  
> @@ -154,15 +229,18 @@ int main(void)
>  	}
>  
>  	report_info("PSCI version %d.%d", PSCI_VERSION_MAJOR(ver),
> -					  PSCI_VERSION_MINOR(ver));
> +					PSCI_VERSION_MINOR(ver));

This change is unneeded. Whitespace change?

> +
>  	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))
> +	if (ERRATA(6c7a5dce22b3)){
>  		report(psci_cpu_on_test(), "cpu-on");
> -	else
> +	} else {
>  		report_skip("Skipping unsafe cpu-on test. Set ERRATA_6c7a5dce22b3=y to enable.");
> +	}

The adition of braces here seems unnecessary. Is it by any chance a
leftover from the previous version of the patches, where they were
necessary?

Otherwise, the patches look good, also ran a few tests for good measure.

Thanks,
Alex

> +	report(psci_cpu_off_test(), "cpu-off");
>  
>  done:
>  #if 0
> -- 
> 2.25.1
>