From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.7 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8D634C433F5 for ; Thu, 16 Sep 2021 14:25:53 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 1EE17611C6 for ; Thu, 16 Sep 2021 14:25:53 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 1EE17611C6 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ilande.co.uk Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=nongnu.org Received: from localhost ([::1]:59252 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mQsKm-0006cz-0t for qemu-devel@archiver.kernel.org; Thu, 16 Sep 2021 10:25:52 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:54102) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mQsFE-0006QE-RG for qemu-devel@nongnu.org; Thu, 16 Sep 2021 10:20:09 -0400 Received: from mail.ilande.co.uk ([2001:41c9:1:41f::167]:40868 helo=mail.default.ilande.bv.iomart.io) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mQsF8-0003x4-VB for qemu-devel@nongnu.org; Thu, 16 Sep 2021 10:20:08 -0400 Received: from host109-153-76-56.range109-153.btcentralplus.com ([109.153.76.56] helo=[192.168.50.176]) by mail.default.ilande.bv.iomart.io with esmtpsa (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1mQsEx-0001fG-Hx; Thu, 16 Sep 2021 15:19:55 +0100 To: Markus Armbruster , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= References: <20210916100554.10963-1-mark.cave-ayland@ilande.co.uk> <20210916100554.10963-12-mark.cave-ayland@ilande.co.uk> <878rzw655h.fsf@dusky.pond.sub.org> From: Mark Cave-Ayland Message-ID: Date: Thu, 16 Sep 2021 15:19:53 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <878rzw655h.fsf@dusky.pond.sub.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-SA-Exim-Connect-IP: 109.153.76.56 X-SA-Exim-Mail-From: mark.cave-ayland@ilande.co.uk Subject: Re: [PATCH v3 11/20] nubus-device: add romfile property for loading declaration ROMs X-SA-Exim-Version: 4.2.1 (built Wed, 08 May 2019 21:11:16 +0000) X-SA-Exim-Scanned: Yes (on mail.default.ilande.bv.iomart.io) Received-SPF: pass client-ip=2001:41c9:1:41f::167; envelope-from=mark.cave-ayland@ilande.co.uk; helo=mail.default.ilande.bv.iomart.io X-Spam_score_int: -33 X-Spam_score: -3.4 X-Spam_bar: --- X-Spam_report: (-3.4 / 5.0 requ) BAYES_00=-1.9, NICE_REPLY_A=-1.488, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: qemu-devel@nongnu.org, laurent@vivier.eu Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" On 16/09/2021 14:06, Markus Armbruster wrote: > Philippe Mathieu-Daudé writes: > >> On 9/16/21 12:05 PM, Mark Cave-Ayland wrote: >>> The declaration ROM is located at the top-most address of the standard slot >>> space. >>> >>> Signed-off-by: Mark Cave-Ayland >>> --- >>> hw/nubus/nubus-device.c | 43 +++++++++++++++++++++++++++++++++++++++- >>> include/hw/nubus/nubus.h | 6 ++++++ >>> 2 files changed, 48 insertions(+), 1 deletion(-) >> >>> @@ -38,10 +43,46 @@ static void nubus_device_realize(DeviceState *dev, Error **errp) >>> memory_region_add_subregion(&nubus->slot_io, slot_offset, >>> &nd->slot_mem); >>> g_free(name); >>> + >>> + /* Declaration ROM */ >>> + if (nd->romfile != NULL) { >>> + path = qemu_find_file(QEMU_FILE_TYPE_BIOS, nd->romfile); >>> + if (path == NULL) { >>> + path = g_strdup(nd->romfile); >>> + } >>> + >>> + size = get_image_size(path); >>> + if (size < 0) { >>> + error_setg(errp, "failed to find romfile \"%s\"", nd->romfile); >>> + g_free(path); >>> + return; >>> + } else if (size == 0) { >>> + error_setg(errp, "romfile \"%s\" is empty", nd->romfile); >>> + g_free(path); >>> + return; >>> + } else if (size > NUBUS_DECL_ROM_MAX_SIZE) { >>> + error_setg(errp, "romfile \"%s\" too large (maximum size 128K)", >>> + nd->romfile); >>> + g_free(path); >>> + return; >>> + } >>> + >>> + name = g_strdup_printf("nubus-slot-%x-declaration-rom", nd->slot); >>> + memory_region_init_rom(&nd->decl_rom, OBJECT(dev), name, size, >>> + &error_fatal); > > Is this error expected to happen? > > If yes, you should quite probably propagate it. > > If no, &error_abort. (goes and looks) Ultimately this gets set from memory_region_init_rom_device_nomigrate() where err is returned from qemu_ram_alloc() which is fairly fatal. So I guess this should be &error_abort then? Note that I copied that part of the logic from hw/pci/pci.c's pci_add_option_rom() so it may also need to be adjusted there. >>> + ret = load_image_mr(path, &nd->decl_rom); >> >> load_image_mr() already calls get_image_size(), rom_add_file() and >> qemu_find_file(). *But* it doesn't takes and Error handle, and report >> error using fprintf()... > > ... except when they don't: > > int load_image_mr(const char *filename, MemoryRegion *mr) > { > int size; > > if (!memory_access_is_direct(mr, false)) { > /* Can only load an image into RAM or ROM */ > ---> return -1; > } > > size = get_image_size(filename); > > if (size < 0 || size > memory_region_size(mr)) { > return -1; > } > if (size > 0) { > if (rom_add_file_mr(filename, mr, -1) < 0) { > return -1; > } > } > return size; > } > > Hot mess! > >> So unfortunately rom_add*() functions are >> kinda outdated and you are doing the right thing to propagate detailled >> errors. > > I can't see errors being propagated, only a warn_report()... > >> Therefore: >> >> Reviewed-by: Philippe Mathieu-Daudé >> >>> + g_free(path); >>> + if (ret < 0) { >>> + warn_report("nubus-device: could not load prom '%s'", nd->romfile); > > ... here. Looking again at pci_add_option_rom() then perhaps this should be error_setg() instead: if you are explicitly trying to load a ROM image, then you should at least be able to get the filename correct. >>> + } >>> + memory_region_add_subregion(&nd->slot_mem, NUBUS_SLOT_SIZE - size, >>> + &nd->decl_rom); >>> + } >>> } ATB, Mark.