Re: [Patch 2/3] UefiCpuPkg/MpInitLib: Use BSP uCode for APs if possible.

["Dong, Eric" <eric.dong@intel.com>]

Hi Laszlo,

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, July 12, 2018 5:42 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: Re: [edk2] [Patch 2/3] UefiCpuPkg/MpInitLib: Use BSP uCode for APs
> if possible.
> 
> On 07/11/18 13:07, Eric Dong wrote:
> > Search uCode costs much time, if AP has same processor type with BSP,
> > AP can use BSP saved uCode info to get better performance.
> >
> > This change enables this solution.
> >
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> >  UefiCpuPkg/Library/MpInitLib/Microcode.c | 34
> +++++++++++++++++++++++++++++---
> >  UefiCpuPkg/Library/MpInitLib/MpLib.c     |  4 ++--
> >  UefiCpuPkg/Library/MpInitLib/MpLib.h     | 11 +++++++++--
> >  3 files changed, 42 insertions(+), 7 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > index e47f9f4f8f..351975e2a2 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > @@ -35,11 +35,13 @@ GetCurrentMicrocodeSignature (
> >  /**
> >    Detect whether specified processor can find matching microcode patch
> and load it.
> >
> > -  @param[in]  CpuMpData  The pointer to CPU MP Data structure.
> > +  @param[in]  CpuMpData    The pointer to CPU MP Data structure.
> > +  @param[in]  IsBspCallIn  Indicate whether the caller is BSP or not.
> >  **/
> >  VOID
> >  MicrocodeDetect (
> > -  IN CPU_MP_DATA             *CpuMpData
> > +  IN CPU_MP_DATA             *CpuMpData,
> > +  IN BOOLEAN                 IsBspCallIn
> >    )
> >  {
> >    UINT32                                  ExtendedTableLength;
> > @@ -58,6 +60,7 @@ MicrocodeDetect (
> >    BOOLEAN                                 CorrectMicrocode;
> >    VOID                                    *MicrocodeData;
> >    MSR_IA32_PLATFORM_ID_REGISTER           PlatformIdMsr;
> > +  UINT32                                  ProcessorFlags;
> >
> >    if (CpuMpData->MicrocodePatchRegionSize == 0) {
> >      //
> > @@ -67,7 +70,7 @@ MicrocodeDetect (
> >    }
> >
> >    CurrentRevision = GetCurrentMicrocodeSignature ();
> > -  if (CurrentRevision != 0) {
> > +  if (CurrentRevision != 0 && !IsBspCallIn) {
> >      //
> >      // Skip loading microcode if it has been loaded successfully
> >      //
> > @@ -87,6 +90,19 @@ MicrocodeDetect (
> >    PlatformIdMsr.Uint64 = AsmReadMsr64 (MSR_IA32_PLATFORM_ID);
> >    PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;
> >
> > +  //
> > +  // Check whether AP has same processor with BSP.
> > +  // If yes, direct use microcode info saved by BSP.
> > +  //
> > +  if (!IsBspCallIn) {
> > +    if ((CpuMpData->ProcessorSignature == Eax.Uint32) &&
> > +        (CpuMpData->ProcessorFlags & (1 << PlatformId)) != 0) {
> 
> Here I have only one comment. (The reason for that is that, on OVMF,
> MicrocodePatchRegionSize is zero, so MicrocodeDetect() will exit
> immediately, on both the APs and the BSP.)
> 
> My comment is that the expression
> 
>   (1 << PlatformId)
> 
> may invoke undefined behavior (and rightfully trigger build breakage with e.g.
> clang) if PlatformId is larger than 31.
> 
> Now, I do see the comment
> 
>   //
>   // The index of platform information resides in bits 50:52 of MSR
> IA32_PLATFORM_ID
>   //
> 
> so I wanted to suggest adding:
> 
>   ASSERT (PlatformId < 8)?
> 
> but then I saw that the same left-shift was already used in two other places.
> 
> So, with or without the ASSERT:
> 
> Acked-by: Laszlo Ersek <lersek@redhat.com>

PlatformId get from below code: 
  PlatformId = (UINT8) PlatformIdMsr.Bits.PlatformId;

PlatformIdMsr.Bits.PlatformId is a three bits value, so it has limit the PlatformId value < 8. So I will not add the ASSERT code.

> 
> Thanks
> Laszlo
> 
> 
> 
> > +        MicrocodeData = (VOID *)(UINTN) CpuMpData-
> >MicrocodeDataAddress;
> > +        LatestRevision = CpuMpData->MicrocodeRevision;
> > +        goto Done;
> > +    }
> > +  }
> > +
> >    LatestRevision = 0;
> >    MicrocodeData  = NULL;
> >    MicrocodeEnd = (UINTN) (CpuMpData->MicrocodePatchAddress +
> > CpuMpData->MicrocodePatchRegionSize);
> > @@ -117,6 +133,7 @@ MicrocodeDetect (
> >          }
> >          if (CheckSum32 == 0) {
> >            CorrectMicrocode = TRUE;
> > +          ProcessorFlags = MicrocodeEntryPoint->ProcessorFlags;
> >          }
> >        } else if ((MicrocodeEntryPoint->DataSize != 0) &&
> >                   (MicrocodeEntryPoint->UpdateRevision >
> > LatestRevision)) { @@ -151,6 +168,7 @@ MicrocodeDetect (
> >                      // Find one
> >                      //
> >                      CorrectMicrocode = TRUE;
> > +                    ProcessorFlags = ExtendedTable->ProcessorFlag;
> >                      break;
> >                    }
> >                  }
> > @@ -188,6 +206,7 @@ MicrocodeDetect (
> >      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN)
> MicrocodeEntryPoint) + TotalSize);
> >    } while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd));
> >
> > +Done:
> >    if (LatestRevision > CurrentRevision) {
> >      //
> >      // BIOS only authenticate updates that contain a numerically
> > larger revision @@ -211,4 +230,13 @@ MicrocodeDetect (
> >        ReleaseSpinLock(&CpuMpData->MpLock);
> >      }
> >    }
> > +
> > +  if (IsBspCallIn && (LatestRevision != 0)) {
> > +    CpuMpData->ProcessorSignature = Eax.Uint32;
> > +    CpuMpData->ProcessorFlags = ProcessorFlags;
> > +    CpuMpData->MicrocodeDataAddress = (UINTN) MicrocodeData;
> > +    CpuMpData->MicrocodeRevision = LatestRevision;
> > +    DEBUG ((DEBUG_INFO, "BSP Microcode:: signature [0x%08x],
> ProcessorFlags [0x%08x], \
> > +       MicroData [0x%08x], Revision [0x%08x]\n", Eax.Uint32,
> > + ProcessorFlags, (UINTN) MicrocodeData, LatestRevision));  }
> >  }
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > index 8b458a4a3a..9179f9ae6d 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > @@ -410,7 +410,7 @@ ApInitializeSync (
> >    //
> >    // Load microcode on AP
> >    //
> > -  MicrocodeDetect (CpuMpData);
> > +  MicrocodeDetect (CpuMpData, FALSE);
> >    //
> >    // Sync BSP's MTRR table to AP
> >    //
> > @@ -1601,7 +1601,7 @@ MpInitLibInitialize (
> >    //
> >    // Load Microcode on BSP
> >    //
> > -  MicrocodeDetect (CpuMpData);
> > +  MicrocodeDetect (CpuMpData, TRUE);
> >    //
> >    // Store BSP's MTRR setting
> >    //
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > index 73e689d969..d897497b77 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > @@ -246,6 +246,11 @@ struct _CPU_MP_DATA {
> >    BOOLEAN                        TimerInterruptState;
> >    UINT64                         MicrocodePatchAddress;
> >    UINT64                         MicrocodePatchRegionSize;
> > +
> > +  UINT32                         ProcessorSignature;
> > +  UINT32                         ProcessorFlags;
> > +  UINT64                         MicrocodeDataAddress;
> > +  UINT32                         MicrocodeRevision;
> >  };
> >
> >  extern EFI_GUID mCpuInitMpLibHobGuid; @@ -547,11 +552,13 @@
> > 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]  CpuMpData    The pointer to CPU MP Data structure.
> > +  @param[in]  IsBspCallIn  Indicate whether the caller is BSP or not.
> >  **/
> >  VOID
> >  MicrocodeDetect (
> > -  IN CPU_MP_DATA             *CpuMpData
> > +  IN CPU_MP_DATA             *CpuMpData,
> > +  IN BOOLEAN                 IsBspCallIn
> >    );
> >
> >  /**
> >
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel