Re: [PATCH] MdeModulePkg/PciBus: Shadow option ROM after BARs are programmed

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----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.wu@intel.com>

Best Regards,
Hao Wu

> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Hao A Wu <hao.a.wu@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