From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36625) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bMQoG-0002zH-B1 for qemu-devel@nongnu.org; Sun, 10 Jul 2016 22:19:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bMQoB-0002Fo-5d for qemu-devel@nongnu.org; Sun, 10 Jul 2016 22:18:59 -0400 Date: Mon, 11 Jul 2016 11:42:01 +1000 From: David Gibson Message-ID: <20160711014201.GE16355@voom.fritz.box> References: <146798352770.17402.11063109294574588761.stgit@bahia.lan> <20160708174701.2686c00b@bahia.lan> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="AjmyJqqohANyBN/e" Content-Disposition: inline In-Reply-To: <20160708174701.2686c00b@bahia.lan> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH] spapr: fix core unplug crash List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org, Bharata B Rao --AjmyJqqohANyBN/e Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Jul 08, 2016 at 05:47:01PM +0200, Greg Kurz wrote: > On Fri, 08 Jul 2016 15:12:07 +0200 > Greg Kurz wrote: >=20 > > If the host has 8 threads/core and the guest is started with: > >=20 > > -smp cores=3D1,threads=3D4,maxcpus=3D12 > >=20 > > It is possible to crash QEMU by doing: > >=20 > > (qemu) device_add host-spapr-cpu-core,core-id=3D16,id=3Dfoo > > (qemu) device_del foo > > Segmentation fault > >=20 > > This is caused because spapr_core_unplug() assumes cpu_dt_id =3D=3D cor= e_id. > > Even if it happens to be the case when the host and guest have the same > > number of threads per core, it is conceptually wrong and we may pass a > > bogus id to spapr_dr_connector_by_id() and spapr_core_release() crashes. > >=20 > > Let's use cc->core_id, which is the id that was used to create th DR > > connector. >=20 > My bad, I got excited and pointed out the wrong culprit... it is cpu_index > again of course ! Please find an updated explanation to be put in the > changelog after "Segmentation fault": >=20 > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > This happens because spapr_core_unplug() assumes cpu_dt_id =3D=3D core_id. > As long as cpu_dt_id is derived from the non-table cpu_index, this is > only true when you plug cores with contiguous ids. >=20 > It is safer to be consistent: the DR connector was created with an > index that is immediately written to cc->core_id, and spapr_core_plug() > also relies on cc->core_id. >=20 > Let's use it also in spapr_core_unplug(). > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D Reworded in place, thanks. >=20 > >=20 > > Signed-off-by: Greg Kurz > > --- > > hw/ppc/spapr_cpu_core.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > >=20 > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c > > index 70b6b0b5ee17..106eaf45b399 100644 > > --- a/hw/ppc/spapr_cpu_core.c > > +++ b/hw/ppc/spapr_cpu_core.c > > @@ -126,11 +126,9 @@ static void spapr_core_release(DeviceState *dev, v= oid *opaque) > > void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev, > > Error **errp) > > { > > - sPAPRCPUCore *core =3D SPAPR_CPU_CORE(OBJECT(dev)); > > - PowerPCCPU *cpu =3D POWERPC_CPU(core->threads); > > - int id =3D ppc_get_vcpu_dt_id(cpu); > > + CPUCore *cc =3D CPU_CORE(dev); > > sPAPRDRConnector *drc =3D > > - spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id); > > + spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cc->core= _id); > > sPAPRDRConnectorClass *drck; > > Error *local_err =3D NULL; > > =20 > >=20 > >=20 >=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 --AjmyJqqohANyBN/e Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXgvloAAoJEGw4ysog2bOSoMQP/1pLGLmC8UexY63jGwIg8NXu fWfha5EIptRArfYFbSyFAlvsZd/Y/dz5EU++qvCuLbUwUABs/E4ZWMpLU55X/pJU JojapF7T25I4zGBlhvpuTEx8nfSBkX4TGPCOvudMHffV2RPmYFJUR8mJ6eOGTLiG /QjKngk5Jy++wZBSUMsENGV8Dc+t8IIMhuCxufhUjeFVPPhSjXQl7FjZye1EvcAQ KTUNFeL/dYqlN+vnHZ2HYFT+kTHeq+R8yZvOxsfj/MXb8eVMvg5WG2hiRb0ie9YI 7bKuduwWam7UCKvAtYTWfK+2MEGwc+3lPX/slcuhhyqxfWiGf2vJfpNyt1/tyVi5 EcRouBPUzouzMJlBqctKyb2FV/k9ofM6gtBnCbTMCczkHrryyJJKlplGehGxWluT E7vYthmdzzgXGoc1guKX+vgYSNXMjF8YnRuh38kcIcsat3rxXo/ZPAVrcWo48Rkp DBbZ9wgruTc2y1B7w7ciU3QU3bxKmxN9obPmmppVlXOmAwNzkj14x5R4S4JpiPbr aoWxglZNQUlEiiAKIwI1NVourC8tq1GE1hE5OwSwHKwoV8ouXHf9TLI3al7BTTMS 97ZjNwEfVZ+wHwlDIpwuA2q2VyGwH2iB3q0Rogu9plO5j397VAfs5N+fWJNcCih3 bA85sdOGO2AGC8CHEByk =Fdvb -----END PGP SIGNATURE----- --AjmyJqqohANyBN/e--