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