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

[Philippe Mathieu-Daudé]

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;
---

> +
>      object_unref(obj);
>      return cpu;
>  
>