Re: [PATCH] CorebootModulePkg/CbSupportDxe: Remove SCI_EN setting

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

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