Re: [PATCH v2] MdeModulePkg/Bus: Enable ascending resource list

[Roman Bacik <roman.bacik@broadcom.com>]

Laszlo,

You are right, the ascending order is not necessary and the resources can
stay in descending order. Thank you very much for your clarification. It is
the second part of the patch, which is necessary:

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
index 2f713fcee95e..6a6a7e73d343 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
@@ -1300,6 +1300,8 @@ ProgramBar (
   case PciBarTypeMem32:
   case PciBarTypePMem32:

+    Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
+    Address %= Base;
     PciIo->Pci.Write (
                  PciIo,
                  EfiPciIoWidthUint32,
@@ -1308,13 +1310,15 @@ ProgramBar (
                  &Address
                  );

-    Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
+    //Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;

     break;

   case PciBarTypeMem64:
   case PciBarTypePMem64:

+    Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
+    Address %= Base;//^M
     Address32 = (UINT32) (Address & 0x00000000FFFFFFFF);

     PciIo->Pci.Write (
@@ -1335,7 +1339,7 @@ ProgramBar (
                  &Address32
                  );

-    Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
+    //Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;

     break;

In particular, the BARs must be programmed with a bus address of 0, not the
memory address of 0x60000000. I will submit another patch enabling this. I
would like to withdraw the current patch.
I appreciate your time and apologise for my misunderstandings,

Roman


> -----Original Message-----
> From: Roman Bacik [mailto:roman.bacik@broadcom.com]
> Sent: Thursday, May 3, 2018 9:46 AM
> To: 'Laszlo Ersek'
> Cc: 'edk2-devel@lists.01.org'; 'Ruiyu Ni'; Vladimir Olovyannikov
> Subject: RE: [edk2] [PATCH v2] MdeModulePkg/Bus: Enable ascending
> resource list
>
> Laszlo,
>
> After changing gcd allocate type to EfiGcdAllocateAnySearchBottomUp we
> are getting a correct placement. However when using the original
> descending
> order, we are getting a crash later unless we change the resource order to
> ascending. That is why we would like to enable ascending order. This is
> the
> crash we are getting without the patch:
>
> PciBus: Resource Map for Bridge [00|00|00]
> Type = PMem32; Base = 0x60000000;       Length = 0x100000;      Alignment
> =
> 0xFFFFF
>    Base = 0x60000000;   Length = 0x20000;       Alignment = 0x1FFFF;
> Owner =
> PCI [01|00|03:18]; Type = PMem64
>    Base = 0x60020000;   Length = 0x20000;       Alignment = 0x1FFFF;
> Owner =
> PCI [01|00|02:18]; Type = PMem64
>    Base = 0x60040000;   Length = 0x20000;       Alignment = 0x1FFFF;
> Owner =
> PCI [01|00|01:18]; Type = PMem64
>    Base = 0x60060000;   Length = 0x20000;       Alignment = 0x1FFFF;
> Owner =
> PCI [01|00|00:18]; Type = PMem64
>    Base = 0x60080000;   Length = 0x10000;       Alignment = 0xFFFF;
> Owner =
> PCI [01|00|03:10]; Type = PMem64
>    Base = 0x60090000;   Length = 0x10000;       Alignment = 0xFFFF;
> Owner =
> PCI [01|00|02:10]; Type = PMem64
>    Base = 0x600A0000;   Length = 0x10000;       Alignment = 0xFFFF;
> Owner =
> PCI [01|00|01:10]; Type = PMem64
>    Base = 0x600B0000;   Length = 0x10000;       Alignment = 0xFFFF;
> Owner =
> PCI [01|00|00:10]; Type = PMem64
>    Base = 0x600C0000;   Length = 0x1000;        Alignment = 0xFFF;
> Owner =
> PCI [01|00|03:20]; Type = PMem64
>    Base = 0x600C1000;   Length = 0x1000;        Alignment = 0xFFF;
> Owner =
> PCI [01|00|02:20]; Type = PMem64
>    Base = 0x600C2000;   Length = 0x1000;        Alignment = 0xFFF;
> Owner =
> PCI [01|00|01:20]; Type = PMem64
>    Base = 0x600C3000;   Length = 0x1000;        Alignment = 0xFFF;
> Owner =
> PCI [01|00|00:20]; Type = PMem64
>
> ...
>
> SError Exception at 0x00000000FDDF6338
> PC 0x0000FDDF6338 (0x0000FDDF4000+0x00002338) [ 0] CpuIo2Dxe.dll PC
> 0x0000FDDF62F4 (0x0000FDDF4000+0x000022F4) [ 0] CpuIo2Dxe.dll PC
> 0x0000FDDF5634 (0x0000FDDF4000+0x00001634) [ 0] CpuIo2Dxe.dll PC
> 0x0000F87EAFAC (0x0000F87E7000+0x00003FAC) [ 1] PciHostBridge.dll PC
> 0x0000F81E926C (0x0000F81DA000+0x0000F26C) [ 2] PciBusDxe.dll PC
> 0x0000F8765F8C (0x0000F8757000+0x0000EF8C) [ 3] cxundi.dll PC
> 0x0000F875C96C (0x0000F8757000+0x0000596C) [ 3] cxundi.dll PC
> 0x0000F875B68C (0x0000F8757000+0x0000468C) [ 3] cxundi.dll PC
> 0x0000F8762480 (0x0000F8757000+0x0000B480) [ 3] cxundi.dll PC
> 0x0000FE66191C (0x0000FE653000+0x0000E91C) [ 4] DxeCore.dll PC
> 0x0000FE660CE8 (0x0000FE653000+0x0000DCE8) [ 4] DxeCore.dll PC
> 0x0000FE661038 (0x0000FE653000+0x0000E038) [ 4] DxeCore.dll PC
> 0x0000F8645114 (0x0000F8636000+0x0000F114) [ 5] BdsDxe.dll PC
> 0x0000F8645184 (0x0000F8636000+0x0000F184) [ 5] BdsDxe.dll PC
> 0x0000F8653B38 (0x0000F8636000+0x0001DB38) [ 5] BdsDxe.dll PC
> 0x0000F8638E34 (0x0000F8636000+0x00002E34) [ 5] BdsDxe.dll PC
> 0x0000FE655524 (0x0000FE653000+0x00002524) [ 6] DxeCore.dll PC
> 0x0000FE6544A0 (0x0000FE653000+0x000014A0) [ 6] DxeCore.dll PC
> 0x0000FE654024 (0x0000FE653000+0x00001024) [ 6] DxeCore.dll PC
> 0x0000850099F4 PC 0x000085009BD8 PC 0x000085000DCC PC 0x000085000FC4
> PC 0x000085000F18 PC 0x0000850008BC
>
> [ 0]
> /local/ldk/edk2/Build/NS2Pkg/NOOPT_GCC5/AARCH64/UefiCpuPkg/CpuIo2
> Dxe/CpuIo2Dxe/DEBUG/CpuIo2Dxe.dll
> [ 1]
> /local/ldk/edk2/Build/NS2Pkg/NOOPT_GCC5/AARCH64/BroadcomPlatformP
> kg/Drivers/BrcmPciHostBridgeDxe/PciHostBridgeDxe/DEBUG/PciHostBridge.
> dll
> [ 2]
> /local/ldk/edk2/Build/NS2Pkg/NOOPT_GCC5/AARCH64/MdeModulePkg/Bus
> /Pci/PciBusDxe/PciBusDxe/DEBUG/PciBusDxe.dll
> [ 3]
> /local/ldk/edk2/Build/NS2Pkg/NOOPT_GCC5/AARCH64/BroadcomPlatformP
> kg/Drivers/CxUndiDxe/cxundi_auto/DEBUG/cxundi.dll
> [ 4]
> /local/ldk/edk2/Build/NS2Pkg/NOOPT_GCC5/AARCH64/MdeModulePkg/Cor
> e/Dxe/DxeMain/DEBUG/DxeCore.dll
> [ 5]
> /local/ldk/edk2/Build/NS2Pkg/NOOPT_GCC5/AARCH64/MdeModulePkg/Uni
> versal/BdsDxe/BdsDxe/DEBUG/BdsDxe.dll
> [ 6]
> /local/ldk/edk2/Build/NS2Pkg/NOOPT_GCC5/AARCH64/MdeModulePkg/Cor
> e/Dxe/DxeMain/DEBUG/DxeCore.dll
>
> Thanks,
>
> Roman
>
> > -----Original Message-----
> > From: Roman Bacik [mailto:roman.bacik@broadcom.com]
> > Sent: Thursday, May 3, 2018 8:23 AM
> > To: 'Laszlo Ersek'
> > Cc: 'edk2-devel@lists.01.org'; 'Ruiyu Ni'; Vladimir Olovyannikov
> > Subject: RE: [edk2] [PATCH v2] MdeModulePkg/Bus: Enable ascending
> > resource list
> >
> > Without any patches we would get the following undesirable placement:
> >
> > PCI Bus First Scanning
> > PciBus: Discovered PPB @ [00|00|00]
> >
> > PciBus: Discovered PCI @ [01|00|00]
> >  ARI: CapOffset = 0x1B8
> >    BAR[0]: Type = PMem64; Alignment = 0xFFFF;   Length = 0x10000;
> Offset
> > = 0x10
> >    BAR[1]: Type = PMem64; Alignment = 0x1FFFF;  Length = 0x20000;
> Offset
> > = 0x18
> >    BAR[2]: Type = PMem64; Alignment = 0xFFF;    Length = 0x1000;
> > Offset
> =
> > 0x20
> >
> > PciBus: Discovered PCI @ [01|00|01]
> >  ARI: CapOffset = 0x1B8
> >    BAR[0]: Type = PMem64; Alignment = 0xFFFF;   Length = 0x10000;
> Offset
> > = 0x10
> >    BAR[1]: Type = PMem64; Alignment = 0x1FFFF;  Length = 0x20000;
> Offset
> > = 0x18
> >    BAR[2]: Type = PMem64; Alignment = 0xFFF;    Length = 0x1000;
> > Offset
> =
> > 0x20
> >
> > PciBus: Discovered PCI @ [01|00|02]
> >  ARI: CapOffset = 0x1B8
> >    BAR[0]: Type = PMem64; Alignment = 0xFFFF;   Length = 0x10000;
> Offset
> > = 0x10
> >    BAR[1]: Type = PMem64; Alignment = 0x1FFFF;  Length = 0x20000;
> Offset
> > = 0x18
> >    BAR[2]: Type = PMem64; Alignment = 0xFFF;    Length = 0x1000;
> > Offset
> =
> > 0x20
> >
> > PciBus: Discovered PCI @ [01|00|03]
> >  ARI: CapOffset = 0x1B8
> >    BAR[0]: Type = PMem64; Alignment = 0xFFFF;   Length = 0x10000;
> Offset
> > = 0x10
> >    BAR[1]: Type = PMem64; Alignment = 0x1FFFF;  Length = 0x20000;
> Offset
> > = 0x18
> >    BAR[2]: Type = PMem64; Alignment = 0xFFF;    Length = 0x1000;
> > Offset
> =
> > 0x20
> >
> > PciBus: Discovered PPB @ [00|00|00]
> >
> > PciBus: Discovered PCI @ [01|00|00]
> >  ARI: CapOffset = 0x1B8
> >    BAR[0]: Type = PMem64; Alignment = 0xFFFF;   Length = 0x10000;
> Offset
> > = 0x10
> >    BAR[1]: Type = PMem64; Alignment = 0x1FFFF;  Length = 0x20000;
> Offset
> > = 0x18
> >    BAR[2]: Type = PMem64; Alignment = 0xFFF;    Length = 0x1000;
> > Offset
> =
> > 0x20
> >
> > PciBus: Discovered PCI @ [01|00|01]
> >  ARI: CapOffset = 0x1B8
> >    BAR[0]: Type = PMem64; Alignment = 0xFFFF;   Length = 0x10000;
> Offset
> > = 0x10
> >    BAR[1]: Type = PMem64; Alignment = 0x1FFFF;  Length = 0x20000;
> Offset
> > = 0x18
> >    BAR[2]: Type = PMem64; Alignment = 0xFFF;    Length = 0x1000;
> > Offset
> =
> > 0x20
> >
> > PciBus: Discovered PCI @ [01|00|02]
> >  ARI: CapOffset = 0x1B8
> >    BAR[0]: Type = PMem64; Alignment = 0xFFFF;   Length = 0x10000;
> Offset
> > = 0x10
> >    BAR[1]: Type = PMem64; Alignment = 0x1FFFF;  Length = 0x20000;
> Offset
> > = 0x18
> >    BAR[2]: Type = PMem64; Alignment = 0xFFF;    Length = 0x1000;
> > Offset
> =
> > 0x20
> >
> > PciBus: Discovered PCI @ [01|00|03]
> >  ARI: CapOffset = 0x1B8
> >    BAR[0]: Type = PMem64; Alignment = 0xFFFF;   Length = 0x10000;
> Offset
> > = 0x10
> >    BAR[1]: Type = PMem64; Alignment = 0x1FFFF;  Length = 0x20000;
> Offset
> > = 0x18
> >    BAR[2]: Type = PMem64; Alignment = 0xFFF;    Length = 0x1000;
> > Offset
> =
> > 0x20
> >
> > PciBus: HostBridge->SubmitResources() - Success
> > PciBus: HostBridge->NotifyPhase(AllocateResources) - Success
> > PciBus: Resource Map for Root Bridge Acpi(PNP0A05,0x0)
> > Type = PMem32; Base = 0x60B00000;       Length = 0x100000;
> > Alignment =
> > 0xFFFFF
> >    Base = 0x60B00000;   Length = 0x100000;      Alignment = 0xFFFFF;
> > Owner
> =
> > PPB [00|00|00:**]
> >
> > PciBus: Resource Map for Bridge [00|00|00]
> > Type = PMem32; Base = 0x60B00000;       Length = 0x100000;
> > Alignment =
> > 0xFFFFF
> >    Base = 0x60B00000;   Length = 0x20000;       Alignment = 0x1FFFF;
> > Owner
> =
> > PCI [01|00|03:18]; Type = PMem64
> >    Base = 0x60B20000;   Length = 0x20000;       Alignment = 0x1FFFF;
> > Owner
> =
> > PCI [01|00|02:18]; Type = PMem64
> >    Base = 0x60B40000;   Length = 0x20000;       Alignment = 0x1FFFF;
> > Owner
> =
> > PCI [01|00|01:18]; Type = PMem64
> >    Base = 0x60B60000;   Length = 0x20000;       Alignment = 0x1FFFF;
> > Owner
> =
> > PCI [01|00|00:18]; Type = PMem64
> >    Base = 0x60B80000;   Length = 0x10000;       Alignment = 0xFFFF;
> > Owner =
> > PCI [01|00|03:10]; Type = PMem64
> >    Base = 0x60B90000;   Length = 0x10000;       Alignment = 0xFFFF;
> > Owner =
> > PCI [01|00|02:10]; Type = PMem64
> >    Base = 0x60BA0000;   Length = 0x10000;       Alignment = 0xFFFF;
> > Owner =
> > PCI [01|00|01:10]; Type = PMem64
> >    Base = 0x60BB0000;   Length = 0x10000;       Alignment = 0xFFFF;
> > Owner =
> > PCI [01|00|00:10]; Type = PMem64
> >    Base = 0x60BC0000;   Length = 0x1000;        Alignment = 0xFFF;
> > Owner =
> > PCI [01|00|03:20]; Type = PMem64
> >    Base = 0x60BC1000;   Length = 0x1000;        Alignment = 0xFFF;
> > Owner =
> > PCI [01|00|02:20]; Type = PMem64
> >    Base = 0x60BC2000;   Length = 0x1000;        Alignment = 0xFFF;
> > Owner =
> > PCI [01|00|01:20]; Type = PMem64
> >    Base = 0x60BC3000;   Length = 0x1000;        Alignment = 0xFFF;
> > Owner =
> > PCI [01|00|00:20]; Type = PMem64
> >
> > Thanks,
> >
> > Roman
> >
> > > -----Original Message-----
> > > From: Roman Bacik [mailto:roman.bacik@broadcom.com]
> > > Sent: Thursday, May 3, 2018 7:58 AM
> > > To: 'Laszlo Ersek'
> > > Cc: 'edk2-devel@lists.01.org'; 'Ruiyu Ni'; Vladimir Olovyannikov
> > > Subject: RE: [edk2] [PATCH v2] MdeModulePkg/Bus: Enable ascending
> > > resource list
> > >
> > > Laszlo,
> > >
> > > Thank you very much for your explanation.
> > >
> > > > -----Original Message-----
> > > > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > > > Sent: Thursday, May 3, 2018 6:52 AM
> > > > To: Roman Bacik
> > > > Cc: edk2-devel@lists.01.org; Ruiyu Ni; Vladimir Olovyannikov
> > > > Subject: Re: [edk2] [PATCH v2] MdeModulePkg/Bus: Enable ascending
> > > > resource list
> > > >
> > > > On 05/02/18 20:07, Roman Bacik wrote:
> > > > > Some processors require resource list sorted in ascending order.
> > > > >
> > > > > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > > > > Cc: Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com>
> > > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > > Signed-off-by: Roman Bacik <roman.bacik@broadcom.com>
> > > > > ---
> > > > >  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf       |  1 +
> > > > >  .../Bus/Pci/PciBusDxe/PciResourceSupport.c         | 43
> > > > ++++++++++++++++++----
> > > > >  MdeModulePkg/MdeModulePkg.dec                      |  3 ++
> > > > >  MdeModulePkg/MdeModulePkg.dsc                      |  1 +
> > > > >  4 files changed, 41 insertions(+), 7 deletions(-)
> > > >
> > > > I don't understand the goal of this patch.
> > > >
> > > > (Please don't take my comments as "review" or arguments against
> > > > this patch, I'd just like to understand the issue.)
> > > >
> > > > To my understanding, InsertResourceNode() keeps PCI resources
> > > > sorted in descending *alignment* order -- because a larger
> > > > power-of-two alignment is also suitable for a smaller power-of-two
> alignment.
> > > >
> > > > Say, we have a bridge with two devices behind it. The first device
> > > > has one MMIO BAR of size 32MB (requiring the same 32MB natural
> > > > alignment), while the other device has one MMIO BAR, of size 4MB
> > > > (requiring the same 4MB natural alignment).
> > > >
> > > > Ordering the resources in descending *alignment* order means that
> > > > we'll 1st allocate the 32MB BAR, at a suitably aligned address.
> > > > The important thing is that the end of that allocation will *also*
> > > > be aligned at 32MB, hence we can allocate, as 2nd step, the 4MB
> > > > BAR right
> > > there. No gaps needed.
> > > >
> > > > If the 4MB BAR was allocated 1st (naturally aligned), then its end
> > > > address might not be naturally aligned for becoming the base
> > > > address of the remaining 32MB BAR. Thus we might have to waste
> > > > some MMIO aperture to align the large BAR correctly.
> > > >
> > > > Here's an example output from PciBusDxe running as part of OVMF:
> > > >
> > > > > PciBus: Resource Map for Root Bridge PciRoot(0x0) [...]
> > > > > Type =  Mem32; Base = 0x80000000;       Length = 0x8100000;
> > Alignment
> > > =
> > > > 0x3FFFFFF
> > > > >    Base = 0x80000000;   Length = 0x4000000;     Alignment =
> > > > > 0x3FFFFFF;
> > > > Owner = PCI [00|02|00:14]
> > > > >    Base = 0x84000000;   Length = 0x4000000;     Alignment =
> > > > > 0x3FFFFFF;
> > > > Owner = PCI [00|02|00:10]
> > > > >    Base = 0x88000000;   Length = 0x2000;        Alignment =
> > > > > 0x1FFF;
> > Owner
> > > =
> > > > PCI [00|02|00:18]
> > > > >    Base = 0x88002000;   Length = 0x1000;        Alignment = 0xFFF;
> Owner
> > =
> > > > PCI [00|07|00:14]
> > > > >    Base = 0x88003000;   Length = 0x1000;        Alignment = 0xFFF;
> Owner
> > =
> > > > PCI [00|06|07:10]
> > > > >    Base = 0x88004000;   Length = 0x1000;        Alignment = 0xFFF;
> Owner
> > =
> > > > PCI [00|05|00:14]
> > > > >    Base = 0x88005000;   Length = 0x1000;        Alignment = 0xFFF;
> Owner
> > =
> > > > PCI [00|03|00:14]
> > > > > [...]
> > > >
> > > > The base addresses are already sorted in ascending order; what's
> > > > descending is the alignment -- and that seems correct, because it
> > > > conserves MMIO aperture (no gaps needed).
> > > >
> > > > Can you please explain what's wrong with this approach for your
> > > > platform, and what the desired behavior is?
> > > >
> > > > Can you post a log snippet that shows a placement that's right for
> > > > your platform?
> > >
> > > We are also patching allocation order bottom up search starting at
> > > BaseAddress.
> > >
> > > This placement is desirable and working for us after applying the
> > > submitted two patches:
> > >
> > > PciBus: Resource Map for Bridge [00|00|00]
> > > Type = PMem32; Base = 0x60000000;       Length = 0x100000;
> > > Alignment
> =
> > > 0xFFFFF
> > >    Base = 0x60000000;   Length = 0x20000;       Alignment = 0x1FFFF;
> Owner
> > =
> > > PCI [01|00|00:18]; Type = PMem64
> > >    Base = 0x60020000;   Length = 0x20000;       Alignment = 0x1FFFF;
> Owner
> > =
> > > PCI [01|00|01:18]; Type = PMem64
> > >    Base = 0x60040000;   Length = 0x20000;       Alignment = 0x1FFFF;
> Owner
> > =
> > > PCI [01|00|02:18]; Type = PMem64
> > >    Base = 0x60060000;   Length = 0x20000;       Alignment = 0x1FFFF;
> Owner
> > =
> > > PCI [01|00|03:18]; Type = PMem64
> > >    Base = 0x60080000;   Length = 0x10000;       Alignment = 0xFFFF;
> > > Owner
> =
> > > PCI [01|00|00:10]; Type = PMem64
> > >    Base = 0x60090000;   Length = 0x10000;       Alignment = 0xFFFF;
> > > Owner
> =
> > > PCI [01|00|01:10]; Type = PMem64
> > >    Base = 0x600A0000;   Length = 0x10000;       Alignment = 0xFFFF;
> > > Owner
> =
> > > PCI [01|00|02:10]; Type = PMem64
> > >    Base = 0x600B0000;   Length = 0x10000;       Alignment = 0xFFFF;
> > > Owner
> =
> > > PCI [01|00|03:10]; Type = PMem64
> > >    Base = 0x600C0000;   Length = 0x1000;        Alignment = 0xFFF;
> > > Owner
> =
> > > PCI [01|00|00:20]; Type = PMem64
> > >    Base = 0x600C1000;   Length = 0x1000;        Alignment = 0xFFF;
> > > Owner
> =
> > > PCI [01|00|01:20]; Type = PMem64
> > >    Base = 0x600C2000;   Length = 0x1000;        Alignment = 0xFFF;
> > > Owner
> =
> > > PCI [01|00|02:20]; Type = PMem64
> > >    Base = 0x600C3000;   Length = 0x1000;        Alignment = 0xFFF;
> > > Owner
> =
> > > PCI [01|00|03:20]; Type = PMem64
> > >
> > > >
> > > > Thanks,
> > > > Laszlo
> > >
> > > Thanks,
> > > Roman