[Qemu-devel] Trouble with pc_system_firmware_init()

[Markus Armbruster]

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.

   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()?

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?