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

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

On 10.08.2016 03:59, 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>
> ---
[...]
> +/*
> + * Start stopped thread cpu_id at entry
> + * Returns:	1 on success or cpu not in stopped state
> + *		0 on failure to start stopped cpu
> + *
> + * Note: This function returns 1 on success in starting a stopped cpu or if the
> + *	 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 */

Maybe add an "assert(outputs[0] != 1)" before the if-statement?

> +		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
> + */
> +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);

This way you only return the success or failure of the last thread that
has been started. All other information will be lost. Wouldn't it be
better to return false as soon as one of the threads could not be started?

> +	return ret;
> +}
> +
> +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);
> +}
> +
> +/*
> + * Start all stopped cpus on the guest at entry with register 3 set to r3
> + * Returns:	1 on success
> + *		0 on failure
> + */
> +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);

I think you don't need the (void*) cast here.

> +	return !(ret || data.init_failed);
> +}

 Thomas


--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, 2016-08-10 at 13:25 +0200, Thomas Huth wrote:
> On 10.08.2016 03:59, 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>
> > ---
> [...]
> > 
> > +/*
> > + * Start stopped thread cpu_id at entry
> > + * Returns:	1 on success or cpu not in stopped state
> > + *		0 on failure to start stopped cpu
> > + *
> > + * Note: This function returns 1 on success in starting a stopped
> > cpu or if the
> > + *	 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 */
> Maybe add an "assert(outputs[0] != 1)" before the if-statement?
> 
I'm torn because if I add the assert then the caller has to check the
cpu-stopped-state as well as it being checked here to avoid calling
this on a !stopped cpu (and hitting the assert) which means that this
will always be checked twice.
I could just define that this must be called on a stopped cpu, not
check in this function and leave it up to the caller. In the event this
is called on an already running cpu I'm not really sure what the
behaviour is though...
The way I'm using this function in the start_cpu function below I
assume that this function doesn't exit the test when an already running
cpu is hit because I basically just want to start all the stopped cpus
somewhere so I call it on every cpu in the system and expect to just
skip already running ones.
I think the best option is to have this return an int based on whether
the cpu was started successfully, the start-cpu call failed or the cpu
passed to the function was not in the stopped state. Then the caller
can just filter on the return value based on whether it cares if an
already running cpu was hit or not. 
> > 
> > +		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
> > + */
> > +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);
> This way you only return the success or failure of the last thread
> that
> has been started. All other information will be lost. Wouldn't it be
> better to return false as soon as one of the threads could not be
> started?
> 
AFAIK that is the current functionality given the "cpu < nr_cpu && ret"
								   ^^^
in the for conditional.
> > 
> > +	return ret;
> > +}
> > +
> > +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);
> > +}
> > +
> > +/*
> > + * Start all stopped cpus on the guest at entry with register 3
> > set to r3
> > + * Returns:	1 on success
> > + *		0 on failure
> > + */
> > +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);
> I think you don't need the (void*) cast here.
> 
That's probably the case
> > 
> > +	return !(ret || data.init_failed);
> > +}
>  Thomas
> 
> 
Thanks for the code review
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 12.08.2016 08:30, Suraj Jitindar Singh wrote:
> On Wed, 2016-08-10 at 13:25 +0200, Thomas Huth wrote:
>> On 10.08.2016 03:59, 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>
>>> ---
>> [...]
>>>
>>> +/*
>>> + * Start stopped thread cpu_id at entry
>>> + * Returns:	1 on success or cpu not in stopped state
>>> + *		0 on failure to start stopped cpu
>>> + *
>>> + * Note: This function returns 1 on success in starting a stopped
>>> cpu or if the
>>> + *	 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 */
>> Maybe add an "assert(outputs[0] != 1)" before the if-statement?
>>
> I'm torn because if I add the assert then the caller has to check the
> cpu-stopped-state as well as it being checked here to avoid calling
> this on a !stopped cpu (and hitting the assert) which means that this
> will always be checked twice.
> [...]

Not sure if you've got me right, or whether I've got your concerns here
right, but what I meant to say was:
query-cpu-stopped-state can return three different values:

 0: The processor thread is in the RTAS stopped state
 1: stop-self is in progress
 2: The processor thread is not in the RTAS stopped state

For 0 and 2, your code is certainly fine. But what happens if the return
value was "stop-self is in progress"? Can "start-cpu" start a CPU again
that is currently in progress of entering the stopped state? If yes, I
think your code is fine as it is, but if not, you end up with a CPU that
is finally stopped again, though you assume that it is running at the
end of your function. In that case it might be helpful to report that
strange state with an assert() to ease debugging later (currently, I
think qemu won't return 1 here, so this should never happen).

Anyway, looking at the code again, I think start-cpu should return an
error if it is not able to start a CPU that is currently in that
"stop-self is in progress" state ... and that should catch the
hypothetical error condition, too. So never mind, there's likely no
additional handling needed here.

>>>
>>> +		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
>>> + */
>>> +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);
>> This way you only return the success or failure of the last thread
>> that
>> has been started. All other information will be lost. Wouldn't it be
>> better to return false as soon as one of the threads could not be
>> started?
>>
> AFAIK that is the current functionality given the "cpu < nr_cpu && ret"
> 								   ^^^
> in the for conditional.

Oh, right, my bad, I overlooked that check. So never mind.

 Thomas

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.

> +
> +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

> +
> +#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 ...

> + *
> + * 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);

 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.

> +
> +/*
> + * Start all stopped cpus on the guest at entry with register 3 set to r3
> + * Returns:	1 on success
> + *		0 on failure

TRUE/FALSE

> + */
> +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)

> +
> +	return !(ret || data.init_failed);

See comment above about setting nr_cpus, and then just confirming they all
started here instead.

> +}
> 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-ppc" 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-12 at 13:19 +0200, Thomas Huth wrote:
> On 12.08.2016 08:30, Suraj Jitindar Singh wrote:
> > 
> > On Wed, 2016-08-10 at 13:25 +0200, Thomas Huth wrote:
> > > 
> > > On 10.08.2016 03:59, 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>
> > > > ---
> > > [...]
> > > > 
> > > > 
> > > > +/*
> > > > + * Start stopped thread cpu_id at entry
> > > > + * Returns:	1 on success or cpu not in stopped state
> > > > + *		0 on failure to start stopped cpu
> > > > + *
> > > > + * Note: This function returns 1 on success in starting a
> > > > stopped
> > > > cpu or if the
> > > > + *	 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 */
> > > Maybe add an "assert(outputs[0] != 1)" before the if-statement?
> > > 
> > I'm torn because if I add the assert then the caller has to check
> > the
> > cpu-stopped-state as well as it being checked here to avoid calling
> > this on a !stopped cpu (and hitting the assert) which means that
> > this
> > will always be checked twice.
> > [...]
> Not sure if you've got me right, or whether I've got your concerns
> here
> right, but what I meant to say was:
> query-cpu-stopped-state can return three different values:
> 
>  0: The processor thread is in the RTAS stopped state
>  1: stop-self is in progress
>  2: The processor thread is not in the RTAS stopped state
> 
> For 0 and 2, your code is certainly fine. But what happens if the
> return
> value was "stop-self is in progress"? Can "start-cpu" start a CPU
> again
> that is currently in progress of entering the stopped state? If yes, 
From my understanding start-cpu is only guaranteed to exhibit defined
behaviour when called on a cpu in the stopped state, otherwise its
behaviour is undefined and probably implementation dependant.
> I
> think your code is fine as it is, but if not, you end up with a CPU
> that
> is finally stopped again, though you assume that it is running at the
> end of your function. In that case it might be helpful to report that
> strange state with an assert() to ease debugging later (currently, I
> think qemu won't return 1 here, so this should never happen).
Yeah it looks like QEMU doesn't have the concept of a transitional
state and for QEMU at least it is safe to call start-cpu on an already
running cpu.
> 
> Anyway, looking at the code again, I think start-cpu should return an
> error if it is not able to start a CPU that is currently in that
> "stop-self is in progress" state ... and that should catch the
> hypothetical error condition, too. So never mind, there's likely no
> additional handling needed here.
I was thinking of changing this function to return 1|2 if the cpu
specified isn't in the stoped state without trying to start it. And
then return the negative error return value if either of the rtas calls
failed. Then it is up to the caller to filter the return value based on
what they think is acceptable (e.g. just ignore if a cpu was already
running).
> 
> > 
> > > 
> > > > 
> > > > 
> > > > +		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
> > > > + */
> > > > +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);
> > > This way you only return the success or failure of the last
> > > thread
> > > that
> > > has been started. All other information will be lost. Wouldn't it
> > > be
> > > better to return false as soon as one of the threads could not be
> > > started?
> > > 
> > AFAIK that is the current functionality given the "cpu < nr_cpu &&
> > ret"
> > 								   ^^^
> > in the for conditional.
> Oh, right, my bad, I overlooked that check. So never mind.
> 
>  Thomas
> 
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" 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-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
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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@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.

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?
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.

> > 
> > > 
> > > +}
> > > 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-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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@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.
> 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-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html