On Thu, 2013-11-07 at 22:12 +0100, Laszlo Ersek wrote: > This is a QEMU bug report, only disguised as an edk2-devel followup. > > The problem in a nutshell is that the OVMF binary, placed into pflash > (read-only KVM memslot) used to run in qemu-1.6, but it fails to start > in qemu-1.7. The OVMF reset vector reads as 0xFF bytes. > > (Jordan and myself started writing these emails in parallel.) > > On 11/07/13 21:27, Jordan Justen wrote: > > On Sun, Nov 3, 2013 at 12:48 PM, Marcel Apfelbaum > > <marce...@redhat.com> wrote: > >> The commit: > >> > >> Commit: a53ae8e934cd54686875b5bcfc2f434244ee55d6 > >> Author: Marcel Apfelbaum <marce...@redhat.com> > >> Date: Mon Sep 16 11:21:16 2013 +0300 > >> > >> hw/pci: partially handle pci master abort > >> > >> introduced a regression on make check: > > > > Laszlo pointed out that my OVMF flash support series was not working > > with QEMU master. It was working with QEMU 1.6.0. I then bisected the > > issue to this commit. It seems this commit regresses -pflash support > > for both KVM and non-KVM modes. > > > > Can you reproduce the issue this with command? > > x86_64-softmmu/qemu-system-x86_64 -pflash pc-bios/bios.bin > > (with or without adding -enable-kvm) > > > > I tried adding this patch ("exec: fix regression by making > > system-memory region UINT64_MAX size") and it did not help the -pflash > > regression. > > > From the edk2-devel discussion: > <http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4787/focus=4812> > > On 11/07/13 19:07, Laszlo Ersek wrote: > > On 11/07/13 17:28, Laszlo Ersek wrote: > >> On 11/06/13 23:29, Jordan Justen wrote: > >>> https://github.com/jljusten/edk2.git ovmf-nvvars-v2 > >>> > >>> This series implements support for QEMU's emulated > >>> system flash. > >>> > >>> This allows for persistent UEFI non-volatile variables. > >>> > >>> Previously we attempted to emulate non-volatile > >>> variables in a few ways, but each of them would fail > >>> in particular situations. > >>> > >>> To support flash based variable storage, we: > >>> * Reserve space in the firmware device image > >>> * Add a new flash firmware volume block driver > >>> * Disable EmuVariableFvbRuntimeDxe (at runtime) if QEMU > >>> flash is available. > >>> > >>> To use: > >>> * QEMU version 1.1 or newer is required without KVM > >>> * KVM support requires Linux 3.7 and QEMU 1.6 > >>> * Run QEMU with -pflash OVMF.fd instead of -L or -bios > >>> or use OvmfPkg/build.sh --enable-flash qemu ... > >>> * If QEMU is 1.6 or newer, then OvmfPkg/build.sh will > >>> automatically enable flash when running QEMU, so in > >>> that case --enable-flash is not required. > >>> > >>> See also: > >>> * http://wiki.qemu.org/Features/PC_System_Flash > >>> > >>> v2: > >>> * Replace > >>> "OvmfPkg/AcpiPlatformDxe/Qemu: Allow high runtime memory regions" > >>> with > >>> "OvmfPkg/AcpiPlatformDxe/Qemu: Decrease upper limit for PCI window > >>> 32" > >>> * Separate portions of > >>> "OvmfPkg/build.sh: Support --enable-flash switch" > >>> out into a new patch > >>> "OvmfPkg/build.sh: Enable flash for QEMU >= 1.6" > >>> * Add "OvmfPkg/README: Add information about OVMF flash layout" > >>> * Update "OvmfPkg: Add NV Variable storage within FD" to also change the > >>> size of PcdVariableStoreSize > >>> * Update commit messages on a couple patches for better clarity > >> > >> Tested in the following configurations: > >> > >> (1) RHEL-6 host (no KVM support, no qemu support -- that is, > >> regression test): RHEL-6, Fedora 19, Windows 2008 R2 (needs CSM), > >> Windows 2012 R2 boot tests work OK. > >> > >> (2) 3.10-based host kernel, qemu v1.7.0-rc0, Xeon W3550 host CPU: > >> Unfortunately qemu dies with the following KVM trace: > >> > >> KVM internal error. Suberror: 1 > >> emulation failure > >> EAX=00000000 EBX=00000000 ECX=00000000 EDX=00000623 > >> ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000 > >> EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0 > >> ES =0000 00000000 0000ffff 00009300 > >> CS =f000 ffff0000 0000ffff 00009b00 > >> SS =0000 00000000 0000ffff 00009300 > >> DS =0000 00000000 0000ffff 00009300 > >> FS =0000 00000000 0000ffff 00009300 > >> GS =0000 00000000 0000ffff 00009300 > >> LDT=0000 00000000 0000ffff 00008200 > >> TR =0000 00000000 0000ffff 00008b00 > >> GDT= 00000000 0000ffff > >> IDT= 00000000 0000ffff > >> CR0=60000010 CR2=00000000 CR3=00000000 CR4=00000000 > >> DR0=0000000000000000 DR1=0000000000000000 DR2=0000000000000000 > >> DR3=0000000000000000 > >> DR6=00000000ffff0ff0 DR7=0000000000000400 > >> EFER=0000000000000000 > >> Code=ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff <ff> ff > >> ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > >> ff ff ff > >> > >> I'm quite unsure, but the CS:IP value seems to point at the reset > >> vector, no? However, the Code=... log only shows 0xFF bytes. > >> > >> (3) 3.10.17 host kernel, qemu v1.7.0-rc0, Athlon II X2 B22 host CPU: > >> almost identical KVM error, with the following difference: > >> > >> --- vmx 2013-11-07 17:23:45.612973935 +0100 > >> +++ svm 2013-11-07 17:24:17.237973059 +0100 > >> @@ -1,6 +1,6 @@ > >> KVM internal error. Suberror: 1 > >> emulation failure > >> -EAX=00000000 EBX=00000000 ECX=00000000 EDX=00000623 > >> +EAX=00000000 EBX=00000000 ECX=00000000 EDX=00000663 > >> ESI=00000000 EDI=00000000 EBP=00000000 ESP=00000000 > >> EIP=0000fff0 EFL=00000002 [-------] CPL=0 II=0 A20=1 SMM=0 HLT=0 > >> ES =0000 00000000 0000ffff 00009300 > >> > >> Any ideas? > > > > I. > > This is a QEMU regression (somewhere between v1.6.0 and v1.7.0-rc0), > > I'll have to bisect it. > > This bug is "caused" by the following commit: > > a53ae8e934cd54686875b5bcfc2f434244ee55d6 is the first bad commit > commit a53ae8e934cd54686875b5bcfc2f434244ee55d6 > Author: Marcel Apfelbaum <marce...@redhat.com> > Date: Mon Sep 16 11:21:16 2013 +0300 > > hw/pci: partially handle pci master abort > > A MemoryRegion with negative priority was created and > it spans over all the pci address space. > It "intercepts" the accesses to unassigned pci > address space and will follow the pci spec: > 1. returns -1 on read > 2. does nothing on write > > Note: setting the RECEIVED MASTER ABORT bit in the STATUS register > of the device that initiated the transaction will be > implemented in another series > > Signed-off-by: Marcel Apfelbaum <marce...@redhat.com> > Acked-by: Michael S. Tsirkin <m...@redhat.com> > Signed-off-by: Michael S. Tsirkin <m...@redhat.com> > > The patch was posted as patch 3/3 of the series > > [Qemu-devel] [PATCH v5 0/3] pci: partially implement master abort > protocol > > http://thread.gmane.org/gmane.comp.emulators.qemu/233798/focus=233801 > > Basically, the series adds a "background" memory region that covers all > "unassigned" PCI addresses, and patch 3/3 specifically makes sure that > writes to this region are dropped, and reads all return -1 (0xFFFFFFFF). > > The read implementation (master_abort_mem_read(), -1) is consistent with > the KVM dump in the quoted part above. > > For some reason, this "background" region pushes into the "foreground" > when it comes to the pflash region just below 4GB (in guest-phys address > space). > > Or, hm, supposing we start to run in real mode at FFFF:0000, the problem > could be with the "isa-bios" region too. > > So I think we have a bug in qemu, which is likely one of the three > below: > > (1) This commit is wrong. Or, > (2) the pflash implementation is wrong, because it doesn't register a > memory region (with appropriate priority) that would catch the > access. > (3) Both this commit and the pflash implementations are right, but this > is an unusual situation that the memory infrastructure doesn't > handle well. > > (I doubt that the problem is in KVM.) > > When the bug hits, the "info qtree" command has the following to say > about the flash device: > > dev: cfi.pflash01, id "" > drive = pflash0 > num-blocks = 512 > sector-length = 4096 > width = 1 > big-endian = 0 > id0 = 0 > id1 = 0 > id2 = 0 > id3 = 0 > name = "system.flash" > irq 0 > mmio 00000000ffe00000/0000000000200000 > > The "info mtree" command lists: > > memory > 0000000000000000-7ffffffffffffffe (prio 0, RW): system > [...] > 0000000060000000-00000000ffffffff (prio 0, RW): alias pci-hole @pci > 0000000060000000-00000000ffffffff > [...] > 00000000ffe00000-00000000ffffffff (prio 0, R-): system.flash > [...] > pci <-------- referred to as "rom_memory" in the source (pci is enabled) > 0000000000000000-7ffffffffffffffe (prio 0, RW): pci > 0000000000000000-7ffffffffffffffe (prio -2147483648, RW): pci-master-abort > [...] > 00000000000e0000-00000000000fffff (prio 1, R-): isa-bios > > For some reason, the range called "pci-master-abort" takes priority over > "isa-bios" (and/or "system.flash"). > > I wrote the attached debug patch (for v1.7.0-rc0). It produces quite a > bit of output, but grepping it for > > isa-bios|system\.flash|pci-master-abort|pci-hole > > results in the following messages: > > adding subregion 'system.flash' to region 'system' at offset ffe00000 > subregion 'system.flash' (prio: 0) loses to sibling subregion > 'icc-apic-container' (prio: 4096) > subregion 'system.flash' (prio: 0) wins over sibling subregion > 'ram-below-4g' (prio: 0) > > adding subregion 'isa-bios' to region 'pci' at offset e0000 > subregion 'isa-bios' (prio: 1) inserted at tail > subregion 'pc.rom' (prio: 1) wins over sibling subregion 'isa-bios' (prio: > 1) > > adding subregion 'pci-master-abort' to region 'pci' at offset 0 > subregion 'pci-master-abort' (prio: -2147483648) loses to sibling subregion > 'pc.rom' (prio: 1) > subregion 'pci-master-abort' (prio: -2147483648) loses to sibling subregion > 'isa-bios' (prio: 1) > subregion 'pci-master-abort' (prio: -2147483648) inserted at tail > > adding subregion 'pci-hole' to region 'system' at offset 60000000 > warning: subregion collision 60000000/a0000000 (pci-hole) vs > ffe00000/200000 (system.flash) Thank you Laszlo for the detailed info! I think the problem is right above. Why pci-hole and system.flash collide? IMHO we should not play with priorities here, better solve the collision.
Thanks, Marcel > subregion 'pci-hole' (prio: 0) loses to sibling subregion > 'icc-apic-container' (prio: 4096) > subregion 'pci-hole' (prio: 0) wins over sibling subregion 'system.flash' > (prio: 0) > > adding subregion 'pci-hole64' to region 'system' at offset 100000000 > subregion 'pci-hole64' (prio: 0) loses to sibling subregion > 'icc-apic-container' (prio: 4096) > subregion 'pci-hole64' (prio: 0) wins over sibling subregion 'pci-hole' > (prio: 0) > subregion 'smram-region' (prio: 1) wins over sibling subregion 'pci-hole64' > (prio: 0) > warning: subregion collision fec00000/1000 (kvm-ioapic) vs > 60000000/a0000000 (pci-hole) > subregion 'kvm-ioapic' (prio: 0) wins over sibling subregion 'pci-hole64' > (prio: 0) > warning: subregion collision fed00000/400 (hpet) vs 60000000/a0000000 > (pci-hole) > > I think I know what's going on... There's even a warning above, printed > by original (albeit disabled) qemu source code. > > The "pci-hole" subregion, which is an alias, takes priority over > "system.flash". And, unfortunately, "pci-hole" provides a window into > "pci-master-abort". > > I think this should be fixable by raising the priority of "system.flash" > to 2048. This way the relationship between "system.flash" and any other > region will not change, except it's going to be reversed with > "pci-hole". > > The 2nd attached patch seems to solve the problem for me. I'll resubmit > it as a standalone patch if it is deemed good. > > With it in place, I can run OVMF just fine. And: > > @@ -28,170 +28,224 @@ > adding subregion 'pci-conf-data' to region 'io' at offset cfc > subregion 'pci-conf-data' (prio: 0) wins over sibling subregion > 'pci-conf-idx' (prio: 0) > adding subregion 'pci-hole' to region 'system' at offset 60000000 > -warning: subregion collision 60000000/a0000000 (pci-hole) vs > ffe00000/200000 (system.flash) > subregion 'pci-hole' (prio: 0) loses to sibling subregion > 'icc-apic-container' (prio: 4096) > -subregion 'pci-hole' (prio: 0) wins over sibling subregion 'system.flash' > (prio: 0) > +subregion 'pci-hole' (prio: 0) loses to sibling subregion 'system.flash' > (prio: 2048) > +subregion 'pci-hole' (prio: 0) wins over sibling subregion 'ram-below-4g' > (prio: 0) > > According to the debug patch, the flash region starts to win over quite > a few unrelated other regions as well. But in practice I could see no > adverse effects -- the priority matters only when the addresses actually > overlap, and "system.flash" should not overlap with anything but > "pci-hole". > > I'm attaching the following files as well: > - the "info mtree" output before and after the patch, > - the full output of the debug patch (before and after). > > Thanks, > Laszlo