* [PATCH] CorebootModulePkg/CbSupportDxe: Enable ACPI via cmd register
@ 2018-05-27 21:57 Matt Delco
2018-05-30 8:27 ` You, Benjamin
0 siblings, 1 reply; 8+ messages in thread
From: Matt Delco @ 2018-05-27 21:57 UTC (permalink / raw)
To: edk2-devel; +Cc: Maurice Ma, Prince Agyeman, Benjamin You
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] CorebootModulePkg/CbSupportDxe: Enable ACPI via cmd register
2018-05-27 21:57 [PATCH] CorebootModulePkg/CbSupportDxe: Enable ACPI via cmd register Matt Delco
@ 2018-05-30 8:27 ` You, Benjamin
2018-05-30 8:40 ` Matt Delco
0 siblings, 1 reply; 8+ messages in thread
From: You, Benjamin @ 2018-05-30 8:27 UTC (permalink / raw)
To: Matt Delco, edk2-devel@lists.01.org
Cc: Ma, Maurice, Agyeman, Prince, You, Benjamin
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] CorebootModulePkg/CbSupportDxe: Enable ACPI via cmd register
2018-05-30 8:27 ` You, Benjamin
@ 2018-05-30 8:40 ` Matt Delco
2018-05-31 2:52 ` You, Benjamin
0 siblings, 1 reply; 8+ messages in thread
From: Matt Delco @ 2018-05-30 8:40 UTC (permalink / raw)
To: You, Benjamin; +Cc: edk2-devel@lists.01.org, Ma, Maurice, Agyeman, Prince
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
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] CorebootModulePkg/CbSupportDxe: Enable ACPI via cmd register
2018-05-30 8:40 ` Matt Delco
@ 2018-05-31 2:52 ` You, Benjamin
2018-05-31 21:55 ` Matt Delco
0 siblings, 1 reply; 8+ messages in thread
From: You, Benjamin @ 2018-05-31 2:52 UTC (permalink / raw)
To: Matt Delco; +Cc: edk2-devel@lists.01.org, Ma, Maurice, Agyeman, Prince
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] CorebootModulePkg/CbSupportDxe: Enable ACPI via cmd register
2018-05-31 2:52 ` You, Benjamin
@ 2018-05-31 21:55 ` Matt Delco
2018-06-01 0:44 ` You, Benjamin
0 siblings, 1 reply; 8+ messages in thread
From: Matt Delco @ 2018-05-31 21:55 UTC (permalink / raw)
To: You, Benjamin; +Cc: edk2-devel@lists.01.org, Ma, Maurice, Agyeman, Prince
Hi,
I commented out the body of OnReadyToBoot() and the result is as expected
(i.e., ACPI events are still received, presumably because the OS enabled
ACPI for itself).
On Wed, May 30, 2018 at 7:52 PM, You, Benjamin <benjamin.you@intel.com>
wrote:
> 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
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] CorebootModulePkg/CbSupportDxe: Enable ACPI via cmd register
2018-05-31 21:55 ` Matt Delco
@ 2018-06-01 0:44 ` You, Benjamin
2018-06-01 2:13 ` Matt Delco
0 siblings, 1 reply; 8+ messages in thread
From: You, Benjamin @ 2018-06-01 0:44 UTC (permalink / raw)
To: Matt Delco; +Cc: edk2-devel@lists.01.org, Ma, Maurice, Agyeman, Prince
Hi, Matt,
Thanks. Would you like to provide an updated patch?
Thanks,
- ben
> From: Matt Delco [mailto:delco@google.com]
> Sent: Friday, June 1, 2018 5:55 AM
> 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,
>
> I commented out the body of OnReadyToBoot() and the result is as expected (i.e., ACPI events are still received, presumably because the OS enabled ACPI for itself).
>
> > On Wed, May 30, 2018 at 7:52 PM, You, Benjamin <benjamin.you@intel.com> wrote:
> > 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] CorebootModulePkg/CbSupportDxe: Enable ACPI via cmd register
2018-06-01 0:44 ` You, Benjamin
@ 2018-06-01 2:13 ` Matt Delco
2018-06-01 2:34 ` You, Benjamin
0 siblings, 1 reply; 8+ messages in thread
From: Matt Delco @ 2018-06-01 2:13 UTC (permalink / raw)
To: You, Benjamin; +Cc: edk2-devel@lists.01.org, Ma, Maurice, Agyeman, Prince
Normally I would provide a patch, but I'm not sure I'd add correctly add
the checks that were proposed for CbParseFadtInfo(). I'd also have to go
through an company-internal review process regarding edk2's contributor
license agreement (I incorrectly assumed this had already been done when I
provided the initial patch). This review might require that I place an
apache2 license and company copyright on patch contributions, and I'd feel
really odd about providing a patch that added a license but otherwise
merely deleted the lines that modify the register. So, for this particular
issue it might be cleaner, faster, and more straightforward if I didn't
provide a patch.
On Thu, May 31, 2018 at 5:44 PM, You, Benjamin <benjamin.you@intel.com>
wrote:
> Hi, Matt,
>
> Thanks. Would you like to provide an updated patch?
>
> Thanks,
>
> - ben
>
> > From: Matt Delco [mailto:delco@google.com]
> > Sent: Friday, June 1, 2018 5:55 AM
> > 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,
> >
> > I commented out the body of OnReadyToBoot() and the result is as
> expected (i.e., ACPI events are still received, presumably because the OS
> enabled ACPI for itself).
> >
> > > On Wed, May 30, 2018 at 7:52 PM, You, Benjamin <benjamin.you@intel.com>
> wrote:
> > > 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
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] CorebootModulePkg/CbSupportDxe: Enable ACPI via cmd register
2018-06-01 2:13 ` Matt Delco
@ 2018-06-01 2:34 ` You, Benjamin
0 siblings, 0 replies; 8+ messages in thread
From: You, Benjamin @ 2018-06-01 2:34 UTC (permalink / raw)
To: Matt Delco; +Cc: edk2-devel@lists.01.org, Ma, Maurice, Agyeman, Prince
Hi Matt,
Sure. I will prepare a patch for this.
Thanks,
- ben
> From: Matt Delco [mailto:delco@google.com]
> Sent: Friday, June 1, 2018 10:13 AM
> 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
>
> Normally I would provide a patch, but I'm not sure I'd add correctly add the checks that were proposed for CbParseFadtInfo(). I'd also have to go through an company-internal review process regarding edk2's contributor license agreement (I incorrectly assumed this had already been done when I provided the initial patch). This review might require that I place an apache2 license and company copyright on patch contributions, and I'd feel really odd about providing a patch that added a license but otherwise merely deleted the lines that modify the register. So, for this particular issue it might be cleaner, faster, and more straightforward if I didn't provide a patch.
>
> > On Thu, May 31, 2018 at 5:44 PM, You, Benjamin <benjamin.you@intel.com> wrote:
> > Hi, Matt,
> >
> > Thanks. Would you like to provide an updated patch?
> >
> > Thanks,
> >
> > - ben
> >
> > > From: Matt Delco [mailto:delco@google.com]
> > > Sent: Friday, June 1, 2018 5:55 AM
> > > 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,
> > >
> > > I commented out the body of OnReadyToBoot() and the result is as expected (i.e., ACPI events are still received, presumably because the OS enabled ACPI for itself).
> > >
> > > > On Wed, May 30, 2018 at 7:52 PM, You, Benjamin <benjamin.you@intel.com> wrote:
> > > > 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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-06-01 2:34 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-05-27 21:57 [PATCH] CorebootModulePkg/CbSupportDxe: Enable ACPI via cmd register Matt Delco
2018-05-30 8:27 ` You, Benjamin
2018-05-30 8:40 ` Matt Delco
2018-05-31 2:52 ` You, Benjamin
2018-05-31 21:55 ` Matt Delco
2018-06-01 0:44 ` You, Benjamin
2018-06-01 2:13 ` Matt Delco
2018-06-01 2:34 ` You, Benjamin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox