All of lore.kernel.org
 help / color / mirror / Atom feed
From: Thiago Jung Bauermann <bauerman@linux.ibm.com>
To: qemu-ppc@nongnu.org
Cc: Thiago Jung Bauermann <bauerman@linux.ibm.com>,
	Eduardo Habkost <ehabkost@redhat.com>,
	qemu-devel@nongnu.org, David Gibson <david@gibson.dropbear.id.au>
Subject: [PATCH] cpu: Add starts_halted() method
Date: Tue,  7 Jul 2020 17:43:33 -0300	[thread overview]
Message-ID: <20200707204333.261506-1-bauerman@linux.ibm.com> (raw)

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


             reply	other threads:[~2020-07-07 20:49 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-07 20:43 Thiago Jung Bauermann [this message]
2020-07-07 21:49 ` [PATCH] cpu: Add starts_halted() method Eduardo Habkost
2020-07-07 23:28   ` Thiago Jung Bauermann
2020-07-08  8:38     ` Philippe Mathieu-Daudé
2020-07-08 10:00       ` David Gibson
2020-07-08 13:14         ` Peter Maydell
2020-07-08 15:25           ` Eduardo Habkost
2020-07-08 15:32             ` Peter Maydell
2020-07-08 16:03               ` Eduardo Habkost
2020-07-08 17:09                 ` Peter Maydell
2020-07-08 17:36                   ` Eduardo Habkost
2020-07-08 20:11                     ` Peter Maydell
2020-07-08 21:32                       ` Eduardo Habkost
2020-07-09  3:05                         ` Thiago Jung Bauermann
2020-07-09  3:26                           ` Thiago Jung Bauermann
2020-07-09 10:24                             ` Philippe Mathieu-Daudé
2020-07-10 20:02                               ` Thiago Jung Bauermann
2020-07-10 20:17                                 ` Eduardo Habkost
     [not found]                           ` <87k0zdm63s.fsf@linaro.org>
2020-07-10 20:16                             ` Thiago Jung Bauermann
2020-07-11 17:55                               ` Alex Bennée
2020-07-08 16:45             ` Philippe Mathieu-Daudé
2020-07-08 21:39               ` Eduardo Habkost
2020-07-09  5:11                 ` Philippe Mathieu-Daudé
2020-07-09  9:54                   ` Greg Kurz
2020-07-09 10:18                     ` Philippe Mathieu-Daudé
2020-07-09 10:55                       ` Greg Kurz
2020-07-09 12:21                         ` Philippe Mathieu-Daudé
2020-07-09 13:13                           ` Greg Kurz
2020-07-09 13:19                             ` Philippe Mathieu-Daudé
2020-07-09 13:40                             ` Peter Maydell

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200707204333.261506-1-bauerman@linux.ibm.com \
    --to=bauerman@linux.ibm.com \
    --cc=david@gibson.dropbear.id.au \
    --cc=ehabkost@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-ppc@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.