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

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

Hi, Mike

I have sent V5 patch which removes the ASSERT and check the FIT pointer
against valid firmware address range (4G-16MB to 4G-0x40). Please help
to review it. Thanks.

Best Regards
Siyuan 

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: 2020年2月17日 4:35
> 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.
> 
> Then the checks against 0xFFFFFFFFFFFFFFFF and 0xEEEEEEEEEEEEEEEE
> should be removed.
> 
> If those checks are important, then replace with checks against
> max physical address.  Or if it is guaranteed to be below 4GB,
> then check against that value with a clear comment for the
> checks being performed.
> 
> Mike
> 
> > -----Original Message-----
> > From: Fu, Siyuan <siyuan.fu@intel.com>
> > Sent: Saturday, February 15, 2020 7:36 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
> >
> > 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
> > > > > >
> > > > > >
> > > > > >