[PATCH] CorebootModulePkg/CbSupportDxe: Remove SCI_EN setting

[Benjamin You <benjamin.you@intel.com>]

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>
---
 CorebootModulePkg/CbSupportDxe/CbSupportDxe.c      | 39 ----------------------
 CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf    |  1 -
 CorebootModulePkg/Library/CbParseLib/CbParseLib.c  | 28 ++++++++++++++++
 .../Library/CbParseLib/CbParseLib.inf              |  3 +-
 4 files changed, 30 insertions(+), 41 deletions(-)

diff --git a/CorebootModulePkg/CbSupportDxe/CbSupportDxe.c b/CorebootModulePkg/CbSupportDxe/CbSupportDxe.c
index c526c9e871..b41c0885f7 100755
--- a/CorebootModulePkg/CbSupportDxe/CbSupportDxe.c
+++ b/CorebootModulePkg/CbSupportDxe/CbSupportDxe.c
@@ -86,31 +86,6 @@ CbReserveResourceInGcd (
   return Status;
 }
 
-/**
-  Notification function of EVT_GROUP_READY_TO_BOOT event group.
-
-  This is a notification function registered on EVT_GROUP_READY_TO_BOOT event group.
-  When the Boot Manager is about to load and execute a boot option, it reclaims variable
-  storage if free size is below the threshold.
-
-  @param  Event        Event whose notification function is being invoked.
-  @param  Context      Pointer to the notification function's context.
-
-**/
-VOID
-EFIAPI
-OnReadyToBoot (
-  IN  EFI_EVENT  Event,
-  IN  VOID       *Context
-  )
-{
-  //
-  // Enable SCI
-  //
-  IoOr16 (mPmCtrlReg, BIT0);
-
-  DEBUG ((EFI_D_ERROR, "Enable SCI bit at 0x%lx before boot\n", (UINT64)mPmCtrlReg));
-}
 
 /**
   Main entry for the Coreboot Support DXE module.
@@ -130,7 +105,6 @@ CbDxeEntryPoint (
   )
 {
   EFI_STATUS Status;
-  EFI_EVENT  ReadyToBootEvent;
   EFI_HOB_GUID_TYPE  *GuidHob;
   SYSTEM_TABLE_INFO  *pSystemTableInfo;
   ACPI_BOARD_INFO    *pAcpiBoardInfo;
@@ -197,19 +171,6 @@ CbDxeEntryPoint (
     ASSERT_EFI_ERROR (Status);
   }
 
-  //
-  // Register callback on the ready to boot event
-  // in order to enable SCI
-  //
-  ReadyToBootEvent = NULL;
-  Status = EfiCreateEventReadyToBootEx (
-                    TPL_CALLBACK,
-                    OnReadyToBoot,
-                    NULL,
-                    &ReadyToBootEvent
-                    );
-  ASSERT_EFI_ERROR (Status);
-
   return EFI_SUCCESS;
 }
 
diff --git a/CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf b/CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf
index 99245183ea..15b0dac774 100644
--- a/CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf
+++ b/CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf
@@ -46,7 +46,6 @@
   DebugLib
   BaseMemoryLib
   UefiLib
-  IoLib
   HobLib
 
 [Guids]
diff --git a/CorebootModulePkg/Library/CbParseLib/CbParseLib.c b/CorebootModulePkg/Library/CbParseLib/CbParseLib.c
index 0909b0f492..cd98a72f01 100644
--- a/CorebootModulePkg/Library/CbParseLib/CbParseLib.c
+++ b/CorebootModulePkg/Library/CbParseLib/CbParseLib.c
@@ -18,6 +18,7 @@
 #include <Library/BaseMemoryLib.h>
 #include <Library/DebugLib.h>
 #include <Library/PcdLib.h>
+#include <Library/IoLib.h>
 #include <Library/CbParseLib.h>
 
 #include <IndustryStandard/Acpi.h>
@@ -412,6 +413,7 @@ CbParseFadtInfo (
   UINTN                                         Entry64Num;
   UINTN                                         Idx;
   RETURN_STATUS                                 Status;
+  BOOLEAN                                       SciEnabled;
 
   Rsdp = NULL;
   Status = RETURN_SUCCESS;
@@ -477,6 +479,32 @@ CbParseFadtInfo (
         ASSERT(Fadt->Pm1aEvtBlk != 0);
         ASSERT(Fadt->Gpe0Blk != 0);
 
+        //
+        // Get SCI_EN value
+        //
+        if (Fadt->Pm1CntLen == 2) {
+          SciEnabled = (IoRead16 (Fadt->Pm1aCntBlk) & BIT0)? TRUE : FALSE;
+        } else if (Fadt->Pm1CntLen == 4) {
+          SciEnabled = (IoRead32 (Fadt->Pm1aCntBlk) & BIT0)? TRUE : FALSE;
+        } else {
+          //
+          // Unsupported PM1 CNT Length, revert to 16 bit
+          //
+          SciEnabled = (IoRead16 (Fadt->Pm1aCntBlk) & BIT0)? TRUE : FALSE;
+        }
+
+        if (!(Fadt->Flags & EFI_ACPI_5_0_HW_REDUCED_ACPI) &&
+            (Fadt->SmiCmd == 0) &&
+            !SciEnabled) {
+          //
+          // The ACPI enabling status is inconsistent: SCI is not enabled but ACPI
+          // table does not provide a means to enable it through FADT->SmiCmd
+          //
+          DEBUG ((DEBUG_ERROR, "ERROR: The ACPI enabling status is inconsistent: SCI is not"
+            " enabled but the ACPI table does not provide a means to enable it through FADT->SmiCmd."
+            " This may cause issues in OS.\n"));
+          ASSERT (FALSE);
+        }
         return RETURN_SUCCESS;
       }
     }
diff --git a/CorebootModulePkg/Library/CbParseLib/CbParseLib.inf b/CorebootModulePkg/Library/CbParseLib/CbParseLib.inf
index d7146a415b..25b847946c 100644
--- a/CorebootModulePkg/Library/CbParseLib/CbParseLib.inf
+++ b/CorebootModulePkg/Library/CbParseLib/CbParseLib.inf
@@ -37,7 +37,8 @@
 [LibraryClasses]
   BaseLib
   BaseMemoryLib
-  DebugLib

+  IoLib
+  DebugLib
   PcdLib
 
 [Pcd]    
-- 
2.14.3.windows.1