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>
---
 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) {
             /*
-             * 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_hotplug_req_add_by_index(drc);
         } else {
             spapr_drc_reset(drc);
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 94afeb399e..aa17626cda 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -70,7 +70,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
     cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
 
     qemu_register_reset(spapr_cpu_reset, cpu);
-    spapr_cpu_reset(cpu);
 }
 
 /*
@@ -208,6 +207,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 1129f344aa..1a38544706 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
-- 
2.14.3

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."

>  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 ?

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.

>              /*
> -             * 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().

>              spapr_hotplug_req_add_by_index(drc);
>          } else {
>              spapr_drc_reset(drc);
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 94afeb399e..aa17626cda 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -70,7 +70,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
>      cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
>  
>      qemu_register_reset(spapr_cpu_reset, cpu);
> -    spapr_cpu_reset(cpu);

spapr_cpu_reset() also sets cs->halted to 1, to let the guest start
non-boot CPUs with an RTAS call. With this change, a hotplugged CPU
is started right away and may run in the guest before the hotplug
path could even reset it.

Not sure how to handle that other than setting halted to 1 in
spapr_cpu_init() as well, but you already nacked that approach
because it would mean "poking specifics in a CPU that hasn't
already been set to a known state by a reset"... 

>  }
>  
>  /*
> @@ -208,6 +207,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 1129f344aa..1a38544706 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

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).

> 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.

> 
> >              /*
> > -             * 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.

> 
> >              spapr_hotplug_req_add_by_index(drc);
> >          } else {
> >              spapr_drc_reset(drc);
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 94afeb399e..aa17626cda 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -70,7 +70,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> >      cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
> >  
> >      qemu_register_reset(spapr_cpu_reset, cpu);
> > -    spapr_cpu_reset(cpu);
> 
> spapr_cpu_reset() also sets cs->halted to 1, to let the guest start
> non-boot CPUs with an RTAS call. With this change, a hotplugged CPU
> is started right away and may run in the guest before the hotplug
> path could even reset it.

Uh.. I was assuming the plug path (the qemu side, obviously, not the
guest side) would be completed before the cpu could execute.  If
that's not the case I'll definitely have to rethink this.

> Not sure how to handle that other than setting halted to 1 in
> spapr_cpu_init() as well, but you already nacked that approach
> because it would mean "poking specifics in a CPU that hasn't
> already been set to a known state by a reset"...

Yeah.. I may have been off base with that comment.  Bear in mind that
at the time I didn't see a specific reason to avoid the double reset -
now I do.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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.
> 
> >   
> > >              spapr_hotplug_req_add_by_index(drc);
> > >          } else {
> > >              spapr_drc_reset(drc);
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index 94afeb399e..aa17626cda 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -70,7 +70,6 @@ static void spapr_cpu_init(sPAPRMachineState *spapr, PowerPCCPU *cpu,
> > >      cpu_ppc_set_papr(cpu, PPC_VIRTUAL_HYPERVISOR(spapr));
> > >  
> > >      qemu_register_reset(spapr_cpu_reset, cpu);
> > > -    spapr_cpu_reset(cpu);  
> > 
> > spapr_cpu_reset() also sets cs->halted to 1, to let the guest start
> > non-boot CPUs with an RTAS call. With this change, a hotplugged CPU
> > is started right away and may run in the guest before the hotplug
> > path could even reset it.  
> 
> Uh.. I was assuming the plug path (the qemu side, obviously, not the
> guest side) would be completed before the cpu could execute.  If
> that's not the case I'll definitely have to rethink this.
> 

It isn't the case unless @halted is set.

> > Not sure how to handle that other than setting halted to 1 in
> > spapr_cpu_init() as well, but you already nacked that approach
> > because it would mean "poking specifics in a CPU that hasn't
> > already been set to a known state by a reset"...  
> 
> Yeah.. I may have been off base with that comment.  Bear in mind that
> at the time I didn't see a specific reason to avoid the double reset -
> now I do.
> 

Fair enough :)

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

On Fri, Apr 20, 2018 at 05:39:42PM +0200, Greg Kurz wrote:
> 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.

Handling the resets via the core level might be a good idea anyway,
but I don't think it can address the problem we're hitting here.

I've investigated further and I'm pretty sure we can't fix this
without generic code changes.  cpu_common_realizefn() (which is called
by our ppc cpu realize hook via the parent_realize chain) contains
this:

    if (dev->hotplugged) {
        cpu_synchronize_post_init(cpu);
        cpu_resume(cpu);
    }

So, as soon as the hotplugged cpu is realized, it's running, which
means by the time we call the plug() hotplug handler we're too late to
do any reset initialization.

I think there are two ways to look at this:

1) The reset handlers are specifically about *system* reset, not
device reset, and so we shoudln't really expect them to be called for
hotplugged devices.  If we want to share reset initialization with
"initial" initialization, we should explicitly call the reset handler
from the (realize time) init code.. which is what we do now.

2) Common core realize should _not_ activate the cpu.  Instead that
should be the plug() handler's job.  This would require changing the
x86 cpu plug handler (and whatever else) to kick off the cpu after
realization.

For now I'm inclined to just let it stay at (1).

The problem I had which I thought required this, doesn't after all.  I
came up with a different solution that involves moving the spapr caps
initialization earlier, instead of the cpu reset later.  That turned
out to be substantially easier than I first thought, and regardless of
what we do above long term, I think it's a better way to handle the
caps.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

On Mon, 18 Jun 2018 13:42:37 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Apr 20, 2018 at 05:39:42PM +0200, Greg Kurz wrote:
> > 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.  
> 
> Handling the resets via the core level might be a good idea anyway,
> but I don't think it can address the problem we're hitting here.
> 
> I've investigated further and I'm pretty sure we can't fix this
> without generic code changes.  cpu_common_realizefn() (which is called
> by our ppc cpu realize hook via the parent_realize chain) contains
> this:
> 
>     if (dev->hotplugged) {
>         cpu_synchronize_post_init(cpu);
>         cpu_resume(cpu);
>     }
> 

Yes, these come from:

commit 13eed94ed5617b98e657163490584dc2a0cc4b32
Author: Igor Mammedov <imammedo@redhat.com>
Date:   Tue Apr 23 10:29:36 2013 +0200

    cpu: Call cpu_synchronize_post_init() from DeviceClass::realize()
    
    If hotplugged, synchronize CPU state to KVM.
    
    Signed-off-by: Igor Mammedov <imammedo@redhat.com>
    Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
    Signed-off-by: Andreas Färber <afaerber@suse.de>

and

commit 6afb4721f3e45da727110470a61aafcd6682395e
Author: Igor Mammedov <imammedo@redhat.com>
Date:   Tue Apr 23 10:29:38 2013 +0200

    cpu: Resume CPU from DeviceClass::realize() if hot-plugged
    
    Signed-off-by: Igor Mammedov <imammedo@redhat.com>
    Signed-off-by: Andreas Färber <afaerber@suse.de>

> So, as soon as the hotplugged cpu is realized, it's running, which
> means by the time we call the plug() hotplug handler we're too late to
> do any reset initialization.
> 
> I think there are two ways to look at this:
> 
> 1) The reset handlers are specifically about *system* reset, not
> device reset, and so we shoudln't really expect them to be called for
> hotplugged devices.  If we want to share reset initialization with
> "initial" initialization, we should explicitly call the reset handler
> from the (realize time) init code.. which is what we do now.
> 
> 2) Common core realize should _not_ activate the cpu.  Instead that
> should be the plug() handler's job.  This would require changing the
> x86 cpu plug handler (and whatever else) to kick off the cpu after
> realization.
> 
> For now I'm inclined to just let it stay at (1).
> 

Maybe Igor and Eduardo (now Cc'd) can provide a hint about 2) ?

> The problem I had which I thought required this, doesn't after all.  I
> came up with a different solution that involves moving the spapr caps
> initialization earlier, instead of the cpu reset later.  That turned
> out to be substantially easier than I first thought, and regardless of
> what we do above long term, I think it's a better way to handle the
> caps.
>