From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40773) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ec1Uk-0003v7-5F for qemu-devel@nongnu.org; Wed, 17 Jan 2018 23:08:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ec1Ug-0003Xb-U9 for qemu-devel@nongnu.org; Wed, 17 Jan 2018 23:08:06 -0500 Date: Thu, 18 Jan 2018 15:07:54 +1100 From: David Gibson Message-ID: <20180118040754.GE30352@umbus.fritz.box> References: <151618081462.20461.3393245354775542888.stgit@bahia.lan> <151618084250.20461.13884400486623011064.stgit@bahia.lan> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="z3ND3gJe4e1E4uwh" Content-Disposition: inline In-Reply-To: <151618084250.20461.13884400486623011064.stgit@bahia.lan> Subject: Re: [Qemu-devel] [PATCH 3/3] spapr: fix device tree properties when using compatibility mode List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org --z3ND3gJe4e1E4uwh Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 >=20 > 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. >=20 > Guest started on a POWER8 host with: > -smp cores=3D2 -machine pseries,max-cpu-compat=3Dcompat7 >=20 > # dtc -f -I fs -O dts /proc/device-tree | egrep 'cpu-version|pa-features' > ibm,pa-features =3D [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 =3D <0x4d0200>; >=20 > ^^^ > second CPU core >=20 > ibm,pa-features =3D <0x600f63f 0xc70080c0>; > cpu-version =3D <0xf000003>; >=20 > ^^^ > boot CPU core >=20 > 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. >=20 > 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. >=20 > 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. >=20 > It is not needed to set the compatibility mode in "start-cpu" anymore, > so the code is dropped. >=20 > Signed-off-by: Greg Kurz 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(-) >=20 > 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); > } > =20 > + /* 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 =3D spapr_ovec_new(); > + > + ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fata= l); > + } > + > qemu_devices_reset(); > =20 > /* DRC reset may cause a device to be unplugged. This will cause tro= ubles > @@ -1504,15 +1513,6 @@ static void spapr_machine_reset(void) > rtas_addr =3D rtas_limit - RTAS_MAX_SIZE; > fdt_addr =3D rtas_addr - FDT_MAX_SIZE; > =20 > - /* 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 =3D spapr_ovec_new(); > - > - ppc_set_compat(first_ppc_cpu, spapr->max_compat_pvr, &error_fata= l); > - } > - > fdt =3D spapr_build_fdt(spapr, rtas_addr, spapr->rtas_size); > =20 > 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 !=3D first_cpu) { > env->spr[SPR_LPCR] &=3D ~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 !=3D first_cpu) { > + ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &error_a= bort); > + } > } > =20 > 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_, sPAPRMac= hineState *spapr, > CPUState *cs =3D CPU(cpu); > CPUPPCState *env =3D &cpu->env; > PowerPCCPUClass *pcc =3D POWERPC_CPU_GET_CLASS(cpu); > - Error *local_err =3D NULL; > =20 > if (!cs->halted) { > rtas_st(rets, 0, RTAS_OUT_HW_ERROR); > @@ -175,14 +174,6 @@ static void rtas_start_cpu(PowerPCCPU *cpu_, sPAPRMa= chineState *spapr, > * new cpu enters */ > kvm_cpu_synchronize_state(cs); > =20 > - /* Set compatibility mode to match existing cpus */ > - ppc_set_compat(cpu, POWERPC_CPU(first_cpu)->compat_pvr, &local_e= rr); > - if (local_err) { > - error_report_err(local_err); > - rtas_st(rets, 0, RTAS_OUT_HW_ERROR); > - return; > - } > - > env->msr =3D (1ULL << MSR_SF) | (1ULL << MSR_ME); > =20 > /* Enable Power-saving mode Exit Cause exceptions for the new CP= U */ >=20 --=20 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 --z3ND3gJe4e1E4uwh Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEdfRlhq5hpmzETofcbDjKyiDZs5IFAlpgHZgACgkQbDjKyiDZ s5IQCxAAxA90H6z4W49XMaDfn8v5SlHC+OVFKjnutt73eheuGZbbebn8KNlCLW9b 84RnnHVwksGWBXO34eX9RRL1lo+J9b6+wYJsrUPpvnr4rupe6EiQkVgCLdph5QaL twJP+e9MhFHeVfa2Nu2GlldorXC0sYNrlhJB7qcjJebPzDtgujvQ52pcsnZ2x0hW oAo9Gdyb/yKCj6pp5nzJ32KOginbFBknkACFO+FLWyqRGan5GqBZQ7HdKMrWO9xF bnTvYR6Wa5saRO3rCcwLIlRAUHd5+KXeV3dazOgZvAiGpy612VQR4eZmmGG7EUIN pGsgtCI4iZVweB2y8VnD/0GRZDUylGpZsvBSIsdnC8b3MQyO8CPY1eFEeHFw/no+ 1gb9CNkftSjQXv9AtytnvVdvbJ3l11QxXKu/o/68T261sg0jKJlv2TIfPpJk+AW/ vU3xlS5UFrr4/uX4dtn+JUoQs74CiFLEhFYsT3nUnzv3I0sK0BFvNCOpLn7svE6C yVVgPhGO/fCc+tHkKGTksniTDVPyyl3xDLnHaWtAf7hAgQkzNyoXTT7qtrsdPYw7 IeKpqUQ9f4b1+KugGbzGBkHO1HAx+0v2xn8BKYYCfpeHkWURnz1oK306d2VSWIkp EMrYUFnlKMi4dHiKcc2j5BsmkvHruWNoey0TZOMmnKYNOIhrP5o= =Kw/y -----END PGP SIGNATURE----- --z3ND3gJe4e1E4uwh--