* [PATCH] CorebootModulePkg/CbSupportDxe: Remove SCI_EN setting
@ 2018-06-07 8:19 Benjamin You
2018-06-07 15:40 ` Ma, Maurice
0 siblings, 1 reply; 4+ messages in thread
From: Benjamin You @ 2018-06-07 8:19 UTC (permalink / raw)
To: edk2-devel; +Cc: maurice.ma, prince.agyeman, 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 | 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] CorebootModulePkg/CbSupportDxe: Remove SCI_EN setting
2018-06-07 8:19 [PATCH] CorebootModulePkg/CbSupportDxe: Remove SCI_EN setting Benjamin You
@ 2018-06-07 15:40 ` Ma, Maurice
2018-06-07 17:07 ` Matt Delco
0 siblings, 1 reply; 4+ messages in thread
From: Ma, Maurice @ 2018-06-07 15:40 UTC (permalink / raw)
To: You, Benjamin; +Cc: Agyeman, Prince, delco@google.com, edk2-devel@lists.01.org
It looks good to me.
Reviewed-by: Maurice Ma <maurice.ma@intel.com>
-----Original Message-----
From: You, Benjamin
Sent: Thursday, June 7, 2018 1:19
To: edk2-devel@lists.01.org
Cc: Ma, Maurice <maurice.ma@intel.com>; Agyeman, Prince <prince.agyeman@intel.com>; delco@google.com
Subject: [PATCH] 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>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] CorebootModulePkg/CbSupportDxe: Remove SCI_EN setting
2018-06-07 15:40 ` Ma, Maurice
@ 2018-06-07 17:07 ` Matt Delco
2018-06-08 6:06 ` You, Benjamin
0 siblings, 1 reply; 4+ messages in thread
From: Matt Delco @ 2018-06-07 17:07 UTC (permalink / raw)
To: Ma, Maurice; +Cc: You, Benjamin, Agyeman, Prince, edk2-devel@lists.01.org
Thanks for working on this. I did a quick test to confirm that things
still work okay. The code change looks fine though there's some optional
optimizations to consider:
The new code added to CbParseFadtInfo() is mostly to add debug checks but I
think it'll still result in a IO port access in a release build so it might
be worthwhile to make the entire code block specific to a debug build
(possibly by sticking the code within a "#if !defined(MDEPKG_NDEBUG)"
block, though I don't see any other such cases in edk2 so I suspect this
isn't an ideal suggestion). Regarding "Pm1CntLen" It looks like the "if
(==2) {} else if (==4) {} else {}" could be simplified to "if (==4) {} else
{}".
In CbDxeEntryPoint() the "Find the acpi board information guid hob" code
block can probably be deleted. Its only remaining use is the debug log
regarding the value of PmCtrlReg, which I suspect isn't that valuable given
the other deletions. This would also allow the removal of the "mPmCtrlReg"
variable at file scope.
On Thu, Jun 7, 2018 at 8:40 AM, Ma, Maurice <maurice.ma@intel.com> wrote:
> It looks good to me.
> Reviewed-by: Maurice Ma <maurice.ma@intel.com>
>
>
> -----Original Message-----
> From: You, Benjamin
> Sent: Thursday, June 7, 2018 1:19
> To: edk2-devel@lists.01.org
> Cc: Ma, Maurice <maurice.ma@intel.com>; Agyeman, Prince <
> prince.agyeman@intel.com>; delco@google.com
> Subject: [PATCH] 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>
> ---
> 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
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] CorebootModulePkg/CbSupportDxe: Remove SCI_EN setting
2018-06-07 17:07 ` Matt Delco
@ 2018-06-08 6:06 ` You, Benjamin
0 siblings, 0 replies; 4+ messages in thread
From: You, Benjamin @ 2018-06-08 6:06 UTC (permalink / raw)
To: Matt Delco, Ma, Maurice; +Cc: Agyeman, Prince, edk2-devel@lists.01.org
Hi Matt,
Thanks. I've submitted patch v2 based on your feedbacks.
- ben
> From: Matt Delco [mailto:delco@google.com]
> Sent: Friday, June 8, 2018 1:07 AM
> To: Ma, Maurice <maurice.ma@intel.com>
> Cc: You, Benjamin <benjamin.you@intel.com>; Agyeman, Prince <prince.agyeman@intel.com>; edk2-devel@lists.01.org
> Subject: Re: [PATCH] CorebootModulePkg/CbSupportDxe: Remove SCI_EN setting
>
> Thanks for working on this. I did a quick test to confirm that things still work okay. The code change looks fine though there's some optional optimizations to consider:
>
> The new code added to CbParseFadtInfo() is mostly to add debug checks but I think it'll still result in a IO port access in a release build so it might be worthwhile to make the entire code block specific to a debug build (possibly by sticking the code within a "#if !defined(MDEPKG_NDEBUG)" block, though I don't see any other such cases in edk2 so I suspect this isn't an ideal suggestion). Regarding "Pm1CntLen" It looks like the "if (==2) {} else if (==4) {} else {}" could be simplified to "if (==4) {} else {}".
>
> In CbDxeEntryPoint() the "Find the acpi board information guid hob" code block can probably be deleted. Its only remaining use is the debug log regarding the value of PmCtrlReg, which I suspect isn't that valuable given the other deletions. This would also allow the removal of the "mPmCtrlReg" variable at file scope.
>
> On Thu, Jun 7, 2018 at 8:40 AM, Ma, Maurice <maurice.ma@intel.com> wrote:
> It looks good to me.
> Reviewed-by: Maurice Ma <maurice.ma@intel.com>
>
>
> > -----Original Message-----
> > From: You, Benjamin
> > Sent: Thursday, June 7, 2018 1:19
> > To: edk2-devel@lists.01.org
> > Cc: Ma, Maurice <maurice.ma@intel.com>; Agyeman, Prince <prince.agyeman@intel.com>; delco@google.com
> > Subject: [PATCH] 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>
> > ---
> > 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
> >
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-06-08 6:06 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-07 8:19 [PATCH] CorebootModulePkg/CbSupportDxe: Remove SCI_EN setting Benjamin You
2018-06-07 15:40 ` Ma, Maurice
2018-06-07 17:07 ` Matt Delco
2018-06-08 6:06 ` You, Benjamin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox