Re: [PATCH v2 2/2] UefiCpuPkg: Shadow microcode patch according to FIT microcode entry.

["Laszlo Ersek" <lersek@redhat.com>]

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.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Siyuan Fu <siyuan.fu@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 <lersek@redhat.com>

Thanks!
Laszlo