Re: [edk2-devel] [PATCH v3] IntelSiliconPkg: FIT based shadow microcode PPI support.

["Michael D Kinney" <michael.d.kinney@intel.com>]

Siyuan,

IntelSiliconPkg/Feature/ShadowMicrocode:

For simple modules that only have a single .c file, there
Is not need to split out a .h file.  Please merge the .h
File content into the .c file and delete the .h file.

More comments inline below.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Siyuan, Fu
> Sent: Tuesday, February 11, 2020 4:48 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>
> Subject: [edk2-devel] [PATCH v3] IntelSiliconPkg: FIT
> based shadow microcode PPI support.
> 
> V3 Changes:
> Remove the feature PCD PcdCpuShadowMicrocodeByFit
> because the whole FIT microcode shadow code is moved to
> this PEIM so platform could disable this feature by not
> include PEIM now.
> 
> V2 Changes:
> Rename EDKII_PEI_CPU_MICROCODE_ID to
> EDKII_PEI_MICROCODE_CPU_ID.
> 
> This patch adds a platform PEIM for FIT based shadow
> microcode PPI support. A detailed design doc can be
> found here:
> https://edk2.groups.io/g/devel/files/Designs/2020/0214/
> Support%20
> the%202nd%20Microcode%20FV%20Flash%20Region.pdf
> 
> TEST: Tested on FIT enabled platform.
> BZ:
> https://tianocore.acgmultimedia.com/show_bug.cgi?id=244
> 9
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
> Signed-off-by: Siyuan Fu <siyuan.fu@intel.com>
> ---
>  .../ShadowMicrocode/ShadowMicrocodePei.c      | 387
> ++++++++++++++++++
>  .../ShadowMicrocode/ShadowMicrocodePei.h      |  62
> +++
>  .../ShadowMicrocode/ShadowMicrocodePei.inf    |  44 ++
>  .../Include/Guid/MicrocodeShadowInfoHob.h     |  57
> +++
>  .../Intel/IntelSiliconPkg/IntelSiliconPkg.dec |   6 +
>  .../Intel/IntelSiliconPkg/IntelSiliconPkg.dsc |   3 +-
>  6 files changed, 558 insertions(+), 1 deletion(-)
> create mode 100644
> Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/S
> hadowMicrocodePei.c
>  create mode 100644
> Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/S
> hadowMicrocodePei.h
>  create mode 100644
> Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode/S
> hadowMicrocodePei.inf
>  create mode 100644
> Silicon/Intel/IntelSiliconPkg/Include/Guid/MicrocodeSha
> dowInfoHob.h
> 
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode
> /ShadowMicrocodePei.c
> b/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode
> /ShadowMicrocodePei.c
> new file mode 100644
> index 0000000000..c754524f41
> --- /dev/null
> +++
> b/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode
> /ShadowMicroc
> +++ odePei.c
> @@ -0,0 +1,387 @@
> +/** @file
> +  Source code file for Platform Init PEI module

This description does not match the content

> +
> +Copyright (c) 2017 - 2019, Intel Corporation. All
> rights reserved.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include "ShadowMicrocodePei.h"
> +
> +EDKII_PEI_SHADOW_MICROCODE_PPI
> mPeiShadowMicrocodePpi = {
> +  ShadowMicrocode
> +};
> +
> +
> +EFI_PEI_PPI_DESCRIPTOR
> mPeiShadowMicrocodePpiList[] = {
> +  {
> +    EFI_PEI_PPI_DESCRIPTOR_PPI |
> EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
> +    &gEdkiiPeiShadowMicrocodePpiGuid,
> +    &mPeiShadowMicrocodePpi
> +  }
> +};
> +
> +/**
> +  Determine if a microcode patch matchs the specific
> processor signature and flag.
> +
> +  @param[in]  CpuIdCount            Number of elements
> in MicrocodeCpuId array.
> +  @param[in]  MicrocodeCpuId        A pointer to an
> array of EDKII_PEI_MICROCODE_CPU_ID
> +                                    structures.
> +  @param[in]  ProcessorSignature    The processor
> signature field value
> +                                    supported by a
> microcode patch.
> +  @param[in]  ProcessorFlags        The prcessor flags
> field value supported by
> +                                    a microcode patch.
> +
> +  @retval TRUE     The specified microcode patch will
> be loaded.
> +  @retval FALSE    The specified microcode patch will
> not be loaded.
> +**/
> +BOOLEAN
> +IsProcessorMatchedMicrocodePatch (
> +  IN  UINTN                           CpuIdCount,
> +  IN  EDKII_PEI_MICROCODE_CPU_ID      *MicrocodeCpuId,
> +  IN UINT32
> ProcessorSignature,
> +  IN UINT32                           ProcessorFlags
> +  )
> +{
> +  UINTN          Index;
> +
> +  for (Index = 0; Index < CpuIdCount; Index++) {
> +    if ((ProcessorSignature ==
> MicrocodeCpuId[Index].ProcessorSignature) &&
> +        (ProcessorFlags & (1 <<
> MicrocodeCpuId[Index].PlatformId)) != 0) {
> +      return TRUE;
> +    }
> +  }
> +
> +  return FALSE;
> +}
> +
> +/**
> +  Check the 'ProcessorSignature' and 'ProcessorFlags'
> of the microcode
> +  patch header with the CPUID and PlatformID of the
> processors within
> +  system to decide if it will be copied into memory.
> +
> +  @param[in]  CpuIdCount            Number of elements
> in MicrocodeCpuId array.
> +  @param[in]  MicrocodeCpuId        A pointer to an
> array of EDKII_PEI_MICROCODE_CPU_ID
> +                                    structures.
> +  @param[in]  MicrocodeEntryPoint   The pointer to the
> microcode patch header.
> +
> +  @retval TRUE     The specified microcode patch need
> to be loaded.
> +  @retval FALSE    The specified microcode patch
> dosen't need to be loaded.
> +**/
> +BOOLEAN
> +IsMicrocodePatchNeedLoad (
> +  IN  UINTN                         CpuIdCount,
> +  IN  EDKII_PEI_MICROCODE_CPU_ID    *MicrocodeCpuId,
> +  CPU_MICROCODE_HEADER
> *MicrocodeEntryPoint
> +  )
> +{
> +  BOOLEAN                                NeedLoad;
> +  UINTN                                  DataSize;
> +  UINTN                                  TotalSize;
> +  CPU_MICROCODE_EXTENDED_TABLE_HEADER
> *ExtendedTableHeader;
> +  UINT32
> ExtendedTableCount;
> +  CPU_MICROCODE_EXTENDED_TABLE
> *ExtendedTable;
> +  UINTN                                  Index;
> +
> +  //
> +  // Check the 'ProcessorSignature' and
> 'ProcessorFlags' in microcode patch header.
> +  //
> +  NeedLoad = IsProcessorMatchedMicrocodePatch (
> +               CpuIdCount,
> +               MicrocodeCpuId,
> +               MicrocodeEntryPoint-
> >ProcessorSignature.Uint32,
> +               MicrocodeEntryPoint->ProcessorFlags
> +               );
> +
> +  //
> +  // If the Extended Signature Table exists, check if
> the processor is
> + in the  // support list  //  DataSize  =
> + MicrocodeEntryPoint->DataSize;  TotalSize = (DataSize
> == 0) ? 2048 :
> + MicrocodeEntryPoint->TotalSize;  if ((!NeedLoad) &&
> (DataSize != 0) &&
> +      (TotalSize - DataSize > sizeof
> (CPU_MICROCODE_HEADER) +
> +                              sizeof
> (CPU_MICROCODE_EXTENDED_TABLE_HEADER))) {
> +    ExtendedTableHeader =
> (CPU_MICROCODE_EXTENDED_TABLE_HEADER *) ((UINT8 *)
> (MicrocodeEntryPoint)
> +                            + DataSize + sizeof
> (CPU_MICROCODE_HEADER));
> +    ExtendedTableCount  = ExtendedTableHeader-
> >ExtendedSignatureCount;
> +    ExtendedTable       =
> (CPU_MICROCODE_EXTENDED_TABLE *) (ExtendedTableHeader +
> 1);
> +
> +    for (Index = 0; Index < ExtendedTableCount; Index
> ++) {
> +      //
> +      // Check the 'ProcessorSignature' and
> 'ProcessorFlag' of the Extended
> +      // Signature Table entry with the CPUID and
> PlatformID of the processors
> +      // within system to decide if it will be copied
> into memory
> +      //
> +      NeedLoad = IsProcessorMatchedMicrocodePatch (
> +                   CpuIdCount,
> +                   MicrocodeCpuId,
> +                   ExtendedTable-
> >ProcessorSignature.Uint32,
> +                   ExtendedTable->ProcessorFlag
> +                   );
> +      if (NeedLoad) {
> +        break;
> +      }
> +      ExtendedTable ++;
> +    }
> +  }
> +
> +  return NeedLoad;
> +}
> +
> +/**
> +  Actual worker function that shadows the required
> microcode patches into memory.
> +
> +  @param[in]       Patches          The pointer to an
> array of information on
> +                                    the microcode
> patches that will be loaded
> +                                    into memory.
> +  @param[in]       PatchCount       The number of
> microcode patches that will
> +                                    be loaded into
> memory.
> +  @param[in]       TotalLoadSize    The total size of
> all the microcode patches
> +                                    to be loaded.
> +  @param[out] BufferSize            Pointer to receive
> the total size of Buffer.
> +  @param[out] Buffer                Pointer to receive
> address of allocated memory
> +                                    with microcode
> patches data in it.
> +**/
> +VOID
> +ShadowMicrocodePatchWorker (
> +  IN  MICROCODE_PATCH_INFO       *Patches,
> +  IN  UINTN                      PatchCount,
> +  IN  UINTN                      TotalLoadSize,
> +  OUT UINTN                      *BufferSize,
> +  OUT VOID                       **Buffer
> +  )
> +{
> +  UINTN                              Index;
> +  VOID
> *MicrocodePatchInRam;
> +  UINT8                              *Walker;
> +  EDKII_MICROCODE_SHADOW_INFO_HOB
> *MicrocodeShadowHob;
> +  UINTN                              HobDataLength;
> +  UINT64
> *MicrocodeAddrInMemory;

Do not abbreviation "Addr".  Expand to "Address".
Same comment applies to entire patch.

> +  UINT64
> *MicrocodeAddrInFlash;
> +
> +  ASSERT ((Patches != NULL) && (PatchCount != 0));
> +
> +  //
> +  // Init microcode shadow info HOB content.
> +  //
> +  HobDataLength = sizeof
> (EDKII_MICROCODE_SHADOW_INFO_HOB) +
> +                  sizeof (UINT64) * PatchCount * 2;
> MicrocodeShadowHob
> + = AllocatePool (HobDataLength);  if
> (MicrocodeShadowHob == NULL) {
> +    ASSERT (FALSE);
> +    return;
> +  }
> +  MicrocodeShadowHob->MicrocodeCount = PatchCount;
> CopyGuid (
> +    &MicrocodeShadowHob->StorageType,
> +    &gEdkiiMicrocodeStorageTypeFlashGuid
> +    );
> +  MicrocodeAddrInMemory = (UINT64 *)
> (MicrocodeShadowHob + 1);
> + MicrocodeAddrInFlash  = MicrocodeAddrInMemory +
> PatchCount;
> +
> +  //
> +  // Allocate memory for microcode shadow operation.
> +  //
> +  MicrocodePatchInRam = AllocatePages
> (EFI_SIZE_TO_PAGES
> + (TotalLoadSize));  if (MicrocodePatchInRam == NULL) {
> +    ASSERT (FALSE);
> +    return;
> +  }
> +
> +  //
> +  // Shadow all the required microcode patches into
> memory  //  for
> + (Walker = MicrocodePatchInRam, Index = 0; Index <
> PatchCount; Index++) {
> +    CopyMem (
> +      Walker,
> +      (VOID *) Patches[Index].Address,
> +      Patches[Index].Size
> +      );
> +    MicrocodeAddrInMemory[Index] = (UINT64) Walker;
> +    MicrocodeAddrInFlash[Index]  = (UINT64)
> Patches[Index].Address;
> +    Walker += Patches[Index].Size;
> +  }
> +
> +  //
> +  // Update the microcode patch related fields in
> CpuMpData  //
> +  *Buffer     = (VOID *) (UINTN) MicrocodePatchInRam;
> +  *BufferSize = TotalLoadSize;
> +
> +  BuildGuidDataHob (
> +    &gEdkiiMicrocodeShadowInfoHobGuid,
> +    MicrocodeShadowHob,
> +    HobDataLength
> +    );
> +
> +  DEBUG ((
> +    DEBUG_INFO,
> +    "%a: Required microcode patches have been loaded
> at 0x%lx, with size 0x%lx.\n",
> +    __FUNCTION__, *Buffer, *BufferSize
> +    ));
> +
> +  return;
> +}
> +
> +/**
> +  Shadow the required microcode patches data into
> memory according
> +  to FIT microcode entry.
> +
> +**/
> +EFI_STATUS
> +ShadowMicrocodePatchByFit (
> +  IN  UINTN
> CpuIdCount,
> +  IN  EDKII_PEI_MICROCODE_CPU_ID
> *MicrocodeCpuId,
> +  OUT UINTN
> *BufferSize,
> +  OUT VOID                                  **Buffer
> +  )
> +{
> +  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;
> +
> +  FitPointer = *(UINT64 *) (UINTN)
> FIT_POINTER_ADDRESS;  if
> + ((FitPointer == 0) ||
> +      (FitPointer == 0xFFFFFFFFFFFFFFFF) ||
> +      (FitPointer == 0xEEEEEEEEEEEEEEEE)) {

Are these constants defined in the FIT include file?
Would be better if they are #defines from FIT include
file or in this module.

> +    //
> +    // No FIT table.
> +    //
> +    ASSERT (FALSE);

Is it appropriate to ASSERT() here?  Can this be removed?
Would a DEBUG_ERROR message be better?

> +    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);

Is it appropriate to ASSERT() here?  Can this be removed?
Would a DEBUG_ERROR message be better?

> +    return EFI_NOT_FOUND;
> +  }
> +
> +  EntryNum = *(UINT32 *)(&FitEntry[0].Size[0]) &
> 0xFFFFFF;
> +
> +  //
> +  // Calculate microcode entry number
> +  //
> +  MaxPatchNumber = 0;
> +  for (Index = 0; Index < EntryNum; Index++) {
> +    if (FitEntry[Index].Type == FIT_TYPE_01_MICROCODE)
> {
> +      MaxPatchNumber++;
> +    }
> +  }
> +  if (MaxPatchNumber == 0) {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  PatchInfoBuffer = AllocatePool (MaxPatchNumber *
> sizeof
> + (MICROCODE_PATCH_INFO));  if (PatchInfoBuffer ==
> NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  //
> +  // Fill up microcode patch info buffer according to
> FIT table.
> +  //
> +  PatchCount = 0;
> +  TotalLoadSize = 0;
> +  for (Index = 0; Index < EntryNum; Index++) {
> +    if (FitEntry[Index].Type == FIT_TYPE_01_MICROCODE)
> {
> +      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *)
> (UINTN) FitEntry[Index].Address;
> +      TotalSize = (MicrocodeEntryPoint->DataSize == 0)
> ? 2048 : MicrocodeEntryPoint->TotalSize;
> +      if (IsMicrocodePatchNeedLoad (CpuIdCount,
> MicrocodeCpuId, MicrocodeEntryPoint)) {
> +        PatchInfoBuffer[PatchCount].Address     =
> (UINTN) MicrocodeEntryPoint;
> +        PatchInfoBuffer[PatchCount].Size        =
> TotalSize;
> +        TotalLoadSize += TotalSize;
> +        PatchCount++;
> +      }
> +    }
> +  }
> +
> +  if (PatchCount != 0) {
> +    DEBUG ((
> +      DEBUG_INFO,
> +      "%a: 0x%x microcode patches will be loaded into
> memory, with size 0x%x.\n",
> +      __FUNCTION__, PatchCount, TotalLoadSize
> +      ));
> +
> +    ShadowMicrocodePatchWorker (PatchInfoBuffer,
> PatchCount,
> + TotalLoadSize, BufferSize, Buffer);  }
> +
> +  FreePool (PatchInfoBuffer);
> +  return EFI_SUCCESS;
> +}
> +
> +
> +/**
> +  Shadow microcode update patches to memory.
> +
> +  The function is used for shadowing microcode update
> patches to a continuous memory.
> +  It shall allocate memory buffer and only shadow the
> microcode patches
> + for those  processors specified by MicrocodeCpuId
> array. The checksum
> + verification may be  skiped in this function so the
> caller must
> + perform checksum verification before  using the
> microcode patches in returned memory buffer.
> +
> +  @param[in]  This                 The PPI instance
> pointer.
> +  @param[in]  CpuIdCount           Number of elements
> in MicrocodeCpuId array.
> +  @param[in]  MicrocodeCpuId       A pointer to an
> array of EDKII_PEI_MICROCODE_CPU_ID
> +                                   structures.
> +  @param[out] BufferSize           Pointer to receive
> the total size of Buffer.
> +  @param[out] Buffer               Pointer to receive
> address of allocated memory
> +                                   with microcode
> patches data in it.
> +
> +  @retval EFI_SUCCESS              The microcode has
> been shadowed to memory.
> +  @retval EFI_OUT_OF_RESOURCES     The operation fails
> due to lack of resources.
> +
> +**/
> +EFI_STATUS
> +ShadowMicrocode (
> +  IN  EDKII_PEI_SHADOW_MICROCODE_PPI        *This,
> +  IN  UINTN
> CpuIdCount,
> +  IN  EDKII_PEI_MICROCODE_CPU_ID
> *MicrocodeCpuId,
> +  OUT UINTN
> *BufferSize,
> +  OUT VOID                                  **Buffer
> +  )
> +{
> +  if (BufferSize == NULL || Buffer == NULL) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  return ShadowMicrocodePatchByFit (CpuIdCount,
> MicrocodeCpuId,
> +BufferSize, Buffer); }
> +
> +
> +/**
> +  Platform Init PEI module entry point
> +
> +  @param[in]  FileHandle           Not used.
> +  @param[in]  PeiServices          General purpose
> services available to every PEIM.
> +
> +  @retval     EFI_SUCCESS          The function
> completes successfully
> +  @retval     EFI_OUT_OF_RESOURCES Insufficient
> resources to create database
> +**/
> +EFI_STATUS
> +EFIAPI
> +ShadowMicrocodePeimInit (
> +  IN       EFI_PEI_FILE_HANDLE  FileHandle,
> +  IN CONST EFI_PEI_SERVICES     **PeiServices
> +  )
> +{
> +  EFI_STATUS                       Status;
> +
> +  //
> +  // Install EDKII Shadow Microcode PPI  //  Status =
> + PeiServicesInstallPpi(mPeiShadowMicrocodePpiList);
> +  ASSERT_EFI_ERROR (Status);
> +
> +  return Status;
> +}
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode
> /ShadowMicrocodePei.h
> b/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode
> /ShadowMicrocodePei.h
> new file mode 100644
> index 0000000000..04fe7cbfd3
> --- /dev/null
> +++
> b/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode
> /ShadowMicroc
> +++ odePei.h
> @@ -0,0 +1,62 @@
> +/** @file
> +  Source code file for Platform Init PEI module

This description does not match the content

> +
> +Copyright (c) 2020, Intel Corporation. All rights
> reserved.<BR>
> +SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef __SHADOW_MICROCODE_PEI_H__
> +#define __SHADOW_MICROCODE_PEI_H__
> +
> +
> +#include <PiPei.h>
> +#include <Ppi/ShadowMicrocode.h>
> +#include <Library/PeiServicesLib.h>
> +#include <Library/HobLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/MemoryAllocationLib.h> #include
> +<IndustryStandard/FirmwareInterfaceTable.h>
> +#include <Register/Intel/Microcode.h>
> +#include <Register/Intel/Cpuid.h>
> +#include <Guid/MicrocodeShadowInfoHob.h> // // Data
> structure for
> +microcode patch information // typedef struct {
> +  UINTN    Address;
> +  UINTN    Size;
> +} MICROCODE_PATCH_INFO;
> +
> +/**
> +  Shadow microcode update patches to memory.
> +
> +  The function is used for shadowing microcode update
> patches to a continuous memory.
> +  It shall allocate memory buffer and only shadow the
> microcode patches
> + for those  processors specified by MicrocodeCpuId
> array. The checksum
> + verification may be  skiped in this function so the
> caller must
> + perform checksum verification before  using the
> microcode patches in returned memory buffer.
> +
> +  @param[in]  This                 The PPI instance
> pointer.
> +  @param[in]  CpuIdCount           Number of elements
> in MicrocodeCpuId array.
> +  @param[in]  MicrocodeCpuId       A pointer to an
> array of EDKII_PEI_MICROCODE_CPU_ID
> +                                   structures.
> +  @param[out] BufferSize           Pointer to receive
> the total size of Buffer.
> +  @param[out] Buffer               Pointer to receive
> address of allocated memory
> +                                   with microcode
> patches data in it.
> +
> +  @retval EFI_SUCCESS              The microcode has
> been shadowed to memory.
> +  @retval EFI_OUT_OF_RESOURCES     The operation fails
> due to lack of resources.
> +
> +**/
> +EFI_STATUS
> +ShadowMicrocode (
> +  IN  EDKII_PEI_SHADOW_MICROCODE_PPI        *This,
> +  IN  UINTN
> CpuIdCount,
> +  IN  EDKII_PEI_MICROCODE_CPU_ID
> *MicrocodeCpuId,
> +  OUT UINTN
> *BufferSize,
> +  OUT VOID                                  **Buffer
> +  );
> +
> +#endif
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode
> /ShadowMicrocodePei.inf
> b/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode
> /ShadowMicrocodePei.inf
> new file mode 100644
> index 0000000000..b2773998ce
> --- /dev/null
> +++
> b/Silicon/Intel/IntelSiliconPkg/Feature/ShadowMicrocode
> /ShadowMicroc
> +++ odePei.inf
> @@ -0,0 +1,44 @@
> +### @file
> +# Component information file for the Platform Init PEI
> module.

This description does not match the file contents.

> +#
> +# Copyright (c) 2020, Intel Corporation. All rights
> reserved.<BR> # #
> +SPDX-License-Identifier: BSD-2-Clause-Patent # ###
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010017
> +  BASE_NAME                      = ShadowMicrocodePei
> +  FILE_GUID                      = 8af4cf68-ebe4-4b21-
> a008-0cb3da277be5
> +  VERSION_STRING                 = 1.0
> +  MODULE_TYPE                    = PEIM
> +  ENTRY_POINT                    =
> ShadowMicrocodePeimInit
> +
> +[Sources]
> +  ShadowMicrocodePei.c
> +  ShadowMicrocodePei.h
> +
> +[LibraryClasses]
> +  PeimEntryPoint
> +  DebugLib
> +  MemoryAllocationLib
> +  BaseMemoryLib
> +  HobLib
> +  PeiServicesLib
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  UefiCpuPkg/UefiCpuPkg.dec
> +  IntelSiliconPkg/IntelSiliconPkg.dec
> +
> +[Ppis]
> +  gEdkiiPeiShadowMicrocodePpiGuid
> ## PRODUCES
> +
> +[Guids]
> +  gEdkiiMicrocodeShadowInfoHobGuid
> +  gEdkiiMicrocodeStorageTypeFlashGuid
> +
> +[Depex]
> +  TRUE
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/Include/Guid/MicrocodeS
> hadowInfoHob.h
> b/Silicon/Intel/IntelSiliconPkg/Include/Guid/MicrocodeS
> hadowInfoHob.h
> new file mode 100644
> index 0000000000..59a38cee74
> --- /dev/null
> +++
> b/Silicon/Intel/IntelSiliconPkg/Include/Guid/MicrocodeS
> hadowInfoHob.
> +++ h
> @@ -0,0 +1,57 @@
> +/** @file
> +  The definition for VTD PMR Regions Information Hob.

This description does not match the content

> +
> +  Copyright (c) 2019, Intel Corporation. All rights
> reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent **/
> +
> +
> +#ifndef _MICROCODE_SHADOW_INFO_HOB_H_
> +#define _MICROCODE_SHADOW_INFO_HOB_H_
> +
> +///
> +/// The Global ID of a GUIDed HOB used to pass
> microcode shadow info to DXE Driver.
> +///
> +#define EDKII_MICROCODE_SHADOW_INFO_HOB_GUID \
> +  { \
> +    0x658903f9, 0xda66, 0x460d, { 0x8b, 0xb0, 0x9d,
> 0x2d, 0xdf, 0x65,
> +0x44, 0x59 } \
> +  }
> +
> +extern EFI_GUID gEdkiiMicrocodeShadowInfoHobGuid;
> +
> +typedef struct {
> +  //
> +  // Number of the microcode patches which have been
> +  // relocated to memory.
> +  //
> +  UINT64    MicrocodeCount;
> +  //
> +  // An EFI_GUID that defines the contents of
> StorageContext.
> +  //
> +  GUID      StorageType;
> +  //
> +  // An array with MicrocodeCount elements that stores
> +  // the shadowed microcode patch address in memory.
> +  //
> +  UINT64    MicrocodeAddrInMemory[0];

Since this is the last field in the structure visible to the 
C compiler, you can use [] instead of [0] so it is interpreted
by the compiler as a flexible array member. This can make 
code that uses this structure easier to read.

https://en.wikipedia.org/wiki/Flexible_array_member


> +  //
> +  // A buffer which contains details about the storage
> information
> +  // specific to StorageType.
> +  //
> +  // UINT8  StorageContext[];
> +} EDKII_MICROCODE_SHADOW_INFO_HOB;
> +
> +//
> +// An EDKII_MICROCODE_SHADOW_INFO_HOB with StorageType
> set to below
> +GUID will // have the StorageContext of an array with
> MicrocodeCount of
> +UINT64 elements // that stores the original microcode
> patch address on
> +flash. This address is // placed in same order as the
> microcode patches in MicrocodeAddrInMemory.
> +//
> +#define EFI_MICROCODE_STORAGE_TPYE_FLASH_GUID \

Typo.  "TPYE" should be "TYPE".


Should a data structure be added that is associated with
EFI_MICROCODE_STORAGE_TYPE_FLASH_GUID so it can be used
to interpret StorageContext field when StorageType matches
this GUID value?  This way, the text in the comments is
not required to know the layout of StorageContext.

  typedef struct {
    UINT64  MicrocodeAddressInFlash[];
  } EFI_MICROCODE_STORAGE_TYPE_FLASH_CONTEXT;

In order to make everything aligned better should the
StorageGuid be listed before MicrocodeCount, so the order
is from largest to smallest.  This also guarantees that
StorageContext is aligned on an 8-byte boundary which is
compatible with a typecast to a StorageType specific structure.

> +  { \
> +    0x2cba01b3, 0xd391, 0x4598, { 0x8d, 0x89, 0xb7,
> 0xfc, 0x39, 0x22,
> +0xfd, 0x71 } \
> +  }
> +
> +extern EFI_GUID gEdkiiMicrocodeStorageTypeFlashGuid;
> +
> +#endif
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
> b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
> index 22ebf19c4e..2d8e40f0b8 100644
> --- a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
> +++ b/Silicon/Intel/IntelSiliconPkg/ IntelSiliconPkg.dec
> @@ -48,6 +48,12 @@

Header of IntelSiliconPkg.dec needs to have copyright updated
to 2020.

>    ## HOB GUID to get memory information after MRC is
> done. The hob data will be used to set the PMR ranges
>    gVtdPmrInfoDataHobGuid = {0x6fb61645, 0xf168,
> 0x46be, { 0x80, 0xec, 0xb5, 0x02, 0x38, 0x5e, 0xe7,
> 0xe7 } }
> 
> +  ## Include/Guid/MicrocodeShadowInfoHob.h
> +  gEdkiiMicrocodeShadowInfoHobGuid = { 0x658903f9,
> 0xda66, 0x460d, {
> + 0x8b, 0xb0, 0x9d, 0x2d, 0xdf, 0x65, 0x44, 0x59 } }
> +
> +  ## Include/Guid/MicrocodeShadowInfoHob.h
> +  gEdkiiMicrocodeStorageTypeFlashGuid = { 0x2cba01b3,
> 0xd391, 0x4598, {
> + 0x8d, 0x89, 0xb7, 0xfc, 0x39, 0x22, 0xfd, 0x71 } }
> +
>  [Ppis]
>    gEdkiiVTdInfoPpiGuid = { 0x8a59fcb3, 0xf191, 0x400c,
> { 0x97, 0x67, 0x67, 0xaf, 0x2b, 0x25, 0x68, 0x4a } }
> 
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
> b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
> index 0a6509d8b3..f995883691 100644
> --- a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
> +++ b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
> @@ -1,7 +1,7 @@
>  ## @file
>  # This package provides common open source Intel
> silicon modules.
>  #
> -# Copyright (c) 2017 - 2019, Intel Corporation. All
> rights reserved.<BR>
> +# Copyright (c) 2017 - 2020, Intel Corporation. All
> rights
> +reserved.<BR>
>  #
>  #    SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -84,6 +84,7 @@
> 
> IntelSiliconPkg/Feature/VTd/PlatformVTdInfoSamplePei/Pl
> atformVTdInfoSamplePei.inf
> 
> IntelSiliconPkg/Feature/Capsule/MicrocodeUpdateDxe/Micr
> ocodeUpdateDxe.inf
> 
> IntelSiliconPkg/Feature/Capsule/Library/MicrocodeFlashA
> ccessLibNull/MicrocodeFlashAccessLibNull.inf
> +
> IntelSiliconPkg/Feature/ShadowMicrocode/ShadowMicrocode
> Pei.inf
> 
> IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/PeiFirmwa
> reBootMediaLib.inf
> 
> IntelSiliconPkg/Library/PeiDxeSmmBootMediaLib/DxeSmmFir
> mwareBootMediaLib.inf
> 
> --
> 2.19.1.windows.1
> 
> 
>