[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory re
From: |
Marcel Apfelbaum |
Subject: |
Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size |
Date: |
Thu, 07 Nov 2013 23:24:30 +0200 |
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
- [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Marcel Apfelbaum, 2013/11/03
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Jordan Justen, 2013/11/07
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Marcel Apfelbaum, 2013/11/07
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Laszlo Ersek, 2013/11/07
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size,
Marcel Apfelbaum <=
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Paolo Bonzini, 2013/11/07
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Marcel Apfelbaum, 2013/11/07
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Peter Maydell, 2013/11/07
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Marcel Apfelbaum, 2013/11/07
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Paolo Bonzini, 2013/11/08
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Peter Maydell, 2013/11/08
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Paolo Bonzini, 2013/11/08
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Marcel Apfelbaum, 2013/11/08
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Marcel Apfelbaum, 2013/11/08
- Re: [Qemu-devel] [PATCH] exec: fix regression by making system-memory region UINT64_MAX size, Paolo Bonzini, 2013/11/08