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

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

[Gerd Hoffmann]

Needed to boot guests with thousands of vcpus.

Gerd Hoffmann (5):
  UefiCpuPkg/MpInitLib: Add ProcessorIndex argument 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/Library/MpInitLib/MpLib.h    |  14 ++-
 UefiCpuPkg/Library/MpInitLib/MpLib.c    | 133 ++++++++++++++++--------
 UefiCpuPkg/Library/MpInitLib/PeiMpLib.c |  48 +++++----
 3 files changed, 127 insertions(+), 68 deletions(-)

-- 
2.43.1



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

[edk2-devel] [PATCH 1/5] UefiCpuPkg/MpInitLib: Add ProcessorIndex argument to GetMpHandOffHob()

[Gerd Hoffmann]

This allows to specify which HOB should be returned in case multiple
MP_HAND_OFF HOBs are present in the system.

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 | 20 +++++++++++++-------
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index a96a6389c17d..7e409cceaddf 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 MP_HAND_OFF GUIDed HOB, starting with ProcessorIndex
+
+  @param[in] ProcessorIndex.
+
+  @return  The pointer to MP_HAND_OFF structure.
+**/
+MP_HAND_OFF *
+GetMpHandOffHob (
+  IN UINT32  ProcessorIndex
+  );
+
 /**
   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..e0a2366073a7 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 MP_HAND_OFF GUIDed HOB, starting with ProcessorIndex
+
+  @param[in] ProcessorIndex.
 
   @return  The pointer to MP_HAND_OFF structure.
 **/
 MP_HAND_OFF *
 GetMpHandOffHob (
-  VOID
+  IN UINT32  ProcessorIndex
   )
 {
   EFI_HOB_GUID_TYPE  *GuidHob;
   MP_HAND_OFF        *MpHandOff;
 
-  MpHandOff = NULL;
-  GuidHob   = GetFirstGuidHob (&mMpHandOffGuid);
-  if (GuidHob != NULL) {
+  for (GuidHob = GetFirstGuidHob (&mMpHandOffGuid);
+       GuidHob != NULL;
+       GuidHob = GetNextGuidHob (&mMpHandOffGuid, GET_NEXT_HOB (GuidHob)))
+  {
     MpHandOff = (MP_HAND_OFF *)GET_GUID_HOB_DATA (GuidHob);
+    if (MpHandOff->ProcessorIndex == ProcessorIndex) {
+      return MpHandOff;
+    }
   }
 
-  return MpHandOff;
+  return NULL;
 }
 
 /**
@@ -2020,7 +2026,7 @@ MpInitLibInitialize (
   UINTN                    BackupBufferAddr;
   UINTN                    ApIdtBase;
 
-  MpHandOff = GetMpHandOffHob ();
+  MpHandOff = GetMpHandOffHob (0);
   if (MpHandOff == NULL) {
     MaxLogicalProcessorNumber = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
   } else {
-- 
2.43.1



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

Re: [edk2-devel] [PATCH 1/5] UefiCpuPkg/MpInitLib: Add ProcessorIndex argument to GetMpHandOffHob()

[Ni, Ray]

>    EFI_HOB_GUID_TYPE  *GuidHob;
>    MP_HAND_OFF        *MpHandOff;
> 
> -  MpHandOff = NULL;
> -  GuidHob   = GetFirstGuidHob (&mMpHandOffGuid);
> -  if (GuidHob != NULL) {
> +  for (GuidHob = GetFirstGuidHob (&mMpHandOffGuid);
> +       GuidHob != NULL;
> +       GuidHob = GetNextGuidHob (&mMpHandOffGuid, GET_NEXT_HOB
> (GuidHob)))
> +  {
>      MpHandOff = (MP_HAND_OFF *)GET_GUID_HOB_DATA (GuidHob);
> +    if (MpHandOff->ProcessorIndex == ProcessorIndex) {
> +      return MpHandOff;
> +    }

Gerd,
The code is doing correctly but I have concerns about the performance impact.

With the prototype GetMpHandOffHob(), callers call it multiple times to enumerate all HOB instances.

But every call is a HOB list enumeration from top of the HOB list which may be slow in a platform
that contains lots of HOB items and the MpHandOff HOB instances happen to be in the very bottom.

How about add another parameter "HobStart" to GetMpHandOffHob()? So that only the first call to
GetMpHandOffHob() is a HOB list enumeration from top, latter calls start from the next HOB of the previous
found MpHandOff HOB instance.


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

Re: [edk2-devel] [PATCH 1/5] UefiCpuPkg/MpInitLib: Add ProcessorIndex argument to GetMpHandOffHob()

[Gerd Hoffmann]

On Mon, Feb 19, 2024 at 02:34:42AM +0000, Ni, Ray wrote:
> > +  for (GuidHob = GetFirstGuidHob (&mMpHandOffGuid);
> > +       GuidHob != NULL;
> > +       GuidHob = GetNextGuidHob (&mMpHandOffGuid, GET_NEXT_HOB
> > (GuidHob)))
> > +  {
> >      MpHandOff = (MP_HAND_OFF *)GET_GUID_HOB_DATA (GuidHob);
> > +    if (MpHandOff->ProcessorIndex == ProcessorIndex) {
> > +      return MpHandOff;
> > +    }
> 
> Gerd,
> The code is doing correctly but I have concerns about the performance impact.
> 
> With the prototype GetMpHandOffHob(), callers call it multiple times to enumerate all HOB instances.
> 
> But every call is a HOB list enumeration from top of the HOB list which may be slow in a platform
> that contains lots of HOB items and the MpHandOff HOB instances happen to be in the very bottom.
> 
> How about add another parameter "HobStart" to GetMpHandOffHob()? So that only the first call to
> GetMpHandOffHob() is a HOB list enumeration from top, latter calls start from the next HOB of the previous
> found MpHandOff HOB instance.

That will only work if the HOBs are returned in ProcessorIndex order.

That happens to be the case in my testing; the HOBs are returned in the
same order they are created by patch #5 of this series.

Is that behavior guaranteed?  MdePkg/Include/Library/HobLib.h doesn't
say anything about the ordering.

take care,
  Gerd



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

Re: [edk2-devel] [PATCH 1/5] UefiCpuPkg/MpInitLib: Add ProcessorIndex argument to GetMpHandOffHob()

[Ni, Ray]

> 
> That will only work if the HOBs are returned in ProcessorIndex order.
> 
> That happens to be the case in my testing; the HOBs are returned in the
> same order they are created by patch #5 of this series.
> 
> Is that behavior guaranteed?  MdePkg/Include/Library/HobLib.h doesn't
> say anything about the ordering.

The HOB item order follows when it's created.
The HOB structure implicitly guarantees it.

I am aware that in a production server UEFI firmware, the number of HOB items is huge.
Enumerating the HOB list multiple times is a big cost.


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

Re: [edk2-devel] [PATCH 1/5] UefiCpuPkg/MpInitLib: Add ProcessorIndex argument to GetMpHandOffHob()

[Laszlo Ersek]

On 2/15/24 10:31, Gerd Hoffmann wrote:
> This allows to specify which HOB should be returned in case multiple
> MP_HAND_OFF HOBs are present in the system.
> 
> 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 | 20 +++++++++++++-------
>  2 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index a96a6389c17d..7e409cceaddf 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 MP_HAND_OFF GUIDed HOB, starting with ProcessorIndex
> +
> +  @param[in] ProcessorIndex.
> +
> +  @return  The pointer to MP_HAND_OFF structure.
> +**/
> +MP_HAND_OFF *
> +GetMpHandOffHob (
> +  IN UINT32  ProcessorIndex
> +  );
> +
>  /**
>    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..e0a2366073a7 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 MP_HAND_OFF GUIDed HOB, starting with ProcessorIndex
> +
> +  @param[in] ProcessorIndex.
>  
>    @return  The pointer to MP_HAND_OFF structure.
>  **/
>  MP_HAND_OFF *
>  GetMpHandOffHob (
> -  VOID
> +  IN UINT32  ProcessorIndex
>    )
>  {
>    EFI_HOB_GUID_TYPE  *GuidHob;
>    MP_HAND_OFF        *MpHandOff;
>  
> -  MpHandOff = NULL;
> -  GuidHob   = GetFirstGuidHob (&mMpHandOffGuid);
> -  if (GuidHob != NULL) {
> +  for (GuidHob = GetFirstGuidHob (&mMpHandOffGuid);
> +       GuidHob != NULL;
> +       GuidHob = GetNextGuidHob (&mMpHandOffGuid, GET_NEXT_HOB (GuidHob)))
> +  {
>      MpHandOff = (MP_HAND_OFF *)GET_GUID_HOB_DATA (GuidHob);
> +    if (MpHandOff->ProcessorIndex == ProcessorIndex) {
> +      return MpHandOff;
> +    }
>    }
>  
> -  return MpHandOff;
> +  return NULL;
>  }
>  
>  /**
> @@ -2020,7 +2026,7 @@ MpInitLibInitialize (
>    UINTN                    BackupBufferAddr;
>    UINTN                    ApIdtBase;
>  
> -  MpHandOff = GetMpHandOffHob ();
> +  MpHandOff = GetMpHandOffHob (0);
>    if (MpHandOff == NULL) {
>      MaxLogicalProcessorNumber = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
>    } else {

Seems to do what it says on the tin; not sure what it's going to be good
for, though.

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



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

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

[Gerd Hoffmann]

Remove the MpHandOff parameter.  This is not useful in case multiple
HOBs are present in the system.  The function will use GetMpHandOffHob()
to loop over all HOBs instead.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 UefiCpuPkg/Library/MpInitLib/MpLib.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index e0a2366073a7..8e6cf50ed171 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1894,26 +1894,32 @@ CheckAllAPs (
 /**
   This function Get BspNumber.
 
-  @param[in] MpHandOff        Pointer to MpHandOff
   @return                     BspNumber
 **/
 UINT32
 GetBspNumber (
-  IN CONST MP_HAND_OFF  *MpHandOff
+  VOID
   )
 {
-  UINT32  ApicId;
-  UINT32  BspNumber;
-  UINT32  Index;
+  UINT32       ApicId;
+  UINT32       BspNumber;
+  UINT32       Index;
+  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 = GetMpHandOffHob (0);
+       MpHandOff != NULL;
+       MpHandOff = GetMpHandOffHob (MpHandOff->ProcessorIndex + MpHandOff->CpuCount))
+  {
+    for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
+      if (MpHandOff->Info[Index].ApicId == ApicId) {
+        BspNumber = MpHandOff->ProcessorIndex + Index;
+      }
     }
   }
 
@@ -1941,7 +1947,7 @@ SwitchApContext (
   UINTN   Index;
   UINT32  BspNumber;
 
-  BspNumber = GetBspNumber (MpHandOff);
+  BspNumber = GetBspNumber ();
 
   for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
     if (Index != BspNumber) {
@@ -2191,7 +2197,7 @@ MpInitLibInitialize (
     }
 
     CpuMpData->CpuCount  = MpHandOff->CpuCount;
-    CpuMpData->BspNumber = GetBspNumber (MpHandOff);
+    CpuMpData->BspNumber = GetBspNumber ();
     CpuInfoInHob         = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
     for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
       InitializeSpinLock (&CpuMpData->CpuData[Index].ApLock);
-- 
2.43.1



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

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

[Ni, Ray]

Since MpHandOff HOBs are created by the PEI instance of MpInitLib, I am ok that the consumer (DXE instance)
assumes the HOB instances are created in the order of ProcessorIndex.

Please update the patch accordingly with the change of GetMpHandOffHob() I commented to patch #1.

Thanks,
Ray
> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Thursday, February 15, 2024 5:32 PM
> To: devel@edk2.groups.io
> Cc: Oliver Steffen <osteffen@redhat.com>; Ni, Ray <ray.ni@intel.com>; Laszlo
> Ersek <lersek@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>;
> Gerd Hoffmann <kraxel@redhat.com>
> Subject: [PATCH 2/5] UefiCpuPkg/MpInitLib: Add support for multiple HOBs to
> GetBspNumber()
> 
> Remove the MpHandOff parameter.  This is not useful in case multiple
> HOBs are present in the system.  The function will use GetMpHandOffHob()
> to loop over all HOBs instead.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index e0a2366073a7..8e6cf50ed171 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1894,26 +1894,32 @@ CheckAllAPs (
>  /**
>    This function Get BspNumber.
> 
> -  @param[in] MpHandOff        Pointer to MpHandOff
>    @return                     BspNumber
>  **/
>  UINT32
>  GetBspNumber (
> -  IN CONST MP_HAND_OFF  *MpHandOff
> +  VOID
>    )
>  {
> -  UINT32  ApicId;
> -  UINT32  BspNumber;
> -  UINT32  Index;
> +  UINT32       ApicId;
> +  UINT32       BspNumber;
> +  UINT32       Index;
> +  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 = GetMpHandOffHob (0);
> +       MpHandOff != NULL;
> +       MpHandOff = GetMpHandOffHob (MpHandOff->ProcessorIndex +
> MpHandOff->CpuCount))
> +  {
> +    for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
> +      if (MpHandOff->Info[Index].ApicId == ApicId) {
> +        BspNumber = MpHandOff->ProcessorIndex + Index;
> +      }
>      }
>    }
> 
> @@ -1941,7 +1947,7 @@ SwitchApContext (
>    UINTN   Index;
>    UINT32  BspNumber;
> 
> -  BspNumber = GetBspNumber (MpHandOff);
> +  BspNumber = GetBspNumber ();
> 
>    for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
>      if (Index != BspNumber) {
> @@ -2191,7 +2197,7 @@ MpInitLibInitialize (
>      }
> 
>      CpuMpData->CpuCount  = MpHandOff->CpuCount;
> -    CpuMpData->BspNumber = GetBspNumber (MpHandOff);
> +    CpuMpData->BspNumber = GetBspNumber ();
>      CpuInfoInHob         = (CPU_INFO_IN_HOB
> *)(UINTN)CpuMpData->CpuInfoInHob;
>      for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
>        InitializeSpinLock (&CpuMpData->CpuData[Index].ApLock);
> --
> 2.43.1



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

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

[Laszlo Ersek]

On 2/15/24 10:31, Gerd Hoffmann wrote:
> Remove the MpHandOff parameter.  This is not useful in case multiple
> HOBs are present in the system.  The function will use GetMpHandOffHob()
> to loop over all HOBs instead.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index e0a2366073a7..8e6cf50ed171 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1894,26 +1894,32 @@ CheckAllAPs (
>  /**
>    This function Get BspNumber.
>  
> -  @param[in] MpHandOff        Pointer to MpHandOff
>    @return                     BspNumber
>  **/
>  UINT32
>  GetBspNumber (
> -  IN CONST MP_HAND_OFF  *MpHandOff
> +  VOID
>    )
>  {
> -  UINT32  ApicId;
> -  UINT32  BspNumber;
> -  UINT32  Index;
> +  UINT32       ApicId;
> +  UINT32       BspNumber;
> +  UINT32       Index;
> +  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 = GetMpHandOffHob (0);
> +       MpHandOff != NULL;
> +       MpHandOff = GetMpHandOffHob (MpHandOff->ProcessorIndex + MpHandOff->CpuCount))
> +  {
> +    for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
> +      if (MpHandOff->Info[Index].ApicId == ApicId) {
> +        BspNumber = MpHandOff->ProcessorIndex + Index;
> +      }
>      }
>    }
>  

(I'm missing the larger picture here -- is this related to the problem
-- too many CPUs to fit their infos into a single HOB -- that Pawel
worked on for a while?

"UefiCpuPkg/Library/MpInitLib/MpHandOff.h" was created in commit
8bb018afaf2a ("UefiCpuPkg: Create MpHandOff.", 2023-07-11); I don't have
memories from that time frame. Either way, I do have a question /
observation here:)

The outer loop is suboptimal, IMO, to just open-coding another HOB scan
-- this approach looks quadratic, even though it could be linear. More
or less, as proposed, we call GetMpHandOffHob() for each MP_HAND_OFF
HOB, which will scan n/2 HOBs on average. (Even if the GUID HOB list is
sorted by ProcessorIndex, we'll scan 1 + 2 + 3 +... HOBs.) But if we
open-coded GetFirstGuidHob() and GetNextGuidHob() here, then a single
scan would suffice.

Laszlo

> @@ -1941,7 +1947,7 @@ SwitchApContext (
>    UINTN   Index;
>    UINT32  BspNumber;
>  
> -  BspNumber = GetBspNumber (MpHandOff);
> +  BspNumber = GetBspNumber ();
>  
>    for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
>      if (Index != BspNumber) {
> @@ -2191,7 +2197,7 @@ MpInitLibInitialize (
>      }
>  
>      CpuMpData->CpuCount  = MpHandOff->CpuCount;
> -    CpuMpData->BspNumber = GetBspNumber (MpHandOff);
> +    CpuMpData->BspNumber = GetBspNumber ();
>      CpuInfoInHob         = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
>      for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
>        InitializeSpinLock (&CpuMpData->CpuData[Index].ApLock);



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

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

[Gerd Hoffmann]

  Hi,

> (I'm missing the larger picture here -- is this related to the problem
> -- too many CPUs to fit their infos into a single HOB -- that Pawel
> worked on for a while?

Different HOB, but similar underlying problem.

At least the HOB structure already has the fields needed to allow
splitting the information into multiple HOBs, even though the current
code doesn't actually do that (which is fixed by this series).

> The outer loop is suboptimal, IMO, to just open-coding another HOB scan
> -- this approach looks quadratic, even though it could be linear. More
> or less, as proposed, we call GetMpHandOffHob() for each MP_HAND_OFF
> HOB, which will scan n/2 HOBs on average. (Even if the GUID HOB list is
> sorted by ProcessorIndex, we'll scan 1 + 2 + 3 +... HOBs.) But if we
> open-coded GetFirstGuidHob() and GetNextGuidHob() here, then a single
> scan would suffice.

Ray raised performance concerns too.

Does HobLib provides any ordering guarantees?  Specifically in case the
HOBs are returned in the same order they are created (which implies they
are sorted by ProcessorIndex because patch #5 creates them in that
order) this can certainly be optimized.

take care,
  Gerd



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

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

[Laszlo Ersek]

On 2/19/24 12:37, Gerd Hoffmann wrote:
>   Hi,
> 
>> (I'm missing the larger picture here -- is this related to the problem
>> -- too many CPUs to fit their infos into a single HOB -- that Pawel
>> worked on for a while?
> 
> Different HOB, but similar underlying problem.
> 
> At least the HOB structure already has the fields needed to allow
> splitting the information into multiple HOBs, even though the current
> code doesn't actually do that (which is fixed by this series).
> 
>> The outer loop is suboptimal, IMO, to just open-coding another HOB scan
>> -- this approach looks quadratic, even though it could be linear. More
>> or less, as proposed, we call GetMpHandOffHob() for each MP_HAND_OFF
>> HOB, which will scan n/2 HOBs on average. (Even if the GUID HOB list is
>> sorted by ProcessorIndex, we'll scan 1 + 2 + 3 +... HOBs.) But if we
>> open-coded GetFirstGuidHob() and GetNextGuidHob() here, then a single
>> scan would suffice.
> 
> Ray raised performance concerns too.
> 
> Does HobLib provides any ordering guarantees?  Specifically in case the
> HOBs are returned in the same order they are created (which implies they
> are sorted by ProcessorIndex because patch #5 creates them in that
> order) this can certainly be optimized.

I don't think there are ordering guarantees, but my larger point in
review is that the order does not matter. I see no reason for addressing
HOBs in increasing processor index order. No loop over the HOBs that
I've seen touched by this patch set seems to depend on the visiting
order of HOBs. So the outer loops that advance with
GetMpHandOffHob(ProcessorIndex) should be rewritable with get-next-GUID-HOB.

Laszlo



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

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

[Gerd Hoffmann]

Remove the MpHandOff parameter.  This is not useful in case multiple
HOBs are present in the system.  The function will use GetMpHandOffHob()
to loop over all HOBs instead.

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

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 7e409cceaddf..a141a95b45ea 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
+  VOID
   );
 
 /**
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 8e6cf50ed171..35f47d3b1289 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1936,32 +1936,41 @@ GetBspNumber (
   begin running the procedure called SwitchContextPerAp.
   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.
 **/
 VOID
 SwitchApContext (
-  IN MP_HAND_OFF  *MpHandOff
+  VOID
   )
 {
-  UINTN   Index;
-  UINT32  BspNumber;
+  UINTN        Index;
+  UINT32       BspNumber;
+  MP_HAND_OFF  *MpHandOff;
 
   BspNumber = GetBspNumber ();
 
-  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 = GetMpHandOffHob (0);
+       MpHandOff != NULL;
+       MpHandOff = GetMpHandOffHob (MpHandOff->ProcessorIndex + MpHandOff->CpuCount))
+  {
+    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 = GetMpHandOffHob (0);
+       MpHandOff != NULL;
+       MpHandOff = GetMpHandOffHob (MpHandOff->ProcessorIndex + MpHandOff->CpuCount))
+  {
+    for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
+      if (MpHandOff->ProcessorIndex + Index != BspNumber) {
+        WaitApWakeup ((UINT32 *)(UINTN)(MpHandOff->Info[Index].StartupSignalAddress));
+      }
     }
   }
 }
@@ -2226,7 +2235,7 @@ MpInitLibInitialize (
       // enables the APs to switch to a different memory section and continue their
       // looping process there.
       //
-      SwitchApContext (MpHandOff);
+      SwitchApContext ();
       //
       // Wait for all APs finished initialization
       //
-- 
2.43.1



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

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

[Ni, Ray]

SwitchApContext() can cache the first instance of MpHandOffHob so that the second enumeration can avoid searching
for the MpHandOffHob from the top of HOB list.

>  VOID
>  SwitchApContext (
> -  IN MP_HAND_OFF  *MpHandOff
> +  VOID
>    )
>  {
> -  UINTN   Index;
> -  UINT32  BspNumber;
> +  UINTN        Index;
> +  UINT32       BspNumber;
> +  MP_HAND_OFF  *MpHandOff;
> 
>    BspNumber = GetBspNumber ();
> 
> -  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 = GetMpHandOffHob (0);
> +       MpHandOff != NULL;
> +       MpHandOff = GetMpHandOffHob (MpHandOff->ProcessorIndex +
> MpHandOff->CpuCount))
> +  {
> +    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 = GetMpHandOffHob (0);
> +       MpHandOff != NULL;
> +       MpHandOff = GetMpHandOffHob (MpHandOff->ProcessorIndex +
> MpHandOff->CpuCount))
> +  {
> +    for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
> +      if (MpHandOff->ProcessorIndex + Index != BspNumber) {
> +        WaitApWakeup ((UINT32
> *)(UINTN)(MpHandOff->Info[Index].StartupSignalAddress));
> +      }
>      }
>    }
>  }
> @@ -2226,7 +2235,7 @@ MpInitLibInitialize (
>        // enables the APs to switch to a different memory section and
> continue their
>        // looping process there.
>        //
> -      SwitchApContext (MpHandOff);
> +      SwitchApContext ();
>        //
>        // Wait for all APs finished initialization
>        //
> --
> 2.43.1



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

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

[Laszlo Ersek]

On 2/15/24 10:31, Gerd Hoffmann wrote:
> Remove the MpHandOff parameter.  This is not useful in case multiple
> HOBs are present in the system.  The function will use GetMpHandOffHob()
> to loop over all HOBs instead.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.h |  2 +-
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 35 +++++++++++++++++-----------
>  2 files changed, 23 insertions(+), 14 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 7e409cceaddf..a141a95b45ea 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
> +  VOID
>    );
>  
>  /**
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 8e6cf50ed171..35f47d3b1289 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1936,32 +1936,41 @@ GetBspNumber (
>    begin running the procedure called SwitchContextPerAp.
>    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.
>  **/
>  VOID
>  SwitchApContext (
> -  IN MP_HAND_OFF  *MpHandOff
> +  VOID
>    )
>  {
> -  UINTN   Index;
> -  UINT32  BspNumber;
> +  UINTN        Index;
> +  UINT32       BspNumber;
> +  MP_HAND_OFF  *MpHandOff;
>  
>    BspNumber = GetBspNumber ();
>  
> -  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 = GetMpHandOffHob (0);
> +       MpHandOff != NULL;
> +       MpHandOff = GetMpHandOffHob (MpHandOff->ProcessorIndex + MpHandOff->CpuCount))
> +  {
> +    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 = GetMpHandOffHob (0);
> +       MpHandOff != NULL;
> +       MpHandOff = GetMpHandOffHob (MpHandOff->ProcessorIndex + MpHandOff->CpuCount))
> +  {
> +    for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
> +      if (MpHandOff->ProcessorIndex + Index != BspNumber) {
> +        WaitApWakeup ((UINT32 *)(UINTN)(MpHandOff->Info[Index].StartupSignalAddress));
> +      }
>      }
>    }
>  }
> @@ -2226,7 +2235,7 @@ MpInitLibInitialize (
>        // enables the APs to switch to a different memory section and continue their
>        // looping process there.
>        //
> -      SwitchApContext (MpHandOff);
> +      SwitchApContext ();
>        //
>        // Wait for all APs finished initialization
>        //

Same comment as under the previous patch. We could just iterate with
MpHandOff over the GUID HOB list in the outer loops, and perform the
proper actions upon

  MpHandOff->ProcessorIndex + Index != BspNumber

in the inner loop.

It is not necessary for us to ask for the HOBs in any particular
sequence, so we shouldn't pay the O(n) lookup price for every HOB in turn.

Laszlo



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

[edk2-devel] [PATCH 4/5] 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 HaveMpHandOff variable to track whenever MP_HAND_OFF HOBs are
present, using the MpHandOff pointer for that does not work because the
variable will be NULL after looping over all HOBs.

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

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 35f47d3b1289..1547b7e74a1a 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -2023,6 +2023,7 @@ MpInitLibInitialize (
   )
 {
   MP_HAND_OFF              *MpHandOff;
+  BOOLEAN                  HaveMpHandOff;
   CPU_INFO_IN_HOB          *CpuInfoInHob;
   UINT32                   MaxLogicalProcessorNumber;
   UINT32                   ApStackSize;
@@ -2035,17 +2036,29 @@ MpInitLibInitialize (
   CPU_MP_DATA              *CpuMpData;
   UINT8                    ApLoopMode;
   UINT8                    *MonitorBuffer;
-  UINTN                    Index;
+  UINTN                    Index, HobIndex;
   UINTN                    ApResetVectorSizeBelow1Mb;
   UINTN                    ApResetVectorSizeAbove1Mb;
   UINTN                    BackupBufferAddr;
   UINTN                    ApIdtBase;
 
-  MpHandOff = GetMpHandOffHob (0);
-  if (MpHandOff == NULL) {
+  MaxLogicalProcessorNumber = 0;
+  HaveMpHandOff             = FALSE;
+
+  while ((MpHandOff = GetMpHandOffHob (MaxLogicalProcessorNumber)) != 0) {
+    DEBUG ((
+      DEBUG_INFO,
+      "%a: ProcessorIndex=%u CpuCount=%u\n",
+      __func__,
+      MpHandOff->ProcessorIndex,
+      MpHandOff->CpuCount
+      ));
+    MaxLogicalProcessorNumber += MpHandOff->CpuCount;
+    HaveMpHandOff              = TRUE;
+  }
+
+  if (!HaveMpHandOff) {
     MaxLogicalProcessorNumber = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
-  } else {
-    MaxLogicalProcessorNumber = MpHandOff->CpuCount;
   }
 
   ASSERT (MaxLogicalProcessorNumber != 0);
@@ -2189,7 +2202,7 @@ MpInitLibInitialize (
   //
   ProgramVirtualWireMode ();
 
-  if (MpHandOff == NULL) {
+  if (!HaveMpHandOff) {
     if (MaxLogicalProcessorNumber > 1) {
       //
       // Wakeup all APs and calculate the processor count in system
@@ -2205,19 +2218,26 @@ MpInitLibInitialize (
       AmdSevUpdateCpuMpData (CpuMpData);
     }
 
-    CpuMpData->CpuCount  = MpHandOff->CpuCount;
+    CpuMpData->CpuCount  = MaxLogicalProcessorNumber;
     CpuMpData->BspNumber = GetBspNumber ();
     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 = GetMpHandOffHob (0);
+         MpHandOff != NULL;
+         MpHandOff = GetMpHandOffHob (MpHandOff->ProcessorIndex + MpHandOff->CpuCount))
+    {
+      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;
+      }
     }
 
+    MpHandOff = GetMpHandOffHob (0);
     DEBUG ((DEBUG_INFO, "MpHandOff->WaitLoopExecutionMode: %04d, sizeof (VOID *): %04d\n", MpHandOff->WaitLoopExecutionMode, sizeof (VOID *)));
     if (MpHandOff->WaitLoopExecutionMode == sizeof (VOID *)) {
       ASSERT (CpuMpData->ApLoopMode != ApInHltLoop);
@@ -2284,7 +2304,7 @@ MpInitLibInitialize (
   // Wakeup APs to do some AP initialize sync (Microcode & MTRR)
   //
   if (CpuMpData->CpuCount > 1) {
-    if (MpHandOff != NULL) {
+    if (HaveMpHandOff) {
       //
       // 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
@@ -2301,7 +2321,7 @@ MpInitLibInitialize (
       CpuPause ();
     }
 
-    if (MpHandOff != NULL) {
+    if (HaveMpHandOff) {
       CpuMpData->InitFlag = ApInitDone;
     }
 
-- 
2.43.1



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

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

[Ni, Ray]

> -  MpHandOff = GetMpHandOffHob (0);
> -  if (MpHandOff == NULL) {
> +  MaxLogicalProcessorNumber = 0;
> +  HaveMpHandOff             = FALSE;

1. Can we cache the first MpHandOff instance? It can be used as an indicator to replace "HaveMpHandOff",
and also can speed up the HOB enumeration in later logic.

> +
> +  while ((MpHandOff = GetMpHandOffHob
> (MaxLogicalProcessorNumber)) != 0) {

2. Can you use "!= NULL" instead of "!= 0"?

> +    DEBUG ((
> +      DEBUG_INFO,
> +      "%a: ProcessorIndex=%u CpuCount=%u\n",
> +      __func__,
> +      MpHandOff->ProcessorIndex,
> +      MpHandOff->CpuCount
> +      ));


3. can you add an assertion "ASSERT (MaxLogicalProcessorNumber == MpHandOff->ProcessorIndex);"?
It's to ensure no gap or overlap.

> +    MaxLogicalProcessorNumber += MpHandOff->CpuCount;
> +    HaveMpHandOff              = TRUE;
> +  }
> +
> +  if (!HaveMpHandOff) {
4. "HaveMpHandOff == TRUE"  --> "FirstMpHandOff != NULL"

> +      for (HobIndex = 0; HobIndex < MpHandOff->CpuCount; HobIndex++)

5. How about "ProcessorIndexInHob"? "HobIndex" is a bit confusing.
> 
> +    MpHandOff = GetMpHandOffHob (0);
6. All the GetMpHandOffHob(0) can be replaced with "FirstMpHandOff".


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

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

[Laszlo Ersek]

On 2/15/24 10:31, 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 HaveMpHandOff variable to track whenever MP_HAND_OFF HOBs are
> present, using the MpHandOff pointer for that does not work because the
> variable will be NULL after looping over all HOBs.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c | 54 +++++++++++++++++++---------
>  1 file changed, 37 insertions(+), 17 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 35f47d3b1289..1547b7e74a1a 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -2023,6 +2023,7 @@ MpInitLibInitialize (
>    )
>  {
>    MP_HAND_OFF              *MpHandOff;
> +  BOOLEAN                  HaveMpHandOff;
>    CPU_INFO_IN_HOB          *CpuInfoInHob;
>    UINT32                   MaxLogicalProcessorNumber;
>    UINT32                   ApStackSize;
> @@ -2035,17 +2036,29 @@ MpInitLibInitialize (
>    CPU_MP_DATA              *CpuMpData;
>    UINT8                    ApLoopMode;
>    UINT8                    *MonitorBuffer;
> -  UINTN                    Index;
> +  UINTN                    Index, HobIndex;
>    UINTN                    ApResetVectorSizeBelow1Mb;
>    UINTN                    ApResetVectorSizeAbove1Mb;
>    UINTN                    BackupBufferAddr;
>    UINTN                    ApIdtBase;
>  
> -  MpHandOff = GetMpHandOffHob (0);
> -  if (MpHandOff == NULL) {
> +  MaxLogicalProcessorNumber = 0;
> +  HaveMpHandOff             = FALSE;
> +
> +  while ((MpHandOff = GetMpHandOffHob (MaxLogicalProcessorNumber)) != 0) {
> +    DEBUG ((
> +      DEBUG_INFO,
> +      "%a: ProcessorIndex=%u CpuCount=%u\n",
> +      __func__,
> +      MpHandOff->ProcessorIndex,
> +      MpHandOff->CpuCount
> +      ));
> +    MaxLogicalProcessorNumber += MpHandOff->CpuCount;
> +    HaveMpHandOff              = TRUE;
> +  }
> +
> +  if (!HaveMpHandOff) {
>      MaxLogicalProcessorNumber = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
> -  } else {
> -    MaxLogicalProcessorNumber = MpHandOff->CpuCount;
>    }
>  
>    ASSERT (MaxLogicalProcessorNumber != 0);

How about:

- just looping over the GUID HOBs, summing their CpuCount fields,

- replacing the HaveMpHandOff variable with the sum being nonzero, after
the loop

(One extra assertion, on top, could be: while iterating over the GUID
HOB list, also perform a maximum search for (MpHandOff->ProcessorIndex +
MpHandOff->CpuCount), and after the loop, check that the maximum found
like this equals the sum of CpuCount fields. But this is really an extra.)

In other words -- again, I don't see any need for going through the HOBs
in a particular order, just for summing their CpuCount fields.


> @@ -2189,7 +2202,7 @@ MpInitLibInitialize (
>    //
>    ProgramVirtualWireMode ();
>  
> -  if (MpHandOff == NULL) {
> +  if (!HaveMpHandOff) {
>      if (MaxLogicalProcessorNumber > 1) {
>        //
>        // Wakeup all APs and calculate the processor count in system
> @@ -2205,19 +2218,26 @@ MpInitLibInitialize (
>        AmdSevUpdateCpuMpData (CpuMpData);
>      }

Hm, okay, considering this code, I do agree we need the "HaveMpHandOff"
variable. Because, at this point, MaxLogicalProcessorNumber will be
nonzero, regardless of how we calculated it it.

>  
> -    CpuMpData->CpuCount  = MpHandOff->CpuCount;
> +    CpuMpData->CpuCount  = MaxLogicalProcessorNumber;
>      CpuMpData->BspNumber = GetBspNumber ();
>      CpuInfoInHob         = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob;
> -    for (Index = 0; Index < CpuMpData->CpuCount; Index++) {

This original code here is a bit confused. Namely, after briefly
auditing MpInitLibInitialize(), it seems like "Index" should always have
been UINT32; making it UINTN is a confusing and unneeded generality.
(But, if you disagree with my claim, please say so.)

And the new variable HobIndex should be UINT32 too.

> -      InitializeSpinLock (&CpuMpData->CpuData[Index].ApLock);
> -      CpuMpData->CpuData[Index].CpuHealthy = (MpHandOff->Info[Index].Health == 0) ? TRUE : FALSE;

Comment on the pre-patch code:

  Predicate ? TRUE : FALSE

is poor style.

> -      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 = GetMpHandOffHob (0);
> +         MpHandOff != NULL;
> +         MpHandOff = GetMpHandOffHob (MpHandOff->ProcessorIndex + MpHandOff->CpuCount))
> +    {
> +      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;
> +      }
>      }

Same observation as before: we don't need to *dicate* the HOB order for
these nested loops, we can (and should) just process the HOBs in
whatever order they appear. That makes the outer loop linear, rather
than quadratic.

>  
> +    MpHandOff = GetMpHandOffHob (0);
>      DEBUG ((DEBUG_INFO, "MpHandOff->WaitLoopExecutionMode: %04d, sizeof (VOID *): %04d\n", MpHandOff->WaitLoopExecutionMode, sizeof (VOID *)));
>      if (MpHandOff->WaitLoopExecutionMode == sizeof (VOID *)) {
>        ASSERT (CpuMpData->ApLoopMode != ApInHltLoop);

This indicates that a refactoring -- "normalization" -- of the
MP_HAND_OFF structure is necessary.

Namely, each MP_HAND_OFF HOB contains a WaitLoopExecutionMode field, but
their values are expected to be identical. And here you have to use one
of the HOBs (doesn't matter which one) for getting that value.

The WaitLoopExecutionMode field should be a singleton, regardless of how
many MP_HAND_OFF HOBs exist. It could be a separate GUID HOB, or perhaps
(for simplicity?) a dynamic UINT32 PCD.

... In fact, the same seems to apply to the StartupSignalValue field.
That field is only ever set to MP_HAND_OFF_SIGNAL, and that setting only
depends on "CpuMpData->ApLoopMode". Therefore both WaitLoopExecutionMode
and StartupSignalValue are singletons, and should not be duplicated;
they should be "normalized out" to dynamic PCDs or a separate (unique)
GUID HOB.

> @@ -2284,7 +2304,7 @@ MpInitLibInitialize (
>    // Wakeup APs to do some AP initialize sync (Microcode & MTRR)
>    //
>    if (CpuMpData->CpuCount > 1) {
> -    if (MpHandOff != NULL) {
> +    if (HaveMpHandOff) {
>        //
>        // 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
> @@ -2301,7 +2321,7 @@ MpInitLibInitialize (
>        CpuPause ();
>      }
>  
> -    if (MpHandOff != NULL) {
> +    if (HaveMpHandOff) {
>        CpuMpData->InitFlag = ApInitDone;
>      }
>  

Laszlo



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

[edk2-devel] [PATCH 5/5] 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.  The actual CPU count per HOB
is much smaller (128) for better test coverage.

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

diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
index f80e00edcff3..a5710789c8c3 100644
--- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
@@ -126,35 +126,41 @@ SaveCpuMpData (
   IN CPU_MP_DATA  *CpuMpData
   )
 {
-  UINT64           Data64;
-  UINTN            Index;
-  CPU_INFO_IN_HOB  *CpuInfoInHob;
-  MP_HAND_OFF      *MpHandOff;
-  UINTN            MpHandOffSize;
+  STATIC CONST UINT32  CpusPerHob = 128;
+  UINT64               Data64;
+  UINT32               Index, HobBase;
+  CPU_INFO_IN_HOB      *CpuInfoInHob;
+  MP_HAND_OFF          *MpHandOff;
+  UINTN                MpHandOffSize;
 
   //
   // 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 % CpusPerHob == 0) {
+      MpHandOffSize = sizeof (MP_HAND_OFF) + sizeof (PROCESSOR_HAND_OFF) * CpusPerHob;
+      MpHandOff     = (MP_HAND_OFF *)BuildGuidHob (&mMpHandOffGuid, MpHandOffSize);
+      ASSERT (MpHandOff != NULL);
+      ZeroMem (MpHandOff, MpHandOffSize);
 
-  for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
-    MpHandOff->Info[Index].ApicId = CpuInfoInHob[Index].ApicId;
-    MpHandOff->Info[Index].Health = CpuInfoInHob[Index].Health;
+      HobBase                   = Index;
+      MpHandOff->ProcessorIndex = HobBase;
+      MpHandOff->CpuCount       = MIN (CpuMpData->CpuCount - HobBase, CpusPerHob);
+
+      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.1



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

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

[Ni, Ray]

> +  STATIC CONST UINT32  CpusPerHob = 128;

1. Can you refer to https://github.com/tianocore/edk2/blob/9979c887ef54023c9e262242db02c92d11b7f1e5/UefiCpuPkg/CpuMpPei/CpuMpPei.c#L572 to avoid
hardcode 128?
The MP_HAND_OFF structure change may cause 128 an invalid value.


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

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

[Laszlo Ersek]

On 2/15/24 10:31, Gerd Hoffmann wrote:
> Add support for splitting Hand-Off data into multiple HOBs.  This is
> required for VMs with thousands of CPUs.  The actual CPU count per HOB
> is much smaller (128) for better test coverage.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 48 ++++++++++++++-----------
>  1 file changed, 27 insertions(+), 21 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index f80e00edcff3..a5710789c8c3 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> @@ -126,35 +126,41 @@ SaveCpuMpData (
>    IN CPU_MP_DATA  *CpuMpData
>    )
>  {
> -  UINT64           Data64;
> -  UINTN            Index;
> -  CPU_INFO_IN_HOB  *CpuInfoInHob;
> -  MP_HAND_OFF      *MpHandOff;
> -  UINTN            MpHandOffSize;
> +  STATIC CONST UINT32  CpusPerHob = 128;

This should be a fixed-at-build PCD. Easy to modify on the build command
line, for test coverage, but for production builds, it should be as
large as possible.

In fact, the code should determine how many CPU entries fit in a single
HOB [*], and the PCD should be checked against it:

- PCD == 0: use the maximum
- PCD > maximum: assert
- otherwise: use the PCD as chunking factor

[*] See BuildGuidHob() in "MdePkg/Library/PeiHobLib/HobLib.c":

|   //
|   // Make sure that data length is not too long.
|   //
|   ASSERT (DataLength <= (0xFFF8 - sizeof (EFI_HOB_GUID_TYPE)));

So the max permitted payload size is 0xFFE0 bytes, if I count right.

  CpusPerHob = (0xFFE0 - sizeof (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);


> +  UINT64               Data64;
> +  UINT32               Index, HobBase;
> +  CPU_INFO_IN_HOB      *CpuInfoInHob;
> +  MP_HAND_OFF          *MpHandOff;
> +  UINTN                MpHandOffSize;
>
>    //
>    // 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 % CpusPerHob == 0) {
> +      MpHandOffSize = sizeof (MP_HAND_OFF) + sizeof (PROCESSOR_HAND_OFF) * CpusPerHob;

I don't like that the last HOB will only be partially filled in, most of
the time. Especially the max chunking factor -- which strives to create
HOBs that are approximately 64KB in size -- might waste ~32KB on
average, using a flat multiplication like the above.

How about:

  UINT32              FreeInHob;
  PROCESSOR_HAND_OFF  *Info;

  FreeInHob = 0;
  for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
    if (FreeInHob == 0) {
      FreeInHob     = MIN (CpusPerHob, CpuMpData->CpuCount - Index);
      MpHandOffSize = sizeof *MpHandOff + FreeInHob * sizeof *Info;
      MpHandOff     = BuildGuidHob (&mMpHandOffGuid, MpHandOffSize);
      ASSERT (MpHandOff != NULL);
      ZeroMem (MpHandOff, MpHandOffSize);
      MpHandOff->ProcessorIndex = Index;
      MpHandOff->CpuCount       = FreeInHob;
      Info                      = MpHandOff->Info;
    }

    Info->ApicId = CpuInfoInHob[Index].ApicId;
    Info->Health = CpuInfoInHob[Index].Health;
    if (CpuMpData->ApLoopMode != ApInHltLoop) {
      Info->StartupSignalAddress    = (UINT64)(UINTN)CpuMpData->CpuData[Index].StartupApSignal;
      Info->StartupProcedureAddress = (UINT64)(UINTN)&CpuMpData->CpuData[Index].ApFunction;
    }

    Info++;
    FreeInHob--;
  }

And then "HobBase" becomes superfluous. (Well, having a HobBase that
carries information between iterations of the loop, versus an Info
pointer, is conceptually the same; it's just that the Info pointer
allows for shorter expressions.)

The UINTN -> UINT32 type change for Index looks fine, however.

> +      MpHandOff     = (MP_HAND_OFF *)BuildGuidHob (&mMpHandOffGuid, MpHandOffSize);
> +      ASSERT (MpHandOff != NULL);
> +      ZeroMem (MpHandOff, MpHandOffSize);
>
> -  for (Index = 0; Index < MpHandOff->CpuCount; Index++) {
> -    MpHandOff->Info[Index].ApicId = CpuInfoInHob[Index].ApicId;
> -    MpHandOff->Info[Index].Health = CpuInfoInHob[Index].Health;
> +      HobBase                   = Index;
> +      MpHandOff->ProcessorIndex = HobBase;
> +      MpHandOff->CpuCount       = MIN (CpuMpData->CpuCount - HobBase, CpusPerHob);

Yes, the MIN() here is my key point, but I think we should also let it
control the allocation!

> +
> +      if (CpuMpData->ApLoopMode != ApInHltLoop) {
> +        MpHandOff->StartupSignalValue    = MP_HAND_OFF_SIGNAL;
> +        MpHandOff->WaitLoopExecutionMode = sizeof (VOID *);
> +      }
> +    }

As noted elsewhere, these fields don't belong in the loop (they don't
belong to MP_HAND_OFF, going forward); the two of them together should
form a new singleton structure.

> +
> +    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;
>      }
>    }
>

Laszlo



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

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

[Ni, Ray]

> 
> > +
> > +      if (CpuMpData->ApLoopMode != ApInHltLoop) {
> > +        MpHandOff->StartupSignalValue    = MP_HAND_OFF_SIGNAL;
> > +        MpHandOff->WaitLoopExecutionMode = sizeof (VOID *);
> > +      }
> > +    }
> 
> As noted elsewhere, these fields don't belong in the loop (they don't
> belong to MP_HAND_OFF, going forward); the two of them together should
> form a new singleton structure.
> 

I agree the StartupSignalValue and WaitLoopExecutionMode are duplicated fields
if multiple MP_HAND_OFF instances are created.

I am ok to leave them as duplicates in this patch series.



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