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