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,
+  IN     UINTN                   TotalLoadSize
+  )
+{
+  UINTN    Index;
+  VOID     *MicrocodePatchInRam;
+  UINT8    *Walker;
+
+  ASSERT ((PatchInfoBuffer != NULL) && (PatchNumber != 0));
+
+  MicrocodePatchInRam = AllocatePages (EFI_SIZE_TO_PAGES (TotalLoadSize));
+  if (MicrocodePatchInRam == NULL) {
+    return;
+  }
+
+  //
+  // 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) {
+      //
+      // Zero-fill the padding area
+      //
+      ZeroMem (
+        Walker + PatchInfoBuffer[Index].Size,
+        PatchInfoBuffer[Index].AlignedSize - PatchInfoBuffer[Index].Size
+        );
+    }
+
+    Walker += PatchInfoBuffer[Index].AlignedSize;
+  }
+
+  //
+  // Update the microcode patch related fields in CpuMpData
+  //
+  CpuMpData->MicrocodePatchAddress    = (UINTN) MicrocodePatchInRam;
+  CpuMpData->MicrocodePatchRegionSize = TotalLoadSize;
+
+  DEBUG ((
+    DEBUG_INFO,
+    "%a: Required microcode patches have been loaded at 0x%lx, with size 0x%lx.\n",
+    __FUNCTION__, CpuMpData->MicrocodePatchAddress, CpuMpData->MicrocodePatchRegionSize
+    ));
+
+  return;
+}
+
+/**
+  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
+  )
+{
+  CPU_MICROCODE_HEADER    *MicrocodeEntryPoint;
+  UINTN                   MicrocodeEnd;
+  UINTN                   DataSize;
+  UINTN                   TotalSize;
+  MICROCODE_PATCH_INFO    *PatchInfoBuffer;
+  UINTN                   MaxPatchNumber;
+  UINTN                   PatchNumber;
+  UINTN                   TotalLoadSize;
+  UINT32                  ProcessorSignature;
+  UINT32                  ProcessorFlags;
+  UINTN                   Index;
+  CPU_AP_DATA             *CpuData;
+  BOOLEAN                 NeedLoad;
+
+  //
+  // Initialize the microcode patch related fields in CpuMpData as the values
+  // specified by the PCD pair. If the microcode patches are loaded into memory,
+  // these fields will be updated.
+  //
+  CpuMpData->MicrocodePatchAddress    = PcdGet64 (PcdCpuMicrocodePatchAddress);
+  CpuMpData->MicrocodePatchRegionSize = PcdGet64 (PcdCpuMicrocodePatchRegionSize);
+
+  MicrocodeEntryPoint    = (CPU_MICROCODE_HEADER *) (UINTN) CpuMpData->MicrocodePatchAddress;
+  MicrocodeEnd           = (UINTN) MicrocodeEntryPoint +
+                           (UINTN) CpuMpData->MicrocodePatchRegionSize;
+  if ((MicrocodeEntryPoint == NULL) || ((UINTN) MicrocodeEntryPoint == MicrocodeEnd)) {
+    //
+    // There is no microcode patches
+    //
+    return;
+  }
+
+  PatchNumber     = 0;
+  MaxPatchNumber  = DEFAULT_MAX_MICROCODE_PATCH_NUM;
+  TotalLoadSize   = 0;
+  PatchInfoBuffer = AllocatePool (MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO));
+  if (PatchInfoBuffer == NULL) {
+    return;
+  }
+
+  //
+  // Process the header of each microcode patch within the region.
+  // The purpose is to decide which microcode patch(es) will be loaded into memory.
+  //
+  do {
+    if (MicrocodeEntryPoint->HeaderVersion != 0x1) {
+      //
+      // Padding data between the microcode patches, skip 1KB to check next entry.
+      //
+      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + SIZE_1KB);
+      continue;
+    }
+
+    DataSize = MicrocodeEntryPoint->DataSize;
+    if (DataSize == 0) {
+      TotalSize = sizeof (CPU_MICROCODE_HEADER) + 2000;
+    } else {
+      TotalSize = sizeof (CPU_MICROCODE_HEADER) + DataSize;
+    }
+
+    if ( (UINTN)MicrocodeEntryPoint > (MAX_ADDRESS - TotalSize) ||
+         ((UINTN)MicrocodeEntryPoint + TotalSize) > MicrocodeEnd ||
+         (TotalSize & 0x3) != 0
+       ) {
+      //
+      // Not a valid microcode header, skip 1KB to check next entry.
+      //
+      MicrocodeEntryPoint = (CPU_MICROCODE_HEADER *) (((UINTN) MicrocodeEntryPoint) + SIZE_1KB);
+      continue;
+    }
+
+    TotalSize = (DataSize == 0) ? 2048 : MicrocodeEntryPoint->TotalSize;
+
+    //
+    // Check the 'ProcessorSignature' & 'ProcessorFlags' of this microcode patch
+    // with the processors' CPUID & PlatformID to decide if it will be copied
+    // into memory
+    //
+    ProcessorSignature = MicrocodeEntryPoint->ProcessorSignature.Uint32;
+    ProcessorFlags     = MicrocodeEntryPoint->ProcessorFlags;
+    NeedLoad           = FALSE;
+    for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
+      CpuData = &CpuMpData->CpuData[Index];
+      if ((ProcessorSignature == CpuData->ProcessorSignature) &&
+          (ProcessorFlags & (1 << CpuData->PlatformId)) != 0) {
+        NeedLoad = TRUE;
+        break;
+      }
+    }
+
+    if (NeedLoad) {
+      PatchNumber++;
+      if (PatchNumber >= MaxPatchNumber) {
+        //
+        // Current 'PatchInfoBuffer' cannot hold the information, double the size
+        // and allocate a new buffer.
+        //
+        if (MaxPatchNumber > MAX_UINTN / 2 / sizeof (MICROCODE_PATCH_INFO)) {
+          //
+          // Overflow check for MaxPatchNumber
+          //
+          goto OnExit;
+        }
+
+        PatchInfoBuffer = ReallocatePool (
+                            MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO),
+                            2 * MaxPatchNumber * sizeof (MICROCODE_PATCH_INFO),
+                            PatchInfoBuffer
+                            );
+        if (PatchInfoBuffer == NULL) {
+          goto OnExit;
+        }
+        MaxPatchNumber = MaxPatchNumber * 2;
+      }
+
+      //
+      // Store the information of this microcode patch
+      //
+      if (TotalSize > MAX_UINTN - TotalLoadSize ||
+          ALIGN_VALUE (TotalSize, SIZE_1KB) > MAX_UINTN - TotalLoadSize) {
+        goto OnExit;
+      }
+      PatchInfoBuffer[PatchNumber - 1].Address     = (UINTN) MicrocodeEntryPoint;
+      PatchInfoBuffer[PatchNumber - 1].Size        = TotalSize;
+      PatchInfoBuffer[PatchNumber - 1].AlignedSize = ALIGN_VALUE (TotalSize, SIZE_1KB);
+      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;
+  }
+
+  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;
+}
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index d5077e080e..199468156b 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -628,10 +628,6 @@ ApWakeupFunction (
       ApTopOfStack  = CpuMpData->Buffer + (ProcessorNumber + 1) * CpuMpData->CpuApStackSize;
       BistData = *(UINT32 *) ((UINTN) ApTopOfStack - sizeof (UINTN));
       //
-      // Do some AP initialize sync
-      //
-      ApInitializeSync (CpuMpData);
-      //
       // CpuMpData->CpuData[0].VolatileRegisters is initialized based on BSP environment,
       //   to initialize AP in InitConfig path.
       // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
@@ -1615,7 +1611,6 @@ MpInitLibInitialize (
   UINTN                    ApResetVectorSize;
   UINTN                    BackupBufferAddr;
   UINTN                    ApIdtBase;
-  VOID                     *MicrocodePatchInRam;
 
   OldCpuMpData = GetCpuMpDataFromGuidedHob ();
   if (OldCpuMpData == NULL) {
@@ -1683,39 +1678,7 @@ MpInitLibInitialize (
   CpuMpData->SwitchBspFlag    = FALSE;
   CpuMpData->CpuData          = (CPU_AP_DATA *) (CpuMpData + 1);
   CpuMpData->CpuInfoInHob     = (UINT64) (UINTN) (CpuMpData->CpuData + MaxLogicalProcessorNumber);
-  if (OldCpuMpData == NULL) {
-    CpuMpData->MicrocodePatchRegionSize = PcdGet64 (PcdCpuMicrocodePatchRegionSize);
-    //
-    // If platform has more than one CPU, relocate microcode to memory to reduce
-    // loading microcode time.
-    //
-    MicrocodePatchInRam = NULL;
-    if (MaxLogicalProcessorNumber > 1) {
-      MicrocodePatchInRam = AllocatePages (
-                              EFI_SIZE_TO_PAGES (
-                                (UINTN)CpuMpData->MicrocodePatchRegionSize
-                                )
-                              );
-    }
-    if (MicrocodePatchInRam == NULL) {
-      //
-      // there is only one processor, or no microcode patch is available, or
-      // memory allocation failed
-      //
-      CpuMpData->MicrocodePatchAddress = PcdGet64 (PcdCpuMicrocodePatchAddress);
-    } else {
-      //
-      // there are multiple processors, and a microcode patch is available, and
-      // memory allocation succeeded
-      //
-      CopyMem (
-        MicrocodePatchInRam,
-        (VOID *)(UINTN)PcdGet64 (PcdCpuMicrocodePatchAddress),
-        (UINTN)CpuMpData->MicrocodePatchRegionSize
-        );
-      CpuMpData->MicrocodePatchAddress = (UINTN)MicrocodePatchInRam;
-    }
-  }else {
+  if (OldCpuMpData != NULL) {
     CpuMpData->MicrocodePatchRegionSize = OldCpuMpData->MicrocodePatchRegionSize;
     CpuMpData->MicrocodePatchAddress    = OldCpuMpData->MicrocodePatchAddress;
   }
@@ -1762,10 +1725,6 @@ MpInitLibInitialize (
       (UINT32 *)(MonitorBuffer + MonitorFilterSize * Index);
   }
   //
-  // Load Microcode on BSP
-  //
-  MicrocodeDetect (CpuMpData, TRUE);
-  //
   // Store BSP's MTRR setting
   //
   MtrrGetAllMtrrs (&CpuMpData->MtrrTable);
@@ -1781,6 +1740,11 @@ MpInitLibInitialize (
       //
       CollectProcessorCount (CpuMpData);
     }
+
+    //
+    // Load required microcode patches data into memory
+    //
+    LoadMicrocodePatch (CpuMpData);
   } else {
     //
     // APs have been wakeup before, just get the CPU Information
@@ -1788,7 +1752,6 @@ MpInitLibInitialize (
     //
     CpuMpData->CpuCount  = OldCpuMpData->CpuCount;
     CpuMpData->BspNumber = OldCpuMpData->BspNumber;
-    CpuMpData->InitFlag  = ApInitReconfig;
     CpuMpData->CpuInfoInHob = OldCpuMpData->CpuInfoInHob;
     CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob;
     for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
@@ -1797,21 +1760,24 @@ MpInitLibInitialize (
       CpuMpData->CpuData[Index].ApFunction = 0;
       CopyMem (&CpuMpData->CpuData[Index].VolatileRegisters, &VolatileRegisters, sizeof (CPU_VOLATILE_REGISTERS));
     }
-    if (MaxLogicalProcessorNumber > 1) {
-      //
-      // Wakeup APs to do some AP initialize sync
-      //
-      WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
-      //
-      // Wait for all APs finished initialization
-      //
-      while (CpuMpData->FinishedCount < (CpuMpData->CpuCount - 1)) {
-        CpuPause ();
-      }
-      CpuMpData->InitFlag = ApInitDone;
-      for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
-        SetApState (&CpuMpData->CpuData[Index], CpuStateIdle);
-      }
+  }
+
+  //
+  // Detect and apply Microcode on BSP
+  //
+  MicrocodeDetect (CpuMpData, TRUE);
+
+  //
+  // Wakeup APs to do some AP initialize sync (Microcode & MTRR)
+  //
+  if (CpuMpData->CpuCount > 1) {
+    WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
+    while (CpuMpData->FinishedCount < (CpuMpData->CpuCount - 1)) {
+      CpuPause ();
+    }
+
+    for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
+      SetApState (&CpuMpData->CpuData[Index], CpuStateIdle);
     }
   }
 
-- 
2.12.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#52511): https://edk2.groups.io/g/devel/message/52511
Mute This Topic: https://groups.io/mt/69242654/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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?


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#52526): https://edk2.groups.io/g/devel/message/52526
Mute This Topic: https://groups.io/mt/69242654/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#52537): https://edk2.groups.io/g/devel/message/52537
Mute This Topic: https://groups.io/mt/69242654/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-