From: Matt Delco <delco@google.com>
To: "You, Benjamin" <benjamin.you@intel.com>
Cc: "edk2-devel@lists.01.org" <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
Date: Thu, 31 May 2018 19:13:01 -0700 [thread overview]
Message-ID: <CAHGX9VprrQwTvLqHxSjTpm9Yxcy7RsM2+=PS0M4rB=1JMJiVww@mail.gmail.com> (raw)
In-Reply-To: <E748835C6D8DB54B8E8AF33091ECC57C6221E9E5@SHSMSX103.ccr.corp.intel.com>
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
>
>
next prev parent reply other threads:[~2018-06-01 2:13 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2018-06-01 2:34 ` You, Benjamin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHGX9VprrQwTvLqHxSjTpm9Yxcy7RsM2+=PS0M4rB=1JMJiVww@mail.gmail.com' \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox