On 17.08.2016 08:48, 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 functions start_all_cpus(), start_cpu() and start_thread() to start
> all stopped threads of all cpus, all stopped threads of a single cpu or a
> single stopped thread of a guest at a given execution location,
> respectively.
>
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@xxxxxxxxx>
> Reviewed-by: Andrew Jones <drjones@xxxxxxxxxx>
>
> ---
>
> Change Log:
>
> V2 -> V3:
> - start_thread now returns int to reflect error, success or failure to
> start thread
> - start_cpu returns number of threads on cpu and number successfully
> started
> - start_all_cpus checks if number of threads started == total number of
> threads - 1
> V3 -> V4:
> - Style changes
> ---
> lib/powerpc/asm/smp.h | 22 +++++++++++
> lib/powerpc/smp.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++
> lib/ppc64/asm/smp.h | 1 +
> powerpc/Makefile.common | 1 +
> 4 files changed, 126 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..21940b4
> --- /dev/null
> +++ b/lib/powerpc/asm/smp.h
> @@ -0,0 +1,22 @@
> +#ifndef _ASMPOWERPC_SMP_H_
> +#define _ASMPOWERPC_SMP_H_
> +
> +#include <libcflat.h>
> +
> +extern int nr_threads;
> +
> +struct start_threads {
> + int nr_threads;
> + int nr_started;
> +};
> +
> +typedef void (*secondary_entry_fn)(void);
> +
> +extern void halt(void);
> +
> +extern int start_thread(int cpu_id, secondary_entry_fn entry, uint32_t r3);
> +extern struct start_threads start_cpu(int cpu_node, secondary_entry_fn entry,
> + uint32_t r3);
> +extern bool start_all_cpus(secondary_entry_fn entry, uint32_t r3);
> +
> +#endif /* _ASMPOWERPC_SMP_H_ */
> diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c
> new file mode 100644
> index 0000000..caa65b9
> --- /dev/null
> +++ b/lib/powerpc/smp.c
> @@ -0,0 +1,102 @@
> +/*
> + * Secondary cpu support
> + *
> + * Copyright 2016 Suraj Jitindar Singh, IBM.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +
> +#include <devicetree.h>
> +#include <asm/setup.h>
> +#include <asm/rtas.h>
> +#include <asm/smp.h>
> +
> +int nr_threads;
> +
> +struct secondary_entry_data {
> + secondary_entry_fn entry;
> + uint64_t r3;
> + int nr_started;
> +};
> +
> +/*
> + * Start stopped thread cpu_id at entry
> + * Returns: <0 on failure to start stopped cpu
> + * 0 on success
> + * >0 on cpu not in stopped state
> + */
> +int start_thread(int cpu_id, secondary_entry_fn entry, uint32_t r3)
> +{
> + int query_token, start_token, outputs[1], ret;
> +
> + query_token = rtas_token("query-cpu-stopped-state");
> + start_token = rtas_token("start-cpu");
> + assert(query_token != RTAS_UNKNOWN_SERVICE &&
> + start_token != RTAS_UNKNOWN_SERVICE);
One TAB too much before start_token ?
> + ret = rtas_call(query_token, 1, 2, outputs, cpu_id);
> + if (ret) /* rtas query call failed */
The comment above is somewhat redundant - the printf statement below
explains the code quite well already.
Also, according to the kernel coding style (which we use for
kvm-unit-tests), I think you should use curly braces for both branches
of a conditional statement if they are required at least for one of the
branches (see chapter 3, right before chapter 3.1).
> + printf("query-cpu-stopped-state failed for cpu %d\n", cpu_id);
> + else if (!outputs[0]) { /* cpu in stopped state */
> + ret = rtas_call(start_token, 3, 1, NULL, cpu_id, entry, r3);
> + if (ret) /* rtas start-cpu call failed */
> + printf("failed to start cpu %d\n", cpu_id);
> + } else /* cpu not in stopped state */
> + ret = outputs[0];
That branch should also get some curly braces.
> +
> + return ret;
> +}
> +
> +/*
> + * Start all stopped threads (vcpus) on cpu_node
> + * Returns: Number of stopped cpus which were successfully started
> + */
> +struct start_threads start_cpu(int cpu_node, secondary_entry_fn entry,
> + uint32_t r3)
> +{
> + int len, i, nr_threads, nr_started = 0;
> + const struct fdt_property *prop;
> + u32 *threads;
> +
> + /* Get the id array of threads on this cpu_node */
> + prop = fdt_get_property(dt_fdt(), cpu_node,
> + "ibm,ppc-interrupt-server#s", &len);
> + assert(prop);
> +
> + nr_threads = len >> 2; /* Divide by 4 since 4 bytes per thread */
> + threads = (u32 *)prop->data; /* Array of valid ids */
> +
> + for (i = 0; i < nr_threads; i++) {
> + if (!start_thread(fdt32_to_cpu(threads[i]), entry, r3))
> + nr_started++;
> + }
> +
> + return (struct start_threads) {nr_threads, nr_started};
Add maybe a space around each curly brace?
> +}
> +
> +static void start_each_secondary(int fdtnode, u32 regval __unused, void *info)
> +{
> + struct secondary_entry_data *datap = info;
> + struct start_threads ret = start_cpu(fdtnode, datap->entry, datap->r3);
> +
> + nr_threads += ret.nr_threads;
> + datap->nr_started += ret.nr_started;
> +}
> +
> +/*
> + * Start all stopped cpus on the guest at entry with register 3 set to r3
> + * We expect that we come in with only one thread currently started
> + * Returns: TRUE on success
> + * FALSE on failure
> + */
> +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3)
> +{
> + struct secondary_entry_data data = { entry, r3, 0 };
> + int ret;
> +
> + ret = dt_for_each_cpu_node(start_each_secondary, &data);
> + assert(ret == 0);
> +
> + /* We expect that we come in with one thread already started */
> + return data.nr_started == nr_threads - 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)
Just some cosmetic nits ... it would be nice if you'd address them in
case you respin, but I'm also fine with the patch in the current shape
already, so:
Reviewed-by: Thomas Huth <thuth@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
