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

[Thomas Huth <thuth@redhat.com>]

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