From patchwork Mon Aug 21 10:35:24 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Gibson X-Patchwork-Id: 9912047 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 9CBA2600C8 for ; Mon, 21 Aug 2017 10:36:35 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 8164428557 for ; Mon, 21 Aug 2017 10:36:35 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 7602728748; Mon, 21 Aug 2017 10:36:35 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 5291A28557 for ; Mon, 21 Aug 2017 10:36:33 +0000 (UTC) Received: from localhost ([::1]:60311 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1djk4O-0000ch-Sd for patchwork-qemu-devel@patchwork.kernel.org; Mon, 21 Aug 2017 06:36:32 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58566) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1djk3b-0000cO-KQ for qemu-devel@nongnu.org; Mon, 21 Aug 2017 06:35:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1djk3Y-00047B-CD for qemu-devel@nongnu.org; Mon, 21 Aug 2017 06:35:43 -0400 Received: from ozlabs.org ([103.22.144.67]:35565) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1djk3X-00040Y-O9; Mon, 21 Aug 2017 06:35:40 -0400 Received: by ozlabs.org (Postfix, from userid 1007) id 3xbVT30lBDz9sNn; Mon, 21 Aug 2017 20:35:35 +1000 (AEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=gibson.dropbear.id.au; s=201602; t=1503311735; bh=BJOFcTTwJRWIFaXmzibsN2HhCOaQVpzTni5MRZ1oG9E=; h=From:To:Cc:Subject:Date:From; b=A94Xm4MV0wBUnzhDlFpNS7KybmU2+Z2/ynaFLIGCxOHDioHWOafHvOekjlBCv/RqK xSGT3C2oEEajNG0dBWV4ZkZU0aH54DAnsMY1HwyMUfqLv6vDFqajmNAo5VsLEcpSgU l/FiHlvIxbJmRM5APk62WOpDFMXD3SPWZcW43MHA= From: David Gibson To: cohuck@redhat.com, lvivier@redhat.com Date: Mon, 21 Aug 2017 20:35:24 +1000 Message-Id: <20170821103524.22619-1-david@gibson.dropbear.id.au> X-Mailer: git-send-email 2.13.5 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 103.22.144.67 Subject: [Qemu-devel] [PATCH] hw/ppc/spapr: Fix segfault when instantiating a 'pc-dimm' without 'memdev' X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: qemu-ppc@nongnu.org, thuth@redhat.com, qemu-devel@nongnu.org, David Gibson Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP From: Thomas Huth 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. 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. Signed-off-by: Thomas Huth Reviewed-by: Igor Mammedov Signed-off-by: David Gibson Reviewed-by: Laurent Vivier --- 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(-) diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 59435390ba..21081041d5 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 = PC_MACHINE_GET_CLASS(pcms); PCDIMMDevice *dimm = PC_DIMM(dev); PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); - MemoryRegion *mr = ddc->get_memory_region(dimm); + MemoryRegion *mr; uint64_t align = TARGET_PAGE_SIZE; bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM); + mr = ddc->get_memory_region(dimm, &local_err); + if (local_err) { + goto out; + } + if (memory_region_get_alignment(mr) && pcmc->enforce_aligned_dimm) { align = memory_region_get_alignment(mr); } @@ -1758,10 +1763,15 @@ static void pc_dimm_unplug(HotplugHandler *hotplug_dev, PCMachineState *pcms = PC_MACHINE(hotplug_dev); PCDIMMDevice *dimm = PC_DIMM(dev); PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); - MemoryRegion *mr = ddc->get_memory_region(dimm); + MemoryRegion *mr; HotplugHandlerClass *hhc; Error *local_err = NULL; + mr = ddc->get_memory_region(dimm, &local_err); + if (local_err) { + goto out; + } + hhc = HOTPLUG_HANDLER_GET_CLASS(pcms->acpi_dev); hhc->unplug(HOTPLUG_HANDLER(pcms->acpi_dev), dev, &local_err); diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c index db896b0bb6..952fce5ec8 100644 --- a/hw/mem/nvdimm.c +++ b/hw/mem/nvdimm.c @@ -71,7 +71,7 @@ static void nvdimm_init(Object *obj) NULL, NULL); } -static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm) +static MemoryRegion *nvdimm_get_memory_region(PCDIMMDevice *dimm, Error **errp) { NVDIMMDevice *nvdimm = NVDIMM(dimm); diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c index ea67b461c2..bdf6649083 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 = PC_DIMM(obj); PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(obj); - mr = ddc->get_memory_region(dimm); + mr = ddc->get_memory_region(dimm, errp); + if (!mr) { + return; + } value = memory_region_size(mr); visit_type_uint64(v, name, &value, errp); @@ -411,9 +414,14 @@ static void pc_dimm_unrealize(DeviceState *dev, Error **errp) host_memory_backend_set_mapped(dimm->hostmem, false); } -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); } static MemoryRegion *pc_dimm_get_vmstate_memory_region(PCDIMMDevice *dimm) diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index f7a19720dc..cec441cbf4 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2772,10 +2772,15 @@ static void spapr_memory_plug(HotplugHandler *hotplug_dev, DeviceState *dev, sPAPRMachineState *ms = SPAPR_MACHINE(hotplug_dev); PCDIMMDevice *dimm = PC_DIMM(dev); PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); - MemoryRegion *mr = ddc->get_memory_region(dimm); - uint64_t align = memory_region_get_alignment(mr); - uint64_t size = memory_region_size(mr); - uint64_t addr; + MemoryRegion *mr; + uint64_t align, size, addr; + + mr = ddc->get_memory_region(dimm, &local_err); + if (local_err) { + goto out; + } + align = memory_region_get_alignment(mr); + size = memory_region_size(mr); 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 = PC_DIMM(dev); PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); - MemoryRegion *mr = ddc->get_memory_region(dimm); - uint64_t size = memory_region_size(mr); + MemoryRegion *mr; + uint64_t size; char *mem_dev; + mr = ddc->get_memory_region(dimm, errp); + if (!mr) { + return; + } + size = 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_state(sPAPRMachineState *ms, { sPAPRDRConnector *drc; PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); - MemoryRegion *mr = ddc->get_memory_region(dimm); + MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort); uint64_t size = memory_region_size(mr); uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE; uint32_t avail_lmbs = 0; @@ -2912,7 +2923,7 @@ void spapr_lmb_release(DeviceState *dev) sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_hotplug_handler(dev)); PCDIMMDevice *dimm = PC_DIMM(dev); PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); - MemoryRegion *mr = ddc->get_memory_region(dimm); + MemoryRegion *mr = ddc->get_memory_region(dimm, &error_abort); sPAPRDIMMState *ds = spapr_pending_dimm_unplugs_find(spapr, PC_DIMM(dev)); /* This information will get lost if a migration occurs @@ -2945,12 +2956,19 @@ static void spapr_memory_unplug_request(HotplugHandler *hotplug_dev, Error *local_err = NULL; PCDIMMDevice *dimm = PC_DIMM(dev); PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); - MemoryRegion *mr = ddc->get_memory_region(dimm); - uint64_t size = memory_region_size(mr); - uint32_t nr_lmbs = 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 = ddc->get_memory_region(dimm, &local_err); + if (local_err) { + goto out; + } + size = memory_region_size(mr); + nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE; + addr_start = object_property_get_uint(OBJECT(dimm), PC_DIMM_ADDR_PROP, &local_err); if (local_err) { diff --git a/include/hw/mem/pc-dimm.h b/include/hw/mem/pc-dimm.h index 1e483f2670..6f8c3eb1b3 100644 --- a/include/hw/mem/pc-dimm.h +++ b/include/hw/mem/pc-dimm.h @@ -71,7 +71,7 @@ typedef struct PCDIMMDeviceClass { /* 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;