Re: [Qemu-devel] [PATCH 2/2] add fw_dir option to option-rom switch

[Glauber Costa]

On Tue, 2011-04-12 at 12:40 -0500, Anthony Liguori wrote:
> On 04/12/2011 12:23 PM, Glauber Costa wrote:
> > The option-rom puts all roms passed by this switch in the genroms directory,
> > through the fw_dir option. But as it turns out, not all roms should be 
> > placed there.
> >
> > VGA roms are a of a different kind. They live in a different segment, and 
> > are scanned
> > first. This patch allows qemu to use external vgaroms such as sgabios, a 
> > rom for a serial
> > graphics adapter.
> 
> Wouldn't it make more sense to have a new PCI device that had sgabios 
> that could play the role of the VGA device.
> 
> Then you could say -vga sga
> 
> Regards,

This would be good, if we included sga along the roms we ship. We're
still, however, left with the problem that in the future, people may
want to use their own roms. And the fact that not all of them should
live in genroms persists.

> Anthony Liguori
> 
> > Without this patch, we lose all the initial BIOS output: until the option 
> > rom is initialized,
> > the only available vga rom is the default cirrus/vesa one.
> >
> > We're also vulnerable to option rom enumeration order: if a vga oprom is 
> > initialized
> > before, say, gpxe, we are able to see its output in the adapter. If it is 
> > initialized
> > after, we miss it.
> >
> > So I believe the proper solution is to allow users to specify that a rom 
> > belongs in vgaroms
> > directory instead.
> >
> > Signed-off-by: Glauber Costa<address@hidden>
> > CC: Gleb Natapov<address@hidden>
> > ---
> >   hw/pc.c         |    7 ++++++-
> >   qemu-config.c   |    3 +++
> >   qemu-options.hx |    7 +++++--
> >   sysemu.h        |    1 +
> >   vl.c            |    1 +
> >   5 files changed, 16 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/pc.c b/hw/pc.c
> > index 8d351ba..736efbb 100644
> > --- a/hw/pc.c
> > +++ b/hw/pc.c
> > @@ -1055,7 +1055,12 @@ void pc_memory_init(ram_addr_t ram_size,
> >       }
> >
> >       for (i = 0; i<  nb_option_roms; i++) {
> > -        rom_add_option(option_rom[i].name, option_rom[i].bootindex);
> > +        /* do it here, instead of in vl.c, to avoid cluttering that file 
> > with x86 material */
> > +        if (!option_rom[i].fw_dir) {
> > +            option_rom[i].fw_dir = "genroms";
> > +        }
> > +        rom_add_file(option_rom[i].name, option_rom[i].fw_dir, 0,
> > +                     option_rom[i].bootindex);
> >       }
> >   }
> >
> > diff --git a/qemu-config.c b/qemu-config.c
> > index 6d9c238..97b8515 100644
> > --- a/qemu-config.c
> > +++ b/qemu-config.c
> > @@ -450,6 +450,9 @@ QemuOptsList qemu_option_rom_opts = {
> >           }, {
> >               .name = "romfile",
> >               .type = QEMU_OPT_STRING,
> > +        }, {
> > +            .name = "fw_dir",
> > +            .type = QEMU_OPT_STRING,
> >           },
> >           { /* end if list */ }
> >       },
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 96927cc..d9eec62 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -2079,10 +2079,10 @@ to cope with initialization race conditions.
> >   ETEXI
> >
> >   DEF("option-rom", HAS_ARG, QEMU_OPTION_option_rom, \
> > -    "-option-rom rom[,bootindex=idx] load a file, rom, into the option ROM 
> > space\n",
> > +    "-option-rom rom[,bootindex=idx][,fw_dir=dir] load a file, rom, into 
> > the option ROM space\n",
> >       QEMU_ARCH_ALL)
> >   STEXI
> > address@hidden -option-rom @var{file}[,address@hidden
> > address@hidden -option-rom @var{file}[,address@hidden,address@hidden
> >   @findex -option-rom
> >   Load the contents of @var{file} as an option ROM.
> >   This option is useful to load things like EtherBoot.
> > @@ -2091,6 +2091,9 @@ This option is useful to load things like EtherBoot.
> >   @item address@hidden
> >   Defines which boot index the option rom will be given in boot entry 
> > vectors,
> >   allowing fine-grained selection of devices boot order.
> > address@hidden address@hidden (x86 only)
> > +Specify under which firmware directory entry this rom should live. Current
> > +allowed values are vgaroms and genroms (default).
> >   @end table
> >
> >   ETEXI
> > diff --git a/sysemu.h b/sysemu.h
> > index 3f7e17e..2f8be32 100644
> > --- a/sysemu.h
> > +++ b/sysemu.h
> > @@ -160,6 +160,7 @@ extern uint64_t node_cpumask[MAX_NODES];
> >   typedef struct QEMUOptionRom {
> >       const char *name;
> >       int32_t bootindex;
> > +    const char *fw_dir;
> >   } QEMUOptionRom;
> >   extern QEMUOptionRom option_rom[MAX_OPTION_ROMS];
> >   extern int nb_option_roms;
> > diff --git a/vl.c b/vl.c
> > index 8bcf2ae..e1d7868 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2675,6 +2675,7 @@ int main(int argc, char **argv, char **envp)
> >                       fprintf(stderr, "Option ROM file is not specified\n");
> >                       exit(1);
> >                   }
> > +                option_rom[nb_option_roms].fw_dir = qemu_opt_get(opts, 
> > "fw_dir");
> >             nb_option_roms++;
> >             break;
> >               case QEMU_OPTION_semihosting:
>