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

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

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
>