Re: [Qemu-devel] [PATCH 3/3] spapr: fix device tree properties when using compatibility mode

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

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

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 <groug@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

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