Re: [PATCH] CorebootModulePkg/CbSupportDxe: Enable ACPI via cmd register

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

Hi, Matt,

The original SCI_EN setting code was added to cover some early buggy 
firmware implementation that fails to enable SMM, and/or fails to fill FADT 
properly, and still needs an SCI_EN to transition to ACPI mode.

However, as you pointed out, setting it in Payload is not the correct way
 to address the issue, which also has some side effects as in your case. 
Instead, Payload should output some reminding message to developer to fix 
the firmware that runs before Payload.

Thanks,

- ben

> From: Matt Delco [mailto:delco@google.com] 
> Sent: Wednesday, May 30, 2018 4:40 PM
> To: You, Benjamin <benjamin.you@intel.com>
> Cc: edk2-devel@lists.01.org; Ma, Maurice <maurice.ma@intel.com>; Agyeman, Prince <prince.agyeman@intel.com>
> Subject: Re: [PATCH] CorebootModulePkg/CbSupportDxe: Enable ACPI via cmd register
> 
> Hi, thanks for taking a look.  I agree that this seems like the OS's responsibility to enable ACPI and I was tempted to just remove the setting of SCI_EN.  However, I figured that there was likely a reason that someone bothered to add the function in the first place so I went with what seemed like the more conservative fix.  I can try testing the removal of SCI_EN though I'll likely be at a remote office for the next 2 days so it may be 3 or more days before I can update the flash on the platform to perform the experiment.
> 
> On Wed, May 30, 2018 at 1:27 AM, You, Benjamin <benjamin.you@intel.com> wrote:
> Hi Matt,
> 
> This is indeed a bug. The Payload should not set the SCI_EN bit. 
> 
> However, the Payload should not trigger an SMI either. It is OS's 
> responsibility to enable ACPI by following FADT's instruction to write 
> some value to some port.
> 
> We think a better fix might be:
> 1) Remove the SCI_EN setting code in ReadyToBoot event handler,
> 2) Add some checking in CbParseFadtInfo() in CbParseLib.c to output some 
>    warning message and ASSERT (FALSE) if ALL of the following conditions 
>    are met:
>    - HARDWARE_REDUCED_ACPI is not set
>    - SMI_CMD field is zero (indicating no need to switch mode, hence 
>      the platform being in ACPI mode)
>    - SCI_EN bit is zero (indicating not in ACPI mode)
> 
> Item 2) above is to remind the developer that in this case, there may be a 
> bug in the firmware that runs before the Payload.
> 
> Could you please try on your platform to completely remove the SCI_EN 
> setting code in ReadToBoot event handler, and see if it works properly 
> (e.g., ACPI events delivered correctly)?
> 
> Thanks,
> 
> - ben
> 
> > -----Original Message-----
> > From: Matt Delco [mailto:delco@google.com] 
> > Sent: Monday, May 28, 2018 5:57 AM
> > To: edk2-devel@lists.01.org
> > Cc: Ma, Maurice <maurice.ma@intel.com>; Agyeman, Prince <prince.agyeman@intel.com>; You, Benjamin <benjamin.you@intel.com>
> > Subject: [PATCH] CorebootModulePkg/CbSupportDxe: Enable ACPI via cmd register
> > 
> > The existing code sets the SCI_EN bit directly, which is a violation
> > of the ACPI spec ("It is the responsibility of the hardware to set or
> > reset this bit. OSPM always preserves this bit position"). The proper
> > way to cause this bit to bit set is to reference the FADT table and
> > write the value of ACPI_ENABLE to SMI_CMD.  The SMI will in turn set
> > the SCI_EN bit.
> > 
> > Prior to this change ACPI events were not being delivered because
> > ACPI was not properly enabled and the OS also did not attempt
> > to enable ACPI since it sees that SCI_EN is already set.  After this
> > change I observed that ACPI events were now being delivered properly.
> > 
> > Cc: Maurice Ma <maurice.ma@intel.com>
> > Cc: Prince Agyeman <prince.agyeman@intel.com>
> > Cc: Benjamin You <benjamin.you@intel.com>
> > 
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Matt Delco <delco@google.com>
> > ---
> >  CorebootModulePkg/CbSupportDxe/CbSupportDxe.c | 79 ++++++++++++++++---
> >  .../CbSupportDxe/CbSupportDxe.inf             |  1 +
> >  2 files changed, 68 insertions(+), 12 deletions(-)
> > 
> > diff --git a/CorebootModulePkg/CbSupportDxe/CbSupportDxe.c b/CorebootModulePkg/CbSupportDxe/CbSupportDxe.c
> > index c526c9e871..4c7917ff2a 100755
> > --- a/CorebootModulePkg/CbSupportDxe/CbSupportDxe.c
> > +++ b/CorebootModulePkg/CbSupportDxe/CbSupportDxe.c
> > @@ -14,7 +14,10 @@
> >  **/
> >  #include "CbSupportDxe.h"
> >  
> > -UINTN mPmCtrlReg = 0;
> > +BOOLEAN mSmiPortFound = FALSE;
> > +UINTN mSmiCommandPort = 0;
> > +UINT8 mAcpiEnable = 0;
> > +
> >  /**
> >    Reserve MMIO/IO resource in GCD
> >  
> > @@ -107,9 +110,63 @@ OnReadyToBoot (
> >    //
> >    // Enable SCI
> >    //
> > -  IoOr16 (mPmCtrlReg, BIT0);
> > +  if (!mSmiPortFound) {
> > +    DEBUG ((DEBUG_ERROR, "SMI port not known so can not enable SCI\n"));
> > +  } else {
> > +    IoWrite8 (mSmiCommandPort, mAcpiEnable);
> > +    DEBUG ((DEBUG_ERROR, "Enable ACPI at 0x%lx with 0x%x before boot\n", (UINT64)mSmiCommandPort, (UINT32)mAcpiEnable));
> > +  }
> > +}
> > +
> > +/**
> > +  Lookup port and value for enabling ACPI
> > +
> > +  @param[in] SystemTable    A pointer to the EFI System Table.
> >  
> > -  DEBUG ((EFI_D_ERROR, "Enable SCI bit at 0x%lx before boot\n", (UINT64)mPmCtrlReg));
> > +**/
> > +VOID
> > +FindAcpiFadtTableInfo (
> > +  IN EFI_SYSTEM_TABLE   *SystemTable
> > +  )
> > +{
> > +  EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER  *Rsdp;
> > +  EFI_ACPI_DESCRIPTION_HEADER                   *Rsdt;
> > +  EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE     *Fadt;
> > +  UINTN                                         Index;
> > +  Rsdp  = NULL;
> > +  Rsdt  = NULL;
> > +  Fadt  = NULL;
> > +
> > +  for (Index = 0; Index < SystemTable->NumberOfTableEntries; Index++) {
> > +    if (CompareGuid (&(SystemTable->ConfigurationTable[Index].VendorGuid), &gEfiAcpi20TableGuid)) {
> > +      Rsdp = SystemTable->ConfigurationTable[Index].VendorTable;
> > +      break;
> > +    }
> > +  }
> > +  if (Rsdp == NULL) {
> > +    return;
> > +  }
> > +
> > +  Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN) Rsdp->RsdtAddress;
> > +  if (Rsdt == NULL || Rsdt->Signature != EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
> > +    return;
> > +  }
> > +  for (Index = sizeof (EFI_ACPI_DESCRIPTION_HEADER); Index < Rsdt->Length; Index = Index + sizeof (UINT32)) {
> > +    UINT32 Data32  = *(UINT32 *) ((UINT8 *) Rsdt + Index);
> > +    Fadt    = (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *) (UINT32 *) (UINTN) Data32;
> > +    if (Fadt->Header.Signature == EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
> > +      break;
> > +    }
> > +  }
> > +
> > +  if (Fadt == NULL || Fadt->Header.Signature != EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) {
> > +    return;
> > +  }
> > +
> > +  mSmiPortFound = TRUE;
> > +  mSmiCommandPort = Fadt->SmiCmd;
> > +  mAcpiEnable = Fadt->AcpiEnable;
> > +  return;
> >  }
> >  
> >  /**
> > @@ -133,7 +190,6 @@ CbDxeEntryPoint (
> >    EFI_EVENT  ReadyToBootEvent;
> >    EFI_HOB_GUID_TYPE  *GuidHob;
> >    SYSTEM_TABLE_INFO  *pSystemTableInfo;
> > -  ACPI_BOARD_INFO    *pAcpiBoardInfo;
> >    FRAME_BUFFER_INFO  *FbInfo;
> >  
> >    Status = EFI_SUCCESS;
> > @@ -172,14 +228,14 @@ CbDxeEntryPoint (
> >    }
> >  
> >    //
> > -  // Find the acpi board information guid hob
> > +  // Find the SMI port in the FADT table in acpi
> >    //
> > -  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));
> > +  FindAcpiFadtTableInfo(SystemTable);
> > +  if (mSmiPortFound) {
> > +    DEBUG ((DEBUG_ERROR, "Found ACPI enable info in FADT: 0x%lx 0x%x\n", (UINT64)mSmiCommandPort, (UINT32)mAcpiEnable));
> > +  } else {
> > +    DEBUG ((DEBUG_ERROR, "Failed to find FADT info for enabling ACPI\n"));
> > +  }
> >  
> >    //
> >    // Find the frame buffer information and update PCDs
> > @@ -212,4 +268,3 @@ CbDxeEntryPoint (
> >  
> >    return EFI_SUCCESS;
> >  }
> > -
> > diff --git a/CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf b/CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf
> > index 99245183ea..e334dafdb7 100644
> > --- a/CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf
> > +++ b/CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf
> > @@ -51,6 +51,7 @@
> >  
> >  [Guids]
> >    gEfiAcpiTableGuid
> > +  gEfiAcpi20TableGuid
> >    gEfiSmbiosTableGuid
> >    gUefiSystemTableInfoGuid
> >    gUefiAcpiBoardInfoGuid
> > -- 
> > 2.17.0.921.gf22659ad46-goog