Re: [Patch] UefiCpuPkg: Add microcode flash address to EDKII microcode patch HOB.

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

On 01/10/20 08:08, Siyuan Fu wrote:
> This patch adds the original microcode flash address to EDKII microcode
> patch HOB after the microcode loaded by processor. This information can
> be used to check if a microcode slot has been used by existing processor
> or not.
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2454
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Siyuan Fu <siyuan.fu@intel.com>
> ---
>  UefiCpuPkg/Include/Guid/MicrocodePatchHob.h | 12 ++++-
>  UefiCpuPkg/Library/MpInitLib/Microcode.c    | 24 +++++----
>  UefiCpuPkg/Library/MpInitLib/MpLib.h        |  9 ++++
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c     | 54 +++++++++++++++++++--
>  4 files changed, 84 insertions(+), 15 deletions(-)

I'll let Eric and Ray review this.

Thanks!
Laszlo

> diff --git a/UefiCpuPkg/Include/Guid/MicrocodePatchHob.h b/UefiCpuPkg/Include/Guid/MicrocodePatchHob.h
> index 2d307fbffb..bbf6f2c66b 100644
> --- a/UefiCpuPkg/Include/Guid/MicrocodePatchHob.h
> +++ b/UefiCpuPkg/Include/Guid/MicrocodePatchHob.h
> @@ -3,7 +3,7 @@
>      A. Base address and size of the loaded microcode patches data;
>      B. Detected microcode patch for each processor within system.
>  
> -  Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2019 - 2020, Intel Corporation. All rights reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  **/
> @@ -38,7 +38,15 @@ typedef struct {
>    // If no microcode patch is detected for certain processor, the relating
>    // element will be set to MAX_UINT64.
>    //
> -  UINT64    ProcessorSpecificPatchOffset[0];
> +  //UINT64    ProcessorSpecificPatchOffset[];
> +  //
> +  // An array with 'ProcessorCount' elements that stores the original
> +  // microcode patch address in flash if the patch has been shadowed to
> +  // memory. This address will be the same one as specified by
> +  // "MicrocodePatchAddress" with "PatchOffset" if the patch wasn't
> +  // shadowed to memory.
> +  //
> +  //UINT64    ProcessorSpecificPatchAddrInRom[];
>  } EDKII_MICROCODE_PATCH_HOB;
>  
>  #endif
> diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> index 9389e52ae5..3af5b8495f 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> @@ -460,6 +460,7 @@ ShadowMicrocodePatchWorker (
>        (VOID *) Patches[Index].Address,
>        Patches[Index].Size
>        );
> +    Patches[Index].AddressInRam = (UINTN) Walker;
>      Walker += Patches[Index].Size;
>    }
>  
> @@ -519,7 +520,7 @@ ShadowMicrocodePatchByPcd (
>    PatchCount      = 0;
>    MaxPatchNumber  = DEFAULT_MAX_MICROCODE_PATCH_NUM;
>    TotalLoadSize   = 0;
> -  PatchInfoBuffer = AllocatePool (MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO));
> +  PatchInfoBuffer = AllocateZeroPool (MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO));
>    if (PatchInfoBuffer == NULL) {
>      return;
>    }
> @@ -563,7 +564,7 @@ ShadowMicrocodePatchByPcd (
>            //
>            // Overflow check for MaxPatchNumber
>            //
> -          goto OnExit;
> +          goto OnError;
>          }
>  
>          PatchInfoBuffer = ReallocatePool (
> @@ -572,7 +573,7 @@ ShadowMicrocodePatchByPcd (
>                              PatchInfoBuffer
>                              );
>          if (PatchInfoBuffer == NULL) {
> -          goto OnExit;
> +          goto OnError;
>          }
>          MaxPatchNumber = MaxPatchNumber * 2;
>        }
> @@ -581,7 +582,7 @@ ShadowMicrocodePatchByPcd (
>        // Store the information of this microcode patch
>        //
>        PatchInfoBuffer[PatchCount - 1].Address = (UINTN) MicrocodeEntryPoint;
> -      PatchInfoBuffer[PatchCount - 1].Size    = TotalSize;
> +      PatchInfoBuffer[PatchCount - 1].Size            = TotalSize;
>        TotalLoadSize += TotalSize;
>      }
>  
> @@ -601,7 +602,11 @@ ShadowMicrocodePatchByPcd (
>      ShadowMicrocodePatchWorker (CpuMpData, PatchInfoBuffer, PatchCount, TotalLoadSize);
>    }
>  
> -OnExit:
> +  CpuMpData->PatchCount = PatchCount;
> +  CpuMpData->PatchInfoBuffer = PatchInfoBuffer;
> +  return;
> +
> +OnError:
>    if (PatchInfoBuffer != NULL) {
>      FreePool (PatchInfoBuffer);
>    }
> @@ -674,7 +679,7 @@ ShadowMicrocodePatchByFit (
>      return EFI_NOT_FOUND;
>    }
>  
> -  PatchInfoBuffer = AllocatePool (MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO));
> +  PatchInfoBuffer = AllocateZeroPool (MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO));
>    if (PatchInfoBuffer == NULL) {
>      return EFI_OUT_OF_RESOURCES;
>    }
> @@ -689,8 +694,8 @@ ShadowMicrocodePatchByFit (
>        MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (UINTN) FitEntry[Index].Address;
>        TotalSize = (MicrocodeEntryPoint->DataSize == 0) ? 2048 : MicrocodeEntryPoint->TotalSize;
>        if (IsMicrocodePatchNeedLoad (CpuMpData, MicrocodeEntryPoint)) {
> -        PatchInfoBuffer[PatchCount].Address     = (UINTN) MicrocodeEntryPoint;
> -        PatchInfoBuffer[PatchCount].Size        = TotalSize;
> +        PatchInfoBuffer[PatchCount].Address = (UINTN) MicrocodeEntryPoint;
> +        PatchInfoBuffer[PatchCount].Size            = TotalSize;
>          TotalLoadSize += TotalSize;
>          PatchCount++;
>        }
> @@ -707,7 +712,8 @@ ShadowMicrocodePatchByFit (
>      ShadowMicrocodePatchWorker (CpuMpData, PatchInfoBuffer, PatchCount, TotalLoadSize);
>    }
>  
> -  FreePool (PatchInfoBuffer);
> +  CpuMpData->PatchCount = PatchCount;
> +  CpuMpData->PatchInfoBuffer = PatchInfoBuffer;
>    return EFI_SUCCESS;
>  }
>  
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 7c62d75acc..51c71ad38e 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -29,6 +29,8 @@
>  #include <Library/MtrrLib.h>
>  #include <Library/HobLib.h>
>  
> +#include <Guid/MicrocodePatchHob.h>
> +
>  #include <IndustryStandard/FirmwareInterfaceTable.h>
>  
>  
> @@ -56,6 +58,7 @@
>  //
>  typedef struct {
>    UINTN    Address;
> +  UINTN    AddressInRam;
>    UINTN    Size;
>  } MICROCODE_PATCH_INFO;
>  
> @@ -273,6 +276,12 @@ struct _CPU_MP_DATA {
>    // driver.
>    //
>    BOOLEAN                        WakeUpByInitSipiSipi;
> +
> +  //
> +  // Shadow infomation of the microcode patch data.
> +  //
> +  UINTN                          PatchCount;
> +  MICROCODE_PATCH_INFO           *PatchInfoBuffer;
>  };
>  
>  extern EFI_GUID mCpuInitMpLibHobGuid;
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index 06e3f5d0d3..07fc05124c 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> @@ -1,7 +1,7 @@
>  /** @file
>    MP initialize support functions for PEI phase.
>  
> -  Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.<BR>
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
>  **/
> @@ -291,6 +291,40 @@ CheckAndUpdateApsStatus (
>  {
>  }
>  
> +/**
> +  Find the corresponding microcode patch address in ROM before the shadow
> +  operation.
> +
> +  @param[in]  CpuMpData             Pointer to the CPU_MP_DATA structure.
> +  @param[in]  MicrocodeEntryAddr    Loaded microcode address to be checked.
> +
> +  @return    Microcode address in ROM, or MAX_UINT64 if this microcode was
> +             not been shadowed.
> +**/
> +UINT64
> +RomAddrOfShadowedMicrocode (
> +  IN CPU_MP_DATA    *CpuMpData,
> +  IN UINT64         MicrocodeEntryAddr
> +  )
> +{
> +  UINT32                       Index;
> +
> +  if (CpuMpData->PatchInfoBuffer == NULL) {
> +    //
> +    // No shadow.
> +    //
> +    return MicrocodeEntryAddr;
> +  }
> +
> +  for (Index = 0; Index < CpuMpData->PatchCount; Index++) {
> +    if (CpuMpData->PatchInfoBuffer[Index].AddressInRam == MicrocodeEntryAddr) {
> +      return CpuMpData->PatchInfoBuffer[Index].Address;
> +    }
> +  }
> +
> +  return MicrocodeEntryAddr;
> +}
> +
>  /**
>    Build the microcode patch HOB that contains the base address and size of the
>    microcode patch stored in the memory.
> @@ -306,8 +340,11 @@ BuildMicrocodeCacheHob (
>    EDKII_MICROCODE_PATCH_HOB    *MicrocodeHob;
>    UINTN                        HobDataLength;
>    UINT32                       Index;
> +  UINT64                       *ProcessorSpecificPatchOffset;
> +  UINT64                       *ProcessorSpecificPatchAddrInRom;
>  
>    HobDataLength = sizeof (EDKII_MICROCODE_PATCH_HOB) +
> +                  sizeof (UINT64) * CpuMpData->CpuCount +
>                    sizeof (UINT64) * CpuMpData->CpuCount;
>  
>    MicrocodeHob  = AllocatePool (HobDataLength);
> @@ -317,10 +354,14 @@ BuildMicrocodeCacheHob (
>    }
>  
>    //
> -  // Store the information of the memory region that holds the microcode patches.
> +  // Store information of the memory region that holds the microcode patches.
>    //
>    MicrocodeHob->MicrocodePatchAddress    = CpuMpData->MicrocodePatchAddress;
>    MicrocodeHob->MicrocodePatchRegionSize = CpuMpData->MicrocodePatchRegionSize;
> +  ProcessorSpecificPatchOffset =
> +    (UINT64*) ((UINTN )MicrocodeHob + sizeof (EDKII_MICROCODE_PATCH_HOB));
> +  ProcessorSpecificPatchAddrInRom =
> +    (UINT64*) ((UINTN )ProcessorSpecificPatchOffset + sizeof (UINT64) * CpuMpData->CpuCount);
>  
>    //
>    // Store the detected microcode patch for each processor as well.
> @@ -328,10 +369,15 @@ BuildMicrocodeCacheHob (
>    MicrocodeHob->ProcessorCount = CpuMpData->CpuCount;
>    for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
>      if (CpuMpData->CpuData[Index].MicrocodeEntryAddr != 0) {
> -      MicrocodeHob->ProcessorSpecificPatchOffset[Index] =
> +      ProcessorSpecificPatchOffset[Index] =
>          CpuMpData->CpuData[Index].MicrocodeEntryAddr - CpuMpData->MicrocodePatchAddress;
> +      ProcessorSpecificPatchAddrInRom[Index] = RomAddrOfShadowedMicrocode (
> +                                                 CpuMpData,
> +                                                 CpuMpData->CpuData[Index].MicrocodeEntryAddr
> +                                                 );
>      } else {
> -      MicrocodeHob->ProcessorSpecificPatchOffset[Index] = MAX_UINT64;
> +      ProcessorSpecificPatchOffset[Index]    = MAX_UINT64;
> +      ProcessorSpecificPatchAddrInRom[Index] = MAX_UINT64;
>      }
>    }
>  
>