Re: [kvm-unit-tests PATCH 3/4] lib/powerpc: Add function to start secondary threads

[Andrew Jones <drjones@redhat.com>]

On Fri, Aug 05, 2016 at 05:33:12PM +1000, Suraj Jitindar Singh wrote:
> Add the lib/powerpc/smp.c file and associated header files as a place
> to implement generic smp functionality for inclusion in tests.
> 
> Add a function "get_secondaries" to start stopped threads of a guest at a
> given function location.
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  lib/powerpc/asm/smp.h   |  8 +++++
>  lib/powerpc/smp.c       | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/ppc64/asm/smp.h     |  1 +
>  powerpc/Makefile.common |  1 +
>  4 files changed, 96 insertions(+)
>  create mode 100644 lib/powerpc/asm/smp.h
>  create mode 100644 lib/powerpc/smp.c
>  create mode 100644 lib/ppc64/asm/smp.h
> 
> diff --git a/lib/powerpc/asm/smp.h b/lib/powerpc/asm/smp.h
> new file mode 100644
> index 0000000..c4e3ad8
> --- /dev/null
> +++ b/lib/powerpc/asm/smp.h
> @@ -0,0 +1,8 @@
> +#ifndef _ASMPOWERPC_SMP_H_
> +#define _ASMPOWERPC_SMP_H_
> +
> +extern void halt(void);
> +
> +extern int get_secondaries(void (* secondary_func)(void));
> +
> +#endif /* _ASMPOWERPC_SMP_H_ */
> diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c
> new file mode 100644
> index 0000000..1f8922e
> --- /dev/null
> +++ b/lib/powerpc/smp.c
> @@ -0,0 +1,86 @@
> +/*
> + * Secondary cpu support
> + *
> + * Copyright 2016 Suraj Jitindar Singh, IBM.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +
> +#include <libfdt/libfdt.h>
> +#include <libfdt/fdt.h>
> +#include <devicetree.h>
> +#include <libcflat.h>
> +#include <string.h>
> +#include <asm/rtas.h>
> +#include <asm/smp.h>
> +
> +/*
> + * Start stopped secondary threads at secondary_func
> + * Returns:  0 on success
> + * 	    -1 on failure
> + */
> +int get_secondaries(void (* secondary_func)(void))
> +{
> +	int cpu_root_node, cpu_node, query_token, start_token;
> +	int ret, outputs[1], nr_cpu, cpu, lenp;
> +	const struct fdt_property *prop;
> +	u32 *cpus;
> +
> +	cpu_root_node = fdt_path_offset(dt_fdt(), "/cpus");
> +	if (cpu_root_node < 0) {
> +		report("cpu root node not found", 0);
> +		return -1;

We only call the report API from unit tests. lib code uses printf.
Also, in general, anything that should never happen (like missing
DT nodes and properties) is probably best using asserts and aborts.

> +	}
> +
> +	query_token = rtas_token("query-cpu-stopped-state");
> +	start_token = rtas_token("start-cpu");
> +	if (query_token == RTAS_UNKNOWN_SERVICE ||
> +			start_token == RTAS_UNKNOWN_SERVICE) {
> +		report("rtas token not found", 0);
> +		return -1;
> +	}
> +
> +	dt_for_each_subnode(cpu_root_node, cpu_node) {
> +		prop = fdt_get_property(dt_fdt(), cpu_node, "device_type", &lenp);
> +		/* Not a cpu node */
> +		if (prop == NULL || lenp != 4 ||
> +				strncmp((char *)prop->data, "cpu", 4))
> +			continue;
> +

Isn't it possible to use dt_for_each_cpu_node? Where the code below is
in the callback?

> +		/* Get the id array of threads on this cpu */
> +		prop = fdt_get_property(dt_fdt(), cpu_node,
> +				"ibm,ppc-interrupt-server#s", &lenp);
> +		if (!prop) {
> +			report("ibm,ppc-interrupt-server#s prop not found"
> +					, 0);
> +			return -1;
> +		}
> +
> +		nr_cpu = lenp >> 2;	/* Divide by 4 since 4 bytes per cpu */
> +		cpus = (u32 *)prop->data; /* Array of valid cpu numbers */
> +
> +		/* Iterate over valid cpus to see if they are stopped */
> +		for (cpu = 0; cpu < nr_cpu; cpu++) {
> +			int cpu_id = fdt32_to_cpu(cpus[cpu]);
> +			/* Query cpu status */
> +			ret = rtas_call(query_token, 1, 2, outputs, cpu_id);
> +			/* RTAS call failed */
> +			if (ret)
> +				goto RTAS_FAILED;
> +			/* cpu is stopped, start it */
> +			if (!*outputs) {
> +				ret = rtas_call(start_token, 3, 1, NULL, cpu_id,
> +						secondary_func,	cpu_id);

Hmm, rtas_calls are generally in unit test territory as they can fail
due to KVM failures. It probably makes sense to return failures for
them, like you do. How about restructuring it though to make sure you
try as much as possible, printing errors as you go.

 int get_secondaries()
 {

  for-each-cpu(cpu) {
     ret = rtas_call(query_token, ...)
     if (ret) {
        printf("query-cpu-stopped-state failed for cpu %d\n);
        failed = true;
        continue;
     }
     ret = rtas_call(start_token, ...)
     if (ret) {
        printf("failed to start cpu %d\n);
        failed = true;
     }
  }

  return failed ? -1 : 0; // get_secondaries could also be bool and then
                          // just return !failed
 }

unit test callers do

 report("starting secondaries", get_secondaries() == 0)

Actually, why call it "get_" secondaries instead of "start_" ?

> +				/* RTAS call failed */
> +				if (ret)
> +					goto RTAS_FAILED;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +
> +RTAS_FAILED:
> +	report("RTAS call failed", 0);
> +	return -1;
> +}
> diff --git a/lib/ppc64/asm/smp.h b/lib/ppc64/asm/smp.h
> new file mode 100644
> index 0000000..67ced75
> --- /dev/null
> +++ b/lib/ppc64/asm/smp.h
> @@ -0,0 +1 @@
> +#include "../../powerpc/asm/smp.h"
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index 404194b..677030a 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -37,6 +37,7 @@ cflatobjs += lib/powerpc/setup.o
>  cflatobjs += lib/powerpc/rtas.o
>  cflatobjs += lib/powerpc/processor.o
>  cflatobjs += lib/powerpc/handlers.o
> +cflatobjs += lib/powerpc/smp.o
>  
>  FLATLIBS = $(libcflat) $(LIBFDT_archive)
>  %.elf: CFLAGS += $(arch_CFLAGS)
> -- 
> 2.5.5
>

Thanks,
drew