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

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

On 04/12/2011 01:13 PM, Glauber Costa wrote:
> 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.

Fortunately, I don't think that there are myriads of people writing 
16-bit option roms so I don't think this is a pressing problem :-)

>   And the fact that not all of them should
> live in genroms persists.

Actually genroms should disappear.  We should make -option-rom work by 
using a dummy PCI device or something like that.

Genroms is a nasty hack.

Regards,

Anthony Liguori

>> 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:
>