Re: [Qemu-devel] [PATCH] Make nic option rom loading less painful.

[Mark McLoughlin <markmc@redhat.com>]

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.