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:c06::231; helo=mail-oi0-x231.google.com; envelope-from=delco@google.com; receiver=edk2-devel@lists.01.org Received: from mail-oi0-x231.google.com (mail-oi0-x231.google.com [IPv6:2607:f8b0:4003:c06::231]) (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 D43B2208F7A22 for ; Thu, 31 May 2018 19:13:02 -0700 (PDT) Received: by mail-oi0-x231.google.com with SMTP id k5-v6so21248208oiw.0 for ; Thu, 31 May 2018 19:13: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=E/yrX3RbWUYYNkqveJZn7P6TcYTx4aVDC6Xs5PmkzBM=; b=mYjcxZZINQs5JhpnAVs6NcIUifJ8rV+o85xs2qWMTdHxyEB407BubK24/s3JYHPMjY DWSQN8JkEWK1lVWYEnOXhwfFILSXQ0XadlhyxowfDd6HozZ/DW1QbZKBNf2ZT7YOmwAV 6zV6cRoqZkQE1jo5h/Ca+gOI7xILjVe0D18eIqxWfRVE6dhWuPsKjr39rABJHni9LHIO rizIBJ/P/zhtEiBkMJxAB/dLRVjxQdrHofgTb3KnpFXnOfdb9buKB0Zf97yk96uvxL7c 7/Q9U1LXRP7L1KSpuIFP1sO4aElYNxDPctBPWLZf38yFg1DiioAFrGITP1Q+OAuLIH5a 4XSg== 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=E/yrX3RbWUYYNkqveJZn7P6TcYTx4aVDC6Xs5PmkzBM=; b=p9Cx5IU29NOcIEalfDbc/sQ/WTYRvvZEajFpMiF1GJsGGrLifPQchuSb6YWAcQKwW0 McEvp8PY5szbi4/Q0+j6WIVRSGqfPJovE1aJM9N7ZsAkxzZKLAyOMvLMFfmxd56kfHKN mDm9VepWMFu+oh94HpPg7Sg8t73p4uhiEuiGz/d4qIVd6G8th6iMd8AJEcQ2Rqyv9fiw 7n1/mKoufAgoOMtFh3EqWstLYyyksmjiOkRFUYzwGISHbnLAWGq57vI2MZfllj+9bDLk eCnUA5qpHUog242EftOoPfYLX3WOA8TC+aFea2irN94cPz48GvClWX/q+8g0WpTHYBpG LzqQ== X-Gm-Message-State: ALKqPwe8zyfzPnBR1aM5nsEC+3UmT7cZPUScqLoSjAdu7dwh8d5wb/tc BcGtUUd4XyYVzJ90jwxgcdHKU/kki/xeRGsBRHvYHa9HVV0= X-Google-Smtp-Source: ADUXVKKvSXMqR6GdVuQF68KNflmx7IN205RdnxiiwF+h32h0o05A8nhhMr3ZYp9GM33OB/DryCZWUQvZd2POu5IXAyM= X-Received: by 2002:aca:4ed1:: with SMTP id c200-v6mr4842187oib.278.1527819181615; Thu, 31 May 2018 19:13:01 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a4a:26ce:0:0:0:0:0 with HTTP; Thu, 31 May 2018 19:13:01 -0700 (PDT) In-Reply-To: References: From: Matt Delco Date: Thu, 31 May 2018 19:13:01 -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: Fri, 01 Jun 2018 02:13:03 -0000 Content-Type: text/plain; charset="UTF-8" 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 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 > > Cc: edk2-devel@lists.01.org; Ma, Maurice ; > Agyeman, Prince > > 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 > 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 < > 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 ; 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 > >