From patchwork Fri Mar 11 11:18:54 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?q?Daniel_P=2E_Berrang=C3=A9?= X-Patchwork-Id: 8564851 Return-Path: X-Original-To: patchwork-qemu-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 6369A9F6E4 for ; Fri, 11 Mar 2016 11:19:20 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 6598A2024D for ; Fri, 11 Mar 2016 11:19:19 +0000 (UTC) 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.kernel.org (Postfix) with ESMTPS id 3801220219 for ; Fri, 11 Mar 2016 11:19:18 +0000 (UTC) Received: from localhost ([::1]:54204 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aeL6D-0005Vb-AD for patchwork-qemu-devel@patchwork.kernel.org; Fri, 11 Mar 2016 06:19:17 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45508) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aeL62-0005VT-0I for qemu-devel@nongnu.org; Fri, 11 Mar 2016 06:19:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aeL5w-0000EX-UR for qemu-devel@nongnu.org; Fri, 11 Mar 2016 06:19:05 -0500 Received: from mx1.redhat.com ([209.132.183.28]:36595) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aeL5w-0000EP-MH for qemu-devel@nongnu.org; Fri, 11 Mar 2016 06:19:00 -0500 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 3EAB96438A; Fri, 11 Mar 2016 11:19:00 +0000 (UTC) Received: from t530wlan.home.berrange.com.com (vpn1-6-25.ams2.redhat.com [10.36.6.25]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u2BBIvRi004540; Fri, 11 Mar 2016 06:18:58 -0500 From: "Daniel P. Berrange" To: qemu-devel@nongnu.org Date: Fri, 11 Mar 2016 11:18:54 +0000 Message-Id: <1457695134-10712-1-git-send-email-berrange@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.39]); Fri, 11 Mar 2016 11:19:00 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Cc: Eduardo Habkost , "Michael S. Tsirkin" , Paolo Bonzini , Richard Henderson Subject: [Qemu-devel] [PATCH v2] hw: fix error reporting for missing option ROMs X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP If QEMU fails to load any of the VGA ROMs, it prints a message to stderr and then carries on as if everything was fine, despite the VGA interface not being functional. This extends the the various rom_add_*() methods in loader.h to accept a 'Error **errp' parameter. The VGA device realizefn() impls can now pass in the errp they already have and get errors reported as fatal problems. Addition of 'Error **errp' to the load_*() methods in loader.h is left as an exercise for future interested developers, since it will require fixing up a great many callers to propagate errors correctly. Signed-off-by: Daniel P. Berrange Reviewed-by: Eric Blake --- Changed in v2: - Use error_fatal instead of NULL in places lacking an Error **errp to propagate to - Use error_setg_file_open instead of error_setg_errno - Mention that load_*() methods are intentionally not converted hw/core/loader.c | 38 +++++++++++++++++++++++--------------- hw/display/cirrus_vga.c | 4 +++- hw/display/vga-isa.c | 4 +++- hw/i386/pc.c | 6 ++++-- hw/i386/pc_sysfw.c | 5 +++-- hw/misc/sga.c | 4 +++- hw/pci/pci.c | 8 ++++++-- include/hw/loader.h | 16 +++++++++------- 8 files changed, 54 insertions(+), 31 deletions(-) diff --git a/hw/core/loader.c b/hw/core/loader.c index 8e8031c..2c9be4e 100644 --- a/hw/core/loader.c +++ b/hw/core/loader.c @@ -142,7 +142,7 @@ int load_image_targphys(const char *filename, return -1; } if (size > 0) { - rom_add_file_fixed(filename, addr, -1); + rom_add_file_fixed(filename, addr, -1, NULL); } return size; } @@ -162,7 +162,7 @@ int load_image_mr(const char *filename, MemoryRegion *mr) return -1; } if (size > 0) { - if (rom_add_file_mr(filename, mr, -1) < 0) { + if (rom_add_file_mr(filename, mr, -1, NULL) < 0) { return -1; } } @@ -831,11 +831,13 @@ static void *rom_set_mr(Rom *rom, Object *owner, const char *name) int rom_add_file(const char *file, const char *fw_dir, hwaddr addr, int32_t bootindex, - bool option_rom, MemoryRegion *mr) + bool option_rom, MemoryRegion *mr, + Error **errp) { MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); Rom *rom; - int rc, fd = -1; + int fd = -1; + ssize_t rc; char devpath[100]; rom = g_malloc0(sizeof(*rom)); @@ -847,8 +849,7 @@ int rom_add_file(const char *file, const char *fw_dir, fd = open(rom->path, O_RDONLY | O_BINARY); if (fd == -1) { - fprintf(stderr, "Could not open option rom '%s': %s\n", - rom->path, strerror(errno)); + error_setg_file_open(errp, errno, rom->path); goto err; } @@ -859,8 +860,9 @@ int rom_add_file(const char *file, const char *fw_dir, rom->addr = addr; rom->romsize = lseek(fd, 0, SEEK_END); if (rom->romsize == -1) { - fprintf(stderr, "rom: file %-20s: get size error: %s\n", - rom->name, strerror(errno)); + error_setg_errno(errp, errno, + "Could not get size of option rom '%s'", + rom->path); goto err; } @@ -868,9 +870,15 @@ int rom_add_file(const char *file, const char *fw_dir, rom->data = g_malloc0(rom->datasize); lseek(fd, 0, SEEK_SET); rc = read(fd, rom->data, rom->datasize); - if (rc != rom->datasize) { - fprintf(stderr, "rom: file %-20s: read error: rc=%d (expected %zd)\n", - rom->name, rc, rom->datasize); + if (rc < 0) { + error_setg_errno(errp, errno, + "Could not read option rom '%s'", + rom->path); + goto err; + } else if (rc != rom->datasize) { + error_setg_errno(errp, errno, + "Short read on option rom '%s' %zd vs %zd", + rom->path, rc, rom->datasize); goto err; } close(fd); @@ -975,14 +983,14 @@ int rom_add_elf_program(const char *name, void *data, size_t datasize, return 0; } -int rom_add_vga(const char *file) +int rom_add_vga(const char *file, Error **errp) { - return rom_add_file(file, "vgaroms", 0, -1, true, NULL); + return rom_add_file(file, "vgaroms", 0, -1, true, NULL, errp); } -int rom_add_option(const char *file, int32_t bootindex) +int rom_add_option(const char *file, int32_t bootindex, Error **errp) { - return rom_add_file(file, "genroms", 0, bootindex, true, NULL); + return rom_add_file(file, "genroms", 0, bootindex, true, NULL, errp); } static void rom_reset(void *unused) diff --git a/hw/display/cirrus_vga.c b/hw/display/cirrus_vga.c index 57b91a7..7fbb2b0 100644 --- a/hw/display/cirrus_vga.c +++ b/hw/display/cirrus_vga.c @@ -2977,7 +2977,9 @@ static void isa_cirrus_vga_realizefn(DeviceState *dev, Error **errp) isa_address_space(isadev), isa_address_space_io(isadev)); s->con = graphic_console_init(dev, 0, s->hw_ops, s); - rom_add_vga(VGABIOS_CIRRUS_FILENAME); + if (rom_add_vga(VGABIOS_CIRRUS_FILENAME, errp) < 0) { + return; + } /* XXX ISA-LFB support */ /* FIXME not qdev yet */ } diff --git a/hw/display/vga-isa.c b/hw/display/vga-isa.c index f5aff1c..4309ae1 100644 --- a/hw/display/vga-isa.c +++ b/hw/display/vga-isa.c @@ -72,7 +72,9 @@ static void vga_isa_realizefn(DeviceState *dev, Error **errp) vga_init_vbe(s, OBJECT(dev), isa_address_space(isadev)); /* ROM BIOS */ - rom_add_vga(VGABIOS_FILENAME); + if (rom_add_vga(VGABIOS_FILENAME, errp) < 0) { + return; + } } static Property vga_isa_properties[] = { diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 56ec6cd..471dcb4 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1264,7 +1264,8 @@ void xen_load_linux(PCMachineState *pcms) for (i = 0; i < nb_option_roms; i++) { assert(!strcmp(option_rom[i].name, "linuxboot.bin") || !strcmp(option_rom[i].name, "multiboot.bin")); - rom_add_option(option_rom[i].name, option_rom[i].bootindex); + rom_add_option(option_rom[i].name, option_rom[i].bootindex, + &error_fatal); } pcms->fw_cfg = fw_cfg; } @@ -1392,7 +1393,8 @@ void pc_memory_init(PCMachineState *pcms, } for (i = 0; i < nb_option_roms; i++) { - rom_add_option(option_rom[i].name, option_rom[i].bootindex); + rom_add_option(option_rom[i].name, option_rom[i].bootindex, + &error_fatal); } pcms->fw_cfg = fw_cfg; } diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c index 2324e70..d8dea64 100644 --- a/hw/i386/pc_sysfw.c +++ b/hw/i386/pc_sysfw.c @@ -178,6 +178,7 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw) MemoryRegion *bios, *isa_bios; int bios_size, isa_bios_size; int ret; + Error *err = NULL; /* BIOS load */ if (bios_name == NULL) { @@ -199,10 +200,10 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw) if (!isapc_ram_fw) { memory_region_set_readonly(bios, true); } - ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1); + ret = rom_add_file_fixed(bios_name, (uint32_t)(-bios_size), -1, &err); if (ret != 0) { bios_error: - fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name); + error_report_err(err); exit(1); } g_free(filename); diff --git a/hw/misc/sga.c b/hw/misc/sga.c index 03b006d..f90ad1f 100644 --- a/hw/misc/sga.c +++ b/hw/misc/sga.c @@ -41,7 +41,9 @@ typedef struct ISASGAState { static void sga_realizefn(DeviceState *dev, Error **errp) { - rom_add_vga(SGABIOS_FILENAME); + if (rom_add_vga(SGABIOS_FILENAME, errp) < 0) { + return; + } } static void sga_class_initfn(ObjectClass *klass, void *data) diff --git a/hw/pci/pci.c b/hw/pci/pci.c index e67664d..728a9ec 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -2065,9 +2065,13 @@ static void pci_add_option_rom(PCIDevice *pdev, bool is_default_rom, } if (class == 0x0300) { - rom_add_vga(pdev->romfile); + if (rom_add_vga(pdev->romfile, errp) < 0) { + return; + } } else { - rom_add_option(pdev->romfile, -1); + if (rom_add_option(pdev->romfile, -1, errp) < 0) { + return; + } } return; } diff --git a/include/hw/loader.h b/include/hw/loader.h index 0ba7808..dc9951d 100644 --- a/include/hw/loader.h +++ b/include/hw/loader.h @@ -2,6 +2,7 @@ #define LOADER_H #include "qapi/qmp/qdict.h" #include "hw/nvram/fw_cfg.h" +#include "qapi/error.h" /* loader.c */ /** @@ -118,7 +119,8 @@ extern bool rom_file_has_mr; int rom_add_file(const char *file, const char *fw_dir, hwaddr addr, int32_t bootindex, - bool option_rom, MemoryRegion *mr); + bool option_rom, MemoryRegion *mr, + Error **errp); MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len, size_t max_len, hwaddr addr, const char *fw_file_name, @@ -132,12 +134,12 @@ int rom_copy(uint8_t *dest, hwaddr addr, size_t size); void *rom_ptr(hwaddr addr); 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) +#define rom_add_file_fixed(_f, _a, _i, e) \ + rom_add_file(_f, NULL, _a, _i, false, NULL, e) #define rom_add_blob_fixed(_f, _b, _l, _a) \ rom_add_blob(_f, _b, _l, _l, _a, NULL, NULL, NULL) -#define rom_add_file_mr(_f, _mr, _i) \ - rom_add_file(_f, NULL, 0, _i, false, mr) +#define rom_add_file_mr(_f, _mr, _i, e) \ + rom_add_file(_f, NULL, 0, _i, false, mr, e) #define PC_ROM_MIN_VGA 0xc0000 #define PC_ROM_MIN_OPTION 0xc8000 @@ -145,7 +147,7 @@ void hmp_info_roms(Monitor *mon, const QDict *qdict); #define PC_ROM_ALIGN 0x800 #define PC_ROM_SIZE (PC_ROM_MAX - PC_ROM_MIN_VGA) -int rom_add_vga(const char *file); -int rom_add_option(const char *file, int32_t bootindex); +int rom_add_vga(const char *file, Error **errp); +int rom_add_option(const char *file, int32_t bootindex, Error **errp); #endif