Re: [PATCH v2 3/9] ppc/spapr: Use start-powered-off CPUState property

[Thiago Jung Bauermann]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 7/22/20 5:50 AM, Thiago Jung Bauermann wrote:
>> PowerPC sPAPR CPUs start in the halted state, and spapr_reset_vcpu()
>> attempts to implement this by setting CPUState::halted to 1. But that's too
>> late for the case of hotplugged CPUs in a machine configure with 2 or more
>> threads per core.
>> 
>> By then, other parts of QEMU have already caused the vCPU to run in an
>> unitialized state a couple of times. For example, ppc_cpu_reset() calls
>> ppc_tlb_invalidate_all(), which ends up calling async_run_on_cpu(). This
>> kicks the new vCPU while it has CPUState::halted = 0, causing QEMU to issue
>> a KVM_RUN ioctl on the new vCPU before the guest is able to make the
>> start-cpu RTAS call to initialize its register state.
>> 
>> This problem doesn't seem to cause visible issues for regular guests, but
>> on a secure guest running under the Ultravisor it does. The Ultravisor
>> relies on being able to snoop on the start-cpu RTAS call to map vCPUs to
>> guests, and this issue causes it to see a stray vCPU that doesn't belong to
>> any guest.
>> 
>> Fix by setting the start-powered-off CPUState property in
>> spapr_create_vcpu(), which makes cpu_common_reset() initialize
>> CPUState::halted to 1 at an earlier moment.
>> 
>> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>> ---
>>  hw/ppc/spapr_cpu_core.c | 12 +++++++-----
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>> 
>> NB: Tested on ppc64le pseries KVM guest with two threads per core. 
>> Hot-plugging additional cores doesn't cause the bug described above
>> anymore.
>> 
>> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
>> index c4f47dcc04..09feeb5f8f 100644
>> --- a/hw/ppc/spapr_cpu_core.c
>> +++ b/hw/ppc/spapr_cpu_core.c
>> @@ -36,11 +36,6 @@ static void spapr_reset_vcpu(PowerPCCPU *cpu)
>>  
>>      cpu_reset(cs);
>>  
>> -    /* All CPUs start halted.  CPU0 is unhalted from the machine level
>> -     * reset code and the rest are explicitly started up by the guest
>> -     * using an RTAS call */
>> -    cs->halted = 1;
>> -
>>      env->spr[SPR_HIOR] = 0;
>>  
>>      lpcr = env->spr[SPR_LPCR];
>> @@ -288,6 +283,13 @@ static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore *sc, 
>> int i, Error **errp)
>>  
>>      cpu->machine_data = g_new0(SpaprCpuState, 1);
>>  
>> +    /*
>> +     * All CPUs start halted. CPU0 is unhalted from the machine level reset 
>> code
>> +     * and the rest are explicitly started up by the guest using an RTAS 
>> call.
>> +     */
>> +    object_property_set_bool(OBJECT(cs), "start-powered-off", true,
>> +                             &error_abort);
>
> Since here object_new() is used, it is simpler to set the field before
> the object is realized, similarly to cs->cpu_index:
>
> -- >8 --
> @@ -275,6 +275,11 @@ static PowerPCCPU *spapr_create_vcpu(SpaprCpuCore
> *sc, int i, Error **errp)
>      cs = CPU(obj);
>      cpu = POWERPC_CPU(obj);
>      cs->cpu_index = cc->core_id + i;
> +    /*
> +     * All CPUs start halted. CPU0 is unhalted from the machine level
> reset code
> +     * and the rest are explicitly started up by the guest using an
> RTAS call.
> +     */
> +    cs->start_powered_off = true;
>      spapr_set_vcpu_id(cpu, cs->cpu_index, &local_err);
>      if (local_err) {
>          goto err;
> ---

Good point. I adopted your suggestion.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center