public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v2] CorebootModulePkg/CbSupportDxe: Remove SCI_EN setting
@ 2018-06-08  6:01 Benjamin You
  2018-06-08 15:42 ` Matt Delco
  0 siblings, 1 reply; 2+ messages in thread
From: Benjamin You @ 2018-06-08  6:01 UTC (permalink / raw)
  To: edk2-devel; +Cc: Maurice Ma, Prince Agyeman, Matt Delco

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      | 51 ----------------------
 CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf    |  1 -
 CorebootModulePkg/Library/CbParseLib/CbParseLib.c  | 34 +++++++++++++++
 .../Library/CbParseLib/CbParseLib.inf              |  3 +-
 4 files changed, 36 insertions(+), 53 deletions(-)

diff --git a/CorebootModulePkg/CbSupportDxe/CbSupportDxe.c b/CorebootModulePkg/CbSupportDxe/CbSupportDxe.c
index c526c9e871..ec42f7ff35 100755
--- a/CorebootModulePkg/CbSupportDxe/CbSupportDxe.c
+++ b/CorebootModulePkg/CbSupportDxe/CbSupportDxe.c
@@ -14,7 +14,6 @@
 **/
 #include "CbSupportDxe.h"
 
-UINTN mPmCtrlReg = 0;
 /**
   Reserve MMIO/IO resource in GCD
 
@@ -86,31 +85,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,10 +104,8 @@ CbDxeEntryPoint (
   )
 {
   EFI_STATUS Status;
-  EFI_EVENT  ReadyToBootEvent;
   EFI_HOB_GUID_TYPE  *GuidHob;
   SYSTEM_TABLE_INFO  *pSystemTableInfo;
-  ACPI_BOARD_INFO    *pAcpiBoardInfo;
   FRAME_BUFFER_INFO  *FbInfo;
 
   Status = EFI_SUCCESS;
@@ -171,16 +143,6 @@ CbDxeEntryPoint (
     ASSERT_EFI_ERROR (Status);
   }
 
-  //
-  // Find the acpi board information guid hob
-  //
-  GuidHob = GetFirstGuidHob (&gUefiAcpiBoardInfoGuid);
-  ASSERT (GuidHob != NULL);
-  pAcpiBoardInfo = (ACPI_BOARD_INFO *)GET_GUID_HOB_DATA (GuidHob);
-
-  mPmCtrlReg = (UINTN)pAcpiBoardInfo->PmCtrlRegBase;
-  DEBUG ((EFI_D_ERROR, "PmCtrlReg at 0x%lx\n", (UINT64)mPmCtrlReg));
-
   //
   // Find the frame buffer information and update PCDs
   //
@@ -197,19 +159,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..da227dea5e 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>
@@ -477,6 +478,39 @@ CbParseFadtInfo (
         ASSERT(Fadt->Pm1aEvtBlk != 0);
         ASSERT(Fadt->Gpe0Blk != 0);
 
+        DEBUG_CODE_BEGIN ();
+          BOOLEAN    SciEnabled;
+
+          //
+          // Check the consistency of SCI enabling
+          //
+
+          //
+          // Get SCI_EN value
+          //
+          if (Fadt->Pm1CntLen == 4) {
+            SciEnabled = (IoRead32 (Fadt->Pm1aCntBlk) & BIT0)? TRUE : FALSE;
+          } else {
+            //
+            // if (Pm1CntLen == 2), use 16 bit IO read;
+            // if (Pm1CntLen != 2 && Pm1CntLen != 4), use 16 bit IO read as a fallback
+            //
+            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);
+          }
+        DEBUG_CODE_END ();
         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



^ permalink raw reply related	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-06-08 15:42 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-08  6:01 [PATCH v2] CorebootModulePkg/CbSupportDxe: Remove SCI_EN setting Benjamin You
2018-06-08 15:42 ` Matt Delco

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox