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