On Fri, 20 Apr 2018 16:34:37 +1000 David Gibson <da...@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 <da...@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 <da...@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 :)
pgpFEViI85ypS.pgp
Description: OpenPGP digital signature
