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

[David Gibson]

On Thu, Jan 18, 2018 at 04:39:30PM +0100, Greg Kurz wrote:
> On Thu, 18 Jan 2018 14:43:40 +1100
> David Gibson <address@hidden> 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 <address@hidden>  
> > 
> > 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 ?

Actually I don't think that dev->hotplugged is any better in terms of
meaning what you want it to.

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

Its effects do, though - e.g. the recent patches to explicitly disable
the decrementer on stopped vcpus, because halted alone wasn't enough
to keep them quiescent.

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

Well, feel free to come up with something.  AIUI there's no urgency
about this, though - I'm not too worried by extraneous resets, as long
as we have enough to be correct.


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



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

signature.asc
Description: PGP signature