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 8F736210D6CFF for ; Thu, 31 May 2018 14:55:02 -0700 (PDT) Received: by mail-ot0-x236.google.com with SMTP id q17-v6so16460604otg.2 for ; Thu, 31 May 2018 14:55:02 -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=PpEqctEW4XEFvwbu8nnBtRki2BZ57meh4StWACOPn78=; b=cXTlyPnKuuqJulrI0kNZup/l1DVOY8P3v/i207ipp1upYP5lVsaHgBR5x7C40ZjT3w Grvo/iusWJrNdfBVzavAkddNmhxFIj/3PsmZLjWBNeBnFvCMSHXcVfbe0bWOkHaMvc/+ mK0qyhohIk8eDaR64vd1v0QhTK/TAckW+y2EeU8aguQPWhaWGZn53pmiiUteb+9J1B5V 7oIo0Is4UBzG6YiZ3iqCeC8vWM/ygxqY6WgSLJ34vi5faA9eqadIEGEZGQsdtJrdp7F+ X8JNB6TrXYkQMBwdPRJ1WE9ogVd1vGZ4d8pltgj4Hi4nfCtYgudpQrSqAIB3vh1nnZGO YPhg== 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=PpEqctEW4XEFvwbu8nnBtRki2BZ57meh4StWACOPn78=; b=cSjV1FQfRYR/3BsjgpXJP1za16YFqFCSq5ESKsBvvqHad20X/X6l4sbgRLLUSGSnhU Iffpsff704Cb4Yg/0oSHmFgXtg2HKiuGgiqc7T4E8+9hZwE3G6GU1cvu+QNMoxEm6bhg MAylOe/IJ6jou0fW+ISqvF/V7bvlMctI6UDUs6HJR6BXuhEdM6/eZDaWsJWvpNNhNN9u UFVtynVJqiKmmRkmp+5c3CU87aFsF/0BSHd3Khr2MYxZ32pseYZ1XDsnX7v5U2rAQch8 HcWt/63DoRaUTHqOiIOWliU1MJ8QNW7Lladz0o5cjpHvlZalcE0LnWhu04FMsKa1DRlm isjQ== X-Gm-Message-State: APt69E09PnlOrCWIYj5ggSeZ/yzJumcIeCBmnsPhycTm0LnKmLapX6wf R2Dq43IFJGzZN1dAA0jcWHivsLqHgK/ZQM9z0nLUJg== X-Google-Smtp-Source: ADUXVKKefHqH8Kg3Y8FWcFolbcv8G3qPsCiYAihdler7S+hJcUTh9zFZppKVeXg4uWiDVfW984cCgWbqxKJWzt9GPbU= X-Received: by 2002:a9d:3bf7:: with SMTP id k110-v6mr5401225otc.172.1527803701465; Thu, 31 May 2018 14:55:01 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a4a:26ce:0:0:0:0:0 with HTTP; Thu, 31 May 2018 14:55:00 -0700 (PDT) In-Reply-To: References: From: Matt Delco Date: Thu, 31 May 2018 14:55:00 -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: Thu, 31 May 2018 21:55:02 -0000 Content-Type: text/plain; charset="UTF-8" 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 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 > > Cc: edk2-devel@lists.01.org; Ma, Maurice ; > Agyeman, Prince > > 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 > 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 > >