* [Qemu-devel] [PATCH] Make nic option rom loading less painful.
@ 2009-06-17 0:08 Glauber Costa
2009-06-17 8:21 ` Mark McLoughlin
0 siblings, 1 reply; 2+ messages in thread
From: Glauber Costa @ 2009-06-17 0:08 UTC (permalink / raw)
To: qemu-devel; +Cc: aliguori
The code how it is today, is totally painful to read and keep.
To begin with, the code is duplicated with the option rom loading
code that linux_boot and vga are already using.
This patch introduces a "bootable" state in NICInfo structure,
that we can use to keep track of whether or not a given nic should
be bootable, avoiding the introduction of yet another global state.
With that in hands, we move the code in vl.c to hw/pc.c, and use
the already existing infra structure to load those option roms.
We lose the checking that currently exists qemu if no optiom roms are
found, but I don't think it is a big deal. It is not consistent with
the behaviour of any other option rom that fails to load. Furthermore,
we can add it around pc.c if anyone really cares.
Signed-off-by: Glauber Costa <glommer@redhat.com>
---
hw/pc.c | 16 +++++++++++++---
vl.c | 40 +++++-----------------------------------
2 files changed, 18 insertions(+), 38 deletions(-)
diff --git a/hw/pc.c b/hw/pc.c
index 0934778..c458ebb 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -976,9 +976,19 @@ static void pc_init1(ram_addr_t ram_size,
oprom_area_size += 2048;
}
- for (i = 0; i < nb_option_roms; i++) {
- oprom_area_size += load_option_rom(option_rom[i],
- 0xc0000 + oprom_area_size, 0xe0000);
+ for (i = 0; i < nb_nics; i++) {
+ char nic_oprom[1024];
+ const char *model = nd_table[i].model;
+
+ if (!nd_table[i].bootable)
+ continue;
+
+ if (model == NULL)
+ model = "ne2k_pci";
+ snprintf(nic_oprom, sizeof(nic_oprom), "pxe-%s.bin", model);
+
+ oprom_area_size += load_option_rom(nic_oprom, 0xc0000 + oprom_area_size,
+ 0xe0000);
}
/* map all the bios at the top of memory */
diff --git a/vl.c b/vl.c
index fcf8532..5a063a0 100644
--- a/vl.c
+++ b/vl.c
@@ -5781,7 +5781,6 @@ int main(int argc, char **argv, char **envp)
exit(1);
}
linux_boot = (kernel_filename != NULL);
- net_boot = (boot_devices_bitmap >> ('n' - 'a')) & 0xF;
if (!linux_boot && *kernel_cmdline != '\0') {
fprintf(stderr, "-append only allowed with -kernel option\n");
@@ -5825,45 +5824,16 @@ int main(int argc, char **argv, char **envp)
#endif
}
+ net_boot = (boot_devices_bitmap >> ('n' - 'a')) & 0xF;
for(i = 0;i < nb_net_clients; i++) {
if (net_client_parse(net_clients[i]) < 0)
exit(1);
+ if (net_boot & (1 << i)) {
+ nd_table[i].bootable = 1;
+ }
}
- net_client_check();
-#ifdef TARGET_I386
- /* XXX: this should be moved in the PC machine instantiation code */
- if (net_boot != 0) {
- int netroms = 0;
- for (i = 0; i < nb_nics && i < 4; i++) {
- const char *model = nd_table[i].model;
- char buf[1024];
- char *filename;
- if (net_boot & (1 << i)) {
- if (model == NULL)
- model = "ne2k_pci";
- snprintf(buf, sizeof(buf), "pxe-%s.bin", model);
- filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, buf);
- if (filename && get_image_size(filename) > 0) {
- if (nb_option_roms >= MAX_OPTION_ROMS) {
- fprintf(stderr, "Too many option ROMs\n");
- exit(1);
- }
- option_rom[nb_option_roms] = qemu_strdup(buf);
- nb_option_roms++;
- netroms++;
- }
- if (filename) {
- qemu_free(filename);
- }
- }
- }
- if (netroms == 0) {
- fprintf(stderr, "No valid PXE rom found for network device\n");
- exit(1);
- }
- }
-#endif
+ net_client_check();
/* init the bluetooth world */
for (i = 0; i < nb_bt_opts; i++)
--
1.6.2.2
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [Qemu-devel] [PATCH] Make nic option rom loading less painful.
2009-06-17 0:08 [Qemu-devel] [PATCH] Make nic option rom loading less painful Glauber Costa
@ 2009-06-17 8:21 ` Mark McLoughlin
0 siblings, 0 replies; 2+ messages in thread
From: Mark McLoughlin @ 2009-06-17 8:21 UTC (permalink / raw)
To: Glauber Costa; +Cc: aliguori, qemu-devel
Hi Glauber,
On Tue, 2009-06-16 at 20:08 -0400, Glauber Costa wrote:
> The code how it is today, is totally painful to read and keep.
> To begin with, the code is duplicated with the option rom loading
> code that linux_boot and vga are already using.
>
> This patch introduces a "bootable" state in NICInfo structure,
No it doesn't, you seem to have forgotten that part :-)
> that we can use to keep track of whether or not a given nic should
> be bootable, avoiding the introduction of yet another global state.
>
> With that in hands, we move the code in vl.c to hw/pc.c, and use
> the already existing infra structure to load those option roms.
>
> We lose the checking that currently exists qemu if no optiom roms are
> found, but I don't think it is a big deal. It is not consistent with
> the behaviour of any other option rom that fails to load. Furthermore,
> we can add it around pc.c if anyone really cares.
We don't need to loose this, see below.
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> ---
> hw/pc.c | 16 +++++++++++++---
> vl.c | 40 +++++-----------------------------------
> 2 files changed, 18 insertions(+), 38 deletions(-)
>
> diff --git a/hw/pc.c b/hw/pc.c
> index 0934778..c458ebb 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -976,9 +976,19 @@ static void pc_init1(ram_addr_t ram_size,
> oprom_area_size += 2048;
> }
>
> - for (i = 0; i < nb_option_roms; i++) {
> - oprom_area_size += load_option_rom(option_rom[i],
> - 0xc0000 + oprom_area_size, 0xe0000);
Looks like this breaks -option-rom
> + for (i = 0; i < nb_nics; i++) {
> + char nic_oprom[1024];
> + const char *model = nd_table[i].model;
Is that a tab?
> +
> + if (!nd_table[i].bootable)
> + continue;
> +
> + if (model == NULL)
> + model = "ne2k_pci";
> + snprintf(nic_oprom, sizeof(nic_oprom), "pxe-%s.bin", model);
> +
> + oprom_area_size += load_option_rom(nic_oprom, 0xc0000 + oprom_area_size,
> + 0xe0000);
> }
>
> /* map all the bios at the top of memory */
> diff --git a/vl.c b/vl.c
> index fcf8532..5a063a0 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -5781,7 +5781,6 @@ int main(int argc, char **argv, char **envp)
> exit(1);
> }
> linux_boot = (kernel_filename != NULL);
> - net_boot = (boot_devices_bitmap >> ('n' - 'a')) & 0xF;
>
> if (!linux_boot && *kernel_cmdline != '\0') {
> fprintf(stderr, "-append only allowed with -kernel option\n");
> @@ -5825,45 +5824,16 @@ int main(int argc, char **argv, char **envp)
> #endif
> }
>
> + net_boot = (boot_devices_bitmap >> ('n' - 'a')) & 0xF;
> for(i = 0;i < nb_net_clients; i++) {
> if (net_client_parse(net_clients[i]) < 0)
> exit(1);
> + if (net_boot & (1 << i)) {
> + nd_table[i].bootable = 1;
> + }
Not every net client is a NIC - e.g. with '-net nic -net user -net nic
-net tap -boot m' the bootable flag will be set on nd_table[2] even
though that's not a valid NIC
Perhaps call something like this once the net_client_parse() loop is
complete?
int net_set_boot_mask(int net_boot_mask)
{
int i;
/* Only the first four NICs may be bootable */
net_boot_mask = net_boot_mask & 0xF;
for (i = 0; i < nb_nics; i++) {
if (net_boot_mask & (1 << i)) {
nd_table[i].bootable = 1;
net_boot_mask &= ~(1 << i);
}
}
if (net_boot_mask) {
fprintf(stderr, "Cannot boot from non-existent NIC\n");
return -1;
}
return 0;
}
> }
> - net_client_check();
>
> -#ifdef TARGET_I386
> - /* XXX: this should be moved in the PC machine instantiation code */
> - if (net_boot != 0) {
> - int netroms = 0;
> - for (i = 0; i < nb_nics && i < 4; i++) {
> - const char *model = nd_table[i].model;
> - char buf[1024];
> - char *filename;
> - if (net_boot & (1 << i)) {
> - if (model == NULL)
> - model = "ne2k_pci";
> - snprintf(buf, sizeof(buf), "pxe-%s.bin", model);
> - filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, buf);
> - if (filename && get_image_size(filename) > 0) {
> - if (nb_option_roms >= MAX_OPTION_ROMS) {
> - fprintf(stderr, "Too many option ROMs\n");
> - exit(1);
> - }
> - option_rom[nb_option_roms] = qemu_strdup(buf);
> - nb_option_roms++;
> - netroms++;
> - }
> - if (filename) {
> - qemu_free(filename);
> - }
> - }
> - }
> - if (netroms == 0) {
> - fprintf(stderr, "No valid PXE rom found for network device\n");
> - exit(1);
> - }
With the check I suggest above in net_set_boot_mask() and the fact that
load_option_rom() aborts if the rom isn't found, I don't think we're
losing any error handling.
Cheers,
Mark.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2009-06-17 8:21 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-06-17 0:08 [Qemu-devel] [PATCH] Make nic option rom loading less painful Glauber Costa
2009-06-17 8:21 ` Mark McLoughlin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).