Re: [PATCH] MdeModulePkg/PciBusDxe: make BAR degradation dependent on OPROM presence

["Yao, Jiewen" <jiewen.yao@intel.com>]

HI Ard
We should not let MdeModulePkg depend on IntelFrameworkPkg.
You patch violates the dependency rule.
I suggest we figure out other solution to resolve the problem.

Thank you
Yao Jiewen

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Ard Biesheuvel
> Sent: Monday, September 12, 2016 9:06 PM
> To: edk2-devel@lists.01.org; Gao, Liming <liming.gao@intel.com>; Zeng,
> Star <star.zeng@intel.com>; Tian, Feng <feng.tian@intel.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>
> Cc: lersek@redhat.com; leif.lindholm@linaro.org; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: [edk2] [PATCH] MdeModulePkg/PciBusDxe: make BAR degradation
> dependent on OPROM presence
> 
> The practice of unconditionally degrading 64-bit PCI MMIO BARs to 32-bit
> if the device in question happens to have an option ROM is based on
> platform constraints, not architectural constraints, and really only makes
> sense on Intel platforms that contain a CSM implementation.
> 
> So let's copy the OVMF code that checks for the presence of the legacy
> BIOS protocol (&gEfiLegacyBiosProtocolGuid), and only perform the BAR
> degradation if this protocol is installed.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c             | 42
> ++++++++++
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h             |  1 +
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf        |  2 +
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c | 83
> ++++++++++----------
>  4 files changed, 88 insertions(+), 40 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c
> index a463bea80f3d..857f3e11b6bd 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.c
> @@ -49,6 +49,39 @@ GLOBAL_REMOVE_IF_UNREFERENCED
> EFI_PCI_HOTPLUG_REQUEST_PROTOCOL mPciHotPlugReques
>  };
> 
>  /**
> +  Legacy BIOS installed callback
> +
> +  @param[in] Event      Event whose notification function is being
> invoked.
> +  @param[in] Context    Pointer to the notification function's context.
> +
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +LegacyBiosInstalledCallBack (
> +  IN EFI_EVENT          Event,
> +  IN VOID               *Context
> +  )
> +{
> +  EFI_STATUS               Status;
> +  EFI_LEGACY_BIOS_PROTOCOL *LegacyBios;
> +
> +  Status = gBS->LocateProtocol (&gEfiLegacyBiosProtocolGuid,
> +                  NULL /* Registration */, (VOID **)&LegacyBios);
> +  if (EFI_ERROR (Status)) {
> +    return;
> +  }
> +
> +  mLegacyBiosInstalled = TRUE;
> +
> +  //
> +  // Close the event and deregister this callback.
> +  //
> +  Status = gBS->CloseEvent (Event);
> +  ASSERT_EFI_ERROR (Status);
> +}
> +
> +/**
>    The Entry Point for PCI Bus module. The user code starts with this
> function.
> 
>    Installs driver module protocols and. Creates virtual device handles for
> ConIn,
> @@ -72,6 +105,7 @@ PciBusEntryPoint (
>  {
>    EFI_STATUS  Status;
>    EFI_HANDLE  Handle;
> +  VOID        *Registration;
> 
>    //
>    // Initializes PCI devices pool
> @@ -91,6 +125,14 @@ PciBusEntryPoint (
>               );
>    ASSERT_EFI_ERROR (Status);
> 
> +  EfiCreateProtocolNotifyEvent (
> +    &gEfiLegacyBiosProtocolGuid,
> +    TPL_CALLBACK,
> +    LegacyBiosInstalledCallBack,
> +    NULL,
> +    &Registration
> +    );
> +
>    if (FeaturePcdGet (PcdPciBusHotplugDeviceSupport)) {
>      //
>      // If Hot Plug is supported, install EFI PCI Hot Plug Request protocol.
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> index b12d7ec5032f..2bf5695476a1 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBus.h
> @@ -321,6 +321,7 @@ extern EFI_PCI_PLATFORM_PROTOCOL
> *gPciPlatformProtocol;
>  extern EFI_PCI_OVERRIDE_PROTOCOL
> *gPciOverrideProtocol;
>  extern BOOLEAN
> mReserveIsaAliases;
>  extern BOOLEAN
> mReserveVgaAliases;
> +extern BOOLEAN
> mLegacyBiosInstalled;
> 
>  /**
>    Macro that checks whether device is a GFX device.
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> index 330ccc8cbffc..b843ccc49934 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciBusDxe.inf
> @@ -66,6 +66,7 @@ [Sources]
>  [Packages]
>    MdePkg/MdePkg.dec
>    MdeModulePkg/MdeModulePkg.dec
> +  IntelFrameworkPkg/IntelFrameworkPkg.dec
> 
>  [LibraryClasses]
>    PcdLib
> @@ -95,6 +96,7 @@ [Protocols]
>    gEfiPciRootBridgeIoProtocolGuid                 ## TO_START
>    gEfiIncompatiblePciDeviceSupportProtocolGuid    ##
> SOMETIMES_CONSUMES
>    gEfiLoadFile2ProtocolGuid                       ##
> SOMETIMES_PRODUCES
> +  gEfiLegacyBiosProtocolGuid                      ##
> SOMETIMES_CONSUMES
> 
>  [FeaturePcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPciBusHotplugDeviceSupport
> ## CONSUMES
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> index b0632d53b82b..6637625b210d 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c
> @@ -17,9 +17,10 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND, EITHER EXPRESS OR IMPLIED.
>  //
>  // The default policy for the PCI bus driver is NOT to reserve I/O ranges for
> both ISA aliases and VGA aliases.
>  //
> -BOOLEAN mReserveIsaAliases = FALSE;
> -BOOLEAN mReserveVgaAliases = FALSE;
> -BOOLEAN mPolicyDetermined  = FALSE;
> +BOOLEAN mReserveIsaAliases    = FALSE;
> +BOOLEAN mReserveVgaAliases    = FALSE;
> +BOOLEAN mPolicyDetermined     = FALSE;
> +BOOLEAN mLegacyBiosInstalled  = FALSE;
> 
>  /**
>    The function is used to skip VGA range.
> @@ -1058,48 +1059,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 (mLegacyBiosInstalled) {
> +    //
> +    // 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;
>    }
> 
>    //
> --
> 2.7.4
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel