[PATCH] cpu: Add starts_halted() method

* [PATCH] cpu: Add starts_halted() method
@ 2020-07-07 20:43 Thiago Jung Bauermann
  2020-07-07 21:49 ` Eduardo Habkost
  0 siblings, 1 reply; 30+ messages in thread
From: Thiago Jung Bauermann @ 2020-07-07 20:43 UTC (permalink / raw)
  To: qemu-ppc; +Cc: Thiago Jung Bauermann, Eduardo Habkost, qemu-devel, David Gibson

PowerPC sPAPRs CPUs start in the halted state, but generic QEMU code
assumes that CPUs start in the non-halted state. spapr_reset_vcpu()
attempts to rectify this by setting CPUState::halted to 1. But that's too
late for hotplugged CPUs in a machine configured with 2 or mor 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 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 adding a starts_halted() method to the CPUState class, and making it
return 1 if the machine is an sPAPR guest.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
---
 hw/core/cpu.c                   |  8 +++++++-
 hw/ppc/spapr_cpu_core.c         |  5 -----
 include/hw/core/cpu.h           |  2 ++
 target/ppc/translate_init.inc.c | 16 ++++++++++++++++
 4 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/hw/core/cpu.c b/hw/core/cpu.c
index 0f23409f1d..8f9a3335d5 100644
--- a/hw/core/cpu.c
+++ b/hw/core/cpu.c
@@ -259,7 +259,7 @@ static void cpu_common_reset(DeviceState *dev)
     }
 
     cpu->interrupt_request = 0;
-    cpu->halted = 0;
+    cpu->halted = cc->starts_halted();
     cpu->mem_io_pc = 0;
     cpu->icount_extra = 0;
     atomic_set(&cpu->icount_decr_ptr->u32, 0);
@@ -275,6 +275,11 @@ static void cpu_common_reset(DeviceState *dev)
     }
 }
 
+static uint32_t cpu_common_starts_halted(void)
+{
+    return 0;
+}
+
 static bool cpu_common_has_work(CPUState *cs)
 {
     return false;
@@ -428,6 +433,7 @@ static void cpu_class_init(ObjectClass *klass, void *data)
     k->cpu_exec_exit = cpu_common_noop;
     k->cpu_exec_interrupt = cpu_common_exec_interrupt;
     k->adjust_watchpoint_address = cpu_adjust_watchpoint_address;
+    k->starts_halted = cpu_common_starts_halted;
     set_bit(DEVICE_CATEGORY_CPU, dc->categories);
     dc->realize = cpu_common_realizefn;
     dc->unrealize = cpu_common_unrealizefn;
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 26ad566f42..d0ad92240c 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];
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index b3f4b79318..7c9cd67e8d 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -219,6 +219,8 @@ typedef struct CPUClass {
     vaddr (*adjust_watchpoint_address)(CPUState *cpu, vaddr addr, int len);
     void (*tcg_initialize)(void);
 
+    uint32_t (*starts_halted)(void);
+
     /* Keep non-pointer data at the end to minimize holes.  */
     int gdb_num_core_regs;
     bool gdb_stop_before_watchpoint;
diff --git a/target/ppc/translate_init.inc.c b/target/ppc/translate_init.inc.c
index 49212bfd90..1dc1ebbdaf 100644
--- a/target/ppc/translate_init.inc.c
+++ b/target/ppc/translate_init.inc.c
@@ -36,6 +36,7 @@
 #include "qapi/visitor.h"
 #include "hw/qdev-properties.h"
 #include "hw/ppc/ppc.h"
+#include "hw/ppc/spapr.h"
 #include "mmu-book3s-v3.h"
 #include "sysemu/qtest.h"
 #include "qemu/cutils.h"
@@ -10646,6 +10647,20 @@ static void ppc_cpu_set_pc(CPUState *cs, vaddr value)
     cpu->env.nip = value;
 }
 
+static uint32_t ppc_cpu_starts_halted(void)
+{
+    SpaprMachineState *spapr =
+        (SpaprMachineState *) object_dynamic_cast(qdev_get_machine(),
+                                                  TYPE_SPAPR_MACHINE);
+
+    /*
+     * In sPAPR, 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.
+     */
+    return spapr != NULL;
+}
+
 static bool ppc_cpu_has_work(CPUState *cs)
 {
     PowerPCCPU *cpu = POWERPC_CPU(cs);
@@ -10922,6 +10937,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void *data)
 #endif
 
     cc->disas_set_info = ppc_disas_set_info;
+    cc->starts_halted = ppc_cpu_starts_halted;
 
     dc->fw_name = "PowerPC,UNKNOWN";
 }


^ permalink raw reply related	[flat|nested] 30+ messages in thread