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

[Matt Delco <delco@google.com>]

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