From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web12.8542.1578576752162163997 for ; Thu, 09 Jan 2020 05:32:32 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=b686mf/p; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1578576751; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Vqwc14BqYyJ0bEBFhgrKaP5V7bageRglGG1BCT/9snI=; b=b686mf/pjYqj4ewoplfI4nDTtkRSpeqbA62Yqag2bEjOEc3Tbf55K2t9aWViSnwG6Jq9UQ R5l6Wz/0mzWXzAaaawTQM7CS3UHqt3drC5hpegTvN50USqv+hjHMN/gFqZqdrM1x+6lA3Q 8Kns5F0AC405O3FZhGiUdD4/UoU3e18= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-103-KwQs8DKJPfK1_QA4pqTpug-1; Thu, 09 Jan 2020 08:32:29 -0500 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 85E79DBEF; Thu, 9 Jan 2020 13:32:27 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (unknown [10.36.118.145]) by smtp.corp.redhat.com (Postfix) with ESMTP id E2555272D3; Thu, 9 Jan 2020 13:32:25 +0000 (UTC) Subject: Re: [PATCH v2 2/2] UefiCpuPkg: Shadow microcode patch according to FIT microcode entry. To: Siyuan Fu , devel@edk2.groups.io Cc: michael.d.kinney@intel.com, liming.gao@intel.com, eric.dong@intel.com, ray.ni@intel.com References: From: "Laszlo Ersek" Message-ID: <71935c5f-9b05-8079-bcf4-d05638a8a6d0@redhat.com> Date: Thu, 9 Jan 2020 14:32:25 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-MC-Unique: KwQs8DKJPfK1_QA4pqTpug-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Hi, On 01/09/20 03:14, Siyuan Fu wrote: > The existing MpInitLib will shadow the microcode update patches from > flash to memory and this is done by searching microcode region specified > by PCD PcdCpuMicrocodePatchAddress and PcdCpuMicrocodePatchRegionSize. > This brings a limition to platform FW that all the microcode patches must > be placed in one continuous flash space. > > This patch shadows microcode update according to FIT microcode entries if > it's present, otherwise it will fallback to original logic (by PCD). > > A new featured PCD gUefiCpuPkgTokenSpaceGuid.PcdCpuShadowMicrocodeByFit > is added for enabling/disabling this support. > > TEST: Tested on FIT enabled platform. > BZ: https://tianocore.acgmultimedia.com/show_bug.cgi?id=2449 > > Cc: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > Signed-off-by: Siyuan Fu > --- > UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 1 + > UefiCpuPkg/Library/MpInitLib/Microcode.c | 262 +++++++++++++----- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 4 +- > UefiCpuPkg/Library/MpInitLib/MpLib.h | 7 +- > UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf | 4 +- > UefiCpuPkg/UefiCpuPkg.dec | 6 + > 6 files changed, 216 insertions(+), 68 deletions(-) > diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec > index 45b267ac61..a6ebdde1cf 100644 > --- a/UefiCpuPkg/UefiCpuPkg.dec > +++ b/UefiCpuPkg/UefiCpuPkg.dec > @@ -139,6 +139,12 @@ > # @Prompt Lock SMM Feature Control MSR. > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmFeatureControlMsrLock|TRUE|BOOLEAN|0x3213210B > > + ## Indicates if FIT based microcode shadowing will be enabled.

> + # TRUE - FIT base microcode shadowing will be enabled.
> + # FALSE - FIT base microcode shadowing will be disabled.
> + # @Prompt FIT based microcode shadowing. > + gUefiCpuPkgTokenSpaceGuid.PcdCpuShadowMicrocodeByFit|FALSE|BOOLEAN|0x3213210D > + > [PcdsFixedAtBuild] > ## List of exception vectors which need switching stack. > # This PCD will only take into effect if PcdCpuStackGuard is enabled. > (1) This looks good, but I think you should extend the "UefiCpuPkg.uni" file accordingly. > diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf > index 326703cc9a..5b4a1f31c8 100644 > --- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf > +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf > @@ -1,7 +1,7 @@ > ## @file > # MP Initialize Library instance for PEI driver. > # > -# Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.
> +# Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.
> # SPDX-License-Identifier: BSD-2-Clause-Patent > # > ## > @@ -60,7 +60,7 @@ > gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize ## CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode ## CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate ## SOMETIMES_CONSUMES > - > + gUefiCpuPkgTokenSpaceGuid.PcdCpuShadowMicrocodeByFit ## CONSUMES > [Guids] > gEdkiiS3SmmInitDoneGuid > gEdkiiMicrocodePatchHobGuid (2) The whitespace update is not ideal here -- I don't think we should remove the empty line, just before [Guids]. Instead, we should add the new entry above the empty line (and keep the empty line). > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf > index cd912ab0c5..0fd420ac93 100644 > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf > @@ -69,4 +69,5 @@ > gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode ## CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate ## SOMETIMES_CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard ## CONSUMES > + gUefiCpuPkgTokenSpaceGuid.PcdCpuShadowMicrocodeByFit ## CONSUMES (3) I think it would look nicer if we kept the new entry right next to the other gUefiCpuPkgTokenSpaceGuid.Xxx PCDs. Because as written, "gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard" is embedded between multiple gUefiCpuPkgTokenSpaceGuid.Xxx PCDs. > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h > index b6e5a1afab..7c62d75acc 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h > @@ -29,6 +29,9 @@ > #include > #include > > +#include > + > + > #define WAKEUP_AP_SIGNAL SIGNATURE_32 ('S', 'T', 'A', 'P') > > #define CPU_INIT_MP_LIB_HOB_GUID \ > @@ -587,12 +590,12 @@ MicrocodeDetect ( > ); > > /** > - Load the required microcode patches data into memory. > + Shadow the required microcode patches data into memory. > > @param[in, out] CpuMpData The pointer to CPU MP Data structure. > **/ > VOID > -LoadMicrocodePatch ( > +ShadowMicrocodeUpdatePatch ( > IN OUT CPU_MP_DATA *CpuMpData > ); > OK. > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index e611a8ca40..6ec9b172b8 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -1,7 +1,7 @@ > /** @file > CPU MP Initialize Library common functions. > > - Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.
> + Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -1744,7 +1744,7 @@ MpInitLibInitialize ( > // > // Load required microcode patches data into memory > // > - LoadMicrocodePatch (CpuMpData); > + ShadowMicrocodeUpdatePatch (CpuMpData); > } else { > // > // APs have been wakeup before, just get the CPU Information OK. > diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c > index 8f4d4da40c..9389e52ae5 100644 > --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c > +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c (snipping some stuff) > /** > - Load the required microcode patches data into memory. > + Shadow the required microcode patches data into memory according to PCD > + PcdCpuMicrocodePatchAddress and PcdCpuMicrocodePatchRegionSize. > > @param[in, out] CpuMpData The pointer to CPU MP Data structure. > **/ > VOID > -LoadMicrocodePatch ( > +ShadowMicrocodePatchByPcd ( > IN OUT CPU_MP_DATA *CpuMpData > ) > { > @@ -423,15 +493,10 @@ LoadMicrocodePatch ( > UINTN MicrocodeEnd; > UINTN DataSize; > UINTN TotalSize; > - CPU_MICROCODE_EXTENDED_TABLE_HEADER *ExtendedTableHeader; > - UINT32 ExtendedTableCount; > - CPU_MICROCODE_EXTENDED_TABLE *ExtendedTable; > MICROCODE_PATCH_INFO *PatchInfoBuffer; > UINTN MaxPatchNumber; > UINTN PatchCount; > UINTN TotalLoadSize; > - UINTN Index; > - BOOLEAN NeedLoad; > > // > // Initialize the microcode patch related fields in CpuMpData as the values > @@ -487,55 +552,7 @@ LoadMicrocodePatch ( > continue; > } > OK. > +/** > + Shadow the required microcode patches data into memory according to FIT microcode entry. > + > + @param[in, out] CpuMpData The pointer to CPU MP Data structure. > + > + @return EFI_SUCCESS Microcode patch is shadowed into memory. > + @return EFI_UNSUPPORTED FIT based microcode shadowing is not supported. > + @return EFI_OUT_OF_RESOURCES No enough memory resource. > + @return EFI_NOT_FOUND There is something wrong in FIT microcode entry. > + > +**/ > +EFI_STATUS > +ShadowMicrocodePatchByFit ( > + IN OUT CPU_MP_DATA *CpuMpData > + ) > +{ > + UINT64 FitPointer; > + FIRMWARE_INTERFACE_TABLE_ENTRY *FitEntry; > + UINT32 EntryNum; > + UINT32 Index; > + MICROCODE_PATCH_INFO *PatchInfoBuffer; > + UINTN MaxPatchNumber; > + CPU_MICROCODE_HEADER *MicrocodeEntryPoint; > + UINTN PatchCount; > + UINTN TotalSize; > + UINTN TotalLoadSize; > + > + if (!FeaturePcdGet (PcdCpuShadowMicrocodeByFit)) { > + return EFI_UNSUPPORTED; > + } OK. > + > + FitPointer = *(UINT64 *) (UINTN) FIT_POINTER_ADDRESS; > + if ((FitPointer == 0) || > + (FitPointer == 0xFFFFFFFFFFFFFFFF) || > + (FitPointer == 0xEEEEEEEEEEEEEEEE)) { > + // > + // No FIT table. > + // > + ASSERT (FALSE); > + return EFI_NOT_FOUND; > + } > + FitEntry = (FIRMWARE_INTERFACE_TABLE_ENTRY *) (UINTN) FitPointer; > + if ((FitEntry[0].Type != FIT_TYPE_00_HEADER) || > + (FitEntry[0].Address != FIT_TYPE_00_SIGNATURE)) { > + // > + // Invalid FIT table, treat it as no FIT table. > + // > + ASSERT (FALSE); > + return EFI_NOT_FOUND; > + } OK. (Snipping the rest of ShadowMicrocodePatchByFit().) > +/** > + Shadow the required microcode patches data into memory. > + > + @param[in, out] CpuMpData The pointer to CPU MP Data structure. > +**/ > +VOID > +ShadowMicrocodeUpdatePatch ( > + IN OUT CPU_MP_DATA *CpuMpData > + ) > +{ > + EFI_STATUS Status; > + > + Status = ShadowMicrocodePatchByFit (CpuMpData); > + if (EFI_ERROR (Status)) { > + ShadowMicrocodePatchByPcd (CpuMpData); > + } > +} OK. With (1) through (3) fixed: Acked-by: Laszlo Ersek Thanks! Laszlo