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

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

[Ard Biesheuvel]

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

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

[Yao, Jiewen]

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

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

[Ard Biesheuvel]

On 12 September 2016 at 14:15, Yao, Jiewen <jiewen.yao@intel.com> wrote:
> 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.
>

Yes, please. And please keep us informed about any solutions you come up with.

Thanks,
Ard.

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

[Ni, Ruiyu]

You could use IncompatiblePciDevice protocol. If you have no clue I will give you more information my tomorrow.

Thanks,
Ray

在 2016年9月12日，下午9:16，Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>> 写道：

On 12 September 2016 at 14:15, Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> wrote:
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.


Yes, please. And please keep us informed about any solutions you come up with.

Thanks,
Ard.

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

[Ard Biesheuvel]

On 12 September 2016 at 14:41, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> You could use IncompatiblePciDevice protocol. If you have no clue I will
> give you more information my tomorrow.
>

That works around the problem, but does not solve it. MdeModulePkg is
supposed to be generic/universal, and still, it contains a Intel/CSM
specific hack to degrade 64-bit BARs of any device that has an option
ROM. This is platform specific policy that does not belong in generic
code, and by the same reasoning that Jiewen argues that MdeModulePkg
should not depend on IntelFrameworkModulePkg, it should not implement
legacy BIOS hacks.

So what would you propose to factor out this functionality? Perhaps a
PciPolicyLib, whose Null implementation does not contain the hack. Or
a feature PCD that can be set to disable this behavior?

In any case, the requirement to implement the IncompatiblePciDevice
protocol to get normal resource allocation behavior is not the best
way to deal with this IMO

Thanks,
Ard.

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

[Laszlo Ersek]

On 09/12/16 15:16, Ard Biesheuvel wrote:
> On 12 September 2016 at 14:15, Yao, Jiewen <jiewen.yao@intel.com> wrote:
>> 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.
>>
> 
> Yes, please. And please keep us informed about any solutions you come up with.

* One idea is to parse the PCI expansion ROM in order to see what image
types are contained within. If there is no (Code type == 0x00) image in
the oprom, then the oprom is useless for legacy boot anyway, so it
shouldn't trigger degradation.

Unfortunately, this wouldn't help a lot in practice, since it's surely
going to be years before hw vendors migrate to pure UEFI oproms on their
graphics and network cards. :(

* Another idea is to check a dynamic PCD that the platform can set. New
PCDs are frowned upon in MdeModulePkg however, so I don't expect this to
be a popular fix.

Thanks
Laszlo

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

[Ni, Ruiyu]

Regards,
Ray

>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
>Sent: Monday, September 12, 2016 9:49 PM
>To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Yao, Jiewen <jiewen.yao@intel.com>
>Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Tian, Feng <feng.tian@intel.com>; edk2-devel@lists.01.org <edk2-devel@ml01.01.org>;
>leif.lindholm@linaro.org; Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
>Subject: Re: [edk2] [PATCH] MdeModulePkg/PciBusDxe: make BAR degradation dependent on OPROM presence
>
>On 09/12/16 15:16, Ard Biesheuvel wrote:
>> On 12 September 2016 at 14:15, Yao, Jiewen <jiewen.yao@intel.com> wrote:
>>> 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.
>>>
>>
>> Yes, please. And please keep us informed about any solutions you come up with.
>
>* One idea is to parse the PCI expansion ROM in order to see what image
>types are contained within. If there is no (Code type == 0x00) image in
>the oprom, then the oprom is useless for legacy boot anyway, so it
>shouldn't trigger degradation.
>
>Unfortunately, this wouldn't help a lot in practice, since it's surely
>going to be years before hw vendors migrate to pure UEFI oproms on their
>graphics and network cards. :(
Yes the first idea doesn't work because it cannot solve all the problems: some
cards may still contain legacy option ROM. The resource degrade happens
before PciBus knows which option rom to run by platform. And we even met case
the card only contains legacy option rom but platform just don't want to dispatch
the legacy rom.

>* Another idea is to check a dynamic PCD that the platform can set. New
>PCDs are frowned upon in MdeModulePkg however, so I don't expect this to
>be a popular fix.

I agree with you.
The requirement of dynamic PCD actually indicates we have a missing interface
between platform and PciBus core driver.
I personally don't like dynamic PCD very much. It's equivalent to protocol. Then
why should we use PCD instead of protocol? To avoid changing spec??

The usage of PciIncompatibleDevice protocol is the solution we came up.
It's also the currently recommended way to solve this type of issue.
ECR 1529 introduced in PI 1.4A was initiated by this issue.



>
>Thanks
>Laszlo
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel

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

[Ard Biesheuvel]

On 13 September 2016 at 06:46, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
>
>
> Regards,
> Ray
>
>>-----Original Message-----
>>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
>>Sent: Monday, September 12, 2016 9:49 PM
>>To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; Yao, Jiewen <jiewen.yao@intel.com>
>>Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Tian, Feng <feng.tian@intel.com>; edk2-devel@lists.01.org <edk2-devel@ml01.01.org>;
>>leif.lindholm@linaro.org; Gao, Liming <liming.gao@intel.com>; Zeng, Star <star.zeng@intel.com>
>>Subject: Re: [edk2] [PATCH] MdeModulePkg/PciBusDxe: make BAR degradation dependent on OPROM presence
>>
>>On 09/12/16 15:16, Ard Biesheuvel wrote:
>>> On 12 September 2016 at 14:15, Yao, Jiewen <jiewen.yao@intel.com> wrote:
>>>> 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.
>>>>
>>>
>>> Yes, please. And please keep us informed about any solutions you come up with.
>>
>>* One idea is to parse the PCI expansion ROM in order to see what image
>>types are contained within. If there is no (Code type == 0x00) image in
>>the oprom, then the oprom is useless for legacy boot anyway, so it
>>shouldn't trigger degradation.
>>
>>Unfortunately, this wouldn't help a lot in practice, since it's surely
>>going to be years before hw vendors migrate to pure UEFI oproms on their
>>graphics and network cards. :(
> Yes the first idea doesn't work because it cannot solve all the problems: some
> cards may still contain legacy option ROM. The resource degrade happens
> before PciBus knows which option rom to run by platform. And we even met case
> the card only contains legacy option rom but platform just don't want to dispatch
> the legacy rom.
>
>>* Another idea is to check a dynamic PCD that the platform can set. New
>>PCDs are frowned upon in MdeModulePkg however, so I don't expect this to
>>be a popular fix.
>
> I agree with you.
> The requirement of dynamic PCD actually indicates we have a missing interface
> between platform and PciBus core driver.
> I personally don't like dynamic PCD very much. It's equivalent to protocol. Then
> why should we use PCD instead of protocol? To avoid changing spec??
>
> The usage of PciIncompatibleDevice protocol is the solution we came up.
> It's also the currently recommended way to solve this type of issue.
> ECR 1529 introduced in PI 1.4A was initiated by this issue.
>

We are all aware that the PciIncompatibleDevice protocol can be used
to work around this. But this means that, while this issue only exists
on X64, all architectures need to install additional protocols and
rely on runtime processing to disable a feature they did not want in
the first place. Is the practice of degrading 64-bit MMIO BARs for
option ROMs even in the specs?

So how about a PCD feature flag? Or even simply a '#ifdef MDE_CPU_X64'
around the block that degrades the 64-bit MMIO BARs if an option ROM
is detected? Ideally, we should implement PciIncompatibleDevice
protocol the other way around, and add a default implementation to
IntelFrameworkModulePkg to performs the BAR degradation on platforms
with a legacy BIOS and option ROMs

-- 
Ard.

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

[Leif Lindholm]

Adding Andrew, Mike,

On Tue, Sep 13, 2016 at 08:43:39AM +0100, Ard Biesheuvel wrote:
> On 13 September 2016 at 06:46, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> >>* Another idea is to check a dynamic PCD that the platform can set. New
> >>PCDs are frowned upon in MdeModulePkg however, so I don't expect this to
> >>be a popular fix.
> >
> > I agree with you.
> > The requirement of dynamic PCD actually indicates we have a
> > missing interface between platform and PciBus core driver.
> > I personally don't like dynamic PCD very much. It's equivalent to
> > protocol. Then why should we use PCD instead of protocol? To avoid
> > changing spec??
> >
> > The usage of PciIncompatibleDevice protocol is the solution we came up.
> > It's also the currently recommended way to solve this type of issue.
> > ECR 1529 introduced in PI 1.4A was initiated by this issue.
>
> We are all aware that the PciIncompatibleDevice protocol can be used
> to work around this. But this means that, while this issue only exists
> on X64, all architectures need to install additional protocols and
> rely on runtime processing to disable a feature they did not want in
> the first place. Is the practice of degrading 64-bit MMIO BARs for
> option ROMs even in the specs?
> 
> So how about a PCD feature flag? Or even simply a '#ifdef MDE_CPU_X64'
> around the block that degrades the 64-bit MMIO BARs if an option ROM
> is detected? Ideally, we should implement PciIncompatibleDevice
> protocol the other way around, and add a default implementation to
> IntelFrameworkModulePkg to performs the BAR degradation on platforms
> with a legacy BIOS and option ROMs

Yes, the situation of requiring an implementation of
PciIncompatibleDevice to support an actually compatible device, in
order to not require it for actually incompatible platforms seems at
the very least somewhat semantically dubious to me.

By order of my preference, I would say we could:
- Invert the use of PciIncompatibleDevice with a default
  implementation for IntelFrameworkModulePkg
- #ifdef MDE_CPU_X64
- Add a dynamic Pcd
- Retain the semantically incorrect use of PciIncompatibleDevice, but
  provide a single implementation reusable across OvmfPkg and all !X64
  platforms.

Regards,

Leif