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

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

> -----Original Message-----
> From: Dong, Eric
> Sent: Monday, December 30, 2019 2:18 PM
> To: Wu, Hao A; devel@edk2.groups.io
> Cc: Ni, Ray; Laszlo Ersek; Zeng, Star; Fu, Siyuan; Kinney, Michael D
> Subject: RE: [PATCH v4 4/6] UefiCpuPkg/MpInitLib: Produce EDKII microcode
> patch HOB
> 
> 
> 
> > -----Original Message-----
> > From: Wu, Hao A
> > Sent: Friday, December 27, 2019 3:32 PM
> > 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 v4 4/6] 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 detected 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      | 20 +++++--
> >  UefiCpuPkg/Library/MpInitLib/MpLib.c          |  8 ++-
> >  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c       | 55 ++++++++++++++++++++
> >  5 files changed, 100 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..885656900c 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                         MicrocodeEntryAddr;
> >  } 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 62a104742a..74a34c48fa 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 @@ -319,6
> > +323,16 @@ Done:
> >      }
> >    }
> >
> > +  if (LatestRevision != 0) {
> > +    //
> > +    // Save the detected microcode patch entry address (including the
> > +    // microcode patch header) for each processor.
> > +    // It will be used when building the microcode patch cache HOB.
> > +    //
> > +    CpuMpData->CpuData[ProcessorNumber].MicrocodeEntryAddr =
> > +      (UINTN) MicrocodeData -  sizeof (CPU_MICROCODE_HEADER);  }
> > +
> 
>  [[Eric]] Not an code error but for easy to understand the logic, propose to
> move above section code to right after "Done:".


Agree.
I will move this chunk to the suggested location.

Best Regards,
Hao Wu


> 
> Thanks,
> Eric
> 
> >    if (IsBspCallIn && (LatestRevision != 0)) {
> >      //
> >      // Save BSP processor info and microcode info for later AP use.
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > index c72bf3c9ee..e611a8ca40 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
> >    //
> > @@ -1761,7 +1765,7 @@ MpInitLibInitialize (
> >    //
> >    // Detect and apply Microcode on BSP
> >    //
> > -  MicrocodeDetect (CpuMpData, TRUE);
> > +  MicrocodeDetect (CpuMpData, CpuMpData->BspNumber);
> >    //
> >    // Store BSP's MTRR setting
> >    //
> > diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> > index 3999603c3e..06e3f5d0d3 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->ProcessorCount = CpuMpData->CpuCount;  for (Index = 0;
> > + Index < CpuMpData->CpuCount; Index++) {
> > +    if (CpuMpData->CpuData[Index].MicrocodeEntryAddr != 0) {
> > +      MicrocodeHob->ProcessorSpecificPatchOffset[Index] =
> > +        CpuMpData->CpuData[Index].MicrocodeEntryAddr - CpuMpData-
> > >MicrocodePatchAddress;
> > +    } else {
> > +      MicrocodeHob->ProcessorSpecificPatchOffset[Index] = MAX_UINT64;
> > +    }
> > +  }
> > +
> > +  BuildGuidDataHob (
> > +    &gEdkiiMicrocodePatchHobGuid,
> > +    MicrocodeHob,
> > +    HobDataLength
> > +    );
> > +
> > +  return;
> > +}
> > +
> > +/**
> >    Initialize global data for MP support.
> >
> >    @param[in] CpuMpData  The pointer to CPU MP Data structure.
> > @@ -302,6 +356,7 @@ InitMpGlobalData (
> >  {
> >    EFI_STATUS  Status;
> >
> > +  BuildMicrocodeCacheHob (CpuMpData);
> >    SaveCpuMpData (CpuMpData);
> >
> >    ///
> > --
> > 2.12.0.windows.1