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

["Siyuan, Fu" <siyuan.fu@intel.com>]

Hi, Mike

Thanks for your comments, I will update patch accordingly and send a V4 for this.

Best Regards
Siyuan 

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: 2020年2月13日 8:58
> To: devel@edk2.groups.io; Fu, Siyuan <siyuan.fu@intel.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>
> Subject: RE: [edk2-devel] [PATCH v3] IntelSiliconPkg: FIT based shadow
> microcode PPI support.
> 
> 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
> >
> >
> >