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

[Suraj Jitindar Singh <sjitindarsingh@gmail.com>]

On Fri, 2016-08-05 at 10:54 +0200, Andrew Jones wrote:
> 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.
Woops, this got copied when I moved this code from a unit test into
a library file. I'll change these to printfs.
> 
> > 
> > +	}
> > +
> > +	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?
I didn't initially use dt_for_each_cpu_node as setting up the callback
function seemed like more effort than it was worth and there is no
mechanism to get a return value back from the function.
But I've thought of a better way to do this using dt_for_each_cpu_node
which I'm going to implement.
> 
> > 
> > +		/* 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.
This would make it more readable in the event of a failure, should be
trivial to change, will do.
> 
>  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)
Will rework it to something like this
> 
> Actually, why call it "get_" secondaries instead of "start_" ?
I agree start sounds better
> 
> > 
> > +				/* 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)