Re: [Qemu-devel] [Qemu-ppc] [PATCH for-2.13 01/10] spapr: Avoid redundant calls to spapr_cpu_reset()

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 11831 bytes --]

On Fri, 20 Apr 2018 11:15:01 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Fri, 20 Apr 2018 16:34:37 +1000
> David Gibson <david@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 <david@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 <david@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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]