| Message ID | 1470382393-24209-3-git-send-email-sjitindarsingh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; + } + + 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; + + /* 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); + /* 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)
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