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

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Mar 06, 2014 at 06:14:11PM +0200, Marcel Apfelbaum wrote:
> 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 <mst@redhat.com>
> > ---
> >  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

Ouch for fixed files this was supposed to be false.
Will send v2.


> >  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), "/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)
> 
> 
>