On Fri, 20 Apr 2018 11:15:01 +0200 Greg Kurz <gr...@kaod.org> wrote: > On Fri, 20 Apr 2018 16:34:37 +1000 > David Gibson <da...@gibson.dropbear.id.au> wrote: > > > On Thu, Apr 19, 2018 at 03:48:23PM +0200, Greg Kurz wrote: > > > On Tue, 17 Apr 2018 17:17:13 +1000 > > > David Gibson <da...@gibson.dropbear.id.au> wrote: > > > > > > > af81cf323c1 "spapr: CPU hotplug support" added a direct call to > > > > spapr_cpu_reset() in spapr_cpu_init(), as well as registering it as a > > > > reset callback. That was in order to make sure that the reset function > > > > got called for a newly hotplugged cpu, which would miss the global > > > > machine > > > > reset. > > > > > > > > However, this change means that spapr_cpu_reset() gets called twice for > > > > normal cold-plugged cpus: once from spapr_cpu_init(), then again during > > > > the system reset. As well as being ugly in its redundancy, the first > > > > call > > > > happens before the machine reset calls have happened, which will cause > > > > problems for some things we're going to want to add. > > > > > > > > So, we remove the reset call from spapr_cpu_init(). We instead put an > > > > explicit reset call in the hotplug specific path. > > > > > > > > Signed-off-by: David Gibson <da...@gibson.dropbear.id.au> > > > > --- > > > > > > I had sent a tentative patch to do something similar earlier this year: > > > > > > https://patchwork.ozlabs.org/patch/862116/ > > > > > > but it got nacked for several reasons, one of them being you were > > > "always wary of using the hotplugged parameter, because what qemu > > > means by it often doesn't line up with what PAPR means by it." > > > > Yeah, I was and am wary of that, but convinced myself it was correct > > in this case (which doesn't really interact with the PAPR meaning of > > hotplug). > > > > > > hw/ppc/spapr.c | 6 ++++-- > > > > hw/ppc/spapr_cpu_core.c | 13 ++++++++++++- > > > > include/hw/ppc/spapr_cpu_core.h | 2 ++ > > > > 3 files changed, 18 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > > index 7b2bc4e25d..81b50af3b5 100644 > > > > --- a/hw/ppc/spapr.c > > > > +++ b/hw/ppc/spapr.c > > > > @@ -3370,9 +3370,11 @@ static void spapr_core_plug(HotplugHandler > > > > *hotplug_dev, DeviceState *dev, > > > > > > > > if (hotplugged) { > > > > > > ... but you rely on it here. Can you explain why it is > > > okay now ? > > > > So the value I actually need here is "wasn't present at the last > > system reset" (with false positives being mostly harmless, but not > > false negatives). > > > > Hmm... It is rather the other way around, sth like "will be caught > by the initial machine reset". > > > > Also, if QEMU is started with -incoming and the CPU core > > > is hotplugged before migration begins, the following will > > > return false: > > > > > > static inline bool spapr_drc_hotplugged(DeviceState *dev) > > > { > > > return dev->hotplugged && !runstate_check(RUN_STATE_INMIGRATE); > > > } > > > > > > and the CPU core won't be reset. > > > > Uh... spapr_dtc_hotplugged() would definitely be wrong here, which is > > why I'm not using it. > > > > This is how hotplugged is set in spapr_core_plug(): > > bool hotplugged = spapr_drc_hotplugged(dev); > > but to detect the "will be caught by the initial machine reset" condition, > we only need to check dev->hotplugged actually. > > > > > > > > /* > > > > - * Send hotplug notification interrupt to the guest only > > > > - * in case of hotplugged CPUs. > > > > + * For hotplugged CPUs, we need to reset them (they missed > > > > + * out on the system reset), and send the guest a > > > > + * notification > > > > */ > > > > + spapr_cpu_core_reset(core); > > > > > > spapr_cpu_reset() also sets the compat mode, which is used > > > to set some properties in the DT, ie, this should be called > > > before spapr_populate_hotplug_cpu_dt(). > > > > Good point. I've moved the reset to fix that. > >
Thinking of it again: since cold-plugged devices reach this before machine
reset, we would then attach to the DRC a DT blob based on a non-reset CPU :-\
Instead of registering a reset handler for each individual CPUs, maybe
we should rather register it a the CPU core level. The handler would
first reset all CPUs in the core and then setup the DRC for new cores only,
like it is done currently in spapr_core_plug().
spapr_core_plug() would then just need to register the reset handler,
and call it only for hotplugged cores.
I've tweaked your patch to implement this, and could do some basic
plug/unplug tests without seeing anything wrong. But I'll be on
vacation next week (currently packing), so I inline it below as
food for thought.
-----------------------------------------------------------------------------------
hw/ppc/spapr.c | 62 ++++++++++++++++++++++++++++-----------
hw/ppc/spapr_cpu_core.c | 27 +++++++++++------
include/hw/ppc/spapr_cpu_core.h | 2 +
3 files changed, 64 insertions(+), 27 deletions(-)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 9bf3eba9c5ea..51a0349c2bcc 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -3388,6 +3388,36 @@ static void *spapr_populate_hotplug_cpu_dt(CPUState *cs,
int *fdt_offset,
return fdt;
}
+static void spapr_core_reset(void *opaque)
+{
+ sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
+ DeviceState *dev = DEVICE(opaque);
+ CPUCore *cc = CPU_CORE(dev);
+ sPAPRCPUCore *sc = SPAPR_CPU_CORE(dev);
+ CPUState *cs = CPU(sc->threads[0]);
+ sPAPRDRConnector *drc;
+
+ spapr_cpu_core_reset(sc);
+
+ drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU,
+ spapr_vcpu_id(spapr, cc->core_id));
+
+ assert(drc);
+
+ if (!drc->dev) {
+ void *fdt;
+ int fdt_offset;
+
+ fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
+
+ spapr_drc_attach(drc, dev, fdt, fdt_offset, &error_abort);
+
+ if (!spapr_drc_hotplugged(dev)) {
+ spapr_drc_reset(drc);
+ }
+ }
+}
+
/* Callback to be called during DRC release. */
void spapr_core_release(DeviceState *dev)
{
@@ -3395,9 +3425,9 @@ void spapr_core_release(DeviceState *dev)
sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(ms);
CPUCore *cc = CPU_CORE(dev);
CPUArchId *core_slot = spapr_find_cpu_slot(ms, cc->core_id, NULL);
+ sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
if (smc->pre_2_10_has_unused_icps) {
- sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
int i;
for (i = 0; i < cc->nr_threads; i++) {
@@ -3407,6 +3437,8 @@ void spapr_core_release(DeviceState *dev)
}
}
+ qemu_unregister_reset(spapr_core_reset, sc);
+
assert(core_slot);
core_slot->cpu = NULL;
object_unparent(OBJECT(dev));
@@ -3448,12 +3480,9 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev,
DeviceState *dev,
sPAPRMachineClass *smc = SPAPR_MACHINE_CLASS(mc);
sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
CPUCore *cc = CPU_CORE(dev);
- CPUState *cs = CPU(core->threads[0]);
sPAPRDRConnector *drc;
- Error *local_err = NULL;
CPUArchId *core_slot;
int index;
- bool hotplugged = spapr_drc_hotplugged(dev);
core_slot = spapr_find_cpu_slot(MACHINE(hotplug_dev), cc->core_id, &index);
if (!core_slot) {
@@ -3467,32 +3496,31 @@ static void spapr_core_plug(HotplugHandler
*hotplug_dev, DeviceState *dev,
g_assert(drc || !mc->has_hotpluggable_cpus);
if (drc) {
- void *fdt;
- int fdt_offset;
-
- fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
-
- spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err);
- if (local_err) {
- g_free(fdt);
- error_propagate(errp, local_err);
- return;
+ /* The boot core and any core specified on the command line will
+ * be reset during the initial machine reset. Filter them out based
+ * on @hotplugged which is set to false in this case. Another case
+ * is when a core is added after the machine was shutdown: no need
+ * to reset here since a system reset is needed to restart the machine.
+ */
+ if (dev->hotplugged && !runstate_check(RUN_STATE_SHUTDOWN)) {
+ spapr_core_reset(core);
}
- if (hotplugged) {
+ if (spapr_drc_hotplugged(dev)) {
/*
* Send hotplug notification interrupt to the guest only
* in case of hotplugged CPUs.
*/
spapr_hotplug_req_add_by_index(drc);
- } else {
- spapr_drc_reset(drc);
}
}
+ qemu_register_reset(spapr_core_reset, core);
+
core_slot->cpu = OBJECT(dev);
if (smc->pre_2_10_has_unused_icps) {
+ CPUState *cs = CPU(core->threads[0]);
int i;
for (i = 0; i < cc->nr_threads; i++) {
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 90e4b72c3e96..aaae90bbcb6d 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -22,9 +22,8 @@
#include "sysemu/hw_accel.h"
#include "qemu/error-report.h"
-static void spapr_cpu_reset(void *opaque)
+static void spapr_cpu_reset(PowerPCCPU *cpu)
{
- PowerPCCPU *cpu = opaque;
CPUState *cs = CPU(cpu);
CPUPPCState *env = &cpu->env;
PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
@@ -50,11 +49,6 @@ static void spapr_cpu_reset(void *opaque)
}
-static void spapr_cpu_destroy(PowerPCCPU *cpu)
-{
- qemu_unregister_reset(spapr_cpu_reset, cpu);
-}
-
static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
Error **errp)
{
@@ -66,8 +60,10 @@ static void spapr_cpu_init(sPAPRMachineState *spapr,
PowerPCCPU *cpu,
/* Enable PAPR mode in TCG or KVM */
cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
- qemu_register_reset(spapr_cpu_reset, cpu);
- spapr_cpu_reset(cpu);
+ /* 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 */
+ CPU(cpu)->halted = 1;
}
/*
@@ -101,7 +97,6 @@ static void spapr_cpu_core_unrealizefn(DeviceState *dev,
Error **errp)
CPUState *cs = CPU(dev);
PowerPCCPU *cpu = POWERPC_CPU(cs);
- spapr_cpu_destroy(cpu);
object_unparent(cpu->intc);
cpu_remove_sync(cs);
object_unparent(obj);
@@ -205,6 +200,18 @@ err:
error_propagate(errp, local_err);
}
+void spapr_cpu_core_reset(sPAPRCPUCore *sc)
+{
+ CPUCore *cc = CPU_CORE(sc);
+ int i;
+
+ for (i = 0; i < cc->nr_threads; i++) {
+ PowerPCCPU *cpu = sc->threads[i];
+
+ spapr_cpu_reset(cpu);
+ }
+}
+
static Property spapr_cpu_core_properties[] = {
DEFINE_PROP_INT32("node-id", sPAPRCPUCore, node_id,
CPU_UNSET_NUMA_NODE_ID),
DEFINE_PROP_END_OF_LIST()
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index 1129f344aa0c..1a38544706e7 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -38,4 +38,6 @@ typedef struct sPAPRCPUCoreClass {
} sPAPRCPUCoreClass;
const char *spapr_get_cpu_core_type(const char *cpu_type);
+void spapr_cpu_core_reset(sPAPRCPUCore *sc);
+
#endif
pgpFoVy1KHnW8.pgp
Description: OpenPGP digital signature
