Hi Matt, Sure. I will prepare a patch for this.
Thanks, - ben > From: Matt Delco [mailto:de...@google.com] > Sent: Friday, June 1, 2018 10:13 AM > To: You, Benjamin <benjamin....@intel.com> > Cc: edk2-devel@lists.01.org; Ma, Maurice <maurice...@intel.com>; Agyeman, > Prince <prince.agye...@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....@intel.com> > > wrote: > > Hi, Matt, > > > > Thanks. Would you like to provide an updated patch? > > > > Thanks, > > > > - ben > > > > > From: Matt Delco [mailto:de...@google.com] > > > Sent: Friday, June 1, 2018 5:55 AM > > > To: You, Benjamin <benjamin....@intel.com> > > > Cc: edk2-devel@lists.01.org; Ma, Maurice <maurice...@intel.com>; Agyeman, > > > Prince <prince.agye...@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....@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:de...@google.com] > > > > > Sent: Wednesday, May 30, 2018 4:40 PM > > > > > To: You, Benjamin <benjamin....@intel.com> > > > > > Cc: edk2-devel@lists.01.org; Ma, Maurice <maurice...@intel.com>; > > > > > Agyeman, Prince <prince.agye...@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....@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:de...@google.com] > > > > > > Sent: Monday, May 28, 2018 5:57 AM > > > > > > To: edk2-devel@lists.01.org > > > > > > Cc: Ma, Maurice <maurice...@intel.com>; Agyeman, Prince > > > > > > <prince.agye...@intel.com>; You, Benjamin <benjamin....@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...@intel.com> > > > > > > Cc: Prince Agyeman <prince.agye...@intel.com> > > > > > > Cc: Benjamin You <benjamin....@intel.com> > > > > > > > > > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > > > > > Signed-off-by: Matt Delco <de...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
