From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4003:c0f::236; helo=mail-ot0-x236.google.com; envelope-from=delco@google.com; receiver=edk2-devel@lists.01.org Received: from mail-ot0-x236.google.com (mail-ot0-x236.google.com [IPv6:2607:f8b0:4003:c0f::236]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 3A77920D7B273 for ; Wed, 30 May 2018 01:40:10 -0700 (PDT) Received: by mail-ot0-x236.google.com with SMTP id y10-v6so20207729otg.10 for ; Wed, 30 May 2018 01:40:09 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=hmS6MoNT8dJM/G7InHypyqyYjqGZxQlynH9ix6gXpwQ=; b=lDAenvCrrfprrOR7M4xy9yxFxC/9r6QseDzMaZ+I6m1N+n6QAIQHgRXwcmx/Y/5DD5 D4yJS7Es6upayjkwwLxGz/3CSoqk4P5EvIFqudBhnyXAF7ANGzBTQESmOX7+XCK6Nkhs u//IeFupaaqxzpx1PF6/mEPrz3xnX/b3SE9Zvz8o6MQXLLNeJnukQ+recKmxCAD5OeCR Gf/jFLAT9F3Keqv6tNUr37EJyO4GaUNN02B3WAnushVz0QG56nrcEbUiYI/+1hwniqjg NXcVbB/Nqc5wkSAMjogs3Sq3Gu7qgBo80c71fS9fBlL24mKlkfQZ0S2CbqvZ8hjgcXUA 5C9A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=hmS6MoNT8dJM/G7InHypyqyYjqGZxQlynH9ix6gXpwQ=; b=eiqc3OWejv/v5zIF65A8YhS5RBGSKq+8WlxOWVr8Nw9deI6E6h+Rm5GBNP9LCPZ29n LibubSDpKrP7aJZ5vpMCWgHPrlf1+fDgh7MjddUW7gpq9YOAnh0sjfutObzEanBWwgP9 KWmv+FrjnNlUigPlkGTlcFhLInHl2VTMt9pJLDxms0wLnaXPKazUPm1NrlIE2NbgSVMb sDGszaOjuqAhFhzWH7NEOSm4ceJ+DD/0nxi5LuxiumFDA6i/U0nOk6oQyMoKge9I8PZp XZrix7MpccJfOdwBuTRNlkutfQMwhUlihJFZwvsp8ZRWjqNrXlX7SZskgMVarFhi68nT cOmg== X-Gm-Message-State: ALKqPwfq2GQB/YQi1RIg5UmAqFfChGjvBHX630qmnGiEJuq3vu/jwTO1 8hM+aWikW60XWGP2aEPLYryF5s7OLusc5hWTb3LGig== X-Google-Smtp-Source: ADUXVKKk/4Y1oIW65dgvZ+nmMQeC13xAXPiV3dEmAj591QwtWUwqZxf4HV59GSe+dzpm/smu4FlBweGkPiiQ5w3YMJ8= X-Received: by 2002:a9d:2b5c:: with SMTP id f28-v6mr1016869otd.317.1527669608961; Wed, 30 May 2018 01:40:08 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a4a:26ce:0:0:0:0:0 with HTTP; Wed, 30 May 2018 01:40:08 -0700 (PDT) In-Reply-To: References: From: Matt Delco Date: Wed, 30 May 2018 01:40:08 -0700 Message-ID: To: "You, Benjamin" Cc: "edk2-devel@lists.01.org" , "Ma, Maurice" , "Agyeman, Prince" X-Content-Filtered-By: Mailman/MimeDel 2.1.26 Subject: Re: [PATCH] CorebootModulePkg/CbSupportDxe: Enable ACPI via cmd register X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 30 May 2018 08:40:10 -0000 Content-Type: text/plain; charset="UTF-8" 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 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 ; Agyeman, Prince < > prince.agyeman@intel.com>; You, Benjamin > > 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 > > Cc: Prince Agyeman > > Cc: Benjamin You > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Matt Delco > > --- > > 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 > >