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

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

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