CorebootModulePkg/CbSupportDxe: Remove SCI_EN setting
authorBenjamin You <benjamin.you@intel.com>
Mon, 4 Jun 2018 03:23:21 +0000 (11:23 +0800)
committerBenjamin You <benjamin.you@intel.com>
Mon, 11 Jun 2018 08:16:30 +0000 (16:16 +0800)
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>
CorebootModulePkg/CbSupportDxe/CbSupportDxe.c
CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf
CorebootModulePkg/Library/CbParseLib/CbParseLib.c
CorebootModulePkg/Library/CbParseLib/CbParseLib.inf

index c526c9e..ec42f7f 100755 (executable)
@@ -14,7 +14,6 @@
 **/\r
 #include "CbSupportDxe.h"\r
 \r
-UINTN mPmCtrlReg = 0;\r
 /**\r
   Reserve MMIO/IO resource in GCD\r
 \r
@@ -86,31 +85,6 @@ CbReserveResourceInGcd (
   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
@@ -130,10 +104,8 @@ CbDxeEntryPoint (
   )\r
 {\r
   EFI_STATUS Status;\r
-  EFI_EVENT  ReadyToBootEvent;\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
@@ -171,16 +143,6 @@ CbDxeEntryPoint (
     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
@@ -197,19 +159,6 @@ CbDxeEntryPoint (
     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
index 9924518..15b0dac 100644 (file)
@@ -46,7 +46,6 @@
   DebugLib\r
   BaseMemoryLib\r
   UefiLib\r
-  IoLib\r
   HobLib\r
 \r
 [Guids]\r
index 0909b0f..da227de 100644 (file)
@@ -18,6 +18,7 @@
 #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
@@ -477,6 +478,39 @@ CbParseFadtInfo (
         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
index d7146a4..25b8479 100644 (file)
@@ -37,7 +37,8 @@
 [LibraryClasses]\r
   BaseLib\r
   BaseMemoryLib\r
-  DebugLib\r\r
+  IoLib\r
+  DebugLib\r
   PcdLib\r
 \r
 [Pcd]    \r