[BUG] pci: mixed allocation pf and non-pf PCI MEM BAR (OVMF crash)
diff mbox series

Message ID alpine.GSO.2.00.1903310940430.7231@dmz.c-home.cz
State New, archived
Headers show
Series
  • [BUG] pci: mixed allocation pf and non-pf PCI MEM BAR (OVMF crash)
Related show

Commit Message

Martin Cerveny March 31, 2019, 8:11 a.m. UTC
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(-)

Comments

Jan Beulich April 1, 2019, 8:59 a.m. UTC | #1
>>> 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
Martin Cerveny April 1, 2019, 6:48 p.m. UTC | #2
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
Igor Druzhinin April 5, 2019, 5 p.m. UTC | #3
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
Martin Cerveny April 6, 2019, 6:39 a.m. UTC | #4
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>
Jan Beulich April 8, 2019, 8:07 a.m. UTC | #5
>>> 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

Patch
diff mbox series

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));