CorebootModulePkg/CbSupportDxe: Remove SCI_EN setting

Current implemenation sets PM1_CNT.SCI_EN bit at ReadyToBoot event.
However, this should not be done because this causes OS to skip triggering
FADT.SMI_CMD, which leads to the functions implemented in the SMI
handler being omitted.

This issue was identified by Matt Delco <delco@google.com>.

The fix does the following:
- The SCI_EN bit setting is removed from CbSupportDxe driver.
- Some additional checks are added in CbParseFadtInfo() in CbParseLib.c to
  output some error message and ASSERT (FALSE) if ALL of the following
  conditions are met:
  1) HARDWARE_REDUCED_ACPI is not set;
  2) SMI_CMD field is zero;
  3) SCI_EN bit is zero;
  which indicates the ACPI enabling status is inconsistent: SCI is not
  enabled but the ACPI table does not provide a means to enable it through
  FADT->SMI_CMD. This may cause issues in OS.

Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Prince Agyeman <prince.agyeman@intel.com>
Cc: Matt Delco <delco@google.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Benjamin You <benjamin.you@intel.com>
Reviewed-by: Maurice Ma <maurice.ma@intel.com>
Reviewed-by: Matt Delco <delco@google.com>

diff --git a/CorebootModulePkg/CbSupportDxe/CbSupportDxe.c b/CorebootModulePkg/CbSupportDxe/CbSupportDxe.c
index c526c9e871db0d4ec38809b075454d6b22a0df42..ec42f7ff3547a969a2a174a4b70fc1742f2b5360 100755 (executable)
--- a/CorebootModulePkg/CbSupportDxe/CbSupportDxe.c
+++ b/CorebootModulePkg/CbSupportDxe/CbSupportDxe.c
@@ -14,7 +14,6 @@
 **/\r
 #include "CbSupportDxe.h"\r
 \r
 **/\r
 #include "CbSupportDxe.h"\r
 \r

-UINTN mPmCtrlReg = 0;\r

 /**\r
   Reserve MMIO/IO resource in GCD\r
 \r
 /**\r
   Reserve MMIO/IO resource in GCD\r
 \r


@@ -86,31 +85,6 @@ CbReserveResourceInGcd (
   return Status;\r
 }\r
 \r
   return Status;\r
 }\r
 \r

-/**\r
-  Notification function of EVT_GROUP_READY_TO_BOOT event group.\r
-\r
-  This is a notification function registered on EVT_GROUP_READY_TO_BOOT event group.\r
-  When the Boot Manager is about to load and execute a boot option, it reclaims variable\r
-  storage if free size is below the threshold.\r
-\r
-  @param  Event        Event whose notification function is being invoked.\r
-  @param  Context      Pointer to the notification function's context.\r
-\r
-**/\r
-VOID\r
-EFIAPI\r
-OnReadyToBoot (\r
-  IN  EFI_EVENT  Event,\r
-  IN  VOID       *Context\r
-  )\r
-{\r
-  //\r
-  // Enable SCI\r
-  //\r
-  IoOr16 (mPmCtrlReg, BIT0);\r
-\r
-  DEBUG ((EFI_D_ERROR, "Enable SCI bit at 0x%lx before boot\n", (UINT64)mPmCtrlReg));\r
-}\r

 \r
 /**\r
   Main entry for the Coreboot Support DXE module.\r
 \r
 /**\r
   Main entry for the Coreboot Support DXE module.\r


@@ -130,10 +104,8 @@ CbDxeEntryPoint (
   )\r
 {\r
   EFI_STATUS Status;\r
   )\r
 {\r
   EFI_STATUS Status;\r

-  EFI_EVENT  ReadyToBootEvent;\r

   EFI_HOB_GUID_TYPE  *GuidHob;\r
   SYSTEM_TABLE_INFO  *pSystemTableInfo;\r
   EFI_HOB_GUID_TYPE  *GuidHob;\r
   SYSTEM_TABLE_INFO  *pSystemTableInfo;\r

-  ACPI_BOARD_INFO    *pAcpiBoardInfo;\r

   FRAME_BUFFER_INFO  *FbInfo;\r
 \r
   Status = EFI_SUCCESS;\r
   FRAME_BUFFER_INFO  *FbInfo;\r
 \r
   Status = EFI_SUCCESS;\r


@@ -171,16 +143,6 @@ CbDxeEntryPoint (
     ASSERT_EFI_ERROR (Status);\r
   }\r
 \r
     ASSERT_EFI_ERROR (Status);\r
   }\r
 \r

-  //\r
-  // Find the acpi board information guid hob\r
-  //\r
-  GuidHob = GetFirstGuidHob (&gUefiAcpiBoardInfoGuid);\r
-  ASSERT (GuidHob != NULL);\r
-  pAcpiBoardInfo = (ACPI_BOARD_INFO *)GET_GUID_HOB_DATA (GuidHob);\r
-\r
-  mPmCtrlReg = (UINTN)pAcpiBoardInfo->PmCtrlRegBase;\r
-  DEBUG ((EFI_D_ERROR, "PmCtrlReg at 0x%lx\n", (UINT64)mPmCtrlReg));\r
-\r

   //\r
   // Find the frame buffer information and update PCDs\r
   //\r
   //\r
   // Find the frame buffer information and update PCDs\r
   //\r


@@ -197,19 +159,6 @@ CbDxeEntryPoint (
     ASSERT_EFI_ERROR (Status);\r
   }\r
 \r
     ASSERT_EFI_ERROR (Status);\r
   }\r
 \r

-  //\r
-  // Register callback on the ready to boot event\r
-  // in order to enable SCI\r
-  //\r
-  ReadyToBootEvent = NULL;\r
-  Status = EfiCreateEventReadyToBootEx (\r
-                    TPL_CALLBACK,\r
-                    OnReadyToBoot,\r
-                    NULL,\r
-                    &ReadyToBootEvent\r
-                    );\r
-  ASSERT_EFI_ERROR (Status);\r
-\r

   return EFI_SUCCESS;\r
 }\r
 \r
   return EFI_SUCCESS;\r
 }\r
 \r

diff --git a/CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf b/CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf
index 99245183eaeb608084676008538656159b67672d..15b0dac77440aaf7591b68de57ee6862f2f348eb 100644 (file)
--- a/CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf
+++ b/CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf
@@ -46,7 +46,6 @@
   DebugLib\r
   BaseMemoryLib\r
   UefiLib\r
   DebugLib\r
   BaseMemoryLib\r
   UefiLib\r

-  IoLib\r

   HobLib\r
 \r
 [Guids]\r
   HobLib\r
 \r
 [Guids]\r

diff --git a/CorebootModulePkg/Library/CbParseLib/CbParseLib.c b/CorebootModulePkg/Library/CbParseLib/CbParseLib.c
index 0909b0f492023b3a029065906006db6bafdcaf6e..da227dea5e275104efa92ce3fddb62344d32881e 100644 (file)
--- a/CorebootModulePkg/Library/CbParseLib/CbParseLib.c
+++ b/CorebootModulePkg/Library/CbParseLib/CbParseLib.c
@@ -18,6 +18,7 @@
 #include <Library/BaseMemoryLib.h>\r
 #include <Library/DebugLib.h>\r
 #include <Library/PcdLib.h>\r
 #include <Library/BaseMemoryLib.h>\r
 #include <Library/DebugLib.h>\r
 #include <Library/PcdLib.h>\r

+#include <Library/IoLib.h>\r

 #include <Library/CbParseLib.h>\r
 \r
 #include <IndustryStandard/Acpi.h>\r
 #include <Library/CbParseLib.h>\r
 \r
 #include <IndustryStandard/Acpi.h>\r


@@ -477,6 +478,39 @@ CbParseFadtInfo (
         ASSERT(Fadt->Pm1aEvtBlk != 0);\r
         ASSERT(Fadt->Gpe0Blk != 0);\r
 \r
         ASSERT(Fadt->Pm1aEvtBlk != 0);\r
         ASSERT(Fadt->Gpe0Blk != 0);\r
 \r

+        DEBUG_CODE_BEGIN ();\r
+          BOOLEAN    SciEnabled;\r
+\r
+          //\r
+          // Check the consistency of SCI enabling\r
+          //\r
+\r
+          //\r
+          // Get SCI_EN value\r
+          //\r
+          if (Fadt->Pm1CntLen == 4) {\r
+            SciEnabled = (IoRead32 (Fadt->Pm1aCntBlk) & BIT0)? TRUE : FALSE;\r
+          } else {\r
+            //\r
+            // if (Pm1CntLen == 2), use 16 bit IO read;\r
+            // if (Pm1CntLen != 2 && Pm1CntLen != 4), use 16 bit IO read as a fallback\r
+            //\r
+            SciEnabled = (IoRead16 (Fadt->Pm1aCntBlk) & BIT0)? TRUE : FALSE;\r
+          }\r
+\r
+          if (!(Fadt->Flags & EFI_ACPI_5_0_HW_REDUCED_ACPI) &&\r
+              (Fadt->SmiCmd == 0) &&\r
+              !SciEnabled) {\r
+            //\r
+            // The ACPI enabling status is inconsistent: SCI is not enabled but ACPI\r
+            // table does not provide a means to enable it through FADT->SmiCmd\r
+            //\r
+            DEBUG ((DEBUG_ERROR, "ERROR: The ACPI enabling status is inconsistent: SCI is not"\r
+              " enabled but the ACPI table does not provide a means to enable it through FADT->SmiCmd."\r
+              " This may cause issues in OS.\n"));\r
+            ASSERT (FALSE);\r
+          }\r
+        DEBUG_CODE_END ();\r

         return RETURN_SUCCESS;\r
       }\r
     }\r
         return RETURN_SUCCESS;\r
       }\r
     }\r

diff --git a/CorebootModulePkg/Library/CbParseLib/CbParseLib.inf b/CorebootModulePkg/Library/CbParseLib/CbParseLib.inf
index d7146a415b794da9c6dd6464387de9464f8e2492..25b847946c2b379e5e7be31747849720f58b469d 100644 (file)
--- a/CorebootModulePkg/Library/CbParseLib/CbParseLib.inf
+++ b/CorebootModulePkg/Library/CbParseLib/CbParseLib.inf
@@ -37,7 +37,8 @@
 [LibraryClasses]\r
   BaseLib\r
   BaseMemoryLib\r
 [LibraryClasses]\r
   BaseLib\r
   BaseMemoryLib\r

-  DebugLib\r\r
+  IoLib\r
+  DebugLib\r

   PcdLib\r
 \r
 [Pcd]    \r
   PcdLib\r
 \r
 [Pcd]    \r