From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53181) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1djhtP-0008UR-5o for qemu-devel@nongnu.org; Mon, 21 Aug 2017 04:17:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1djhtL-0006Mm-TX for qemu-devel@nongnu.org; Mon, 21 Aug 2017 04:17:03 -0400 Date: Mon, 21 Aug 2017 17:55:20 +1000 From: David Gibson Message-ID: <20170821075520.GJ12356@umbus.fritz.box> References: <1503297029-28436-1-git-send-email-thuth@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="va4/JQ6j8/8uipEp" Content-Disposition: inline In-Reply-To: <1503297029-28436-1-git-send-email-thuth@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2] hw/ppc/spapr: Fix segfault when instantiating a 'pc-dimm' without 'memdev' List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth Cc: qemu-devel@nongnu.org, Paolo Bonzini , Eduardo Habkost , qemu-ppc@nongnu.org, "Michael S. Tsirkin" , Igor Mammedov , Xiao Guangrong --va4/JQ6j8/8uipEp Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Aug 21, 2017 at 08:30:29AM +0200, Thomas Huth wrote: > QEMU currently crashes when trying to use a 'pc-dimm' on the pseries > machine without specifying its 'memdev' property. This happens because > pc_dimm_get_memory_region() does not check whether the 'memdev' property > has properly been set by the user. Looking closer at this function, it's > also obvious that it is using &error_abort to call another function - and > this is bad in a function that is used in the hot-plugging calling chain > since this can also cause QEMU to exit unexpectedly. >=20 > So let's fix these issues in a proper way now: Add a "Error **errp" > parameter to pc_dimm_get_memory_region() which we use in case the 'memdev' > property has not been set by the user, and which we can use instead of > the &error_abort, and change the callers of get_memory_region() to make > use of this "errp" parameter for proper error checking. >=20 > Signed-off-by: Thomas Huth ppc portions Acked-by: David Gibson and the rest Reviewed-by: David Gibson I'm happy to queue this or have someone else queue it. > --- > hw/i386/pc.c | 14 ++++++++++++-- > hw/mem/nvdimm.c | 2 +- > hw/mem/pc-dimm.c | 14 +++++++++++--- > hw/ppc/spapr.c | 42 ++++++++++++++++++++++++++++++------------ > include/hw/mem/pc-dimm.h | 2 +- > 5 files changed, 55 insertions(+), 19 deletions(-) >=20 > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 5943539..2108104 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1691,10 +1691,15 @@ static void pc_dimm_plug(HotplugHandler *hotplug_= dev, > PCMachineClass *pcmc =3D PC_MACHINE_GET_CLASS(pcms); > PCDIMMDevice *dimm =3D PC_DIMM(dev); > PCDIMMDeviceClass *ddc =3D PC_DIMM_GET_CLASS(dimm); > - MemoryRegion *mr =3D ddc->get_memory_region(dimm); > + MemoryRegion *mr; > uint64_t align =3D TARGET_PAGE_SIZE; > bool is_nvdimm =3D object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM); > =20 > + mr =3D ddc->get_memory_region(dimm, &local_err); > + if (local_err) { > + goto out; > + } > + > if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) { > align =3D memory_region_get_alignment(mr); > } > @@ -1758,10 +1763,15 @@ static void pc_dimm_unplug(HotplugHandler *hotplu= g_dev, > PCMachineState *pcms =3D PC_MACHINE(hotplug_dev); > PCDIMMDevice *dimm =3D PC_DIMM(dev); > PCDIMMDeviceClass *ddc =3D PC_DIMM_GET_CLASS(dimm); > - MemoryRegion *mr =3D ddc->get_memory_region(dimm); > + MemoryRegion *mr; > HotplugHandlerClass *hhc; > Error *local_err =3D NULL; > =20 > + mr =3D ddc->get_memory_region(dimm, &local_err); > + if (local_err) { > + goto out; > + } > + > hhc =3D HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev); > hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err); > =20 > diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c > index db896b0..952fce5 100644 > --- a/hw/mem/nvdimm.c > +++ b/hw/mem/nvdimm.c > @@ -71,7 +71,7 @@ static void nvdimm_init(Object *obj) > NULL, NULL); > } > =20 > -static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm) > +static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error = **errp) > { > NVDIMMDevice *nvdimm =3D NVDIMM(dimm); > =20 > diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c > index ea67b46..bdf6649 100644 > --- a/hw/mem/pc-dimm.c > +++ b/hw/mem/pc-dimm.c > @@ -363,7 +363,10 @@ static void pc_dimm_get_size(Object *obj, Visitor *v= , const char *name, > PCDIMMDevice *dimm =3D PC_DIMM(obj); > PCDIMMDeviceClass *ddc =3D PC_DIMM_GET_CLASS(obj); > =20 > - mr =3D ddc->get_memory_region(dimm); > + mr =3D ddc->get_memory_region(dimm, errp); > + if (!mr) { > + return; > + } > value =3D memory_region_size(mr); > =20 > visit_type_uint64(v, name, &value, errp); > @@ -411,9 +414,14 @@ static void pc_dimm_unrealize(DeviceState *dev, Erro= r **errp) > host_memory_backend_set_mapped(dimm->hostmem, false); > } > =20 > -static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm) > +static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm, Error= **errp) > { > - return host_memory_backend_get_memory(dimm->hostmem, &error_abort); > + if (!dimm->hostmem) { > + error_setg(errp, "'" PC_DIMM_MEMDEV_PROP "' property must be set= "); > + return NULL; > + } > + > + return host_memory_backend_get_memory(dimm->hostmem, errp); > } > =20 > static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dim= m) > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index f7a1972..cec441c 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2772,10 +2772,15 @@ static void spapr_memory_plug(HotplugHandler *hot= plug_dev, DeviceState *dev, > sPAPRMachineState *ms =3D SPAPR_MACHINE(hotplug_dev); > PCDIMMDevice *dimm =3D PC_DIMM(dev); > PCDIMMDeviceClass *ddc =3D PC_DIMM_GET_CLASS(dimm); > - MemoryRegion *mr =3D ddc->get_memory_region(dimm); > - uint64_t align =3D memory_region_get_alignment(mr); > - uint64_t size =3D memory_region_size(mr); > - uint64_t addr; > + MemoryRegion *mr; > + uint64_t align, size, addr; > + > + mr =3D ddc->get_memory_region(dimm, &local_err); > + if (local_err) { > + goto out; > + } > + align =3D memory_region_get_alignment(mr); > + size =3D memory_region_size(mr); > =20 > pc_dimm_memory_plug(dev, &ms->hotplug_memory, mr, align, &local_err); > if (local_err) { > @@ -2808,10 +2813,16 @@ static void spapr_memory_pre_plug(HotplugHandler = *hotplug_dev, DeviceState *dev, > { > PCDIMMDevice *dimm =3D PC_DIMM(dev); > PCDIMMDeviceClass *ddc =3D PC_DIMM_GET_CLASS(dimm); > - MemoryRegion *mr =3D ddc->get_memory_region(dimm); > - uint64_t size =3D memory_region_size(mr); > + MemoryRegion *mr; > + uint64_t size; > char *mem_dev; > =20 > + mr =3D ddc->get_memory_region(dimm, errp); > + if (!mr) { > + return; > + } > + size =3D memory_region_size(mr); > + > if (size % SPAPR_MEMORY_BLOCK_SIZE) { > error_setg(errp, "Hotplugged memory size must be a multiple of " > "%lld MB", SPAPR_MEMORY_BLOCK_SIZE / M_BYTE); > @@ -2882,7 +2893,7 @@ static sPAPRDIMMState *spapr_recover_pending_dimm_s= tate(sPAPRMachineState *ms, > { > sPAPRDRConnector *drc; > PCDIMMDeviceClass *ddc =3D PC_DIMM_GET_CLASS(dimm); > - MemoryRegion *mr =3D ddc->get_memory_region(dimm); > + MemoryRegion *mr =3D ddc->get_memory_region(dimm, &error_abort); > uint64_t size =3D memory_region_size(mr); > uint32_t nr_lmbs =3D size / SPAPR_MEMORY_BLOCK_SIZE; > uint32_t avail_lmbs =3D 0; > @@ -2912,7 +2923,7 @@ void spapr_lmb_release(DeviceState *dev) > sPAPRMachineState *spapr =3D SPAPR_MACHINE(qdev_get_hotplug_handler(= dev)); > PCDIMMDevice *dimm =3D PC_DIMM(dev); > PCDIMMDeviceClass *ddc =3D PC_DIMM_GET_CLASS(dimm); > - MemoryRegion *mr =3D ddc->get_memory_region(dimm); > + MemoryRegion *mr =3D ddc->get_memory_region(dimm, &error_abort); > sPAPRDIMMState *ds =3D spapr_pending_dimm_unplugs_find(spapr, PC_DIM= M(dev)); > =20 > /* This information will get lost if a migration occurs > @@ -2945,12 +2956,19 @@ static void spapr_memory_unplug_request(HotplugHa= ndler *hotplug_dev, > Error *local_err =3D NULL; > PCDIMMDevice *dimm =3D PC_DIMM(dev); > PCDIMMDeviceClass *ddc =3D PC_DIMM_GET_CLASS(dimm); > - MemoryRegion *mr =3D ddc->get_memory_region(dimm); > - uint64_t size =3D memory_region_size(mr); > - uint32_t nr_lmbs =3D size / SPAPR_MEMORY_BLOCK_SIZE; > - uint64_t addr_start, addr; > + MemoryRegion *mr; > + uint32_t nr_lmbs; > + uint64_t size, addr_start, addr; > int i; > sPAPRDRConnector *drc; > + > + mr =3D ddc->get_memory_region(dimm, &local_err); > + if (local_err) { > + goto out; > + } > + size =3D memory_region_size(mr); > + nr_lmbs =3D size / SPAPR_MEMORY_BLOCK_SIZE; > + > addr_start =3D object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_P= ROP, > &local_err); > if (local_err) { > diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h > index 1e483f2..6f8c3eb 100644 > --- a/include/hw/mem/pc-dimm.h > +++ b/include/hw/mem/pc-dimm.h > @@ -71,7 +71,7 @@ typedef struct PCDIMMDeviceClass { > =20 > /* public */ > void (*realize)(PCDIMMDevice *dimm, Error **errp); > - MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm); > + MemoryRegion *(*get_memory_region)(PCDIMMDevice *dimm, Error **errp); > MemoryRegion *(*get_vmstate_memory_region)(PCDIMMDevice *dimm); > } PCDIMMDeviceClass; > =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 --va4/JQ6j8/8uipEp Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEdfRlhq5hpmzETofcbDjKyiDZs5IFAlmakeUACgkQbDjKyiDZ s5LeLg/9EBADz4sm1w+6zEstghZpdHFwCsK0muJ5rZmJGpAslWJL1lGK/1Ng0qF4 fFmuu+HtE4Zut0avPSMzevAQ/KPvZgF9DyYsUdZWAN+PqBSJYpMXGlqP5meGG8Xe jQX4EoEf1TffIjxOoonUuUmWQQpGajUtHuC2sepAcclWfGkGRqZQBbCu/oyEETdF cxZ+8xN8fO2lMp7n10U424Ens5ddXkt0CJ2kvmW+TelWK4D1t+453yneqbntqj/Z HXtoejy3QiRONbjniALMp4afBOzdPMB6gfbpE/ZXZMKtHqCvuX8wG81RjIWmtots OxA1ShCZDVXwadn/6VvqAWDx2KuubDa12EH/HCoHjEYuONrAAeGBmWV/IQbuVRyt FzZOPYct4A2zIXLG0llgTGoYVO2YB7eb8ouf4d6UN5TPcpZO664HMEeNXVhjve+u dxbs8PIKI+ngQ3N7Gu7DSTQ49bgMr9NKALvCAxhoNTHA879hndk7YwCh8NNmod7b BbFoqOA5rjtkyzUNFyKyhum57nTLgg/PqQWPjyo3hSV9SDgf9qQ6Z2ZMWclEwDWD btqNKoiCWm6IGNn6Tj6JrL/iGjSVdfPuB/exRT0IDRgUmSz5fryBCdeZo60jhilQ dD/cZh8kyNdU78Qo0M7kJjgqnpROPAVChdIlHROloKnTmFZsoGM= =gRRa -----END PGP SIGNATURE----- --va4/JQ6j8/8uipEp--