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

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

6 minor comments.

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