From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59338) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLavp-00065r-ET for qemu-devel@nongnu.org; Thu, 06 Mar 2014 11:14:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WLavj-0005ba-EH for qemu-devel@nongnu.org; Thu, 06 Mar 2014 11:14:01 -0500 Received: from mx1.redhat.com ([209.132.183.28]:47748) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WLavi-0005bK-7y for qemu-devel@nongnu.org; Thu, 06 Mar 2014 11:13:55 -0500 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s26GDp1b015804 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 6 Mar 2014 11:13:51 -0500 Message-ID: <1394122451.2663.13.camel@localhost.localdomain> From: Marcel Apfelbaum Date: Thu, 06 Mar 2014 18:14:11 +0200 In-Reply-To: <1394120854-5392-1-git-send-email-mst@redhat.com> References: <1394120854-5392-1-git-send-email-mst@redhat.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 1/2] pc: avoid duplicate names for ROM MRs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: imammedo@redhat.com, pbonzini@redhat.com, qemu-devel@nongnu.org, kraxel@redhat.com On Thu, 2014-03-06 at 17:48 +0200, Michael S. Tsirkin wrote: > Since > commit 04920fc0faa4760f9c4fc0e73b992b768099be70 > loader: store FW CFG ROM files in RAM > RAM MRs including ROM files in FW CFGs are created > and named using the file basename. > > This becomes problematic if these names are > supplied by user, since the basename might not > be unique. > > There are two cases we care about: > - option-rom flag. > - option ROM for devices. This triggers e.g. when > using rombar=0. > > At the moment we get an assert. E.g > qemu -option-rom /usr/share/ipxe/8086100e.rom -option-rom > /usr/share/ipxe.efi/8086100e.rom > RAMBlock "/rom@genroms/8086100e.rom" already registered, abort! > > This is a regression from 1.7. > > For now let's keep it simple and just avoid creating the > MRs in this case. > > Signed-off-by: Michael S. Tsirkin > --- > include/hw/loader.h | 6 ++++-- > hw/core/loader.c | 10 ++++++---- > 2 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/include/hw/loader.h b/include/hw/loader.h > index 91b0122..edca1ed 100644 > --- a/include/hw/loader.h > +++ b/include/hw/loader.h > @@ -44,9 +44,11 @@ void pstrcpy_targphys(const char *name, > const char *source); > > extern bool rom_file_in_ram; > +extern bool option_rom_in_ram; > > int rom_add_file(const char *file, const char *fw_dir, > - hwaddr addr, int32_t bootindex); > + hwaddr addr, int32_t bootindex, > + bool option); Hi Michael, I don't understand the usage of the new "option" parameter. It is always set to true by the caller Thanks, Marcel > void *rom_add_blob(const char *name, const void *blob, size_t len, > hwaddr addr, const char *fw_file_name, > FWCfgReadCallback fw_callback, void *callback_opaque); > @@ -60,7 +62,7 @@ void *rom_ptr(hwaddr addr); > void do_info_roms(Monitor *mon, const QDict *qdict); > > #define rom_add_file_fixed(_f, _a, _i) \ > - rom_add_file(_f, NULL, _a, _i) > + rom_add_file(_f, NULL, _a, _i, true) > #define rom_add_blob_fixed(_f, _b, _l, _a) \ > rom_add_blob(_f, _b, _l, _a, NULL, NULL, NULL) > > diff --git a/hw/core/loader.c b/hw/core/loader.c > index 0634bee..a19b158 100644 > --- a/hw/core/loader.c > +++ b/hw/core/loader.c > @@ -54,6 +54,7 @@ > > #include > > +bool option_rom_in_ram = false; > bool rom_file_in_ram = true; > > static int roms_loaded; > @@ -624,7 +625,8 @@ 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) > + hwaddr addr, int32_t bootindex, > + bool option) > { > Rom *rom; > int rc, fd = -1; > @@ -676,7 +678,7 @@ int rom_add_file(const char *file, const char *fw_dir, > basename); > snprintf(devpath, sizeof(devpath), "/rom@%s", fw_file_name); > > - if (rom_file_in_ram) { > + if ((!option || option_rom_in_ram) && rom_file_in_ram) { > data = rom_set_mr(rom, OBJECT(fw_cfg), devpath); > } else { > data = rom->data; > @@ -755,12 +757,12 @@ int rom_add_elf_program(const char *name, void *data, size_t datasize, > > int rom_add_vga(const char *file) > { > - return rom_add_file(file, "vgaroms", 0, -1); > + return rom_add_file(file, "vgaroms", 0, -1, true); > } > > int rom_add_option(const char *file, int32_t bootindex) > { > - return rom_add_file(file, "genroms", 0, bootindex); > + return rom_add_file(file, "genroms", 0, bootindex, true); > } > > static void rom_reset(void *unused)