Re: [Qemu-devel] [PATCH 1/2] pc: avoid duplicate names for ROM MRs

[Marcel Apfelbaum]

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 "/address@hidden/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 <address@hidden>
> ---
>  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 <zlib.h>
>  
> +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), "/address@hidden", 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)