[edk2-devel] [PATCH v3 0/6] UefiCpuPkg/MpInitLib: Add support for multiple MP_HAND_OFF HOBs

[edk2-devel] [PATCH v3 0/6] UefiCpuPkg/MpInitLib: Add support for multiple MP_HAND_OFF HOBs

[Gerd Hoffmann]

Needed to boot guests with thousands of vcpus.

v3:
 - refine comments and commit messages.
 - fix MaxCpusPerHob calculation.
 - pick up review tags.
 - add patch to speed up GetBspNumber a bit.
v2:
 - rework HOB loops for better performance: O(n) instead of O(n^2).

Gerd Hoffmann (6):
  UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetMpHandOffHob
  UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetBspNumber()
  UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
    SwitchApContext()
  UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
    MpInitLibInitialize
  UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SaveCpuMpData()
  UefiCpuPkg/MpInitLib: return early in GetBspNumber()

 UefiCpuPkg/Library/MpInitLib/MpLib.h    |  14 ++-
 UefiCpuPkg/Library/MpInitLib/MpLib.c    | 157 +++++++++++++++---------
 UefiCpuPkg/Library/MpInitLib/PeiMpLib.c |  44 ++++---
 3 files changed, 142 insertions(+), 73 deletions(-)

-- 
2.43.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115826): https://edk2.groups.io/g/devel/message/115826
Mute This Topic: https://groups.io/mt/104510905/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 1/6] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetMpHandOffHob

[Gerd Hoffmann]

Rename the function to GetNextMpHandOffHob(), add MP_HAND_OFF parameter.
When called with NULL pointer return the body of the first HOB, otherwise
return the next in the chain.

Also add the function prototype to the MpLib.h header file.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.h | 12 ++++++++++++
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 26 ++++++++++++++++----------
 2 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index a96a6389c17d..fab2b2d493ca 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -485,6 +485,18 @@ SwitchApContext (
   IN MP_HAND_OFF  *MpHandOff
   );
 
+/**
+  Get pointer to next MP_HAND_OFF GUIDed HOB body.
+
+  @param[in] MpHandOff  Previous HOB body.  Pass NULL to get the first HOB.
+
+  @return  The pointer to MP_HAND_OFF structure.
+**/
+MP_HAND_OFF *
+GetNextMpHandOffHob (
+  IN CONST MP_HAND_OFF  *MpHandOff
+  );
+
 /**
   Get available EfiBootServicesCode memory below 4GB by specified size.
 
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index cdfb570e61a0..16fc7dc066fd 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1961,25 +1961,31 @@ SwitchApContext (
 }
 
 /**
-  Get pointer to MP_HAND_OFF GUIDed HOB.
+  Get pointer to next MP_HAND_OFF GUIDed HOB body.
+
+  @param[in] MpHandOff  Previous HOB body.  Pass NULL to get the first HOB.
 
   @return  The pointer to MP_HAND_OFF structure.
 **/
 MP_HAND_OFF *
-GetMpHandOffHob (
-  VOID
+GetNextMpHandOffHob (
+  IN CONST MP_HAND_OFF  *MpHandOff
   )
 {
   EFI_HOB_GUID_TYPE  *GuidHob;
-  MP_HAND_OFF        *MpHandOff;
 
-  MpHandOff = NULL;
-  GuidHob   = GetFirstGuidHob (&mMpHandOffGuid);
-  if (GuidHob != NULL) {
-    MpHandOff = (MP_HAND_OFF *)GET_GUID_HOB_DATA (GuidHob);
+  if (MpHandOff == NULL) {
+    GuidHob = GetFirstGuidHob (&mMpHandOffGuid);
+  } else {
+    GuidHob = (VOID *)(((UINT8 *)MpHandOff) - sizeof (EFI_HOB_GUID_TYPE));
+    GuidHob = GetNextGuidHob (&mMpHandOffGuid, GET_NEXT_HOB (GuidHob));
   }
 
-  return MpHandOff;
+  if (GuidHob == NULL) {
+    return NULL;
+  }
+
+  return (MP_HAND_OFF *)GET_GUID_HOB_DATA (GuidHob);
 }
 
 /**
@@ -2020,7 +2026,7 @@ MpInitLibInitialize (
   UINTN                    BackupBufferAddr;
   UINTN                    ApIdtBase;
 
-  MpHandOff = GetMpHandOffHob ();
+  MpHandOff = GetNextMpHandOffHob (NULL);
   if (MpHandOff == NULL) {
     MaxLogicalProcessorNumber = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
   } else {
-- 
2.43.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115828): https://edk2.groups.io/g/devel/message/115828
Mute This Topic: https://groups.io/mt/104510907/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 1/6] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetMpHandOffHob

[Ni, Ray]

> +  @param[in] MpHandOff  Previous HOB body.  Pass NULL to get the
> first HOB.

Can you replace "...HOB Body.  Pass NULL..." with "...HOB Body. Pass NULL..."?
Your current comments contain two spaces.

With that, Reviewed-by: Ray Ni <ray.ni@intel.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115865): https://edk2.groups.io/g/devel/message/115865
Mute This Topic: https://groups.io/mt/104510907/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 1/6] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetMpHandOffHob

[Laszlo Ersek]

Hi Ray,

On 2/23/24 03:35, Ni, Ray wrote:
>> +  @param[in] MpHandOff  Previous HOB body.  Pass NULL to get the
>> first HOB.
> 
> Can you replace "...HOB Body.  Pass NULL..." with "...HOB Body. Pass NULL..."?
> Your current comments contain two spaces.
> 
> With that, Reviewed-by: Ray Ni <ray.ni@intel.com>

The two spaces after the dot are valid English -- it's called "English
spacing after a full stop":

https://en.wikipedia.org/wiki/Full_stop#Spacing_after_a_full_stop

Some projects use English spacing in comments and documentation, so a
developer working on multiple projects may not find it easy to switch
between French and English spacing. (If you check Gerd's commit messages
in this series, those also use English spacing.)

I could edit out the second spaces before merging this series, but I
think that wouldn't be right; again, it's valid English. (E.g., we also
don't try to enforce American vs. British English spelling in comments,
even though the American one is much more common in the codebase.)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115957): https://edk2.groups.io/g/devel/message/115957
Mute This Topic: https://groups.io/mt/104510907/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 2/6] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetBspNumber()

[Gerd Hoffmann]

Rename the MpHandOff parameter to FirstMpHandOff.  Add a loop so the
function inspects all HOBs present in the system.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 16fc7dc066fd..76449f73f4a7 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1894,26 +1894,33 @@ CheckAllAPs (
 /**
   This function Get BspNumber.
 
-  @param[in] MpHandOff        Pointer to MpHandOff
+  @param[in] FirstMpHandOff   Pointer to first MpHandOff HOB body.
   @return                     BspNumber
 **/
 UINT32
 GetBspNumber (
-  IN CONST MP_HAND_OFF  *MpHandOff
+  IN CONST MP_HAND_OFF  *FirstMpHandOff
   )
 {
-  UINT32  ApicId;
-  UINT32  BspNumber;
-  UINT32  Index;
+  UINT32             ApicId;
+  UINT32             BspNumber;
+  UINT32             Index;
+  CONST MP_HAND_OFF  *MpHandOff;
 
   //
   // Get the processor number for the BSP
   //
   BspNumber = MAX_UINT32;
   ApicId    = GetInitialApicId ();
-  for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
-    if (MpHandOff->Info[Index].ApicId == ApicId) {
-      BspNumber = Index;
+
+  for (MpHandOff = FirstMpHandOff;
+       MpHandOff != NULL;
+       MpHandOff = GetNextMpHandOffHob (MpHandOff))
+  {
+    for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
+      if (MpHandOff->Info[Index].ApicId == ApicId) {
+        BspNumber = MpHandOff->ProcessorIndex + Index;
+      }
     }
   }
 
-- 
2.43.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115825): https://edk2.groups.io/g/devel/message/115825
Mute This Topic: https://groups.io/mt/104510904/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 3/6] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SwitchApContext()

[Gerd Hoffmann]

Rename the MpHandOff parameter to FirstMpHandOff.  Add loops so the
function inspects all HOBs present in the system.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.h |  2 +-
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 35 ++++++++++++++++++----------
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index fab2b2d493ca..3a7b9896cff4 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -482,7 +482,7 @@ GetWakeupBuffer (
 **/
 VOID
 SwitchApContext (
-  IN MP_HAND_OFF  *MpHandOff
+  IN CONST MP_HAND_OFF  *FirstMpHandOff
   );
 
 /**
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 76449f73f4a7..8eab8b636d78 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1938,31 +1938,42 @@ GetBspNumber (
   This procedure allows the AP to switch to another section of
   memory and continue its loop there.
 
-  @param[in] MpHandOff  Pointer to MP hand-off data structure.
+  @param[in] FirstMpHandOff  Pointer to first MP hand-off HOB body.
 **/
 VOID
 SwitchApContext (
-  IN MP_HAND_OFF  *MpHandOff
+  IN CONST MP_HAND_OFF  *FirstMpHandOff
   )
 {
-  UINTN   Index;
-  UINT32  BspNumber;
+  UINTN              Index;
+  UINT32             BspNumber;
+  CONST MP_HAND_OFF  *MpHandOff;
 
-  BspNumber = GetBspNumber (MpHandOff);
+  BspNumber = GetBspNumber (FirstMpHandOff);
 
-  for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
-    if (Index != BspNumber) {
-      *(UINTN *)(UINTN)MpHandOff->Info[Index].StartupProcedureAddress = (UINTN)SwitchContextPerAp;
-      *(UINT32 *)(UINTN)MpHandOff->Info[Index].StartupSignalAddress   = MpHandOff->StartupSignalValue;
+  for (MpHandOff = FirstMpHandOff;
+       MpHandOff != NULL;
+       MpHandOff = GetNextMpHandOffHob (MpHandOff))
+  {
+    for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
+      if (MpHandOff->ProcessorIndex + Index != BspNumber) {
+        *(UINTN *)(UINTN)MpHandOff->Info[Index].StartupProcedureAddress = (UINTN)SwitchContextPerAp;
+        *(UINT32 *)(UINTN)MpHandOff->Info[Index].StartupSignalAddress   = MpHandOff->StartupSignalValue;
+      }
     }
   }
 
   //
   // Wait all APs waken up if this is not the 1st broadcast of SIPI
   //
-  for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
-    if (Index != BspNumber) {
-      WaitApWakeup ((UINT32 *)(UINTN)(MpHandOff->Info[Index].StartupSignalAddress));
+  for (MpHandOff = FirstMpHandOff;
+       MpHandOff != NULL;
+       MpHandOff = GetNextMpHandOffHob (MpHandOff))
+  {
+    for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
+      if (MpHandOff->ProcessorIndex + Index != BspNumber) {
+        WaitApWakeup ((UINT32 *)(UINTN)(MpHandOff->Info[Index].StartupSignalAddress));
+      }
     }
   }
 }
-- 
2.43.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115827): https://edk2.groups.io/g/devel/message/115827
Mute This Topic: https://groups.io/mt/104510906/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 4/6] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to MpInitLibInitialize

[Gerd Hoffmann]

Loop over all MP_HAND_OFF HOBs instead of expecting a single HOB
covering all CPUs in the system.

Add a new FirstMpHandOff variable, which caches the first HOB body for
faster lookups.  It is also used to check whenever MP_HAND_OFF HOBs are
present.  Using the MpHandOff pointer for that does not work any more
because the variable will be NULL at the end of HOB loops.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Ray Ni <ray.ni@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 68 +++++++++++++++++++---------
 1 file changed, 47 insertions(+), 21 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 8eab8b636d78..4b6d6d02b027 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -2025,6 +2025,7 @@ MpInitLibInitialize (
   VOID
   )
 {
+  MP_HAND_OFF              *FirstMpHandOff;
   MP_HAND_OFF              *MpHandOff;
   CPU_INFO_IN_HOB          *CpuInfoInHob;
   UINT32                   MaxLogicalProcessorNumber;
@@ -2038,17 +2039,31 @@ MpInitLibInitialize (
   CPU_MP_DATA              *CpuMpData;
   UINT8                    ApLoopMode;
   UINT8                    *MonitorBuffer;
-  UINTN                    Index;
+  UINT32                   Index, HobIndex;
   UINTN                    ApResetVectorSizeBelow1Mb;
   UINTN                    ApResetVectorSizeAbove1Mb;
   UINTN                    BackupBufferAddr;
   UINTN                    ApIdtBase;
 
-  MpHandOff = GetNextMpHandOffHob (NULL);
-  if (MpHandOff == NULL) {
+  FirstMpHandOff = GetNextMpHandOffHob (NULL);
+  if (FirstMpHandOff != NULL) {
+    MaxLogicalProcessorNumber = 0;
+    for (MpHandOff = FirstMpHandOff;
+         MpHandOff != NULL;
+         MpHandOff = GetNextMpHandOffHob (MpHandOff))
+    {
+      DEBUG ((
+        DEBUG_INFO,
+        "%a: ProcessorIndex=%u CpuCount=%u\n",
+        __func__,
+        MpHandOff->ProcessorIndex,
+        MpHandOff->CpuCount
+        ));
+      ASSERT (MaxLogicalProcessorNumber == MpHandOff->ProcessorIndex);
+      MaxLogicalProcessorNumber += MpHandOff->CpuCount;
+    }
+  } else {
     MaxLogicalProcessorNumber = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
-  } else {
-    MaxLogicalProcessorNumber = MpHandOff->CpuCount;
   }
 
   ASSERT (MaxLogicalProcessorNumber != 0);
@@ -2192,7 +2207,7 @@ MpInitLibInitialize (
   //
   ProgramVirtualWireMode ();
 
-  if (MpHandOff == NULL) {
+  if (FirstMpHandOff == NULL) {
     if (MaxLogicalProcessorNumber > 1) {
       //
       // Wakeup all APs and calculate the processor count in system
@@ -2208,21 +2223,32 @@ MpInitLibInitialize (
       AmdSevUpdateCpuMpData (CpuMpData);
     }
 
-    CpuMpData->CpuCount  = MpHandOff->CpuCount;
-    CpuMpData->BspNumber = GetBspNumber (MpHandOff);
+    CpuMpData->CpuCount  = MaxLogicalProcessorNumber;
+    CpuMpData->BspNumber = GetBspNumber (FirstMpHandOff);
     CpuInfoInHob         = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
-    for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
-      InitializeSpinLock (&CpuMpData->CpuData[Index].ApLock);
-      CpuMpData->CpuData[Index].CpuHealthy = (MpHandOff->Info[Index].Health == 0) ? TRUE : FALSE;
-      CpuMpData->CpuData[Index].ApFunction = 0;
-      CpuInfoInHob[Index].InitialApicId    = MpHandOff->Info[Index].ApicId;
-      CpuInfoInHob[Index].ApTopOfStack     = CpuMpData->Buffer + (Index + 1) * CpuMpData->CpuApStackSize;
-      CpuInfoInHob[Index].ApicId           = MpHandOff->Info[Index].ApicId;
-      CpuInfoInHob[Index].Health           = MpHandOff->Info[Index].Health;
+    for (MpHandOff = FirstMpHandOff;
+         MpHandOff != NULL;
+         MpHandOff = GetNextMpHandOffHob (MpHandOff))
+    {
+      for (HobIndex = 0; HobIndex < MpHandOff->CpuCount; HobIndex++) {
+        Index = MpHandOff->ProcessorIndex + HobIndex;
+        InitializeSpinLock (&CpuMpData->CpuData[Index].ApLock);
+        CpuMpData->CpuData[Index].CpuHealthy = (MpHandOff->Info[HobIndex].Health == 0) ? TRUE : FALSE;
+        CpuMpData->CpuData[Index].ApFunction = 0;
+        CpuInfoInHob[Index].InitialApicId    = MpHandOff->Info[HobIndex].ApicId;
+        CpuInfoInHob[Index].ApTopOfStack     = CpuMpData->Buffer + (Index + 1) * CpuMpData->CpuApStackSize;
+        CpuInfoInHob[Index].ApicId           = MpHandOff->Info[HobIndex].ApicId;
+        CpuInfoInHob[Index].Health           = MpHandOff->Info[HobIndex].Health;
+      }
     }
 
-    DEBUG ((DEBUG_INFO, "MpHandOff->WaitLoopExecutionMode: %04d, sizeof (VOID *): %04d\n", MpHandOff->WaitLoopExecutionMode, sizeof (VOID *)));
-    if (MpHandOff->WaitLoopExecutionMode == sizeof (VOID *)) {
+    DEBUG ((
+      DEBUG_INFO,
+      "FirstMpHandOff->WaitLoopExecutionMode: %04d, sizeof (VOID *): %04d\n",
+      FirstMpHandOff->WaitLoopExecutionMode,
+      sizeof (VOID *)
+      ));
+    if (FirstMpHandOff->WaitLoopExecutionMode == sizeof (VOID *)) {
       ASSERT (CpuMpData->ApLoopMode != ApInHltLoop);
 
       CpuMpData->FinishedCount                        = 0;
@@ -2238,7 +2264,7 @@ MpInitLibInitialize (
       // enables the APs to switch to a different memory section and continue their
       // looping process there.
       //
-      SwitchApContext (MpHandOff);
+      SwitchApContext (FirstMpHandOff);
       //
       // Wait for all APs finished initialization
       //
@@ -2287,7 +2313,7 @@ MpInitLibInitialize (
   // Wakeup APs to do some AP initialize sync (Microcode & MTRR)
   //
   if (CpuMpData->CpuCount > 1) {
-    if (MpHandOff != NULL) {
+    if (FirstMpHandOff != NULL) {
       //
       // Only needs to use this flag for DXE phase to update the wake up
       // buffer. Wakeup buffer allocated in PEI phase is no longer valid
@@ -2304,7 +2330,7 @@ MpInitLibInitialize (
       CpuPause ();
     }
 
-    if (MpHandOff != NULL) {
+    if (FirstMpHandOff != NULL) {
       CpuMpData->InitFlag = ApInitDone;
     }
 
-- 
2.43.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115829): https://edk2.groups.io/g/devel/message/115829
Mute This Topic: https://groups.io/mt/104510909/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 4/6] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to MpInitLibInitialize

[Laszlo Ersek]

On 2/22/24 17:01, Gerd Hoffmann wrote:
> Loop over all MP_HAND_OFF HOBs instead of expecting a single HOB
> covering all CPUs in the system.
> 
> Add a new FirstMpHandOff variable, which caches the first HOB body for
> faster lookups.  It is also used to check whenever MP_HAND_OFF HOBs are
> present.  Using the MpHandOff pointer for that does not work any more
> because the variable will be NULL at the end of HOB loops.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Ray Ni <ray.ni@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 68 +++++++++++++++++++---------
>  1 file changed, 47 insertions(+), 21 deletions(-)

Based on comparison against v2 (commit message update, plus plan to
extract WaitLoopExecutionMode and StartupSignalValue to a new,
library-internal, HOB):

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115961): https://edk2.groups.io/g/devel/message/115961
Mute This Topic: https://groups.io/mt/104510909/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 5/6] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SaveCpuMpData()

[Gerd Hoffmann]

Add support for splitting Hand-Off data into multiple HOBs.
This is required for VMs with thousands of CPUs.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 44 +++++++++++++++----------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
index f80e00edcff3..00d48c2ab531 100644
--- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
@@ -126,35 +126,45 @@ SaveCpuMpData (
   IN CPU_MP_DATA  *CpuMpData
   )
 {
+  UINT32           MaxCpusPerHob, CpusInHob;
   UINT64           Data64;
-  UINTN            Index;
+  UINT32           Index, HobBase;
   CPU_INFO_IN_HOB  *CpuInfoInHob;
   MP_HAND_OFF      *MpHandOff;
   UINTN            MpHandOffSize;
 
+  MaxCpusPerHob = (0xFFF8 - sizeof (EFI_HOB_GUID_TYPE) - sizeof (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);
+
   //
   // When APs are in a state that can be waken up by a store operation to a memory address,
   // report the MP_HAND_OFF data for DXE to use.
   //
-  CpuInfoInHob  = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
-  MpHandOffSize = sizeof (MP_HAND_OFF) + sizeof (PROCESSOR_HAND_OFF) * CpuMpData->CpuCount;
-  MpHandOff     = (MP_HAND_OFF *)BuildGuidHob (&mMpHandOffGuid, MpHandOffSize);
-  ASSERT (MpHandOff != NULL);
-  ZeroMem (MpHandOff, MpHandOffSize);
-  MpHandOff->ProcessorIndex = 0;
+  CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
 
-  MpHandOff->CpuCount = CpuMpData->CpuCount;
-  if (CpuMpData->ApLoopMode != ApInHltLoop) {
-    MpHandOff->StartupSignalValue    = MP_HAND_OFF_SIGNAL;
-    MpHandOff->WaitLoopExecutionMode = sizeof (VOID *);
-  }
+  for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
+    if (Index % MaxCpusPerHob == 0) {
+      HobBase   = Index;
+      CpusInHob = MIN (CpuMpData->CpuCount - HobBase, MaxCpusPerHob);
 
-  for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
-    MpHandOff->Info[Index].ApicId = CpuInfoInHob[Index].ApicId;
-    MpHandOff->Info[Index].Health = CpuInfoInHob[Index].Health;
+      MpHandOffSize = sizeof (MP_HAND_OFF) + sizeof (PROCESSOR_HAND_OFF) * CpusInHob;
+      MpHandOff     = (MP_HAND_OFF *)BuildGuidHob (&mMpHandOffGuid, MpHandOffSize);
+      ASSERT (MpHandOff != NULL);
+      ZeroMem (MpHandOff, MpHandOffSize);
+
+      MpHandOff->ProcessorIndex = HobBase;
+      MpHandOff->CpuCount       = CpusInHob;
+
+      if (CpuMpData->ApLoopMode != ApInHltLoop) {
+        MpHandOff->StartupSignalValue    = MP_HAND_OFF_SIGNAL;
+        MpHandOff->WaitLoopExecutionMode = sizeof (VOID *);
+      }
+    }
+
+    MpHandOff->Info[Index-HobBase].ApicId = CpuInfoInHob[Index].ApicId;
+    MpHandOff->Info[Index-HobBase].Health = CpuInfoInHob[Index].Health;
     if (CpuMpData->ApLoopMode != ApInHltLoop) {
-      MpHandOff->Info[Index].StartupSignalAddress    = (UINT64)(UINTN)CpuMpData->CpuData[Index].StartupApSignal;
-      MpHandOff->Info[Index].StartupProcedureAddress = (UINT64)(UINTN)&CpuMpData->CpuData[Index].ApFunction;
+      MpHandOff->Info[Index-HobBase].StartupSignalAddress    = (UINT64)(UINTN)CpuMpData->CpuData[Index].StartupApSignal;
+      MpHandOff->Info[Index-HobBase].StartupProcedureAddress = (UINT64)(UINTN)&CpuMpData->CpuData[Index].ApFunction;
     }
   }
 
-- 
2.43.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115830): https://edk2.groups.io/g/devel/message/115830
Mute This Topic: https://groups.io/mt/104510911/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 5/6] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SaveCpuMpData()

[Ni, Ray]

Thanks,
Ray
> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Friday, February 23, 2024 12:01 AM
> To: devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Rahul R <rahul.r.kumar@intel.com>; Oliver Steffen <osteffen@redhat.com>;
> Gerd Hoffmann <kraxel@redhat.com>
> Subject: [PATCH v3 5/6] UefiCpuPkg/MpInitLib: Add support for multiple
> HOBs to SaveCpuMpData()
> 
> Add support for splitting Hand-Off data into multiple HOBs.
> This is required for VMs with thousands of CPUs.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 44 +++++++++++++++----------
>  1 file changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index f80e00edcff3..00d48c2ab531 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> @@ -126,35 +126,45 @@ SaveCpuMpData (
>    IN CPU_MP_DATA  *CpuMpData
>    )
>  {
> +  UINT32           MaxCpusPerHob, CpusInHob;
>    UINT64           Data64;
> -  UINTN            Index;
> +  UINT32           Index, HobBase;

Can you please split the local variable declarations in multiple lines?
With that, Reviewed-by: Ray Ni <ray.ni@intel.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115863): https://edk2.groups.io/g/devel/message/115863
Mute This Topic: https://groups.io/mt/104510911/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 5/6] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SaveCpuMpData()

[Laszlo Ersek]

On 2/23/24 03:16, Ni, Ray wrote:
> 
> 
> Thanks,
> Ray
>> -----Original Message-----
>> From: Gerd Hoffmann <kraxel@redhat.com>
>> Sent: Friday, February 23, 2024 12:01 AM
>> To: devel@edk2.groups.io
>> Cc: Laszlo Ersek <lersek@redhat.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
>> Rahul R <rahul.r.kumar@intel.com>; Oliver Steffen <osteffen@redhat.com>;
>> Gerd Hoffmann <kraxel@redhat.com>
>> Subject: [PATCH v3 5/6] UefiCpuPkg/MpInitLib: Add support for multiple
>> HOBs to SaveCpuMpData()
>>
>> Add support for splitting Hand-Off data into multiple HOBs.
>> This is required for VMs with thousands of CPUs.
>>
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 44 +++++++++++++++----------
>>  1 file changed, 27 insertions(+), 17 deletions(-)
>>
>> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>> b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>> index f80e00edcff3..00d48c2ab531 100644
>> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
>> @@ -126,35 +126,45 @@ SaveCpuMpData (
>>    IN CPU_MP_DATA  *CpuMpData
>>    )
>>  {
>> +  UINT32           MaxCpusPerHob, CpusInHob;
>>    UINT64           Data64;
>> -  UINTN            Index;
>> +  UINT32           Index, HobBase;
> 
> Can you please split the local variable declarations in multiple lines?
> With that, Reviewed-by: Ray Ni <ray.ni@intel.com>

I'll edit the patch just before merging the series.

Laszlo

> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115963): https://edk2.groups.io/g/devel/message/115963
Mute This Topic: https://groups.io/mt/104510911/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 5/6] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SaveCpuMpData()

[Laszlo Ersek]

On 2/22/24 17:01, Gerd Hoffmann wrote:
> Add support for splitting Hand-Off data into multiple HOBs.
> This is required for VMs with thousands of CPUs.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 44 +++++++++++++++----------
>  1 file changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index f80e00edcff3..00d48c2ab531 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> @@ -126,35 +126,45 @@ SaveCpuMpData (
>    IN CPU_MP_DATA  *CpuMpData
>    )
>  {
> +  UINT32           MaxCpusPerHob, CpusInHob;
>    UINT64           Data64;
> -  UINTN            Index;
> +  UINT32           Index, HobBase;
>    CPU_INFO_IN_HOB  *CpuInfoInHob;
>    MP_HAND_OFF      *MpHandOff;
>    UINTN            MpHandOffSize;
>  
> +  MaxCpusPerHob = (0xFFF8 - sizeof (EFI_HOB_GUID_TYPE) - sizeof (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);
> +
>    //
>    // When APs are in a state that can be waken up by a store operation to a memory address,
>    // report the MP_HAND_OFF data for DXE to use.
>    //
> -  CpuInfoInHob  = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
> -  MpHandOffSize = sizeof (MP_HAND_OFF) + sizeof (PROCESSOR_HAND_OFF) * CpuMpData->CpuCount;
> -  MpHandOff     = (MP_HAND_OFF *)BuildGuidHob (&mMpHandOffGuid, MpHandOffSize);
> -  ASSERT (MpHandOff != NULL);
> -  ZeroMem (MpHandOff, MpHandOffSize);
> -  MpHandOff->ProcessorIndex = 0;
> +  CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
>  
> -  MpHandOff->CpuCount = CpuMpData->CpuCount;
> -  if (CpuMpData->ApLoopMode != ApInHltLoop) {
> -    MpHandOff->StartupSignalValue    = MP_HAND_OFF_SIGNAL;
> -    MpHandOff->WaitLoopExecutionMode = sizeof (VOID *);
> -  }
> +  for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> +    if (Index % MaxCpusPerHob == 0) {
> +      HobBase   = Index;
> +      CpusInHob = MIN (CpuMpData->CpuCount - HobBase, MaxCpusPerHob);
>  
> -  for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
> -    MpHandOff->Info[Index].ApicId = CpuInfoInHob[Index].ApicId;
> -    MpHandOff->Info[Index].Health = CpuInfoInHob[Index].Health;
> +      MpHandOffSize = sizeof (MP_HAND_OFF) + sizeof (PROCESSOR_HAND_OFF) * CpusInHob;
> +      MpHandOff     = (MP_HAND_OFF *)BuildGuidHob (&mMpHandOffGuid, MpHandOffSize);
> +      ASSERT (MpHandOff != NULL);
> +      ZeroMem (MpHandOff, MpHandOffSize);
> +
> +      MpHandOff->ProcessorIndex = HobBase;
> +      MpHandOff->CpuCount       = CpusInHob;
> +
> +      if (CpuMpData->ApLoopMode != ApInHltLoop) {
> +        MpHandOff->StartupSignalValue    = MP_HAND_OFF_SIGNAL;
> +        MpHandOff->WaitLoopExecutionMode = sizeof (VOID *);
> +      }
> +    }
> +
> +    MpHandOff->Info[Index-HobBase].ApicId = CpuInfoInHob[Index].ApicId;
> +    MpHandOff->Info[Index-HobBase].Health = CpuInfoInHob[Index].Health;
>      if (CpuMpData->ApLoopMode != ApInHltLoop) {
> -      MpHandOff->Info[Index].StartupSignalAddress    = (UINT64)(UINTN)CpuMpData->CpuData[Index].StartupApSignal;
> -      MpHandOff->Info[Index].StartupProcedureAddress = (UINT64)(UINTN)&CpuMpData->CpuData[Index].ApFunction;
> +      MpHandOff->Info[Index-HobBase].StartupSignalAddress    = (UINT64)(UINTN)CpuMpData->CpuData[Index].StartupApSignal;
> +      MpHandOff->Info[Index-HobBase].StartupProcedureAddress = (UINT64)(UINTN)&CpuMpData->CpuData[Index].ApFunction;
>      }
>    }
>  

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115962): https://edk2.groups.io/g/devel/message/115962
Mute This Topic: https://groups.io/mt/104510911/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v3 6/6] UefiCpuPkg/MpInitLib: return early in GetBspNumber()

[Gerd Hoffmann]

After finding the BSP Number return the result instead of
continuing to loop over the remaining processors.

Suggested-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 4b6d6d02b027..2051554207dc 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1903,15 +1903,13 @@ GetBspNumber (
   )
 {
   UINT32             ApicId;
-  UINT32             BspNumber;
   UINT32             Index;
   CONST MP_HAND_OFF  *MpHandOff;
 
   //
   // Get the processor number for the BSP
   //
-  BspNumber = MAX_UINT32;
-  ApicId    = GetInitialApicId ();
+  ApicId = GetInitialApicId ();
 
   for (MpHandOff = FirstMpHandOff;
        MpHandOff != NULL;
@@ -1919,14 +1917,13 @@ GetBspNumber (
   {
     for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
       if (MpHandOff->Info[Index].ApicId == ApicId) {
-        BspNumber = MpHandOff->ProcessorIndex + Index;
+        return MpHandOff->ProcessorIndex + Index;
       }
     }
   }
 
-  ASSERT (BspNumber != MAX_UINT32);
-
-  return BspNumber;
+  ASSERT (FALSE);
+  return 0;
 }
 
 /**
-- 
2.43.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115831): https://edk2.groups.io/g/devel/message/115831
Mute This Topic: https://groups.io/mt/104510913/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 6/6] UefiCpuPkg/MpInitLib: return early in GetBspNumber()

[Ni, Ray]

> +  ASSERT (FALSE);

How about ASSERT (EFI_NOT_FOUND)? Which is more meaningful than FALSE.

No matter you change or not, Reviewed-by: Ray Ni <ray.ni@intel.com>

> +  return 0;
>  }
> 
>  /**
> --
> 2.43.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115864): https://edk2.groups.io/g/devel/message/115864
Mute This Topic: https://groups.io/mt/104510913/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 6/6] UefiCpuPkg/MpInitLib: return early in GetBspNumber()

[Laszlo Ersek]

On 2/23/24 03:33, Ni, Ray wrote:
>> +  ASSERT (FALSE);
> 
> How about ASSERT (EFI_NOT_FOUND)? Which is more meaningful than FALSE.

I'll edit the patch -- assuming you mean

  ASSERT_EFI_ERROR (EFI_NOT_FOUND);

Laszlo

> 
> No matter you change or not, Reviewed-by: Ray Ni <ray.ni@intel.com>
> 
>> +  return 0;
>>  }
>>
>>  /**
>> --
>> 2.43.2
> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115964): https://edk2.groups.io/g/devel/message/115964
Mute This Topic: https://groups.io/mt/104510913/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 6/6] UefiCpuPkg/MpInitLib: return early in GetBspNumber()

[Laszlo Ersek]

On 2/22/24 17:01, Gerd Hoffmann wrote:
> After finding the BSP Number return the result instead of
> continuing to loop over the remaining processors.
> 
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 4b6d6d02b027..2051554207dc 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1903,15 +1903,13 @@ GetBspNumber (
>    )
>  {
>    UINT32             ApicId;
> -  UINT32             BspNumber;
>    UINT32             Index;
>    CONST MP_HAND_OFF  *MpHandOff;
>  
>    //
>    // Get the processor number for the BSP
>    //
> -  BspNumber = MAX_UINT32;
> -  ApicId    = GetInitialApicId ();
> +  ApicId = GetInitialApicId ();
>  
>    for (MpHandOff = FirstMpHandOff;
>         MpHandOff != NULL;
> @@ -1919,14 +1917,13 @@ GetBspNumber (
>    {
>      for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
>        if (MpHandOff->Info[Index].ApicId == ApicId) {
> -        BspNumber = MpHandOff->ProcessorIndex + Index;
> +        return MpHandOff->ProcessorIndex + Index;
>        }
>      }
>    }
>  
> -  ASSERT (BspNumber != MAX_UINT32);
> -
> -  return BspNumber;
> +  ASSERT (FALSE);
> +  return 0;
>  }
>  
>  /**

There's a somewhat sneaky change in here: if we fail to find the BSP,
then (beyond tripping an assert like before) we now return 0, rather
than MAX_UINT32.

However, that's arguably more sensible, even.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115965): https://edk2.groups.io/g/devel/message/115965
Mute This Topic: https://groups.io/mt/104510913/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 0/6] UefiCpuPkg/MpInitLib: Add support for multiple MP_HAND_OFF HOBs

[Laszlo Ersek]

On 2/22/24 17:01, Gerd Hoffmann wrote:
> Needed to boot guests with thousands of vcpus.
> 
> v3:
>  - refine comments and commit messages.
>  - fix MaxCpusPerHob calculation.
>  - pick up review tags.
>  - add patch to speed up GetBspNumber a bit.
> v2:
>  - rework HOB loops for better performance: O(n) instead of O(n^2).
> 
> Gerd Hoffmann (6):
>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetMpHandOffHob
>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetBspNumber()
>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
>     SwitchApContext()
>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
>     MpInitLibInitialize
>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SaveCpuMpData()
>   UefiCpuPkg/MpInitLib: return early in GetBspNumber()
> 
>  UefiCpuPkg/Library/MpInitLib/MpLib.h    |  14 ++-
>  UefiCpuPkg/Library/MpInitLib/MpLib.c    | 157 +++++++++++++++---------
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c |  44 ++++---
>  3 files changed, 142 insertions(+), 73 deletions(-)

I created PR <https://github.com/tianocore/edk2/pull/5410> with the
"push" label set 40 minutes ago, for merging this series; however, the
CI system seems to be stuck. Everything on Azure seems to be in Queued
status, nothing is running.

Any ideas?

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115967): https://edk2.groups.io/g/devel/message/115967
Mute This Topic: https://groups.io/mt/104510905/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 0/6] UefiCpuPkg/MpInitLib: Add support for multiple MP_HAND_OFF HOBs

[Joey Vagedes via groups.io]

Hi Lazlo,

I just looked at the pipelines - Looks like everything is fine, there is just currently a backup of runners of jobs in the runners. It is common for jobs that end in CODE_COVERAGE to appear frozen in queued status, but this is expected as this job not queued until all others have finished, which means it gets put at the end of the list of pipelines to execute.

Taking a look at all the runners (https://dev.azure.com/tianocore/edk2-ci/_settings/buildqueue?_a=concurrentJobs [Click "Microsoft Hosted", "View in-progress jobs"]), I don't see any runners that are frozen, which is why it appears it's just a backup.

I'll continue to monitor.

Thanks,
Joey

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Monday, February 26, 2024 7:08 AM
To: devel@edk2.groups.io; kraxel@redhat.com
Cc: Ni, Ray <ray.ni@intel.com>; Rahul Kumar <rahul1.kumar@intel.com>; Oliver Steffen <osteffen@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Joey Vagedes <joeyvagedes@microsoft.com>
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v3 0/6] UefiCpuPkg/MpInitLib: Add support for multiple MP_HAND_OFF HOBs

[You don't often get email from lersek@redhat.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]

On 2/22/24 17:01, Gerd Hoffmann wrote:
> Needed to boot guests with thousands of vcpus.
>
> v3:
>  - refine comments and commit messages.
>  - fix MaxCpusPerHob calculation.
>  - pick up review tags.
>  - add patch to speed up GetBspNumber a bit.
> v2:
>  - rework HOB loops for better performance: O(n) instead of O(n^2).
>
> Gerd Hoffmann (6):
>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetMpHandOffHob
>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetBspNumber()
>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
>     SwitchApContext()
>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
>     MpInitLibInitialize
>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SaveCpuMpData()
>   UefiCpuPkg/MpInitLib: return early in GetBspNumber()
>
>  UefiCpuPkg/Library/MpInitLib/MpLib.h    |  14 ++-
>  UefiCpuPkg/Library/MpInitLib/MpLib.c    | 157 +++++++++++++++---------
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c |  44 ++++---
>  3 files changed, 142 insertions(+), 73 deletions(-)

I created PR <https://github.com/tianocore/edk2/pull/5410> with the "push" label set 40 minutes ago, for merging this series; however, the CI system seems to be stuck. Everything on Azure seems to be in Queued status, nothing is running.

Any ideas?

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115972): https://edk2.groups.io/g/devel/message/115972
Mute This Topic: https://groups.io/mt/104510905/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 0/6] UefiCpuPkg/MpInitLib: Add support for multiple MP_HAND_OFF HOBs

[Laszlo Ersek]

Hi Joey,
On 2/26/24 16:41, Joey Vagedes wrote:
> Hi Lazlo,
> 
> I just looked at the pipelines - Looks like everything is fine, there is just currently a backup of runners of jobs in the runners. It is common for jobs that end in CODE_COVERAGE to appear frozen in queued status, but this is expected as this job not queued until all others have finished, which means it gets put at the end of the list of pipelines to execute.
> 
> Taking a look at all the runners (https://dev.azure.com/tianocore/edk2-ci/_settings/buildqueue?_a=concurrentJobs [Click "Microsoft Hosted", "View in-progress jobs"]), I don't see any runners that are frozen, which is why it appears it's just a backup.
> 
> I'll continue to monitor.

thanks -- meanwhile I've also found the blockage, just from a different
perspective.

I've known for a while that it is not ideal to have multiple PRs open at
the same time with the "push" label set. They will either compete for
resources (slow), or one will block the other ("queued" state); however,
the main annoyance is that once the first PR gets merged, the HEAD of
the master branch will move for all the other PRs, and then those PRs
will fail to merge even if their CIs run to completion. So in such
cases, the maintainer needs to notice the problem in the first place
(possibly after having waited for 30+ minutes), then perform a manual
rebase (using the github web UI or a local rebase + force push), and
then pray that their next attempt will get to the front of the queue.

It would be much better if:

- *either* the "preempted" PRs rebased themselves automatically to the
new master HEAD, and restarted the CI + merge train,

- *or*, if at least one PR with "push" were in progress at the time of
the maintainer creating a new PR with "push", the CI run wouldn't even
*start* -- instead, the maintainer would *immediately* get information
about being blocked (and about the inevitable need to rebase later, once
that "other" -- earlier-filed -- PR completes).

Now, I've been trying to implement strategy#2 myself, by checking the
"open PRs" view (with an eye towards the "push" label among those PRs)
just before submitting a PR myself. I did that this time as well, but
didn't see any concurrent push-PR. That's why I was confused -- first I
didn't understand what blocked my CI tasks, and then I didn't understand
what had preempted (= de-synced) my PR. When I fetched locally afresh, I
found two new commits, but didn't understand where those had come from,
given that I couldn't see any push-PR concurrently to mine.

*However*, searching my edk2-devel folder for the subjects of those
suddenly appearing new commits, I figured out the problem: PR
<https://github.com/tianocore/edk2/pull/5187> was created in *December*
last year, but Liming set the "push" label on it only ~2 hours ago. In
other words, PR#5187 was the one to hold up and preempt mine. So, why
did I not see it in the "open PRs" view? (Because, as I wrote above, if
I had seen it, I wouldn't have submitted mine in the first place -- I'd
have known it would be useless, due to the master branch moving forward
anyway!)

Well the reason I didn't see PR#5187 (donning the "push" label at that)
was that PR#5187 had been *old as heck*, and so it simply didn't make
the first page of the "open PRs" listing! I didn't expect two months old
PRs to be merged.

So, my lesson from this is: it's not the plain "open PRs" view that I
need to check, when implementing approach #2 myself. Instead, I have to
explicitly search for open PRs with the "push" label:

https://github.com/tianocore/edk2/labels/push

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115973): https://edk2.groups.io/g/devel/message/115973
Mute This Topic: https://groups.io/mt/104510905/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 0/6] UefiCpuPkg/MpInitLib: Add support for multiple MP_HAND_OFF HOBs

[Laszlo Ersek]

On 2/26/24 16:41, Joey Vagedes via groups.io wrote:

> Taking a look at all the runners
> (https://dev.azure.com/tianocore/edk2-ci/_settings/buildqueue?_a=concurrentJobs
> [Click "Microsoft Hosted", "View in-progress jobs"]), I don't see any
> runners that are frozen, which is why it appears it's just a backup.

... unfortunately this URL seems to require a Microsoft account; it's
not publicly accessible.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115974): https://edk2.groups.io/g/devel/message/115974
Mute This Topic: https://groups.io/mt/104510905/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 0/6] UefiCpuPkg/MpInitLib: Add support for multiple MP_HAND_OFF HOBs

[Laszlo Ersek]

On 2/22/24 17:01, Gerd Hoffmann wrote:
> Needed to boot guests with thousands of vcpus.
>
> v3:
>  - refine comments and commit messages.
>  - fix MaxCpusPerHob calculation.
>  - pick up review tags.
>  - add patch to speed up GetBspNumber a bit.
> v2:
>  - rework HOB loops for better performance: O(n) instead of O(n^2).
>
> Gerd Hoffmann (6):
>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetMpHandOffHob
>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetBspNumber()
>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
>     SwitchApContext()
>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
>     MpInitLibInitialize
>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SaveCpuMpData()
>   UefiCpuPkg/MpInitLib: return early in GetBspNumber()
>
>  UefiCpuPkg/Library/MpInitLib/MpLib.h    |  14 ++-
>  UefiCpuPkg/Library/MpInitLib/MpLib.c    | 157 +++++++++++++++---------
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c |  44 ++++---
>  3 files changed, 142 insertions(+), 73 deletions(-)
>

BTW, differences in PR#5410 relative to v3 as posted:

1:  678ed78d24a3 ! 1:  ecd6c4bb3396 UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetMpHandOffHob
    @@ Commit message

         Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
         Message-Id: <20240222160106.686484-2-kraxel@redhat.com>
    +    Reviewed-by: Ray Ni <ray.ni@intel.com>
    +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>

      ## UefiCpuPkg/Library/MpInitLib/MpLib.h ##
     @@ UefiCpuPkg/Library/MpInitLib/MpLib.h: SwitchApContext (
2:  23b3e66f9935 = 2:  189467980103 UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetBspNumber()
3:  e712d36775d0 = 3:  8ab0f63c0f04 UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SwitchApContext()
4:  9a81417f4b76 ! 4:  995a8ace7801 UefiCpuPkg/MpInitLib: Add support for multiple HOBs to MpInitLibInitialize
    @@ Commit message
         Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
         Reviewed-by: Ray Ni <ray.ni@intel.com>
         Message-Id: <20240222160106.686484-5-kraxel@redhat.com>
    +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>

      ## UefiCpuPkg/Library/MpInitLib/MpLib.c ##
     @@ UefiCpuPkg/Library/MpInitLib/MpLib.c: MpInitLibInitialize (
5:  3a089b25725e ! 5:  f23c0d125e48 UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SaveCpuMpData()
    @@ Commit message

         Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
         Message-Id: <20240222160106.686484-6-kraxel@redhat.com>
    +    Reviewed-by: Ray Ni <ray.ni@intel.com>
    +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
    +    [lersek@redhat.com: define one local variable per line [Ray]]

      ## UefiCpuPkg/Library/MpInitLib/PeiMpLib.c ##
     @@ UefiCpuPkg/Library/MpInitLib/PeiMpLib.c: SaveCpuMpData (
        IN CPU_MP_DATA  *CpuMpData
        )
      {
    -+  UINT32           MaxCpusPerHob, CpusInHob;
    ++  UINT32           MaxCpusPerHob;
    ++  UINT32           CpusInHob;
        UINT64           Data64;
     -  UINTN            Index;
    -+  UINT32           Index, HobBase;
    ++  UINT32           Index;
    ++  UINT32           HobBase;
        CPU_INFO_IN_HOB  *CpuInfoInHob;
        MP_HAND_OFF      *MpHandOff;
        UINTN            MpHandOffSize;
6:  09435495e6e1 ! 6:  fbd8a114cd6e UefiCpuPkg/MpInitLib: return early in GetBspNumber()
    @@ Commit message
         Suggested-by: Laszlo Ersek <lersek@redhat.com>
         Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
         Message-Id: <20240222160106.686484-7-kraxel@redhat.com>
    +    Reviewed-by: Ray Ni <ray.ni@intel.com>
    +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
    +    [lersek@redhat.com: s/ASSERT (FALSE)/ASSERT_EFI_ERROR (EFI_NOT_FOUND)/ [Ray]]

      ## UefiCpuPkg/Library/MpInitLib/MpLib.c ##
     @@ UefiCpuPkg/Library/MpInitLib/MpLib.c: GetBspNumber (
    @@ UefiCpuPkg/Library/MpInitLib/MpLib.c: GetBspNumber (
     -  ASSERT (BspNumber != MAX_UINT32);
     -
     -  return BspNumber;
    -+  ASSERT (FALSE);
    ++  ASSERT_EFI_ERROR (EFI_NOT_FOUND);
     +  return 0;
      }



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115969): https://edk2.groups.io/g/devel/message/115969
Mute This Topic: https://groups.io/mt/104510905/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 0/6] UefiCpuPkg/MpInitLib: Add support for multiple MP_HAND_OFF HOBs

[Laszlo Ersek]

On 2/26/24 16:19, Laszlo Ersek wrote:
> On 2/22/24 17:01, Gerd Hoffmann wrote:
>> Needed to boot guests with thousands of vcpus.
>>
>> v3:
>>  - refine comments and commit messages.
>>  - fix MaxCpusPerHob calculation.
>>  - pick up review tags.
>>  - add patch to speed up GetBspNumber a bit.
>> v2:
>>  - rework HOB loops for better performance: O(n) instead of O(n^2).
>>
>> Gerd Hoffmann (6):
>>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetMpHandOffHob
>>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetBspNumber()
>>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
>>     SwitchApContext()
>>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
>>     MpInitLibInitialize
>>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SaveCpuMpData()
>>   UefiCpuPkg/MpInitLib: return early in GetBspNumber()
>>
>>  UefiCpuPkg/Library/MpInitLib/MpLib.h    |  14 ++-
>>  UefiCpuPkg/Library/MpInitLib/MpLib.c    | 157 +++++++++++++++---------
>>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c |  44 ++++---
>>  3 files changed, 142 insertions(+), 73 deletions(-)
>>
> 
> BTW, differences in PR#5410 relative to v3 as posted:
> 
> 1:  678ed78d24a3 ! 1:  ecd6c4bb3396 UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetMpHandOffHob
>     @@ Commit message
> 
>          Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>          Message-Id: <20240222160106.686484-2-kraxel@redhat.com>
>     +    Reviewed-by: Ray Ni <ray.ni@intel.com>
>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
>       ## UefiCpuPkg/Library/MpInitLib/MpLib.h ##
>      @@ UefiCpuPkg/Library/MpInitLib/MpLib.h: SwitchApContext (
> 2:  23b3e66f9935 = 2:  189467980103 UefiCpuPkg/MpInitLib: Add support for multiple HOBs to GetBspNumber()
> 3:  e712d36775d0 = 3:  8ab0f63c0f04 UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SwitchApContext()
> 4:  9a81417f4b76 ! 4:  995a8ace7801 UefiCpuPkg/MpInitLib: Add support for multiple HOBs to MpInitLibInitialize
>     @@ Commit message
>          Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>          Reviewed-by: Ray Ni <ray.ni@intel.com>
>          Message-Id: <20240222160106.686484-5-kraxel@redhat.com>
>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
>       ## UefiCpuPkg/Library/MpInitLib/MpLib.c ##
>      @@ UefiCpuPkg/Library/MpInitLib/MpLib.c: MpInitLibInitialize (
> 5:  3a089b25725e ! 5:  f23c0d125e48 UefiCpuPkg/MpInitLib: Add support for multiple HOBs to SaveCpuMpData()
>     @@ Commit message
> 
>          Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>          Message-Id: <20240222160106.686484-6-kraxel@redhat.com>
>     +    Reviewed-by: Ray Ni <ray.ni@intel.com>
>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>     +    [lersek@redhat.com: define one local variable per line [Ray]]
> 
>       ## UefiCpuPkg/Library/MpInitLib/PeiMpLib.c ##
>      @@ UefiCpuPkg/Library/MpInitLib/PeiMpLib.c: SaveCpuMpData (
>         IN CPU_MP_DATA  *CpuMpData
>         )
>       {
>     -+  UINT32           MaxCpusPerHob, CpusInHob;
>     ++  UINT32           MaxCpusPerHob;
>     ++  UINT32           CpusInHob;
>         UINT64           Data64;
>      -  UINTN            Index;
>     -+  UINT32           Index, HobBase;
>     ++  UINT32           Index;
>     ++  UINT32           HobBase;
>         CPU_INFO_IN_HOB  *CpuInfoInHob;
>         MP_HAND_OFF      *MpHandOff;
>         UINTN            MpHandOffSize;
> 6:  09435495e6e1 ! 6:  fbd8a114cd6e UefiCpuPkg/MpInitLib: return early in GetBspNumber()
>     @@ Commit message
>          Suggested-by: Laszlo Ersek <lersek@redhat.com>
>          Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>          Message-Id: <20240222160106.686484-7-kraxel@redhat.com>
>     +    Reviewed-by: Ray Ni <ray.ni@intel.com>
>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>     +    [lersek@redhat.com: s/ASSERT (FALSE)/ASSERT_EFI_ERROR (EFI_NOT_FOUND)/ [Ray]]
> 
>       ## UefiCpuPkg/Library/MpInitLib/MpLib.c ##
>      @@ UefiCpuPkg/Library/MpInitLib/MpLib.c: GetBspNumber (
>     @@ UefiCpuPkg/Library/MpInitLib/MpLib.c: GetBspNumber (
>      -  ASSERT (BspNumber != MAX_UINT32);
>      -
>      -  return BspNumber;
>     -+  ASSERT (FALSE);
>     ++  ASSERT_EFI_ERROR (EFI_NOT_FOUND);
>      +  return 0;
>       }
> 

Merged as commit range 1f161a7915e1..d25421d0d8cd, via
<https://github.com/tianocore/edk2/pull/5410>.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115978): https://edk2.groups.io/g/devel/message/115978
Mute This Topic: https://groups.io/mt/104510905/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 0/6] UefiCpuPkg/MpInitLib: Add support for multiple MP_HAND_OFF HOBs

[Ni, Ray]

Thank you, Laszlo!
The changes you made look good to me.

By the way, is the following a common way to call out additional changes?
>     +    [lersek@redhat.com: define one local variable per line [Ray]]

Thanks,
Ray
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> Sent: Monday, February 26, 2024 11:20 PM
> To: devel@edk2.groups.io; kraxel@redhat.com
> Cc: Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
> Oliver Steffen <osteffen@redhat.com>
> Subject: Re: [edk2-devel] [PATCH v3 0/6] UefiCpuPkg/MpInitLib: Add support
> for multiple MP_HAND_OFF HOBs
> 
> On 2/22/24 17:01, Gerd Hoffmann wrote:
> > Needed to boot guests with thousands of vcpus.
> >
> > v3:
> >  - refine comments and commit messages.
> >  - fix MaxCpusPerHob calculation.
> >  - pick up review tags.
> >  - add patch to speed up GetBspNumber a bit.
> > v2:
> >  - rework HOB loops for better performance: O(n) instead of O(n^2).
> >
> > Gerd Hoffmann (6):
> >   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
> GetMpHandOffHob
> >   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
> GetBspNumber()
> >   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
> >     SwitchApContext()
> >   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
> >     MpInitLibInitialize
> >   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
> SaveCpuMpData()
> >   UefiCpuPkg/MpInitLib: return early in GetBspNumber()
> >
> >  UefiCpuPkg/Library/MpInitLib/MpLib.h    |  14 ++-
> >  UefiCpuPkg/Library/MpInitLib/MpLib.c    | 157
> +++++++++++++++---------
> >  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c |  44 ++++---
> >  3 files changed, 142 insertions(+), 73 deletions(-)
> >
> 
> BTW, differences in PR#5410 relative to v3 as posted:
> 
> 1:  678ed78d24a3 ! 1:  ecd6c4bb3396 UefiCpuPkg/MpInitLib: Add support
> for multiple HOBs to GetMpHandOffHob
>     @@ Commit message
> 
>          Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>          Message-Id: <20240222160106.686484-2-kraxel@redhat.com>
>     +    Reviewed-by: Ray Ni <ray.ni@intel.com>
>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
>       ## UefiCpuPkg/Library/MpInitLib/MpLib.h ##
>      @@ UefiCpuPkg/Library/MpInitLib/MpLib.h: SwitchApContext (
> 2:  23b3e66f9935 = 2:  189467980103 UefiCpuPkg/MpInitLib: Add support
> for multiple HOBs to GetBspNumber()
> 3:  e712d36775d0 = 3:  8ab0f63c0f04 UefiCpuPkg/MpInitLib: Add support
> for multiple HOBs to SwitchApContext()
> 4:  9a81417f4b76 ! 4:  995a8ace7801 UefiCpuPkg/MpInitLib: Add support
> for multiple HOBs to MpInitLibInitialize
>     @@ Commit message
>          Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>          Reviewed-by: Ray Ni <ray.ni@intel.com>
>          Message-Id: <20240222160106.686484-5-kraxel@redhat.com>
>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
>       ## UefiCpuPkg/Library/MpInitLib/MpLib.c ##
>      @@ UefiCpuPkg/Library/MpInitLib/MpLib.c: MpInitLibInitialize (
> 5:  3a089b25725e ! 5:  f23c0d125e48 UefiCpuPkg/MpInitLib: Add support
> for multiple HOBs to SaveCpuMpData()
>     @@ Commit message
> 
>          Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>          Message-Id: <20240222160106.686484-6-kraxel@redhat.com>
>     +    Reviewed-by: Ray Ni <ray.ni@intel.com>
>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>     +    [lersek@redhat.com: define one local variable per line [Ray]]
> 
>       ## UefiCpuPkg/Library/MpInitLib/PeiMpLib.c ##
>      @@ UefiCpuPkg/Library/MpInitLib/PeiMpLib.c: SaveCpuMpData (
>         IN CPU_MP_DATA  *CpuMpData
>         )
>       {
>     -+  UINT32           MaxCpusPerHob, CpusInHob;
>     ++  UINT32           MaxCpusPerHob;
>     ++  UINT32           CpusInHob;
>         UINT64           Data64;
>      -  UINTN            Index;
>     -+  UINT32           Index, HobBase;
>     ++  UINT32           Index;
>     ++  UINT32           HobBase;
>         CPU_INFO_IN_HOB  *CpuInfoInHob;
>         MP_HAND_OFF      *MpHandOff;
>         UINTN            MpHandOffSize;
> 6:  09435495e6e1 ! 6:  fbd8a114cd6e UefiCpuPkg/MpInitLib: return early
> in GetBspNumber()
>     @@ Commit message
>          Suggested-by: Laszlo Ersek <lersek@redhat.com>
>          Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>          Message-Id: <20240222160106.686484-7-kraxel@redhat.com>
>     +    Reviewed-by: Ray Ni <ray.ni@intel.com>
>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>     +    [lersek@redhat.com: s/ASSERT (FALSE)/ASSERT_EFI_ERROR
> (EFI_NOT_FOUND)/ [Ray]]
> 
>       ## UefiCpuPkg/Library/MpInitLib/MpLib.c ##
>      @@ UefiCpuPkg/Library/MpInitLib/MpLib.c: GetBspNumber (
>     @@ UefiCpuPkg/Library/MpInitLib/MpLib.c: GetBspNumber (
>      -  ASSERT (BspNumber != MAX_UINT32);
>      -
>      -  return BspNumber;
>     -+  ASSERT (FALSE);
>     ++  ASSERT_EFI_ERROR (EFI_NOT_FOUND);
>      +  return 0;
>       }
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116002): https://edk2.groups.io/g/devel/message/116002
Mute This Topic: https://groups.io/mt/104510905/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v3 0/6] UefiCpuPkg/MpInitLib: Add support for multiple MP_HAND_OFF HOBs

[Laszlo Ersek]

On 2/27/24 07:28, Ni, Ray wrote:
> Thank you, Laszlo!
> The changes you made look good to me.
> 
> By the way, is the following a common way to call out additional changes?
>>     +    [lersek@redhat.com: define one local variable per line [Ray]]

I don't know of any other project that uses it.

I must have seen Jordan a decade ago (?) use this way for marking
last-minute maintainer changes, and I've been following the pattern since.

I think it's preferable to silently editing a patch; after all, the
contributor did not verbatim post the patch that's about to be merged.

Laszlo

> 
> Thanks,
> Ray
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
>> Ersek
>> Sent: Monday, February 26, 2024 11:20 PM
>> To: devel@edk2.groups.io; kraxel@redhat.com
>> Cc: Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
>> Oliver Steffen <osteffen@redhat.com>
>> Subject: Re: [edk2-devel] [PATCH v3 0/6] UefiCpuPkg/MpInitLib: Add support
>> for multiple MP_HAND_OFF HOBs
>>
>> On 2/22/24 17:01, Gerd Hoffmann wrote:
>>> Needed to boot guests with thousands of vcpus.
>>>
>>> v3:
>>>  - refine comments and commit messages.
>>>  - fix MaxCpusPerHob calculation.
>>>  - pick up review tags.
>>>  - add patch to speed up GetBspNumber a bit.
>>> v2:
>>>  - rework HOB loops for better performance: O(n) instead of O(n^2).
>>>
>>> Gerd Hoffmann (6):
>>>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
>> GetMpHandOffHob
>>>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
>> GetBspNumber()
>>>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
>>>     SwitchApContext()
>>>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
>>>     MpInitLibInitialize
>>>   UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
>> SaveCpuMpData()
>>>   UefiCpuPkg/MpInitLib: return early in GetBspNumber()
>>>
>>>  UefiCpuPkg/Library/MpInitLib/MpLib.h    |  14 ++-
>>>  UefiCpuPkg/Library/MpInitLib/MpLib.c    | 157
>> +++++++++++++++---------
>>>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c |  44 ++++---
>>>  3 files changed, 142 insertions(+), 73 deletions(-)
>>>
>>
>> BTW, differences in PR#5410 relative to v3 as posted:
>>
>> 1:  678ed78d24a3 ! 1:  ecd6c4bb3396 UefiCpuPkg/MpInitLib: Add support
>> for multiple HOBs to GetMpHandOffHob
>>     @@ Commit message
>>
>>          Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>          Message-Id: <20240222160106.686484-2-kraxel@redhat.com>
>>     +    Reviewed-by: Ray Ni <ray.ni@intel.com>
>>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>>       ## UefiCpuPkg/Library/MpInitLib/MpLib.h ##
>>      @@ UefiCpuPkg/Library/MpInitLib/MpLib.h: SwitchApContext (
>> 2:  23b3e66f9935 = 2:  189467980103 UefiCpuPkg/MpInitLib: Add support
>> for multiple HOBs to GetBspNumber()
>> 3:  e712d36775d0 = 3:  8ab0f63c0f04 UefiCpuPkg/MpInitLib: Add support
>> for multiple HOBs to SwitchApContext()
>> 4:  9a81417f4b76 ! 4:  995a8ace7801 UefiCpuPkg/MpInitLib: Add support
>> for multiple HOBs to MpInitLibInitialize
>>     @@ Commit message
>>          Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>          Reviewed-by: Ray Ni <ray.ni@intel.com>
>>          Message-Id: <20240222160106.686484-5-kraxel@redhat.com>
>>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>>       ## UefiCpuPkg/Library/MpInitLib/MpLib.c ##
>>      @@ UefiCpuPkg/Library/MpInitLib/MpLib.c: MpInitLibInitialize (
>> 5:  3a089b25725e ! 5:  f23c0d125e48 UefiCpuPkg/MpInitLib: Add support
>> for multiple HOBs to SaveCpuMpData()
>>     @@ Commit message
>>
>>          Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>          Message-Id: <20240222160106.686484-6-kraxel@redhat.com>
>>     +    Reviewed-by: Ray Ni <ray.ni@intel.com>
>>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>     +    [lersek@redhat.com: define one local variable per line [Ray]]
>>
>>       ## UefiCpuPkg/Library/MpInitLib/PeiMpLib.c ##
>>      @@ UefiCpuPkg/Library/MpInitLib/PeiMpLib.c: SaveCpuMpData (
>>         IN CPU_MP_DATA  *CpuMpData
>>         )
>>       {
>>     -+  UINT32           MaxCpusPerHob, CpusInHob;
>>     ++  UINT32           MaxCpusPerHob;
>>     ++  UINT32           CpusInHob;
>>         UINT64           Data64;
>>      -  UINTN            Index;
>>     -+  UINT32           Index, HobBase;
>>     ++  UINT32           Index;
>>     ++  UINT32           HobBase;
>>         CPU_INFO_IN_HOB  *CpuInfoInHob;
>>         MP_HAND_OFF      *MpHandOff;
>>         UINTN            MpHandOffSize;
>> 6:  09435495e6e1 ! 6:  fbd8a114cd6e UefiCpuPkg/MpInitLib: return early
>> in GetBspNumber()
>>     @@ Commit message
>>          Suggested-by: Laszlo Ersek <lersek@redhat.com>
>>          Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>>          Message-Id: <20240222160106.686484-7-kraxel@redhat.com>
>>     +    Reviewed-by: Ray Ni <ray.ni@intel.com>
>>     +    Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>     +    [lersek@redhat.com: s/ASSERT (FALSE)/ASSERT_EFI_ERROR
>> (EFI_NOT_FOUND)/ [Ray]]
>>
>>       ## UefiCpuPkg/Library/MpInitLib/MpLib.c ##
>>      @@ UefiCpuPkg/Library/MpInitLib/MpLib.c: GetBspNumber (
>>     @@ UefiCpuPkg/Library/MpInitLib/MpLib.c: GetBspNumber (
>>      -  ASSERT (BspNumber != MAX_UINT32);
>>      -
>>      -  return BspNumber;
>>     -+  ASSERT (FALSE);
>>     ++  ASSERT_EFI_ERROR (EFI_NOT_FOUND);
>>      +  return 0;
>>       }
>>
>>
>>
>> 
>>
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116034): https://edk2.groups.io/g/devel/message/116034
Mute This Topic: https://groups.io/mt/104510905/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-