Re: [Qemu-devel] [PATCH] Reorganize option rom (+linux kernel) loading.

[Blue Swirl <blauwirbel@gmail.com>]

On Wed, Sep 30, 2009 at 6:43 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> This patch adds infrastructure to maintain memory regions which must be
> restored on reset.  That includes roms (vga bios and option roms on pc),
> but is also used when loading linux kernels directly.  Features:
>
>  - loading files is supported.
>  - passing blobs is supported.
>  - target address range is supported (for optionrom area).
>  - fixed target memory address is supported (linux kernel).
>
> The final memory layout is created once all memory regions are
> registered.  The option roms get addresses assigned and the
> registered regions are checked against overlaps.  Finally all data
> is copyed to the guest memory.
>
> Advantages:
>
>  (1) Filling memory on initial boot and on reset takes the same
>      code path, making reset more robust.
>  (2) The need to keep track of the option rom load address is gone.
>  (3) Due to (2) option roms can be loaded outside pc_init().  This
>      allows to move the pxe rom loading into the nic drivers for
>      example.
>
> Additional bonus:  There is a 'info roms' monitor command now.
>
> The patch also switches over pc.c and removes the
> option_rom_setup_reset() and load_option_rom() functions.

Nice idea. The implementation seems to be buggy, I only ever see one
rom and for example sparc-test crashes when issuing 'info roms'.

Perhaps 'info roms' could also check whether the rom has been changed?
On most platforms roms are really read only memories.

> +    fd = open(rom->path, O_RDONLY);
> +    if (-1 == fd) {

Not again!

> +    rom->align    = align;
> +    rom->min      = min;
> +    rom->max      = max;
> +    rom->romsize  = lseek(fd, 0, SEEK_END);
> +    rom->data     = qemu_mallocz(rom->romsize);

There are plenty of extra spaces.