| Message ID | alpine.GSO.2.00.1903310940430.7231@dmz.c-home.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [BUG] pci: mixed allocation pf and non-pf PCI MEM BAR (OVMF crash) | expand |
>>> On 31.03.19 at 10:11, <martin@c-home.cz> wrote: > There is problem in PCI device allocation algorithm (pci_setup()). > Algorithm allocates PCI BAR sorted by size and this allows > mixed allocation of prefetchable and non-prefetchable PCI MEM BAR. > This leads to wrong config of PCI root port (see "Type 1 Configuration > Space Registers (Root Ports)"). > > Tested with version xen 4.11.1 + "export > OVMF_UPSTREAM_REVISION=ef529e6ab7c31290a33045bb1f1837447cc0eb56" > (embeded commit OVMF does not work (crashed even in Win10.iso and > uncompilable with newer gcc)). > > Attached also testing patch. I agree the problem wants addressing, but I'm afraid the patch is incomplete: Afaict it won't work if there is a non-prefetchable BAR larger than the smallest prefetchable one, due to the necessary extra padding that would need inserting between the two. Also the patch lack your S-o-b and has a couple of style issues (hard tab used, indentation, lack of blanks inside if(), unnecessary double use of PCI_BASE_ADDRESS_MEM_PREFETCH). As to its description - do you perhaps mean "interleaved" instead of "fragmented" and "not allowed" instead of "not supported"? Jan
Hello. On Mon, 1 Apr 2019, Jan Beulich wrote: >>>> On 31.03.19 at 10:11, <martin@c-home.cz> wrote: >> There is problem in PCI device allocation algorithm (pci_setup()). >> Algorithm allocates PCI BAR sorted by size and this allows >> mixed allocation of prefetchable and non-prefetchable PCI MEM BAR. >> This leads to wrong config of PCI root port (see "Type 1 Configuration >> Space Registers (Root Ports)"). >> >> Tested with version xen 4.11.1 + "export >> OVMF_UPSTREAM_REVISION=ef529e6ab7c31290a33045bb1f1837447cc0eb56" >> (embeded commit OVMF does not work (crashed even in Win10.iso and >> uncompilable with newer gcc)). >> >> Attached also testing patch. > > I agree the problem wants addressing, but I'm afraid the patch is > incomplete: Afaict it won't work if there is a non-prefetchable BAR > larger than the smallest prefetchable one, due to the necessary > extra padding that would need inserting between the two. I make this patch as proof-of-concept to get it compatible with OVMF. But I am not aware if OVMF is OK and all other consequences, like: - allocation aligning as you mention - PF region to be placed first or last in MMIO hole - some PCI express documents says that all 64bit MEM BAR must be prefetchable [ But if I activate "usbversion=3" it generates 64bit MEM BAR but without prefetchable bit (so PCI_BASE_ADDRESS_MEM_PREFETCH is implicit?) and actual code allocates MMIO under 4G. But OVMF requires that 64bit MEM BAR must be placed over 4G (just another problem/BUG). ] (example - https://resources.infosecinstitute.com/system-address-map-initialization-x86x64-architecture-part-2-pci-express-based-systems/ ) > Also the patch lack your S-o-b and has a couple of style issues > (hard tab used, indentation, lack of blanks inside if(), unnecessary > double use of PCI_BASE_ADDRESS_MEM_PREFETCH). > As to its description - do you perhaps mean "interleaved" instead > of "fragmented" and "not allowed" instead of "not supported"? Rework/use the patch as needed. Martin
On 01/04/2019 19:48, Martin Cerveny wrote: > Hello. > > On Mon, 1 Apr 2019, Jan Beulich wrote: >>>>> On 31.03.19 at 10:11, <martin@c-home.cz> wrote: >>> There is problem in PCI device allocation algorithm (pci_setup()). >>> Algorithm allocates PCI BAR sorted by size and this allows >>> mixed allocation of prefetchable and non-prefetchable PCI MEM BAR. >>> This leads to wrong config of PCI root port (see "Type 1 Configuration >>> Space Registers (Root Ports)"). >>> >>> Tested with version xen 4.11.1 + "export >>> OVMF_UPSTREAM_REVISION=ef529e6ab7c31290a33045bb1f1837447cc0eb56" >>> (embeded commit OVMF does not work (crashed even in Win10.iso and >>> uncompilable with newer gcc)). >>> >>> Attached also testing patch. >> >> I agree the problem wants addressing, but I'm afraid the patch is >> incomplete: Afaict it won't work if there is a non-prefetchable BAR >> larger than the smallest prefetchable one, due to the necessary >> extra padding that would need inserting between the two. > > I make this patch as proof-of-concept to get it compatible with OVMF. > But I am not aware if OVMF is OK and all other consequences, like: > > - allocation aligning as you mention > - PF region to be placed first or last in MMIO hole - some PCI express > documents says that all 64bit MEM BAR must be prefetchable > [ But if I activate "usbversion=3" it generates 64bit MEM BAR but > without prefetchable bit (so PCI_BASE_ADDRESS_MEM_PREFETCH is implicit?) > and actual code allocates MMIO under 4G. But OVMF requires that 64bit > MEM BAR must be placed over 4G (just another problem/BUG). ] > > (example - > https://resources.infosecinstitute.com/system-address-map-initialization-x86x64-architecture-part-2-pci-express-based-systems/ > ) > Hi Martin, We're aware of the problem and it needs to be fixed on OVMF side where Xen specific code incorrectly tries to initialize a prefetchable aperture where it in fact doesn't exist in our root bridge. I'm actively working on a series for OVMF which addresses the problems that you mentioned [1]. As to hvmloader, coexisting of prefetchable and non-prefetchable BARs in a single aperture is perfectly normal from PCI specification point of view (e.g. see "PCI-TO-PCI BRIDGE ARCHITECTURE SPECIFICATION, REV 1.2"). [1] https://marc.info/?l=xen-devel&m=155187612626640&w=2 Igor
Hello. On Fri, 5 Apr 2019, Igor Druzhinin wrote: > On 01/04/2019 19:48, Martin Cerveny wrote: >> On Mon, 1 Apr 2019, Jan Beulich wrote: >>>>>> On 31.03.19 at 10:11, <martin@c-home.cz> wrote: >>>> There is problem in PCI device allocation algorithm (pci_setup()). >>>> Algorithm allocates PCI BAR sorted by size and this allows >>>> mixed allocation of prefetchable and non-prefetchable PCI MEM BAR. >>>> This leads to wrong config of PCI root port (see "Type 1 Configuration >>>> Space Registers (Root Ports)"). >>>> >>>> Tested with version xen 4.11.1 + "export >>>> OVMF_UPSTREAM_REVISION=ef529e6ab7c31290a33045bb1f1837447cc0eb56" >>>> (embeded commit OVMF does not work (crashed even in Win10.iso and >>>> uncompilable with newer gcc)). >>>> >>>> Attached also testing patch. >>> >>> I agree the problem wants addressing, but I'm afraid the patch is >>> incomplete: Afaict it won't work if there is a non-prefetchable BAR >>> larger than the smallest prefetchable one, due to the necessary >>> extra padding that would need inserting between the two. >> >> I make this patch as proof-of-concept to get it compatible with OVMF. >> But I am not aware if OVMF is OK and all other consequences, like: >> >> - allocation aligning as you mention >> - PF region to be placed first or last in MMIO hole - some PCI express >> documents says that all 64bit MEM BAR must be prefetchable >> [ But if I activate "usbversion=3" it generates 64bit MEM BAR but >> without prefetchable bit (so PCI_BASE_ADDRESS_MEM_PREFETCH is implicit?) >> and actual code allocates MMIO under 4G. But OVMF requires that 64bit >> MEM BAR must be placed over 4G (just another problem/BUG). ] >> >> (example - >> https://resources.infosecinstitute.com/system-address-map-initialization-x86x64-architecture-part-2-pci-express-based-systems/ >> ) >> > > Hi Martin, > > We're aware of the problem and it needs to be fixed on OVMF side where > Xen specific code incorrectly tries to initialize a prefetchable > aperture where it in fact doesn't exist in our root bridge. I'm actively > working on a series for OVMF which addresses the problems that you > mentioned [1]. > > As to hvmloader, coexisting of prefetchable and non-prefetchable BARs in > a single aperture is perfectly normal from PCI specification point of > view (e.g. see "PCI-TO-PCI BRIDGE ARCHITECTURE SPECIFICATION, REV 1.2"). > > [1] https://marc.info/?l=xen-devel&m=155187612626640&w=2 Good news! I see ignoring PF/non-PF differences in your patch probably it can be possible in virtualized world. I do not known. And do not forget that XEN using (old) fork of OVMF :-( XEN4.12 - commit ef529e6ab7c31290a33045bb1f1837447cc0eb56 (2018-07-25) XEN4.11.1 - commit 947f3737abf65fda63f3ffd97fddfa6986986868 (2017-09-19) Thanks, M.C>
>>> On 05.04.19 at 19:00, <igor.druzhinin@citrix.com> wrote: > As to hvmloader, coexisting of prefetchable and non-prefetchable BARs in > a single aperture is perfectly normal from PCI specification point of > view (e.g. see "PCI-TO-PCI BRIDGE ARCHITECTURE SPECIFICATION, REV 1.2"). After my earlier response I too have been thinking about this some more. Looking at the spec, I can't seem to find any statement either to the effect of what you say, or to the effect of the opposite. From the text one might imply that putting prefetchable regions inside a non-prefetchable bridge window ought to be acceptable, albeit perhaps inefficient. But of course that's limited to cases where all BARs can be fit below 4Gb. But I don't think that spec is applicable here at all, as long as we don't emulate any PCI-to-PCI bridges. There are only a host bridge, a PCI-to-ISA one, and a "misc" one (the PM functions of PIIX4) right now. In any event I agree that at present mixing types ought to be fine. This would likely change once we finally gain support for emulating something like the Q35. Jan
diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c index 0b708bf578..42a3dd5e62 100644 --- a/tools/firmware/hvmloader/pci.c +++ b/tools/firmware/hvmloader/pci.c @@ -97,7 +97,7 @@ void pci_setup(void) uint32_t bar_reg; uint64_t bar_sz; } *bars = (struct bars *)scratch_start; - unsigned int i, nr_bars = 0; + unsigned int i, nr_bars = 0, nr_bars_pref = 0; uint64_t mmio_hole_size = 0; const char *s; @@ -253,9 +253,19 @@ void pci_setup(void) if ( bar_sz == 0 ) continue; - for ( i = 0; i < nr_bars; i++ ) - if ( bars[i].bar_sz < bar_sz ) - break; + if (((bar_data & PCI_BASE_ADDRESS_SPACE) == + PCI_BASE_ADDRESS_SPACE_MEMORY) && + ((bar_data & PCI_BASE_ADDRESS_MEM_PREFETCH) == + PCI_BASE_ADDRESS_MEM_PREFETCH)) { + for ( i = 0; i < nr_bars_pref; i++ ) + if ( bars[i].bar_sz < bar_sz ) + break; + nr_bars_pref++; + } + else + for ( i = nr_bars_pref; i < nr_bars; i++ ) + if ( bars[i].bar_sz < bar_sz ) + break; if ( i != nr_bars ) memmove(&bars[i+1], &bars[i], (nr_bars-i) * sizeof(*bars));
Hello. There is problem in PCI device allocation algorithm (pci_setup()). Algorithm allocates PCI BAR sorted by size and this allows mixed allocation of prefetchable and non-prefetchable PCI MEM BAR. This leads to wrong config of PCI root port (see "Type 1 Configuration Space Registers (Root Ports)"). Tested with version xen 4.11.1 + "export OVMF_UPSTREAM_REVISION=ef529e6ab7c31290a33045bb1f1837447cc0eb56" (embeded commit OVMF does not work (crashed even in Win10.iso and uncompilable with newer gcc)). Attached also testing patch. Thanks, Martin Cerveny --------------------------------------------------------------------- Verifiable in crash of OVMF (UEFI firmware) [OVMF DEBUG]: ASSERT_EFI_ERROR (Status = Not Found) ASSERT /root/rpmbuild/BUILD/xen-4.11.1/tools/firmware/ovmf-dir-remote/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c(513): !EFI_ERROR (Status) --------------------------------------------------------------------- Summary of errored output: [xl dmesg] (d1) [ 395.698273] pci dev 0b:0 bar 14 size 010000000: 0e0000008 (d1) [ 395.714836] pci dev 0b:0 bar 18 size 002000000: 0f0000000 (d1) [ 395.715369] pci dev 02:0 bar 14 size 001000000: 0f2000008 (d1) [ 395.716049] pci dev 0b:0 bar 10 size 001000000: 0f3000000 (d1) [ 395.716479] pci dev 04:0 bar 30 size 000040000: 0f4000000 (d1) [ 395.716908] pci dev 0b:0 bar 30 size 000010000: 0f4040000 - *008 == prefetchable MEM BAR (PCI_BASE_ADDRESS_MEM_PREFETCH) [OVMF DEBUG]: InitRootBridge: populated root bus 0, with room for 0 subordinate bus(es) RootBridge: PciRoot(0x0) Support/Attr: 7007F / 7007F DmaAbove4G: No NoExtConfSpace: Yes AllocAttr: 0 () Bus: 0 - 0 Translation=0 Io: C000 - C2EF Translation=0 Mem: F0000000 - F40550FF Translation=0 MemAbove4G: FFFFFFFFFFFFFFFF - 0 Translation=0 PMem: E0000000 - F2FFFFFF Translation=0 PMemAbove4G: FFFFFFFFFFFFFFFF - 0 Translation=0 - check overlap of Mem an PMem! - check code tools/firmware/ovmf-dir-remote/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c // // Add all the Mem/PMem aperture to GCD // Mem/PMem shouldn't overlap with each other // Root bridge which needs to combine MEM and PMEM should only report // the MEM aperture in Mem // --------------------------------------------------------------------- Summary of OK/expected output: [xl dmesg] (d2) [ 1833.077182] pci dev 0b:0 bar 14 size 010000000: 0e0000008 (d2) [ 1833.077798] pci dev 02:0 bar 14 size 001000000: 0f0000008 (d2) [ 1833.103301] pci dev 0b:0 bar 18 size 002000000: 0f2000000 (d2) [ 1833.103882] pci dev 0b:0 bar 10 size 001000000: 0f4000000 (d2) [ 1833.104164] pci dev 04:0 bar 30 size 000040000: 0f5000000 (d2) [ 1833.104429] pci dev 0b:0 bar 30 size 000010000: 0f5040000 [OVMF DEBUG] RootBridge: PciRoot(0x0) Support/Attr: 7007F / 7007F DmaAbove4G: No NoExtConfSpace: Yes AllocAttr: 0 () Bus: 0 - 0 Translation=0 Io: C000 - C2EF Translation=0 Mem: F2000000 - F50550FF Translation=0 MemAbove4G: FFFFFFFFFFFFFFFF - 0 Translation=0 PMem: E0000000 - F0FFFFFF Translation=0 PMemAbove4G: FFFFFFFFFFFFFFFF - 0 Translation=0 --------------------------------------------------------------------- From b8d8f9f5ada9568f672f4ce9d68fe0f66cae44f5 Mon Sep 17 00:00:00 2001 From: Martin Cerveny <M.Cerveny@computer.org> Date: Sun, 31 Mar 2019 09:26:05 +0200 Subject: [PATCH] pci: Merge allocation of prefetchable MEM BAR Fragmented allocation of NON-prefetchable (MMIO) and prefetchable PCI MEM BAR not supported - see "Type 1 Configuration Space Registers (Root Ports)". --- tools/firmware/hvmloader/pci.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)