On Wed, Jan 17, 2018 at 10:20:42AM +0100, Greg Kurz wrote: > Commit 51f84465dd98 changed the compatility mode setting logic: > - machine reset only sets compatibility mode for the boot CPU > - compatibility mode is set for other CPUs when they are put online > by the guest with the "start-cpu" RTAS call > > This causes a regression for machines started with max-compat-cpu: > the device tree nodes related to secondary CPU cores contain wrong > "cpu-version" and "ibm,pa-features" values, as shown below. > > Guest started on a POWER8 host with: > -smp cores=2 -machine pseries,max-cpu-compat=compat7 > > # dtc -f -I fs -O dts /proc/device-tree | egrep 'cpu-version|pa-features' > ibm,pa-features = [18 00 f6 3f c7 c0 80 f0 80 00 > 00 00 00 00 00 00 00 00 80 00 80 00 80 00 00 00]; > cpu-version = <0x4d0200>; > > ^^^ > second CPU core > > ibm,pa-features = <0x600f63f 0xc70080c0>; > cpu-version = <0xf000003>; > > ^^^ > boot CPU core > > The second core is advertised in raw POWER8 mode. This happens because > CAS assumes all CPUs to have the same compatibility mode. Since the > boot CPU already has the requested compatibility mode, the CAS code > does not set it for the secondary one, and exposes the bogus device > tree properties in in the CAS response to the guest. > > A similar situation is observed when hot-plugging a CPU core. The > related device tree properties are generated and exposed to guest > with the "ibm,configure-connector" RTAS before "start-cpu" is called. > The CPU core is advertised to the guest in raw mode as well. > > It both cases, it boils down to the fact that "start-cpu" happens too > late. This can be fixed globally by propagating the compatibility mode > of the boot CPU to the other CPUs during reset. For this to work, the > compatibility mode of the boot CPU must be set before the machine code > actually resets all CPUs. > > It is not needed to set the compatibility mode in "start-cpu" anymore, > so the code is dropped. > > Signed-off-by: Greg Kurz <gr...@kaod.org>
Applied to ppc-for-2.12.
I forgot that the RTAS call came after constructing the device tree
fragment, and is therefore too late to set the compatibility mode.
> ---
> hw/ppc/spapr.c | 18 +++++++++---------
> hw/ppc/spapr_cpu_core.c | 7 +++++++
> hw/ppc/spapr_rtas.c | 9 ---------
> 3 files changed, 16 insertions(+), 18 deletions(-)
>
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a2ff401f738a..a00bff823f95 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1484,6 +1484,15 @@ static void spapr_machine_reset(void)
> spapr_setup_hpt_and_vrma(spapr);
> }
>
> + /* if this reset wasn't generated by CAS, we should reset our
> + * negotiated options and start from scratch */
> + if (!spapr->cas_reboot) {
> + spapr_ovec_cleanup(spapr->ov5_cas);
> + spapr->ov5_cas = spapr_ovec_new();
> +
> + ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
> + }
> +
> qemu_devices_reset();
>
> /* DRC reset may cause a device to be unplugged. This will cause troubles
> @@ -1504,15 +1513,6 @@ static void spapr_machine_reset(void)
> rtas_addr = rtas_limit - RTAS_MAX_SIZE;
> fdt_addr = rtas_addr - FDT_MAX_SIZE;
>
> - /* if this reset wasn't generated by CAS, we should reset our
> - * negotiated options and start from scratch */
> - if (!spapr->cas_reboot) {
> - spapr_ovec_cleanup(spapr->ov5_cas);
> - spapr->ov5_cas = spapr_ovec_new();
> -
> - ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fatal);
> - }
> -
> fdt = spapr_build_fdt(spapr, rtas_addr, spapr->rtas_size);
>
> spapr_load_rtas(spapr, fdt, rtas_addr);
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 268be7784efb..f5e5ead5f4b0 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -44,6 +44,13 @@ void spapr_cpu_reset(void *opaque)
> if (cs != first_cpu) {
> env->spr[SPR_LPCR] &= ~pcc->lpcr_pm;
> }
> +
> + /* Set compatibility mode to match the boot CPU, which was either set
> + * by the machine reset code or by CAS. This should never fail.
> + */
> + if (cs != first_cpu) {
> + ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr,
> &error_abort);
> + }
> }
>
> static void spapr_cpu_destroy(PowerPCCPU *cpu)
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 2b89e1d448e4..4bb939d3d111 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -163,7 +163,6 @@ static void rtas_start_cpu(PowerPCCPU *cpu_,
> sPAPRMachineState *spapr,
> CPUState *cs = CPU(cpu);
> CPUPPCState *env = &cpu->env;
> PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> - Error *local_err = NULL;
>
> if (!cs->halted) {
> rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> @@ -175,14 +174,6 @@ static void rtas_start_cpu(PowerPCCPU *cpu_,
> sPAPRMachineState *spapr,
> * new cpu enters */
> kvm_cpu_synchronize_state(cs);
>
> - /* Set compatibility mode to match existing cpus */
> - ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &local_err);
> - if (local_err) {
> - error_report_err(local_err);
> - rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> - return;
> - }
> -
> env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
>
> /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
>
--
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
