On Thu, Aug 16, 2012 at 4:16 AM, Markus Armbruster <arm...@redhat.com> 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
