Re: [Qemu-devel] Trouble with pc_system_firmware_init()

[Jordan Justen]

On Thu, Aug 16, 2012 at 4:16 AM, Markus Armbruster <address@hidden> wrote:
> I noticed a few things I'd like to discuss.
>
> 0. pc_fw_add_pflash_drv() lacks error checking, patch coming.
>
> 1. Watch this:
>
>     $ qemu-system-x86_64 -nodefaults -S -vnc :0 -bios /dev/null
>     Bad ram offset 8000000
>     Aborted (core dumped)
>
>    Backtrace:
>
>     #0  0x0000003b0de35925 in raise () from /lib64/libc.so.6
>     #1  0x0000003b0de370d8 in abort () from /lib64/libc.so.6
>     #2  0x000000000061b606 in qemu_get_ram_ptr (addr=134217728)
>         at /work/armbru/qemu/exec.c:2716
>     #3  0x000000000067c71a in memory_region_get_ram_ptr (mr=0x13bb358)
>         at /work/armbru/qemu/memory.c:1136
>     #4  0x0000000000548b6b in pflash_cfi01_register (base=4294967296, 
> qdev=0x0,
>         name=0x803e3e "system.flash", size=0, bs=0x13b9940, sector_len=4096,
>         nb_blocs=0, width=1, id0=0, id1=0, id2=0, id3=0, be=0)
>         at /work/armbru/qemu/hw/pflash_cfi01.c:609
>     #5  0x0000000000647de3 in pc_system_flash_init (rom_memory=0x13a6400,
>         pflash_drv=0x13aee20) at /work/armbru/qemu/hw/i386/../pc_sysfw.c:134
>     #6  0x0000000000648112 in pc_system_firmware_init (rom_memory=0x13a6400)
>         at /work/armbru/qemu/hw/i386/../pc_sysfw.c:234
>     #7  0x0000000000646607 in pc_memory_init (system_memory=0x138d4f0,
>         kernel_filename=0x0, kernel_cmdline=0x7e6fba "", initrd_filename=0x0,
>         below_4g_mem_size=134217728, above_4g_mem_size=0, 
> rom_memory=0x13a6400,
>         ram_memory=0x7fffffffdbe0) at /work/armbru/qemu/hw/i386/../pc.c:985
>     #8  0x000000000064714a in pc_init1 (system_memory=0x138d4f0, system_io=
>         0x138d5c0, ram_size=134217728, boot_device=0x7fffffffdf00 "cad",
>         kernel_filename=0x0, kernel_cmdline=0x7e6fba "", initrd_filename=0x0,
>         cpu_model=0x0, pci_enabled=1, kvmclock_enabled=1)
>         at /work/armbru/qemu/hw/i386/../pc_piix.c:177
>     #9  0x00000000006477ed in pc_init_pci (ram_size=134217728, boot_device=
>     ---Type <return> to continue, or q <return> to quit---
>         0x7fffffffdf00 "cad", kernel_filename=0x0, kernel_cmdline=0x7e6fba "",
>         initrd_filename=0x0, cpu_model=0x0)
>         at /work/armbru/qemu/hw/i386/../pc_piix.c:297
>     #10 0x00000000005883d4 in main (argc=7, argv=0x7fffffffe138, envp=
>         0x7fffffffe178) at /work/armbru/qemu/vl.c:3571
>
>    Same with an empty regular file instead of /dev/null.

Not good. Sorry if the pc_sysfw series (1b89fa) introduced this.

>    Shouldn't pflash_cfi01_register() survive funny sizes?  Its code to
>    check the size is disabled since commit c8b153d7 "Malta flash
>    support.":
>
>     /* XXX: to be fixed */
> #if 0
>     if (total_len != (8 * 1024 * 1024) && total_len != (16 * 1024 * 1024) &&
>         total_len != (32 * 1024 * 1024) && total_len != (64 * 1024 * 1024))
>         return NULL;
> #endif
>
>    And why do we pass the size both as (sector_len, nb_blocs) and size?
>    Since commit cfe5f011 "pflash_cfi01/pflash_cfi02: convert to memory
>    API".
>
>    pc_system_flash_init() rejects sizes that aren't a multiple of 4096
>    (hardcoded).  Why not just zero-pad up to the next muliple?  If
>    that's a bad idea: shouldn't size checking be left to
>    pflash_cfi01_register()?

I don't think we should grow the flash size. Especially when the flash
end is aligned at 4GB.

> 2. If kvm_enabled(), we add a pflash_cfi01 device to hold the BIOS, else
> we map a ROM the old-fashioned way.  Doesn't make that guest ABI depend
> on the accelerator?

KVM uses the ROM method because KVM cannot execute code from a pflash device.

I want to fix this, but so far the time I've spent investigating it
has not yielded any results.

-Jordan