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