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

[Anthony Liguori <aliguori@us.ibm.com>]

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,

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<glommer@redhat.com>
> CC: Gleb Natapov<gleb@redhat.com>
> ---
>   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
> -@item -option-rom @var{file}[,bootindex=@var{bootindex}]
> +@item -option-rom @var{file}[,bootindex=@var{bootindex}][,fw_dir=@var{dir}]
>   @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 bootindex=@var{bootindex}
>   Defines which boot index the option rom will be given in boot entry vectors,
>   allowing fine-grained selection of devices boot order.
> +@item fw_dir=@var{dir} (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: