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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

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



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux