On Mon, 2016-08-15 at 08:27 +0200, Andrew Jones wrote:
> On Mon, Aug 15, 2016 at 11:58:46AM +1000, Suraj Jitindar Singh wrote:
> >
> > On Fri, 2016-08-12 at 19:07 +0200, Andrew Jones wrote:
> > >
> > > On Wed, Aug 10, 2016 at 11:59:36AM +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 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>
> > > > ---
> > > > lib/powerpc/asm/smp.h | 15 +++++++
> > > > lib/powerpc/smp.c | 115
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++
> > > > lib/ppc64/asm/smp.h | 1 +
> > > > powerpc/Makefile.common | 1 +
> > > > 4 files changed, 132 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..a4f3e7f
> > > > --- /dev/null
> > > > +++ b/lib/powerpc/asm/smp.h
> > > > @@ -0,0 +1,15 @@
> > > > +#ifndef _ASMPOWERPC_SMP_H_
> > > > +#define _ASMPOWERPC_SMP_H_
> > > > +
> > > > +#include <libcflat.h>
> > > > +#include <stdint.h>
> > > nit: no need to include stdint.h, libcflat.h already does, so
> > > most
> > > files neglect it.
> > Will remove
> > >
> > >
> > > >
> > > >
> > > > +
> > > > +typedef void (*secondary_entry_fn)(void);
> > > > +
> > > > +extern void halt(void);
> > > > +
> > > > +bool start_thread(int cpu_id, secondary_entry_fn entry,
> > > > uint32_t
> > > > r3);
> > > > +bool start_cpu(int cpu_node, secondary_entry_fn entry,
> > > > uint32_t
> > > > r3);
> > > > +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3);
> > > nit: kvm-unit-tests likes to use 'extern' on function
> > > declarations
> > Ok, I'll add this
> > >
> > >
> > > >
> > > >
> > > > +
> > > > +#endif /* _ASMPOWERPC_SMP_H_ */
> > > > diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c
> > > > new file mode 100644
> > > > index 0000000..8968907
> > > > --- /dev/null
> > > > +++ b/lib/powerpc/smp.c
> > > > @@ -0,0 +1,115 @@
> > > > +/*
> > > > + * Secondary cpu support
> > > > + *
> > > > + * Copyright 2016 Suraj Jitindar Singh, IBM.
> > > > + *
> > > > + * This work is licensed under the terms of the GNU LGPL,
> > > > version
> > > > 2.
> > > > + */
> > > > +
> > > > +#include <stdint.h>
> > > same nit as above
> > >
> > > >
> > > >
> > > > +#include <libfdt/libfdt.h>
> > > > +#include <libfdt/fdt.h>
> > > nit: only need libfdt/libfdt.h, as that includes fdt.h
> > >
> > > >
> > > >
> > > > +#include <devicetree.h>
> > > > +#include <libcflat.h>
> > > > +#include <string.h>
> > > > +#include <asm/rtas.h>
> > > > +#include <asm/smp.h>
> > > > +
> > > > +struct secondary_entry_data {
> > > > + secondary_entry_fn entry;
> > > > + uint64_t r3;
> > > > + bool init_failed;
> > > > +};
> > > > +
> > > > +/*
> > > > + * Start stopped thread cpu_id at entry
> > > > + * Returns: 1 on success or cpu not in stopped state
> > > > + * 0 on failure to start stopped cpu
> > > Returns: TRUE ...
> > > FALSE ...
> > Same thing, but ok
> > >
> > >
> > > >
> > > >
> > > > + *
> > > > + * Note: This function returns 1 on success in starting a
> > > > stopped
> > > > cpu or if the
> > > returns true
> > >
> > > >
> > > >
> > > > + * given cpu was not in the stopped state. Thus this
> > > > can
> > > > be called on a
> > > > + * list of cpus and all the stopped ones will be
> > > > started
> > > > while false
> > > > + * won't be returned if some cpus in that list were
> > > > already running. Thus
> > > > + * the user should check that cpus passed to this
> > > > function
> > > > are already in
> > > > + * the stopped state if they want to guarantee that a
> > > > return value of
> > > > + * true corresponds to the given cpu now executing at
> > > > entry. This
> > > > + * function checks again however as calling cpu-start
> > > > on a
> > > > not stopped
> > > > + * cpu results in undefined behaviour.
> > > > + */
> > > > +bool 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);
> > > > +
> > > > + ret = rtas_call(query_token, 1, 2, outputs, cpu_id);
> > > > + if (ret) {
> > > > + printf("query-cpu-stopped-state failed for cpu
> > > > %d\n", cpu_id);
> > > > + return false;
> > > > + }
> > > > +
> > > > + if (!outputs[0]) { /* cpu in stopped state */
> > > > + ret = rtas_call(start_token, 3, 1, NULL,
> > > > cpu_id,
> > > > entry, r3);
> > > > + if (ret) {
> > > > + printf("failed to start cpu %d\n",
> > > > cpu_id);
> > > > + return false;
> > > > + }
> > > > + }
> > > > +
> > > > + return true;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Start all stopped threads (vcpus) on cpu_node
> > > > + * Returns: 1 on success
> > > > + * 0 on failure
> > > TRUE/FALSE, but I'd actually change the return type to struct
> > > start_threads;
> > >
> > > struct start_threads {
> > > int nr_threads;
> > > int nr_started;
> > > };
> > >
> > > Then...
> > >
> > > >
> > > >
> > > > + */
> > > > +bool start_cpu(int cpu_node, secondary_entry_fn entry,
> > > > uint32_t
> > > > r3)
> > > > +{
> > > > + const struct fdt_property *prop;
> > > > + int len, nr_cpu, cpu;
> > > > + u32 *cpus;
> > > > + bool ret = true;
> > > > +
> > > > + /* 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_cpu = len >> 2; /* Divide by 4 since 4 bytes per
> > > > cpu */
> > > > + cpus = (u32 *)prop->data; /* Array of valid cpu
> > > > numbers */
> > > > +
> > > > + for (cpu = 0; cpu < nr_cpu && ret; cpu++)
> > > > + ret = start_thread(fdt32_to_cpu(cpus[cpu]),
> > > > entry,
> > > > r3);
> > > > + return ret;
> > > ...change this to
> > >
> > > nr_threads = len >> 2; /* Divide by 4 since 4 bytes per cpu */
> > > cpus = (u32 *)prop->data; /* Array of valid cpu numbers */
> > >
> > > for (cpu = 0; cpu < nr_threads; cpu++)
> > > nr_started += start_thread(fdt32_to_cpu(cpus[cpu]), entry,
> > > r3);
> > I guess this fits the more generic use case. Although I can't
> > really
> > see a scenario where the caller wants to start all cpus and
> > continue
> > starting them when one fails, that is if one fails to start you
> > probably might as well return an error code immediately.
> > >
> > >
> > > return (struct start_threads){ nr_threads, nr_started };
> > >
> > > and...
> > >
> > > >
> > > >
> > > > +}
> > > > +
> > > > +static void start_each_secondary(int fdtnode, u32 regval
> > > > __unused,
> > > > void *info)
> > > > +{
> > > > + struct secondary_entry_data *datap = info;
> > > > +
> > > > + datap->init_failed |= !start_cpu(fdtnode, datap-
> > > > >entry,
> > > > datap->r3);
> > > > +}
> > > ...change init_failed to nr_started
> > >
> > > start_threads = start_cpu(fdtnode, datap->entry, datap->r3);
> > > nr_cpus += start_threads.nr_threads; // nr_cpus is global like
> > > ARM
> > > has
> > > datap->nr_started += start_threads.nr_started;
> > >
> > > and below just check that datap->nr_started == nr_cpus.
> > nr_cpus is set during setup so it would be possible to just have
> > the
> > above return nr_started and then check this accumulated value
> > against
> > nr_cpu below.
> > >
> > >
> > > >
> > > >
> > > > +
> > > > +/*
> > > > + * Start all stopped cpus on the guest at entry with register
> > > > 3
> > > > set to r3
> > > > + * Returns: 1 on success
> > > > + * 0 on failure
> > > TRUE/FALSE
> > Ok
> > >
> > >
> > > >
> > > >
> > > > + */
> > > > +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3)
> > > > +{
> > > > + struct secondary_entry_data data = {
> > > > + entry,
> > > > + r3,
> > > > + false
> > > > + };
> > > > + int ret;
> > > > +
> > > > + ret = dt_for_each_cpu_node(&start_each_secondary,
> > > > (void *)
> > > > &data);
> > > assert(ret == 0)
> > Sounds good
> > >
> > >
> > > >
> > > >
> > > > +
> > > > + return !(ret || data.init_failed);
> > > See comment above about setting nr_cpus, and then just confirming
> > > they all
> > > started here instead.
> > I think I'll change the above so that start_cpu returns the number
> > of
> > cpus started (we already know the total number of cpus so the
> > struct is
> > unnecessary), we come in with one cpu already started so I'll check
> > that nr_started == nr_cpu - 1.
> I completely forgot that I wrote code setting up nr_cpus... After
> reading this patch, I actually assumed I hadn't, because I didn't
> recall addressing threads. So is the nr_cpus in setup correct?
nr_cpus in setup is done as just that, the number of cpus, not threads.
Which I didn't realise until I read the code closer.
> Without accounting for threads, then it can't be, can it? Maybe
> this patch series should start by fixing that, and also bumping
> NR_CPUS in lib/powerpc/asm/setup.h up to whatever makes more sense.
Depending on what we want nr_cpus to be. I suggest we keep it as the
number of cpus and I implement this as you initially suggested using
struct start_threads to keep track of the total number of threads and
the number successfully started, checking that the number started is
one less than the total number.
This is how I plan on implementing it currently for the next revision.
>
> >
> > >
> > >
> > > >
> > > >
> > > > +}
> > > > 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)
> > Thanks for the review
> Thanks,
> drew
--
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
