[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH 3/3] spapr: fix device tree properties when using
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [PATCH 3/3] spapr: fix device tree properties when using compatibility mode |
Date: |
Thu, 18 Jan 2018 15:07:54 +1100 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
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 <address@hidden>
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