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

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: Ni, Ray
> Sent: Tuesday, December 24, 2019 11:54 AM
> To: Wu, Hao A; devel@edk2.groups.io
> Cc: Dong, Eric; Laszlo Ersek; Zeng, Star; Fu, Siyuan; Kinney, Michael D
> Subject: RE: [PATCH v1 4/4] UefiCpuPkg/MpInitLib: Produce EDKII microcode
> patch HOB
> 
> MicrocodeDetect() is called in DXE phase as well.
> But it doesn't use the new MicrocodePatch HOB to speed up the
> microcode detection. Why?


I found that within function MicrocodeDetect(), it has logic to check whether
a microcode patch has already been applied on a processor previously:

  if (CurrentRevision != 0 && !IsBspCallIn) {
    //
    // Skip loading microcode if it has been loaded successfully
    //
    return;
  }

My take is that if the microcode patch detection and application have been done
once in the PEI phase, the above check can ensure such process will not be
repeated again in the DXE phase.

Best Regards,
Hao Wu


> 
> > -----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