From mboxrd@z Thu Jan 1 00:00:00 1970 From: Anthony Liguori Subject: Re: [PATCH 2/2] add fw_dir option to option-rom switch Date: Tue, 12 Apr 2011 13:31:28 -0500 Message-ID: <4DA49A80.3080701@us.ibm.com> References: <1302629037-16084-1-git-send-email-glommer@redhat.com> <1302629037-16084-3-git-send-email-glommer@redhat.com> <4DA48E93.90505@us.ibm.com> <1302631996.3021.28.camel@mothafucka.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: Gleb Natapov , mtosatti@redhat.com, qemu-devel@nongnu.org, kvm@vger.kernel.org, avi@redhat.com To: Glauber Costa Return-path: In-Reply-To: <1302631996.3021.28.camel@mothafucka.localdomain> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org Sender: qemu-devel-bounces+gceq-qemu-devel=gmane.org@nongnu.org List-Id: kvm.vger.kernel.org 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 >>> CC: Gleb Natapov >>> --- >>> 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: >