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

From: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
To: Andrew Jones <drjones@redhat.com>
Cc: kvm@vger.kernel.org, pbonzini@redhat.com, rkrcmar@redhat.com,
	kvm-ppc@vger.kernel.org, lvivier@redhat.com, thuth@redhat.com
Subject: Re: [kvm-unit-tests PATCH V2 3/4] lib/powerpc: Add function to start secondary threads
Date: Mon, 15 Aug 2016 11:58:46 +1000	[thread overview]
Message-ID: <1471226326.4052.24.camel@gmail.com> (raw)
In-Reply-To: <20160812170750.lqd7cbjovejyhyvl@kamzik.localdomain>

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@gmail.com>
> > ---
> >  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.
> 
> > 
> > +}
> > 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