diff mbox series

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

Message ID 20200722035016.469075-4-bauerman@linux.ibm.com
State New
Headers show
Series Generalize start-powered-off property from ARM | expand

Commit Message

Thiago Jung Bauermann July 22, 2020, 3:50 a.m. UTC
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.

Comments

Philippe Mathieu-Daudé July 22, 2020, 7:13 a.m. UTC | #1
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;
>  
>
Thiago Jung Bauermann July 23, 2020, 12:40 a.m. UTC | #2
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.
diff mbox series

Patch

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);
+
     object_unref(obj);
     return cpu;