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

[Gerd Hoffmann <kraxel@redhat.com>]

   Hi,

> Nice idea. The implementation seems to be buggy, I only ever see one
> rom

On !pc platforms only elf roms will show up.

> and for example sparc-test crashes when issuing 'info roms'.

Huh?  Works for me.  Details please.  sparc is one of the archs I've 
actually tested:

(qemu) info roms
addr=0000000070000000 size=0x0e4000 name="phdr #0: 
/home/kraxel/projects/qemu/pc-bios/openbios-sparc32"

ppc has two entries from the two elf phdr sections:

(qemu) info roms
addr=00000000fff00000 size=0x052c88 name="phdr #0: 
/home/kraxel/projects/qemu/pc-bios/openbios-ppc"
addr=00000000fffffffc size=0x000004 name="phdr #1: 
/home/kraxel/projects/qemu/pc-bios/openbios-ppc"

pc has multiple entries when loading option roms (for network booting), 
looks like this then:

(qemu) info roms
addr=00000000000c0000 size=0x008c00 name="vgabios-cirrus.bin"
addr=00000000000c9000 size=0x008000 name="pxe-e1000.bin"

Booting linux kernels (pc too) directly will produce even more entries:

(qemu) info roms
addr=0000000000010000 size=0x003800 name="linux-setup"
addr=0000000000020000 size=0x000001 name="linux-cmdline"
addr=00000000000c0000 size=0x008c00 name="vgabios-cirrus.bin"
addr=00000000000c9000 size=0x000200 name="linux-bootsect"
addr=0000000000100000 size=0x342910 
name="/boot/vmlinuz-2.6.30.5-43.fc11.x86_64"

> Perhaps 'info roms' could also check whether the rom has been changed?

Point being?

> On most platforms roms are really read only memories.

Yep, I've noticed on a quick glimpse.  So in that case writing roms on 
reset isn't very useful because the guest shouldn't be able to modify 
them in the first place.  It doesn't harm though, except that we keep a 
copy of the roms around for no reason.  Maybe make restore-on-reset opt-in?

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

Oops.  Fixed.

>> +    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.

The vertical alignment makes the code more readable IMHO.

cheers,
   Gerd