Re: [Qemu-devel] [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: 5473 bytes --]

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

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