> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Wednesday, December 05, 2018 5:25 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A
> Subject: [PATCH] MdeModulePkg/PciBus: Shadow option ROM after BARs
> are programmed
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1376
>
> Today's implementation reuses the 32bit MMIO resource requested by
> all PCI devices MMIO BARs when shadowing the option ROM.
> Take a simple example, a system has only one PCI device. It requires
> 8MB 32bit MMIO and contains a 4MB option ROM. Today's implementation
> only requests 8MB (max of 4M and 8M) 32bit MMIO from
> PciHostBridgeResourceAllocation protocol. Let's assume the MMIO range
> [3GB, 3GB+8MB) is allocated. The 3GB base address is firstly
> programmed to the option ROM BAR for option ROM shadow. Then the
> option ROM decoding is turned off and 3GB base address is programmed
> to the 32bit MMIO BAR.
>
> It doesn't cause issues when the device doesn't request too much
> MMIO.
> But when the device contains a 64bit MMIO BAR which requests 4GB MMIO
> and a 4MB option ROM. Let's assume [3GB, 3GB+8MB) 32bit MMIO range is
"[3GB, 3GB+4MB)" here?
> allocated for the option ROM. When the option ROM is being shadowed,
> 64bit MMIO BAR is programmed to value 0, which means [0, 4GB) MMIO is
> given to the 64bit BAR.
> The range overlaps with the option ROM range which may cause the
> device malfunction (e.g.: option ROM cannot be read out) when the
> device has two separate decoders: one for MMIO BAR, the other for
> option ROM.
>
> The patch requests dedicated MEM32 resource for Option ROMs and
> moves the Option ROM shadow logic after all MMIO BARs are programmed.
> The MMIO BAR setting to 0 when shadowing Option ROM is also skipped
> because the MMIO BAR already contains the correct value.
Reviewed-by: Hao Wu <hao.a...@intel.com>
Best Regards,
Hao Wu
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu...@intel.com>
> Cc: Hao A Wu <hao.a...@intel.com>
> ---
> MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h | 3 +-
> MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 71
> ++++++++--------------
> .../Bus/Pci/PciBusDxe/PciOptionRomSupport.c | 13 ----
> .../Bus/Pci/PciBusDxe/PciResourceSupport.c | 37 ++++++++++-
> 4 files changed, 62 insertions(+), 62 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> index f9eebdd5a8..6938802857 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> @@ -1,7 +1,7 @@
> /** @file
> Header files and data structures needed by PCI Bus module.
>
> -Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD
> License
> which accompanies this distribution. The full text of the license may be
> found at
> @@ -67,6 +67,7 @@ typedef enum {
> PciBarTypePMem32,
> PciBarTypeMem64,
> PciBarTypePMem64,
> + PciBarTypeOpRom,
> PciBarTypeIo,
> PciBarTypeMem,
> PciBarTypeMaxType
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> index b81f81a136..7255bcfbbc 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> @@ -24,6 +24,7 @@ CHAR16 *mBarTypeStr[] = {
> L"PMem32",
> L" Mem64",
> L"PMem64",
> + L" OpRom",
> L" Io",
> L" Mem",
> L"Unknow"
> @@ -425,15 +426,9 @@ PciHostBridgeResourceAllocator (
> PCI_RESOURCE_NODE PMem32Pool;
> PCI_RESOURCE_NODE Mem64Pool;
> PCI_RESOURCE_NODE PMem64Pool;
> - BOOLEAN ReAllocate;
> EFI_DEVICE_HANDLE_EXTENDED_DATA_PAYLOAD HandleExtendedData;
> EFI_RESOURCE_ALLOC_FAILURE_ERROR_DATA_PAYLOAD
> AllocFailExtendedData;
>
> - //
> - // Reallocate flag
> - //
> - ReAllocate = FALSE;
> -
> //
> // It may try several times if the resource allocation fails
> //
> @@ -514,6 +509,17 @@ PciHostBridgeResourceAllocator (
> PciResUsageTypical
> );
>
> + //
> + // Get the max ROM size that the root bridge can process
> + // Insert to resource map so that there will be dedicate MEM32 resource
> range for Option ROM.
> + // All devices' Option ROM share the same MEM32 resource.
> + //
> + MaxOptionRomSize = GetMaxOptionRomSize (RootBridgeDev);
> + RootBridgeDev->PciBar[0].BarType = PciBarTypeOpRom;
> + RootBridgeDev->PciBar[0].Length = MaxOptionRomSize;
> + RootBridgeDev->PciBar[0].Alignment = MaxOptionRomSize - 1;
> + GetResourceFromDevice (RootBridgeDev, IoBridge, Mem32Bridge,
> PMem32Bridge, Mem64Bridge, PMem64Bridge);
> +
> //
> // Create resourcemap by going through all the devices subject to this
> root bridge
> //
> @@ -526,38 +532,6 @@ PciHostBridgeResourceAllocator (
> PMem64Bridge
> );
>
> - //
> - // Get the max ROM size that the root bridge can process
> - //
> - RootBridgeDev->RomSize = Mem32Bridge->Length;
> -
> - //
> - // Skip to enlarge the resource request during realloction
> - //
> - if (!ReAllocate) {
> - //
> - // Get Max Option Rom size for current root bridge
> - //
> - MaxOptionRomSize = GetMaxOptionRomSize (RootBridgeDev);
> -
> - //
> - // Enlarger the mem32 resource to accomdate the option rom
> - // if the mem32 resource is not enough to hold the rom
> - //
> - if (MaxOptionRomSize > Mem32Bridge->Length) {
> -
> - Mem32Bridge->Length = MaxOptionRomSize;
> - RootBridgeDev->RomSize = MaxOptionRomSize;
> -
> - //
> - // Alignment should be adjusted as well
> - //
> - if (Mem32Bridge->Alignment < MaxOptionRomSize - 1) {
> - Mem32Bridge->Alignment = MaxOptionRomSize - 1;
> - }
> - }
> - }
> -
> //
> // Based on the all the resource tree, construct ACPI resource node to
> // submit the resource aperture to pci host bridge protocol
> @@ -760,8 +734,6 @@ PciHostBridgeResourceAllocator (
> if (EFI_ERROR (Status)) {
> return Status;
> }
> -
> - ReAllocate = TRUE;
> }
> }
> //
> @@ -828,11 +800,6 @@ PciHostBridgeResourceAllocator (
> &PMem64Base
> );
>
> - //
> - // Process option rom for this root bridge
> - //
> - ProcessOptionRom (RootBridgeDev, Mem32Base, RootBridgeDev-
> >RomSize);
> -
> //
> // Create the entire system resource map from the information collected
> by
> // enumerator. Several resource tree was created
> @@ -889,6 +856,20 @@ PciHostBridgeResourceAllocator (
> PMem64Bridge
> );
>
> + //
> + // Process Option ROM for this root bridge after all BARs are programmed.
> + // The PPB's MEM32 RANGE BAR is re-programmed to the Option ROM
> BAR Base in order to
> + // shadow the Option ROM of the devices under the PPB.
> + // After the shadow, Option ROM BAR decoding is turned off and the
> PPB's MEM32 RANGE
> + // BAR is restored back to the original value.
> + // The original value is programmed by ProgramResource() above.
> + //
> + DEBUG ((
> + DEBUG_INFO, "Process Option ROM: BAR Base/Length = %lx/%lx\n",
> + RootBridgeDev->PciBar[0].BaseAddress, RootBridgeDev-
> >PciBar[0].Length
> + ));
> + ProcessOptionRom (RootBridgeDev, RootBridgeDev-
> >PciBar[0].BaseAddress, RootBridgeDev->PciBar[0].Length);
> +
> IoBridge ->PciDev->PciBar[IoBridge ->Bar].BaseAddress = IoBase;
> Mem32Bridge ->PciDev->PciBar[Mem32Bridge ->Bar].BaseAddress =
> Mem32Base;
> PMem32Bridge->PciDev->PciBar[PMem32Bridge->Bar].BaseAddress =
> PMem32Base;
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> index c2be85a906..aa314474dd 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciOptionRomSupport.c
> @@ -583,23 +583,10 @@ RomDecode (
> )
> {
> UINT32 Value32;
> - UINT32 Offset;
> - UINT32 OffsetMax;
> EFI_PCI_IO_PROTOCOL *PciIo;
>
> PciIo = &PciDevice->PciIo;
> if (Enable) {
> - //
> - // Clear all bars
> - //
> - OffsetMax = 0x24;
> - if (IS_PCI_BRIDGE(&PciDevice->Pci)) {
> - OffsetMax = 0x14;
> - }
> -
> - for (Offset = 0x10; Offset <= OffsetMax; Offset += sizeof (UINT32)) {
> - PciIo->Pci.Write (PciIo, EfiPciIoWidthUint32, Offset, 1, &gAllZero);
> - }
>
> //
> // set the Rom base address: now is hardcode
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> index f3e51d6150..d3cbefbadf 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> @@ -446,13 +446,14 @@ GetResourceFromDevice (
> switch ((PciDev->PciBar)[Index].BarType) {
>
> case PciBarTypeMem32:
> + case PciBarTypeOpRom:
>
> Node = CreateResourceNode (
> PciDev,
> (PciDev->PciBar)[Index].Length,
> (PciDev->PciBar)[Index].Alignment,
> Index,
> - PciBarTypeMem32,
> + (PciDev->PciBar)[Index].BarType,
> PciResUsageTypical
> );
>
> @@ -1307,7 +1308,13 @@ ProgramBar (
> 1,
> &Address
> );
> + //
> + // Continue to the case PciBarTypeOpRom to set the BaseAddress.
> + // PciBarTypeOpRom is a virtual BAR only in root bridge, to capture
> + // the MEM32 resource requirement for Option ROM shadow.
> + //
>
> + case PciBarTypeOpRom:
> Node->PciDev->PciBar[Node->Bar].BaseAddress = Address;
>
> break;
> @@ -1656,6 +1663,8 @@ ProgrameUpstreamBridgeForRom (
> {
> PCI_IO_DEVICE *Parent;
> PCI_RESOURCE_NODE Node;
> + UINT64 Base;
> + UINT64 Length;
> //
> // For root bridge, just return.
> //
> @@ -1667,7 +1676,6 @@ ProgrameUpstreamBridgeForRom (
> }
>
> Node.PciDev = Parent;
> - Node.Length = PciDevice->RomSize;
> Node.Alignment = 0;
> Node.Bar = PPB_MEM32_RANGE;
> Node.ResType = PciBarTypeMem32;
> @@ -1677,10 +1685,33 @@ ProgrameUpstreamBridgeForRom (
> // Program PPB to only open a single <= 16MB apperture
> //
> if (Enable) {
> + //
> + // Save the original PPB_MEM32_RANGE BAR.
> + // The values will be changed by ProgramPpbApperture().
> + //
> + Base = Parent->PciBar[Node.Bar].BaseAddress;
> + Length = Parent->PciBar[Node.Bar].Length;
> +
> + //
> + // Only cover MMIO for Option ROM.
> + //
> + Node.Length = PciDevice->RomSize;
> ProgramPpbApperture (OptionRomBase, &Node);
> +
> + //
> + // Restore the original PPB_MEM32_RANGE BAR.
> + // So the MEM32 RANGE BAR register can be restored when disable the
> decoding.
> + //
> + Parent->PciBar[Node.Bar].BaseAddress = Base;
> + Parent->PciBar[Node.Bar].Length = Length;
> +
> PCI_ENABLE_COMMAND_REGISTER (Parent,
> EFI_PCI_COMMAND_MEMORY_SPACE);
> } else {
> - InitializePpb (Parent);
> + //
> + // Cover 32bit MMIO for devices below the bridge.
> + //
> + Node.Length = Parent->PciBar[Node.Bar].Length;
> + ProgramPpbApperture (Parent->PciBar[Node.Bar].BaseAddress,
> &Node);
> PCI_DISABLE_COMMAND_REGISTER (Parent,
> EFI_PCI_COMMAND_MEMORY_SPACE);
> }
>
> --
> 2.16.1.windows.1
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
