* [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