Re: [PATCH v2] MdeModulePkg/PciBusDxe: make OPROM BAR degradation configurable

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 19 September 2016 at 09:36, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> The 'universal' PCI bus driver in MdeModulePkg contains a quirk to
> degrade 64-bit PCI MMIO BARs to 32-bit in the presence of an option
> ROM on the same PCI controller.
>
> This quirk is highly specific to not just the X64 architecture in general,
> but to the PC platform in particular, given that only X64 platforms that
> require legacy PC BIOS compatibility require it. However, making the
> quirk dependent on the presence of the legacy BIOS protocol met with
> resistance, due to the fact that it introduces a dependency on the
> IntelFrameworkModulePkg package.
>
> So instead, make the quirk configurable, by introducing a feature flag PCD
> which defaults to TRUE only for X64.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> v2: followed the suggestion of Andrew Fish to introduce a new feature flag
>     PCD that controls the PCI BAR degradation behavior.
>

Ping? Any comments?

>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf        |  1 +
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 76 ++++++++++----------
>  MdeModulePkg/MdeModulePkg.dec                       | 12 ++++
>  3 files changed, 52 insertions(+), 37 deletions(-)
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> index 330ccc8cbffc..fc49f3d9412c 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> @@ -100,6 +100,7 @@ [FeaturePcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPciBusHotplugDeviceSupport  ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPciBridgeIoAlignmentProbe   ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdUnalignedPciIoEnable        ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdPciDegradeBarsForOptionRom  ## CONSUMES
>
>  [Pcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSrIovSystemPageSize         ## SOMETIMES_CONSUMES
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> index b0632d53b82b..37dc03e90358 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> @@ -1058,48 +1058,50 @@ DegradeResource (
>    LIST_ENTRY           *NextChildNodeLink;
>    PCI_RESOURCE_NODE    *ResourceNode;
>
> -  //
> -  // If any child device has both option ROM and 64-bit BAR, degrade its PMEM64/MEM64
> -  // requests in case that if a legacy option ROM image can not access 64-bit resources.
> -  //
> -  ChildDeviceLink = Bridge->ChildList.ForwardLink;
> -  while (ChildDeviceLink != NULL && ChildDeviceLink != &Bridge->ChildList) {
> -    PciIoDevice = PCI_IO_DEVICE_FROM_LINK (ChildDeviceLink);
> -    if (PciIoDevice->RomSize != 0) {
> -      if (!IsListEmpty (&Mem64Node->ChildList)) {
> -        ChildNodeLink = Mem64Node->ChildList.ForwardLink;
> -        while (ChildNodeLink != &Mem64Node->ChildList) {
> -          ResourceNode = RESOURCE_NODE_FROM_LINK (ChildNodeLink);
> -          NextChildNodeLink = ChildNodeLink->ForwardLink;
> -
> -          if ((ResourceNode->PciDev == PciIoDevice) &&
> -              (ResourceNode->Virtual || !PciIoDevice->PciBar[ResourceNode->Bar].BarTypeFixed)
> -              ) {
> -            RemoveEntryList (ChildNodeLink);
> -            InsertResourceNode (Mem32Node, ResourceNode);
> +  if (FeaturePcdGet (PcdPciDegradeBarsForOptionRom)) {
> +    //
> +    // If any child device has both option ROM and 64-bit BAR, degrade its PMEM64/MEM64
> +    // requests in case that if a legacy option ROM image can not access 64-bit resources.
> +    //
> +    ChildDeviceLink = Bridge->ChildList.ForwardLink;
> +    while (ChildDeviceLink != NULL && ChildDeviceLink != &Bridge->ChildList) {
> +      PciIoDevice = PCI_IO_DEVICE_FROM_LINK (ChildDeviceLink);
> +      if (PciIoDevice->RomSize != 0) {
> +        if (!IsListEmpty (&Mem64Node->ChildList)) {
> +          ChildNodeLink = Mem64Node->ChildList.ForwardLink;
> +          while (ChildNodeLink != &Mem64Node->ChildList) {
> +            ResourceNode = RESOURCE_NODE_FROM_LINK (ChildNodeLink);
> +            NextChildNodeLink = ChildNodeLink->ForwardLink;
> +
> +            if ((ResourceNode->PciDev == PciIoDevice) &&
> +                (ResourceNode->Virtual || !PciIoDevice->PciBar[ResourceNode->Bar].BarTypeFixed)
> +                ) {
> +              RemoveEntryList (ChildNodeLink);
> +              InsertResourceNode (Mem32Node, ResourceNode);
> +            }
> +            ChildNodeLink = NextChildNodeLink;
>            }
> -          ChildNodeLink = NextChildNodeLink;
> -        }
> -      }
> +        }
>
> -      if (!IsListEmpty (&PMem64Node->ChildList)) {
> -        ChildNodeLink = PMem64Node->ChildList.ForwardLink;
> -        while (ChildNodeLink != &PMem64Node->ChildList) {
> -          ResourceNode = RESOURCE_NODE_FROM_LINK (ChildNodeLink);
> -          NextChildNodeLink = ChildNodeLink->ForwardLink;
> -
> -          if ((ResourceNode->PciDev == PciIoDevice) &&
> -              (ResourceNode->Virtual || !PciIoDevice->PciBar[ResourceNode->Bar].BarTypeFixed)
> -              ) {
> -            RemoveEntryList (ChildNodeLink);
> -            InsertResourceNode (PMem32Node, ResourceNode);
> +        if (!IsListEmpty (&PMem64Node->ChildList)) {
> +          ChildNodeLink = PMem64Node->ChildList.ForwardLink;
> +          while (ChildNodeLink != &PMem64Node->ChildList) {
> +            ResourceNode = RESOURCE_NODE_FROM_LINK (ChildNodeLink);
> +            NextChildNodeLink = ChildNodeLink->ForwardLink;
> +
> +            if ((ResourceNode->PciDev == PciIoDevice) &&
> +                (ResourceNode->Virtual || !PciIoDevice->PciBar[ResourceNode->Bar].BarTypeFixed)
> +                ) {
> +              RemoveEntryList (ChildNodeLink);
> +              InsertResourceNode (PMem32Node, ResourceNode);
> +            }
> +            ChildNodeLink = NextChildNodeLink;
>            }
> -          ChildNodeLink = NextChildNodeLink;
> -        }
> -      }
> +        }
>
> +      }
> +      ChildDeviceLink = ChildDeviceLink->ForwardLink;
>      }
> -    ChildDeviceLink = ChildDeviceLink->ForwardLink;
>    }
>
>    //
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index 8d90f169b26e..d8cb9c119598 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -749,6 +749,18 @@ [PcdsFeatureFlag]
>    # @Prompt Turn on PS2 Mouse Extended Verification
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPs2MouseExtendedVerification|TRUE|BOOLEAN|0x00010075
>
> +[PcdsFeatureFlag.X64]
> +  ## Indicates whether 64-bit PCI MMIO BARs should degrade to 32-bit in the presence of an option ROM
> +  #  On X64 platforms, Option ROMs may contain code that executes in the context of a legacy BIOS (CSM),
> +  #  which requires that all PCI MMIO BARs are located below 4 GB
> +  #   TRUE  - All PCI MMIO BARs of a device will be located below 4 GB if it has an option ROM
> +  #   FALSE - PCI MMIO BARs of a device may be located above 4 GB even if it has an option ROM
> +  # @Prompt Degrade 64-bit PCI MMIO BARs for legacy BIOS option ROMs
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdPciDegradeBarsForOptionRom|TRUE|BOOLEAN|0x0001003a
> +
> +[PcdsFeatureFlag.IA32, PcdsFeatureFlag.ARM, PcdsFeatureFlag.AARCH64]
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdPciDegradeBarsForOptionRom|FALSE|BOOLEAN|0x0001003a
> +
>  [PcdsFeatureFlag.IA32, PcdsFeatureFlag.X64]
>    ## Indicates if DxeIpl should switch to long mode to enter DXE phase.
>    #  It is assumed that 64-bit DxeCore is built in firmware if it is true; otherwise 32-bit DxeCore
> --
> 2.7.4
>