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

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

Hi, Mike

The FITPointer points to the FIT table address on flash (within top 16MB 
of 4GB). It's not a memory address and normally it's always large than
the MemoryTop address in PHIT. So we couldn't use PHIT HOB to check
the FIT pointer.



Best Regards
Siyuan 

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: 2020年2月15日 7:07
> To: Fu, Siyuan <siyuan.fu@intel.com>; devel@edk2.groups.io; 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,
> 
> If the FIT is not valid, then the API should just return
> an error without ASSERT().  Not all system support FIT or
> fill in FIT.  The code is more generic if it just does
> the check and returns an error.

This is an optional PEIM and only these systems which
support FIT should use it. Other platforms should not
include this PEIM so MpInitLib will still use the PCD
based microcode shadow as usual.
But it's ok to me if you think it's necessary to remove
these ASSERT so any platform can include this PEIM
without additional concern. I will update patch for this.

> 
> The check looks incomplete to me.  We know that max physical
> address of the CPU from the PHIT HOB.  If the physical address
> value is greater than the max physical address, then the
> pointer is invalid.  This would cover the 2 point cases of
> all Fs and all Es.

The FITPointer points to the FIT table address on flash (within top
16MB of 4GB). It's not a memory address and normally it's always
larger than the MemoryTop address in PHIT. So we couldn't use
PHIT HOB to check the FIT pointer.

Best Regards,
Siyuan

> 
> Mike
> 
> > -----Original Message-----
> > From: Fu, Siyuan <siyuan.fu@intel.com>
> > Sent: Thursday, February 13, 2020 5:33 PM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>;
> > devel@edk2.groups.io
> > 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.
> >
> > Hi Mike
> >
> > See my reply for the ASSERT and magic number around FIT
> > table parsing code.
> >
> > > -----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
> > Trim long patch content.
> >
> > > > +
> > > > +**/
> > > > +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.
> >
> > These values are not defined in FIT include file or FIT
> > specification.
> > The only way to identify if FIT table is exist in FIT
> > spec is the _FIT_
> > signature, which defined in FIT header file as
> > FIT_TYPE_00_SIGNATURE and check below.
> >
> > This if check is copied from the
> > InitializeFitMicrocodeInfo() function
> > in
> > Silicon\Intel\IntelSiliconPkg\Feature\Capsule\Microcode
> > UpdateDxe\MicrocodeFmp.c.
> > I think it just assumes the default value of flash
> > content is 0xFF
> > or 0xEE and check that.
> >
> > This is also why I use ASSERT if the flash content
> > doesn't seems
> > like a valid FIT table in below if checks. FIT boot is
> > critical to
> > processor microcode load and BIOS RTU setup. And
> > including
> > this PEIM into the platform means the platform owner
> > want to
> > use FIT based boot and microcode loading. These ASSERTs
> > would
> > be helpful to let them if the FIT table content is
> > invalid in a DEBUG
> > version BIOS image.
> >
> > >
> > > > +    //
> > > > +    // 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
> > > >
> > > >
> > > >