From patchwork Tue Nov 29 19:55:32 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laszlo Ersek X-Patchwork-Id: 84905 Delivered-To: patch@linaro.org Received: by 10.182.1.168 with SMTP id 8csp2483014obn; Tue, 29 Nov 2016 11:57:37 -0800 (PST) X-Received: by 10.237.36.154 with SMTP id t26mr25091317qtc.188.1480449457756; Tue, 29 Nov 2016 11:57:37 -0800 (PST) Return-Path: Received: from lists.gnu.org (lists.gnu.org. [2001:4830:134:3::11]) by mx.google.com with ESMTPS id x59si28338332qte.57.2016.11.29.11.57.37 for (version=TLS1 cipher=AES128-SHA bits=128/128); Tue, 29 Nov 2016 11:57:37 -0800 (PST) Received-SPF: pass (google.com: domain of qemu-devel-bounces+patch=linaro.org@nongnu.org designates 2001:4830:134:3::11 as permitted sender) client-ip=2001:4830:134:3::11; Authentication-Results: mx.google.com; spf=pass (google.com: domain of qemu-devel-bounces+patch=linaro.org@nongnu.org designates 2001:4830:134:3::11 as permitted sender) smtp.mailfrom=qemu-devel-bounces+patch=linaro.org@nongnu.org Received: from localhost ([::1]:39048 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cBoX2-0006po-H3 for patch@linaro.org; Tue, 29 Nov 2016 14:57:36 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57927) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1cBoVJ-00066y-3o for qemu-devel@nongnu.org; Tue, 29 Nov 2016 14:55:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1cBoVH-0000TS-Hz for qemu-devel@nongnu.org; Tue, 29 Nov 2016 14:55:49 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51864) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1cBoVD-0000Qs-Vs; Tue, 29 Nov 2016 14:55:44 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D95177F7B6; Tue, 29 Nov 2016 19:55:42 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-57.phx2.redhat.com [10.3.116.57]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uATJtaE4030186; Tue, 29 Nov 2016 14:55:40 -0500 From: Laszlo Ersek To: qemu devel list Date: Tue, 29 Nov 2016 20:55:32 +0100 Message-Id: <20161129195533.23113-2-lersek@redhat.com> In-Reply-To: <20161129195533.23113-1-lersek@redhat.com> References: <20161129195533.23113-1-lersek@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Tue, 29 Nov 2016 19:55:42 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PATCH for-2.8 v2 1/2] loader: fix handling of custom address spaces when adding ROM blobs 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: Peter Maydell , "Michael S. Tsirkin" , Shannon Zhao , Alistair Francis , Michael Walle , qemu-arm@nongnu.org, Paolo Bonzini , Igor Mammedov Errors-To: qemu-devel-bounces+patch=linaro.org@nongnu.org Sender: "Qemu-devel" * Commit 3e76099aacb4 ("loader: Allow a custom AddressSpace when loading ROMs") introduced the "Rom.as" field: (1) It modified the utility callers of rom_insert() to take "as" as a new parameter from *their* callers, and set "rom->as" from that parameter. The functions covered were rom_add_file() and rom_add_elf_program(). (2) It also modified rom_insert() itself, to auto-assign "&address_space_memory", in case the external caller passed -- and the utility caller forwarded -- as=NULL. Except, commit 3e76099aacb4 forgot to update the third utility caller of rom_insert(), under point (1), namely rom_add_blob(). * Later, commit 5e774eb3bd264 ("loader: Add AddressSpace loading support to uImages") added the load_uimage_as() function, and the rom_add_blob_fixed_as() function-like macro, with the necessary changes elsewhere to propagate the new "as" parameter to rom_add_blob(): load_uimage_as() load_uboot_image() rom_add_blob_fixed_as() rom_add_blob() At this point, the signature (and workings) of rom_add_blob() had been broken already, and the rom_add_blob_fixed_as() macro passed its "_as" parameter to rom_add_blob() as "callback_opaque". Given that the "fw_callback" parameter itself was set to NULL (correctly), this did no additional damage (the opaque arg would never be used), but ultimately it broke the new functionality of load_uimage_as(). * The load_uimage_as() function would be put to use in one of the later patches, commit e481a1f63c93 ("generic-loader: Add a generic loader"). * We can fix this only in a unified patch now. Append "AddressSpace *as" to the signature of rom_add_blob(), and handle the new parameter. Pass NULL from all current callers, except from rom_add_blob_fixed_as(), where "_as" has to be bumped to the proper position. * Note that rom_add_file() rejects the case when both "mr" and "as" are passed in as non-NULL. The action that this is apparently supposed to prevent is the rom->mr = mr; assignment (that's the only place where the "mr" parameter is used in rom_add_file()). In rom_add_blob() though, we have no "mr" parameter, and the actions done on the fw_cfg branch: if (fw_file_name && fw_cfg) { if (mc->rom_file_has_mr) { data = rom_set_mr(rom, OBJECT(fw_cfg), devpath); mr = rom->mr; } else { data = rom->data; } reflect those that are performed by rom_add_file() too (with mr==NULL): if (rom->fw_file && fw_cfg) { if ((!option_rom || mc->option_rom_has_mr) && mc->rom_file_has_mr) { data = rom_set_mr(rom, OBJECT(fw_cfg), devpath); } else { data = rom->data; } Hence we need no additional restrictions in rom_add_blob(). * Stable is not affected as both problematic commits appeared first in v2.8.0-rc0. Cc: "Michael S. Tsirkin" Cc: Alistair Francis Cc: Igor Mammedov Cc: Michael Walle Cc: Paolo Bonzini Cc: Peter Maydell Cc: Shannon Zhao Cc: qemu-arm@nongnu.org Cc: qemu-devel@nongnu.org Fixes: 3e76099aacb4dae0d37ebf95305369e03d1491e6 Fixes: 5e774eb3bd264c76484906f4bd0fb38e00b8090e Signed-off-by: Laszlo Ersek Reviewed-by: Alistair Francis Reviewed-by: Michael S. Tsirkin --- Notes: v2: - no changes, pick up R-b's from Alistair and Michael hw/lm32/lm32_hwsetup.h | 2 +- include/hw/loader.h | 6 +++--- hw/arm/virt-acpi-build.c | 2 +- hw/core/loader.c | 4 +++- hw/i386/acpi-build.c | 2 +- 5 files changed, 9 insertions(+), 7 deletions(-) -- 2.9.2 diff --git a/hw/lm32/lm32_hwsetup.h b/hw/lm32/lm32_hwsetup.h index b71e6eafba48..23e18784dffd 100644 --- a/hw/lm32/lm32_hwsetup.h +++ b/hw/lm32/lm32_hwsetup.h @@ -75,7 +75,7 @@ static inline void hwsetup_create_rom(HWSetup *hw, hwaddr base) { rom_add_blob("hwsetup", hw->data, TARGET_PAGE_SIZE, - TARGET_PAGE_SIZE, base, NULL, NULL, NULL); + TARGET_PAGE_SIZE, base, NULL, NULL, NULL, NULL); } static inline void hwsetup_add_u8(HWSetup *hw, uint8_t u) diff --git a/include/hw/loader.h b/include/hw/loader.h index 038170624d8f..0c864cfd6046 100644 --- a/include/hw/loader.h +++ b/include/hw/loader.h @@ -180,7 +180,7 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len, size_t max_len, hwaddr addr, const char *fw_file_name, FWCfgReadCallback fw_callback, - void *callback_opaque); + void *callback_opaque, AddressSpace *as); int rom_add_elf_program(const char *name, void *data, size_t datasize, size_t romsize, hwaddr addr, AddressSpace *as); int rom_check_and_register_reset(void); @@ -194,7 +194,7 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict); #define rom_add_file_fixed(_f, _a, _i) \ rom_add_file(_f, NULL, _a, _i, false, NULL, NULL) #define rom_add_blob_fixed(_f, _b, _l, _a) \ - rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL) + rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, NULL) #define rom_add_file_mr(_f, _mr, _i) \ rom_add_file(_f, NULL, 0, _i, false, _mr, NULL) #define rom_add_file_as(_f, _as, _i) \ @@ -202,7 +202,7 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict); #define rom_add_file_fixed_as(_f, _a, _i, _as) \ rom_add_file(_f, NULL, _a, _i, false, NULL, _as) #define rom_add_blob_fixed_as(_f, _b, _l, _a, _as) \ - rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, _as) + rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL, _as) #define PC_ROM_MIN_VGA 0xc0000 #define PC_ROM_MIN_OPTION 0xc8000 diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c index f953610018c4..d4160dfa7d34 100644 --- a/hw/arm/virt-acpi-build.c +++ b/hw/arm/virt-acpi-build.c @@ -809,7 +809,7 @@ static MemoryRegion *acpi_add_rom_blob(AcpiBuildState *build_state, uint64_t max_size) { return rom_add_blob(name, blob->data, acpi_data_len(blob), max_size, -1, - name, virt_acpi_build_update, build_state); + name, virt_acpi_build_update, build_state, NULL); } static const VMStateDescription vmstate_virt_acpi_build = { diff --git a/hw/core/loader.c b/hw/core/loader.c index 6e022b5ad583..c0d645a87134 100644 --- a/hw/core/loader.c +++ b/hw/core/loader.c @@ -978,7 +978,8 @@ err: MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len, size_t max_len, hwaddr addr, const char *fw_file_name, - FWCfgReadCallback fw_callback, void *callback_opaque) + FWCfgReadCallback fw_callback, void *callback_opaque, + AddressSpace *as) { MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); Rom *rom; @@ -986,6 +987,7 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len, rom = g_malloc0(sizeof(*rom)); rom->name = g_strdup(name); + rom->as = as; rom->addr = addr; rom->romsize = max_len ? max_len : len; rom->datasize = len; diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 45a2ccfc4c60..9708cdc463df 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -2936,7 +2936,7 @@ static MemoryRegion *acpi_add_rom_blob(AcpiBuildState *build_state, uint64_t max_size) { return rom_add_blob(name, blob->data, acpi_data_len(blob), max_size, -1, - name, acpi_build_update, build_state); + name, acpi_build_update, build_state, NULL); } static const VMStateDescription vmstate_acpi_build = {