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

[Suraj Jitindar Singh <sjitindarsingh@gmail.com>]

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