Re: [Qemu-devel] [PATCH 2/3] spapr_cpu_core: don't reset CPUs during realization

[Greg Kurz <groug@kaod.org>]

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

On Thu, 18 Jan 2018 14:43:40 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jan 17, 2018 at 10:20:35AM +0100, Greg Kurz wrote:
> > When QEMU is started, all cold-plugged CPUs are reset twice: first
> > during initialization and then during machine reset. This is sub-
> > optimal.
> > 
> > The first reset is only needed for hot-plugged CPUs because the CPU
> > hotplug code doesn't reset them. This patch adds the necessary code
> > to reset hot-plugged CPUs on the CPU core hotplug path, and removes
> > the now useless initial CPU reset.
> > 
> > We just need to mark the newly created CPU as halted to prevent it
> > to run until it is put online later.
> > 
> > Full CPU reset is now explicitely triggered from the machine code
> > only, either during system reset or during CPU hotplug.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> Hrm, this looks ok in outline, but makes me nervous in a couple of
> ways.
> 
> 
> > ---
> >  hw/ppc/spapr.c                  |    8 ++++++++
> >  hw/ppc/spapr_cpu_core.c         |    8 ++++++--
> >  include/hw/ppc/spapr_cpu_core.h |    2 ++
> >  3 files changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index bca838fce638..a2ff401f738a 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -3336,6 +3336,14 @@ static void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >          void *fdt;
> >          int fdt_offset;
> >  
> > +        if (hotplugged) {  
> 
> First, I'm always wary of using the hotplugged parameter, because what
> qemu means by it often doesn't line up with what PAPR means by it.
> 

Hmmm... you're right, hotplugged in QDEV simply means that the device
was not created during initial machine startup.

ie, any device added with QMP/HMP is always hotplugged.

To cope with the DRC state management, commit 94fd9cbaa3190 added the
fact that QEMU mustn't be waiting for an incoming migration as well.

ie, if QEMU is started with -incoming and CPUs are added before migration
starts, like libvirt does, this code wouldn't reset the CPUs...

I guess we should check qdev->hotplugged instead. Makes sense ?

> > +            int i;
> > +
> > +            for (i = 0; i < cc->nr_threads; i++) {
> > +                spapr_cpu_reset(core->threads[i]);
> > +            }
> > +        }
> > +
> >          fdt = spapr_populate_hotplug_cpu_dt(cs, &fdt_offset, spapr);
> >  
> >          spapr_drc_attach(drc, dev, fdt, fdt_offset, &local_err);
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index ac19b2e0b72c..268be7784efb 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -22,7 +22,7 @@
> >  #include "sysemu/hw_accel.h"
> >  #include "qemu/error-report.h"
> >  
> > -static void spapr_cpu_reset(void *opaque)
> > +void spapr_cpu_reset(void *opaque)
> >  {
> >      PowerPCCPU *cpu = opaque;
> >      CPUState *cs = CPU(cpu);
> > @@ -63,7 +63,11 @@ 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);
> > +
> > +    /* CPU must not execute anything until explicitely started otherwise the
> > +     * guest will crash.
> > +     */
> > +    CPU(cpu)->halted = 1;  
> 
> And poking specifics in a CPU that hasn't already been set to a known
> state by a reset also worries me.
> 

IIUC the halted flag doesn't really depend any CPU state. It is
only a way to prevent the CPU from executing, which is needed
if the CPU wasn't set to a known state.

FWIW I've seen other places where it is set before resetting the
CPU (eg, s390_cpu_initfn() or cpu_devinit() for Sun4m).

I was thinking of another solution: create a DeviceClass reset function
that would call spapr_cpu_reset() for all CPUs of a core, and register
it in spapr_cpu_core_class_init(). It would be called by QDEV during
realization of hot-plugged cores only. Unfortunately, this also happens
after the call to spapr_core_plug() (see device_set_realized()).

> >  }
> >  
> >  /*
> > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> > index 1129f344aa0c..763a2168461e 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_reset(void *opaque);
> > +
> >  #endif
> >   
> 


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