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

[David Gibson <david@gibson.dropbear.id.au>]

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

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.

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

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