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 <eric.d...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Signed-off-by: Siyuan Fu <siyuan...@intel.com>
> ---
> 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.<BR><BR>
> + # TRUE - FIT base microcode shadowing will be enabled.<BR>
> + # FALSE - FIT base microcode shadowing will be disabled.<BR>
> + # @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.<BR>
> +# Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
> # 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 <Library/MtrrLib.h>
> #include <Library/HobLib.h>
>
> +#include <IndustryStandard/FirmwareInterfaceTable.h>
> +
> +
> #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.<BR>
> + Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
> 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 <ler...@redhat.com>
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53082): https://edk2.groups.io/g/devel/message/53082
Mute This Topic: https://groups.io/mt/69559936/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
