Re: [PATCH v1 4/4] UefiCpuPkg/MpInitLib: Produce EDKII microcode patch HOB

["Ni, Ray" <ray.ni@intel.com>]

MicrocodeDetect() is called in DXE phase as well.
But it doesn't use the new MicrocodePatch HOB to speed up the
microcode detection. Why?

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Tuesday, December 24, 2019 9:37 AM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Zeng, Star
> <star.zeng@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>
> Subject: [PATCH v1 4/4] UefiCpuPkg/MpInitLib: Produce EDKII microcode
> patch HOB
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2430
> 
> This commit will update the MpInitLib to:
> 
> A. Collect the base address and size information after microcode patches
>    being loaded into memory;
> B. Collect the applied microcode patch for each processor within system;
> C. Based on the collected information, produce the EDKII microcode patch
>    HOB.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |  1 +
>  UefiCpuPkg/Library/MpInitLib/MpLib.h          | 24 +++++++--
>  UefiCpuPkg/Library/MpInitLib/Microcode.c      | 16 ++++--
>  UefiCpuPkg/Library/MpInitLib/MpLib.c          |  8 ++-
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c       | 55 ++++++++++++++++++++
>  5 files changed, 96 insertions(+), 8 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> index 1538185ef9..326703cc9a 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> @@ -63,3 +63,4 @@ [Pcd]
> 
>  [Guids]
>    gEdkiiS3SmmInitDoneGuid
> +  gEdkiiMicrocodePatchHobGuid
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 56b0df664a..fb251d7aef 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -138,6 +138,7 @@ typedef struct {
>    EFI_EVENT                      WaitEvent;
>    UINT32                         ProcessorSignature;
>    UINT8                          PlatformId;
> +  UINT64                         MicrocodeData;
>  } CPU_AP_DATA;
> 
>  //
> @@ -580,13 +581,15 @@ CheckAndUpdateApsStatus (
>  /**
>    Detect whether specified processor can find matching microcode patch and
> load it.
> 
> -  @param[in]  CpuMpData    The pointer to CPU MP Data structure.
> -  @param[in]  IsBspCallIn  Indicate whether the caller is BSP or not.
> +  @param[in]  CpuMpData        The pointer to CPU MP Data structure.
> +  @param[in]  ProcessorNumber  The handle number of the processor. The
> range is
> +                               from 0 to the total number of logical processors
> +                               minus 1.
>  **/
>  VOID
>  MicrocodeDetect (
>    IN CPU_MP_DATA             *CpuMpData,
> -  IN BOOLEAN                 IsBspCallIn
> +  IN UINTN                   ProcessorNumber
>    );
> 
>  /**
> @@ -619,5 +622,20 @@ EnableDebugAgent (
>    VOID
>    );
> 
> +/**
> +  Find the current Processor number by APIC ID.
> +
> +  @param[in]  CpuMpData         Pointer to PEI CPU MP Data
> +  @param[out] ProcessorNumber   Return the pocessor number found
> +
> +  @retval EFI_SUCCESS          ProcessorNumber is found and returned.
> +  @retval EFI_NOT_FOUND        ProcessorNumber is not found.
> +**/
> +EFI_STATUS
> +GetProcessorNumber (
> +  IN CPU_MP_DATA               *CpuMpData,
> +  OUT UINTN                    *ProcessorNumber
> +  );
> +
>  #endif
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> index 68088b26a5..bbc40f81bf 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> @@ -65,13 +65,15 @@ GetCurrentMicrocodeSignature (
>           It does not guarantee that the data has not been modified.
>           CPU has its own mechanism to verify Microcode Binary part.
> 
> -  @param[in]  CpuMpData    The pointer to CPU MP Data structure.
> -  @param[in]  IsBspCallIn  Indicate whether the caller is BSP or not.
> +  @param[in]  CpuMpData        The pointer to CPU MP Data structure.
> +  @param[in]  ProcessorNumber  The handle number of the processor. The
> range is
> +                               from 0 to the total number of logical processors
> +                               minus 1.
>  **/
>  VOID
>  MicrocodeDetect (
>    IN CPU_MP_DATA             *CpuMpData,
> -  IN BOOLEAN                 IsBspCallIn
> +  IN UINTN                   ProcessorNumber
>    )
>  {
>    UINT32                                  ExtendedTableLength;
> @@ -93,6 +95,7 @@ MicrocodeDetect (
>    MSR_IA32_PLATFORM_ID_REGISTER           PlatformIdMsr;
>    UINT32                                  ProcessorFlags;
>    UINT32                                  ThreadId;
> +  BOOLEAN                                 IsBspCallIn;
> 
>    //
>    // set ProcessorFlags to suppress incorrect compiler/analyzer warnings
> @@ -107,6 +110,7 @@ MicrocodeDetect (
>    }
> 
>    CurrentRevision = GetCurrentMicrocodeSignature ();
> +  IsBspCallIn     = (ProcessorNumber == (UINTN)CpuMpData->BspNumber) ?
> TRUE : FALSE;
>    if (CurrentRevision != 0 && !IsBspCallIn) {
>      //
>      // Skip loading microcode if it has been loaded successfully
> @@ -316,6 +320,12 @@ Done:
>        DEBUG ((EFI_D_ERROR, "Updated microcode signature [0x%08x] does
> not match \
>                  loaded microcode signature [0x%08x]\n", CurrentRevision,
> LatestRevision));
>        ReleaseSpinLock(&CpuMpData->MpLock);
> +    } else {
> +      //
> +      // Save the detected microcode patch address for each processor.
> +      // It will be used when building the microcode patch cache HOB.
> +      //
> +      CpuMpData->CpuData[ProcessorNumber].MicrocodeData = (UINTN)
> MicrocodeData;
>      }
>    }
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 199468156b..8f4b2b1973 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -399,12 +399,16 @@ ApInitializeSync (
>    )
>  {
>    CPU_MP_DATA  *CpuMpData;
> +  UINTN        ProcessorNumber;
> +  EFI_STATUS   Status;
> 
>    CpuMpData = (CPU_MP_DATA *) Buffer;
> +  Status = GetProcessorNumber (CpuMpData, &ProcessorNumber);
> +  ASSERT_EFI_ERROR (Status);
>    //
>    // Load microcode on AP
>    //
> -  MicrocodeDetect (CpuMpData, FALSE);
> +  MicrocodeDetect (CpuMpData, ProcessorNumber);
>    //
>    // Sync BSP's MTRR table to AP
>    //
> @@ -1765,7 +1769,7 @@ MpInitLibInitialize (
>    //
>    // Detect and apply Microcode on BSP
>    //
> -  MicrocodeDetect (CpuMpData, TRUE);
> +  MicrocodeDetect (CpuMpData, CpuMpData->BspNumber);
> 
>    //
>    // Wakeup APs to do some AP initialize sync (Microcode & MTRR)
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index 3999603c3e..977818fda4 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> @@ -9,6 +9,7 @@
>  #include "MpLib.h"
>  #include <Library/PeiServicesLib.h>
>  #include <Guid/S3SmmInitDone.h>
> +#include <Guid/MicrocodePatchHob.h>
> 
>  /**
>    S3 SMM Init Done notification function.
> @@ -291,6 +292,59 @@ CheckAndUpdateApsStatus (
>  }
> 
>  /**
> +  Build the microcode patch HOB that contains the base address and size of
> the
> +  microcode patch stored in the memory.
> +
> +  @param[in]  CpuMpData    Pointer to the CPU_MP_DATA structure.
> +
> +**/
> +VOID
> +BuildMicrocodeCacheHob (
> +  IN CPU_MP_DATA    *CpuMpData
> +  )
> +{
> +  EDKII_MICROCODE_PATCH_HOB    *MicrocodeHob;
> +  UINTN                        HobDataLength;
> +  UINT32                       Index;
> +
> +  HobDataLength = sizeof (EDKII_MICROCODE_PATCH_HOB) +
> +                  sizeof (UINT64) * CpuMpData->CpuCount;
> +
> +  MicrocodeHob  = AllocatePool (HobDataLength);
> +  if (MicrocodeHob == NULL) {
> +    ASSERT (FALSE);
> +    return;
> +  }
> +
> +  //
> +  // Store the information of the memory region that holds the microcode
> patches.
> +  //
> +  MicrocodeHob->MicrocodePatchAddress    = CpuMpData-
> >MicrocodePatchAddress;
> +  MicrocodeHob->MicrocodePatchRegionSize = CpuMpData-
> >MicrocodePatchRegionSize;
> +
> +  //
> +  // Store the detected microcode patch for each processor as well.
> +  //
> +  MicrocodeHob->ProcessorNumber = CpuMpData->CpuCount;
> +  for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> +    if (CpuMpData->CpuData[Index].MicrocodeData != 0) {
> +      MicrocodeHob->DetectedPatchOffset[Index] = CpuMpData-
> >CpuData[Index].MicrocodeData -
> +                                                 CpuMpData->MicrocodePatchAddress;
> +    } else {
> +      MicrocodeHob->DetectedPatchOffset[Index] = MAX_UINT64;
> +    }
> +  }
> +
> +  BuildGuidDataHob (
> +    &gEdkiiMicrocodePatchHobGuid,
> +    MicrocodeHob,
> +    HobDataLength
> +    );
> +
> +  return;
> +}
> +
> +/**
>    Initialize global data for MP support.
> 
>    @param[in] CpuMpData  The pointer to CPU MP Data structure.
> @@ -303,6 +357,7 @@ InitMpGlobalData (
>    EFI_STATUS  Status;
> 
>    SaveCpuMpData (CpuMpData);
> +  BuildMicrocodeCacheHob (CpuMpData);
> 
>    ///
>    /// Install Notify
> --
> 2.12.0.windows.1