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::233; helo=mail-oi0-x233.google.com; envelope-from=delco@google.com; receiver=edk2-devel@lists.01.org Received: from mail-oi0-x233.google.com (mail-oi0-x233.google.com [IPv6:2607:f8b0:4003:c06::233]) (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 377E2210E12A6 for ; Fri, 8 Jun 2018 08:42:33 -0700 (PDT) Received: by mail-oi0-x233.google.com with SMTP id t133-v6so12205666oif.10 for ; Fri, 08 Jun 2018 08:42:33 -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=9eLf9Y0Pad/nzxYvOfNyyJcXqe4SS0Ahfb5R3pgw09g=; b=ohB2Tjwt4iv1cwsihnGQmbOFdWJkHcjFPM4bHEemncS2wIFbqBxLvFAhW3OfuPibKJ u3RxFQkLpNQ5Xv9uoVeQk/CqlJHghCulwh/E7Qtfbjk6/iuaKka9zDf7c4a2LXNwO6JU 5tHspHFPG91hAQ6wZZAfvm+mVcLcAOgcfmNAqKNeRU4Spt38ohMiOTwKsGUTvyZ1P5tF AA+Ir0AcoMlcxHXPrOXNyLTR2OZs3yMsB49SeBBbXIHrRN2EHfsUW9zzRkKS35GHnxLs Bgd8EnQBuj16mMW4M8sqeiaKzeU10VfkWnAt4nxBZyx22zz6JoxgHo9loXWQIWH4SpAV 7moA== 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=9eLf9Y0Pad/nzxYvOfNyyJcXqe4SS0Ahfb5R3pgw09g=; b=TmdiXjofhXybeGIjwNk4RysletrcsSKva9CmO6t0iQY+XiewsZojLs5jyNr5/xLoeU xRz/oJbC6gHugR+O1Vi+EWgBdB7LhBoc3Bm/LOXB+af+8BMkvjcEqpiF2jgraHtsdYya 3R+VhNMauNcxCRBhb1Cp99hb69Sufn33KfPVXtbJ5Kd/okpI1niqCz+1uTkLMBZqNL27 MQaPBJNNodH25ytyR7fljjvosoNGIBlCzBs7RJZKWWv2C8ZnA45iCEKsT+mqHuOmM6Qr 5xWO/JmSqDVrsQK6yOqjteXtSquHCRQeux2I86ztjt1Xjqu7bXTuYaiyAMlCF3O3h/Z1 YOYA== X-Gm-Message-State: APt69E3IUN28PX0jINsjUUtZ+2JbetyZMx8RQi5f0gUlcvIkS8lTOEDP tvHgCMgFaTzrcL5n2U+x3XJzQF+OSndn4sBymVXBPA== X-Google-Smtp-Source: ADUXVKJEzQ8Lsvxzbv+t94zBDkKzETstt9Rva3aZGWOFaGrt2M67Ihb5YrNxNO5K7AurYgI5uv52TpwFhYaSJxBh2pg= X-Received: by 2002:aca:d4c7:: with SMTP id l190-v6mr3645311oig.102.1528472551762; Fri, 08 Jun 2018 08:42:31 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a4a:c2:0:0:0:0:0 with HTTP; Fri, 8 Jun 2018 08:42:31 -0700 (PDT) In-Reply-To: <20180608060116.33200-1-benjamin.you@intel.com> References: <20180608060116.33200-1-benjamin.you@intel.com> From: Matt Delco Date: Fri, 8 Jun 2018 08:42:31 -0700 Message-ID: To: Benjamin You Cc: edk2-devel@lists.01.org, Maurice Ma , Prince Agyeman X-Content-Filtered-By: Mailman/MimeDel 2.1.26 Subject: Re: [PATCH v2] CorebootModulePkg/CbSupportDxe: Remove SCI_EN setting 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, 08 Jun 2018 15:42:33 -0000 Content-Type: text/plain; charset="UTF-8" This looks good to me, thanks again. On Thu, Jun 7, 2018 at 11:01 PM, Benjamin You wrote: > Current implemenation sets PM1_CNT.SCI_EN bit at ReadyToBoot event. > However, this should not be done because this causes OS to skip triggering > FADT.SMI_CMD, which leads to the functions implemented in the SMI > handler being omitted. > > This issue was identified by Matt Delco . > > The fix does the following: > - The SCI_EN bit setting is removed from CbSupportDxe driver. > - Some additional checks are added in CbParseFadtInfo() in CbParseLib.c to > output some error message and ASSERT (FALSE) if ALL of the following > conditions are met: > 1) HARDWARE_REDUCED_ACPI is not set; > 2) SMI_CMD field is zero; > 3) SCI_EN bit is zero; > which indicates the ACPI enabling status is inconsistent: SCI is not > enabled but the ACPI table does not provide a means to enable it through > FADT->SMI_CMD. This may cause issues in OS. > > Cc: Maurice Ma > Cc: Prince Agyeman > Cc: Matt Delco > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Benjamin You > --- > CorebootModulePkg/CbSupportDxe/CbSupportDxe.c | 51 > ---------------------- > CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf | 1 - > CorebootModulePkg/Library/CbParseLib/CbParseLib.c | 34 +++++++++++++++ > .../Library/CbParseLib/CbParseLib.inf | 3 +- > 4 files changed, 36 insertions(+), 53 deletions(-) > > diff --git a/CorebootModulePkg/CbSupportDxe/CbSupportDxe.c > b/CorebootModulePkg/CbSupportDxe/CbSupportDxe.c > index c526c9e871..ec42f7ff35 100755 > --- a/CorebootModulePkg/CbSupportDxe/CbSupportDxe.c > +++ b/CorebootModulePkg/CbSupportDxe/CbSupportDxe.c > @@ -14,7 +14,6 @@ > **/ > #include "CbSupportDxe.h" > > -UINTN mPmCtrlReg = 0; > /** > Reserve MMIO/IO resource in GCD > > @@ -86,31 +85,6 @@ CbReserveResourceInGcd ( > return Status; > } > > -/** > - Notification function of EVT_GROUP_READY_TO_BOOT event group. > - > - This is a notification function registered on EVT_GROUP_READY_TO_BOOT > event group. > - When the Boot Manager is about to load and execute a boot option, it > reclaims variable > - storage if free size is below the threshold. > - > - @param Event Event whose notification function is being invoked. > - @param Context Pointer to the notification function's context. > - > -**/ > -VOID > -EFIAPI > -OnReadyToBoot ( > - IN EFI_EVENT Event, > - IN VOID *Context > - ) > -{ > - // > - // Enable SCI > - // > - IoOr16 (mPmCtrlReg, BIT0); > - > - DEBUG ((EFI_D_ERROR, "Enable SCI bit at 0x%lx before boot\n", > (UINT64)mPmCtrlReg)); > -} > > /** > Main entry for the Coreboot Support DXE module. > @@ -130,10 +104,8 @@ CbDxeEntryPoint ( > ) > { > EFI_STATUS Status; > - EFI_EVENT ReadyToBootEvent; > EFI_HOB_GUID_TYPE *GuidHob; > SYSTEM_TABLE_INFO *pSystemTableInfo; > - ACPI_BOARD_INFO *pAcpiBoardInfo; > FRAME_BUFFER_INFO *FbInfo; > > Status = EFI_SUCCESS; > @@ -171,16 +143,6 @@ CbDxeEntryPoint ( > ASSERT_EFI_ERROR (Status); > } > > - // > - // Find the acpi board information guid hob > - // > - 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)); > - > // > // Find the frame buffer information and update PCDs > // > @@ -197,19 +159,6 @@ CbDxeEntryPoint ( > ASSERT_EFI_ERROR (Status); > } > > - // > - // Register callback on the ready to boot event > - // in order to enable SCI > - // > - ReadyToBootEvent = NULL; > - Status = EfiCreateEventReadyToBootEx ( > - TPL_CALLBACK, > - OnReadyToBoot, > - NULL, > - &ReadyToBootEvent > - ); > - ASSERT_EFI_ERROR (Status); > - > return EFI_SUCCESS; > } > > diff --git a/CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf > b/CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf > index 99245183ea..15b0dac774 100644 > --- a/CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf > +++ b/CorebootModulePkg/CbSupportDxe/CbSupportDxe.inf > @@ -46,7 +46,6 @@ > DebugLib > BaseMemoryLib > UefiLib > - IoLib > HobLib > > [Guids] > diff --git a/CorebootModulePkg/Library/CbParseLib/CbParseLib.c > b/CorebootModulePkg/Library/CbParseLib/CbParseLib.c > index 0909b0f492..da227dea5e 100644 > --- a/CorebootModulePkg/Library/CbParseLib/CbParseLib.c > +++ b/CorebootModulePkg/Library/CbParseLib/CbParseLib.c > @@ -18,6 +18,7 @@ > #include > #include > #include > +#include > #include > > #include > @@ -477,6 +478,39 @@ CbParseFadtInfo ( > ASSERT(Fadt->Pm1aEvtBlk != 0); > ASSERT(Fadt->Gpe0Blk != 0); > > + DEBUG_CODE_BEGIN (); > + BOOLEAN SciEnabled; > + > + // > + // Check the consistency of SCI enabling > + // > + > + // > + // Get SCI_EN value > + // > + if (Fadt->Pm1CntLen == 4) { > + SciEnabled = (IoRead32 (Fadt->Pm1aCntBlk) & BIT0)? TRUE : > FALSE; > + } else { > + // > + // if (Pm1CntLen == 2), use 16 bit IO read; > + // if (Pm1CntLen != 2 && Pm1CntLen != 4), use 16 bit IO read > as a fallback > + // > + SciEnabled = (IoRead16 (Fadt->Pm1aCntBlk) & BIT0)? TRUE : > FALSE; > + } > + > + if (!(Fadt->Flags & EFI_ACPI_5_0_HW_REDUCED_ACPI) && > + (Fadt->SmiCmd == 0) && > + !SciEnabled) { > + // > + // The ACPI enabling status is inconsistent: SCI is not > enabled but ACPI > + // table does not provide a means to enable it through > FADT->SmiCmd > + // > + DEBUG ((DEBUG_ERROR, "ERROR: The ACPI enabling status is > inconsistent: SCI is not" > + " enabled but the ACPI table does not provide a means to > enable it through FADT->SmiCmd." > + " This may cause issues in OS.\n")); > + ASSERT (FALSE); > + } > + DEBUG_CODE_END (); > return RETURN_SUCCESS; > } > } > diff --git a/CorebootModulePkg/Library/CbParseLib/CbParseLib.inf > b/CorebootModulePkg/Library/CbParseLib/CbParseLib.inf > index d7146a415b..25b847946c 100644 > --- a/CorebootModulePkg/Library/CbParseLib/CbParseLib.inf > +++ b/CorebootModulePkg/Library/CbParseLib/CbParseLib.inf > @@ -37,7 +37,8 @@ > [LibraryClasses] > BaseLib > BaseMemoryLib > - DebugLib > > + IoLib > + DebugLib > PcdLib > > [Pcd] > -- > 2.14.3.windows.1 > >