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>
This commit is contained in:
Benjamin You 2018-06-04 11:23:21 +08:00
parent f75c747828
commit 271d8cd7df
4 changed files with 36 additions and 53 deletions

View File

@ -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;
}

View File

@ -46,7 +46,6 @@
DebugLib
BaseMemoryLib
UefiLib
IoLib
HobLib
[Guids]

View File

@ -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;
}
}

View File

@ -37,6 +37,7 @@
[LibraryClasses]
BaseLib
BaseMemoryLib
IoLib
DebugLib
PcdLib