Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size

[Marcel Apfelbaum]

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
> > <address@hidden> wrote:
> >> The commit:
> >>
> >> Commit: a53ae8e934cd54686875b5bcfc2f434244ee55d6
> >> Author: Marcel Apfelbaum <address@hidden>
> >> 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 <address@hidden>
>   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 <address@hidden>
>       Acked-by: Michael S. Tsirkin <address@hidden>
>       Signed-off-by: Michael S. Tsirkin <address@hidden>
> 
> 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