Re: [PATCH v1 2/4] UefiCpuPkg/MpInitLib: Reduce the size when loading microcode patches

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

> -----Original Message-----
> From: Ni, Ray
> Sent: Tuesday, December 24, 2019 11:32 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 2/4] UefiCpuPkg/MpInitLib: Reduce the size when
> loading microcode patches
> 
> 6 minor comments.


Hello Ray,

Thanks for the feedbacks.

For 1,2,4,5, I will update the patch to address your comments.

For 3, my concern is that:

ALIGN_VALUE (TotalSize , SIZE_1KB) 

might have a chance to overflow, how about changing the logic to:

      if (TotalSize > ALIGN_VALUE (TotalSize, SIZE_1KB) ||
          ALIGN_VALUE (TotalSize, SIZE_1KB) > MAX_UINTN - TotalLoadSize) {
        goto OnExit;
      }

For 6, the 'ApInitReconfig' flag value is still being used in function
ResetProcessorToIdleState().

Also, I found that there are several places in MpInitLib that use:

  CpuMpData->InitFlag != ApInitDone
  CpuMpData->InitFlag != ApInitConfig

In some if statements.

So I prefer the evaluation of the removal of the 'ApInitReconfig' flag value to
be handled in another separate patch series. Do you agree?

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 2/4] UefiCpuPkg/MpInitLib: Reduce the size when
> > loading microcode patches
> >
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2429
> >
> > This commit will attempt to reduce the copy size when loading the
> > microcode patches data from flash into memory.
> >
> > Such optimization is done by a pre-process of the microcode patch headers
> > (on flash). A microcode patch will be loaded into memory only when the
> > below 2 criteria are met:
> >
> > A. With a microcode patch header (which means the data is not padding data
> >    between microcode patches);
> > B. The 'ProcessorSignature' & 'ProcessorFlags' fields in the header match
> >    at least one processor within system.
> >
> > Criterion B will require all the processors to be woken up once to collect
> > their CPUID and Platform ID information. Hence, this commit will move the
> > copy, detect and apply of microcode patch on BSP and APs after all the
> > processors have been woken up.
> >
> > 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/MpLib.h     |  24 ++
> >  UefiCpuPkg/Library/MpInitLib/Microcode.c | 235 ++++++++++++++++++++
> >  UefiCpuPkg/Library/MpInitLib/MpLib.c     |  82 ++-----
> >  3 files changed, 283 insertions(+), 58 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > index 4440dc2701..56b0df664a 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> > @@ -44,6 +44,20 @@
> >  #define CPU_SWITCH_STATE_LOADED 2
> >
> >  //
> > +// Default maximum number of entries to store the microcode patches
> > information
> > +//
> > +#define DEFAULT_MAX_MICROCODE_PATCH_NUM 8
> > +
> > +//
> > +// Data structure for microcode patch information
> > +//
> > +typedef struct {
> > +  UINTN    Address;
> > +  UINTN    Size;
> > +  UINTN    AlignedSize;
> > +} MICROCODE_PATCH_INFO;
> > +
> > +//
> >  // CPU exchange information for switch BSP
> >  //
> >  typedef struct {
> > @@ -576,6 +590,16 @@ MicrocodeDetect (
> >    );
> >
> >  /**
> > +  Load the required microcode patches data into memory.
> > +
> > +  @param[in, out]  CpuMpData    The pointer to CPU MP Data structure.
> > +**/
> > +VOID
> > +LoadMicrocodePatch (
> > +  IN OUT CPU_MP_DATA             *CpuMpData
> > +  );
> > +
> > +/**
> >    Detect whether Mwait-monitor feature is supported.
> >
> >    @retval TRUE    Mwait-monitor feature is supported.
> > diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > index 199b1f23ce..68088b26a5 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c
> > @@ -331,3 +331,238 @@ Done:
> >         MicroData [0x%08x], Revision [0x%08x]\n", Eax.Uint32, ProcessorFlags,
> > (UINTN) MicrocodeData, LatestRevision));
> >    }
> >  }
> > +
> > +/**
> > +  Actual worker function that loads the required microcode patches into
> > memory.
> > +
> > +  @param[in, out]  CpuMpData          The pointer to CPU MP Data structure.
> > +  @param[in]       PatchInfoBuffer    The pointer to an array of information
> on
> > +                                      the microcode patches that will be loaded
> > +                                      into memory.
> > +  @param[in]       PatchNumber        The number of microcode patches that
> > will
> > +                                      be loaded into memory.
> > +  @param[in]       TotalLoadSize      The total size of all the microcode
> > +                                      patches to be loaded.
> > +**/
> > +VOID
> > +LoadMicrocodePatchWorker (
> > +  IN OUT CPU_MP_DATA             *CpuMpData,
> > +  IN     MICROCODE_PATCH_INFO    *PatchInfoBuffer,
> > +  IN     UINTN                   PatchNumber,
> 
> 1. "PatchInfoBuffer" -> "Patches"?
> "PatchNumber" -> "PatchCount"?
> 
> > +  //
> > +  // Load all the required microcode patches into memory
> > +  //
> > +  for (Walker = MicrocodePatchInRam, Index = 0; Index < PatchNumber;
> > Index++) {
> > +    CopyMem (
> > +      Walker,
> > +      (VOID *) PatchInfoBuffer[Index].Address,
> > +      PatchInfoBuffer[Index].Size
> > +      );
> > +
> > +    if (PatchInfoBuffer[Index].AlignedSize > PatchInfoBuffer[Index].Size) {
> 
> 2. This if-check is not needed because AlignedSize always >= Size.
> Below ZeroMem() will directly return due to the 2nd parameter is 0.
> 
> > +      //
> > +      // Zero-fill the padding area
> > +      //
> > +      ZeroMem (
> > +        Walker + PatchInfoBuffer[Index].Size,
> > +        PatchInfoBuffer[Index].AlignedSize - PatchInfoBuffer[Index].Size
> > +        );
> > +    }
> > +
> 
> > +      if (TotalSize > MAX_UINTN - TotalLoadSize ||
> > +          ALIGN_VALUE (TotalSize, SIZE_1KB) > MAX_UINTN - TotalLoadSize) {
> > +        goto OnExit;
> > +      }
> 3. ALIGN_VALUE (TotalSize , SIZE_1KB) always >= TotalSize.
> So can we only check
>   ALIGN_VALUE (TotalSize, SIZE_1KB) > MAX_UINTN - TotalLoadSize
> ?
> 
> 
> > +      PatchInfoBuffer[PatchNumber - 1].Address     = (UINTN)
> > MicrocodeEntryPoint;
> > +      PatchInfoBuffer[PatchNumber - 1].Size        = TotalSize;
> > +      PatchInfoBuffer[PatchNumber - 1].AlignedSize = ALIGN_VALUE
> > (TotalSize, SIZE_1KB);
> 
> 4. "PatchNumber" -> "PatchCount"?
> 
> > +      TotalLoadSize += PatchInfoBuffer[PatchNumber - 1].AlignedSize;
> > +    }
> > +
> > +    //
> > +    // Process the next microcode patch
> > +    //
> > +    MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN)
> > MicrocodeEntryPoint) + TotalSize);
> > +  } while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd));
> > +
> > +  if (PatchNumber == 0) {
> > +    //
> > +    // No patch needs to be loaded
> > +    //
> > +    goto OnExit;
> 
> 5. This "goto" is not necessary. You can call below two lines when PatchCount !=
> 0.
> Less "goto" is better.
> 
> > +  }
> > +
> > +  DEBUG ((
> > +    DEBUG_INFO,
> > +    "%a: 0x%x microcode patches will be loaded into memory, with size
> > 0x%x.\n",
> > +    __FUNCTION__, PatchNumber, TotalLoadSize
> > +    ));
> > +
> > +  LoadMicrocodePatchWorker (CpuMpData, PatchInfoBuffer, PatchNumber,
> > TotalLoadSize);
> > +
> > +OnExit:
> > +  if (PatchInfoBuffer != NULL) {
> > +    FreePool (PatchInfoBuffer);
> > +  }
> > +  return;
> > +}
> 
> > @@ -1788,7 +1752,6 @@ MpInitLibInitialize (
> >      //
> >      CpuMpData->CpuCount  = OldCpuMpData->CpuCount;
> >      CpuMpData->BspNumber = OldCpuMpData->BspNumber;
> > -    CpuMpData->InitFlag  = ApInitReconfig;
> 
> 6. Do you think that ApInitReconfig definition can be removed?