From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MGqP8-0008D1-4M for qemu-devel@nongnu.org; Wed, 17 Jun 2009 04:21:58 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MGqP3-0008Bf-0P for qemu-devel@nongnu.org; Wed, 17 Jun 2009 04:21:57 -0400 Received: from [199.232.76.173] (port=46398 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MGqP2-0008Bc-TI for qemu-devel@nongnu.org; Wed, 17 Jun 2009 04:21:52 -0400 Received: from mx2.redhat.com ([66.187.237.31]:42164) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MGqP2-0005DJ-GP for qemu-devel@nongnu.org; Wed, 17 Jun 2009 04:21:52 -0400 Subject: Re: [Qemu-devel] [PATCH] Make nic option rom loading less painful. From: Mark McLoughlin In-Reply-To: <1245197338-22063-1-git-send-email-glommer@redhat.com> References: <1245197338-22063-1-git-send-email-glommer@redhat.com> Content-Type: text/plain Date: Wed, 17 Jun 2009 09:21:48 +0100 Message-Id: <1245226908.27028.23.camel@blaa> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Reply-To: Mark McLoughlin List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Glauber Costa Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org 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 > --- > 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.