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

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

[Gerd Hoffmann]

Needed to boot guests with thousands of vcpus.

v2:
 - rework HOB loops for better performance: O(n) instead of O(n^2).

Gerd Hoffmann (5):
  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/Library/MpInitLib/MpLib.h    |  14 ++-
 UefiCpuPkg/Library/MpInitLib/MpLib.c    | 150 ++++++++++++++++--------
 UefiCpuPkg/Library/MpInitLib/PeiMpLib.c |  44 ++++---
 3 files changed, 140 insertions(+), 68 deletions(-)

-- 
2.43.2



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

[edk2-devel] [PATCH v2 1/5] 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 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..bc2a0232291d 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.
+
+  @param[in] MpHandOff  Previous HOB.  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..e764bc9e4228 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.
+
+  @param[in] MpHandOff  Previous HOB.  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 (#115667): https://edk2.groups.io/g/devel/message/115667
Mute This Topic: https://groups.io/mt/104472308/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ni, Ray]

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Wednesday, February 21, 2024 1:50 AM
> To: devel@edk2.groups.io
> Cc: Oliver Steffen <osteffen@redhat.com>; Laszlo Ersek
> <lersek@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [PATCH v2 1/5] UefiCpuPkg/MpInitLib: Add support for multiple
> HOBs to GetMpHandOffHob
> 
> Rename the function to GetNextMpHandOffHob(), add MP_HAND_OFF
> parameter.
> When called with NULL pointer return 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..bc2a0232291d 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.
> +
> +  @param[in] MpHandOff  Previous HOB.  Pass NULL to get the first
> HOB.

Can you please emphasize in above comments that MpHandOff points to the GUIDed HOB data?
The function implementation assumes that.



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

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

[Laszlo Ersek]

On 2/21/24 04:35, Ni, Ray wrote:
>> -----Original Message-----
>> From: Gerd Hoffmann <kraxel@redhat.com>
>> Sent: Wednesday, February 21, 2024 1:50 AM
>> To: devel@edk2.groups.io
>> Cc: Oliver Steffen <osteffen@redhat.com>; Laszlo Ersek
>> <lersek@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Ni, Ray
>> <ray.ni@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
>> Subject: [PATCH v2 1/5] UefiCpuPkg/MpInitLib: Add support for multiple
>> HOBs to GetMpHandOffHob
>>
>> Rename the function to GetNextMpHandOffHob(), add MP_HAND_OFF
>> parameter.
>> When called with NULL pointer return 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..bc2a0232291d 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.
>> +
>> +  @param[in] MpHandOff  Previous HOB.  Pass NULL to get the first
>> HOB.
> 
> Can you please emphasize in above comments that MpHandOff points to the GUIDed HOB data?
> The function implementation assumes that.

+1; had the same observation.

Laszlo



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

[edk2-devel] [PATCH v2 2/5] 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>
---
 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 e764bc9e4228..8f198ff6d817 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.
   @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 (#115668): https://edk2.groups.io/g/devel/message/115668
Mute This Topic: https://groups.io/mt/104472310/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ni, Ray]

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

Thanks,
Ray
> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Wednesday, February 21, 2024 1:50 AM
> To: devel@edk2.groups.io
> Cc: Oliver Steffen <osteffen@redhat.com>; Laszlo Ersek
> <lersek@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [PATCH v2 2/5] UefiCpuPkg/MpInitLib: Add support for multiple
> HOBs to GetBspNumber()
> 
> 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>
> ---
>  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 e764bc9e4228..8f198ff6d817 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.
>    @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 (#115705): https://edk2.groups.io/g/devel/message/115705
Mute This Topic: https://groups.io/mt/104472310/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ni, Ray]

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

Thanks,
Ray
> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Wednesday, February 21, 2024 1:50 AM
> To: devel@edk2.groups.io
> Cc: Oliver Steffen <osteffen@redhat.com>; Laszlo Ersek
> <lersek@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [PATCH v2 2/5] UefiCpuPkg/MpInitLib: Add support for multiple
> HOBs to GetBspNumber()
> 
> 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>
> ---
>  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 e764bc9e4228..8f198ff6d817 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.
>    @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 (#115704): https://edk2.groups.io/g/devel/message/115704
Mute This Topic: https://groups.io/mt/104472310/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

On 2/20/24 18:49, Gerd Hoffmann wrote:
> 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>
> ---
>  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 e764bc9e4228..8f198ff6d817 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.
>    @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;
> +      }
>      }
>    }
>  

It's strange that the pre-patch code keeps looking even after finding
the BSP by APIC-ID; now it's stranger yet, because we even go to further
HOBs.

In theory, the inner loop body could grow a "break", and the outer loop
condition could be restricted with

  && (BspNumber == MAX_UINT32)

but that would be a separate patch.

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



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

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

[Laszlo Ersek]

On 2/20/24 18:49, Gerd Hoffmann wrote:
> 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>
> ---
>  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 e764bc9e4228..8f198ff6d817 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.

... also, it would be more precise to say "first MpHandOff HOB body".

Laszlo

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



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

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

[Ni, Ray]

> > +  @param[in] FirstMpHandOff   Pointer to first MpHandOff HOB.
> 
> ... also, it would be more precise to say "first MpHandOff HOB body".
> 
"body" is a very good term that emphasizes it doesn't point to a separate storage. 



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

[edk2-devel] [PATCH v2 3/5] 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>
---
 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 bc2a0232291d..b5214b904b41 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 8f198ff6d817..c13de34e5769 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.
 **/
 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 (#115665): https://edk2.groups.io/g/devel/message/115665
Mute This Topic: https://groups.io/mt/104472306/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ni, Ray]

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

Thanks,
Ray
> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Wednesday, February 21, 2024 1:50 AM
> To: devel@edk2.groups.io
> Cc: Oliver Steffen <osteffen@redhat.com>; Laszlo Ersek
> <lersek@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
> Subject: [PATCH v2 3/5] UefiCpuPkg/MpInitLib: Add support for multiple
> HOBs to SwitchApContext()
> 
> 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>
> ---
>  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 bc2a0232291d..b5214b904b41 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 8f198ff6d817..c13de34e5769 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.
>  **/
>  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 (#115713): https://edk2.groups.io/g/devel/message/115713
Mute This Topic: https://groups.io/mt/104472306/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

On 2/20/24 18:49, Gerd Hoffmann wrote:
> 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>
> ---
>  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 bc2a0232291d..b5214b904b41 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 8f198ff6d817..c13de34e5769 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.

comment should say sth like "Pointer to first MP hand-off HOB body".

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


>  **/
>  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));
> +      }
>      }
>    }
>  }



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

[edk2-devel] [PATCH v2 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 | 68 +++++++++++++++++++---------
 1 file changed, 47 insertions(+), 21 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index c13de34e5769..80585f676b1d 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 (#115669): https://edk2.groups.io/g/devel/message/115669
Mute This Topic: https://groups.io/mt/104472311/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ni, Ray]

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

The commit message may need to be updated as "HaveMpHandOff" variable is gone in this version.

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



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

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

[Laszlo Ersek]

On 2/20/24 18:49, 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 | 68 +++++++++++++++++++---------
>  1 file changed, 47 insertions(+), 21 deletions(-)

as Ray says, the commit message is a bit stale

> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index c13de34e5769..80585f676b1d 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;

The code looks otherwise OK, but I'm not happy that
WaitLoopExecutionMode (and StartupSignalValue) are replicated over all
the HOBs, just like in v1. IMO, that will only make it harder for others
to understand the code / data structures, and therefore it increases
technical debt.

I understand that Ray is OK with that, so I won't try to block the
patch, but I'm not comfortable giving it an R-b myself, due to the
increase in technical debt.

Laszlo

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



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

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

[Gerd Hoffmann]

  Hi,
 
> The code looks otherwise OK, but I'm not happy that
> WaitLoopExecutionMode (and StartupSignalValue) are replicated over all
> the HOBs, just like in v1. IMO, that will only make it harder for others
> to understand the code / data structures, and therefore it increases
> technical debt.
> 
> I understand that Ray is OK with that, so I won't try to block the
> patch, but I'm not comfortable giving it an R-b myself, due to the
> increase in technical debt.

I can try to address that, but this certainly will be a separate
patch.

Given that the HOB structure is defined in locally in the library
I assume this is considered private and there are no compatibility
concerns when changing the structs?

Any preference where the fields should be moved to?  PCD?  Separate
HOB?  Something else?

take care,
  Gerd



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

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

[Ni, Ray]

I prefer HOB instead of dynamic PCD.
And let's keep the new singleton HOB structure as an internal interface between
PEI MpInitLib and DXE MpInitLib.


Thanks,
Ray
> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Thursday, February 22, 2024 8:29 PM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: devel@edk2.groups.io; Oliver Steffen <osteffen@redhat.com>; Kumar,
> Rahul R <rahul.r.kumar@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [PATCH v2 4/5] UefiCpuPkg/MpInitLib: Add support
> for multiple HOBs to MpInitLibInitialize
> 
>   Hi,
> 
> > The code looks otherwise OK, but I'm not happy that
> > WaitLoopExecutionMode (and StartupSignalValue) are replicated over all
> > the HOBs, just like in v1. IMO, that will only make it harder for others
> > to understand the code / data structures, and therefore it increases
> > technical debt.
> >
> > I understand that Ray is OK with that, so I won't try to block the
> > patch, but I'm not comfortable giving it an R-b myself, due to the
> > increase in technical debt.
> 
> I can try to address that, but this certainly will be a separate
> patch.
> 
> Given that the HOB structure is defined in locally in the library
> I assume this is considered private and there are no compatibility
> concerns when changing the structs?
> 
> Any preference where the fields should be moved to?  PCD?  Separate
> HOB?  Something else?
> 
> take care,
>   Gerd



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

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

[Laszlo Ersek]

On 2/23/24 01:23, Ni, Ray wrote:
> I prefer HOB instead of dynamic PCD.
> And let's keep the new singleton HOB structure as an internal interface between
> PEI MpInitLib and DXE MpInitLib.

Sounds good to me, thanks
Laszlo

>> -----Original Message-----
>> From: Gerd Hoffmann <kraxel@redhat.com>
>> Sent: Thursday, February 22, 2024 8:29 PM
>> To: Laszlo Ersek <lersek@redhat.com>
>> Cc: devel@edk2.groups.io; Oliver Steffen <osteffen@redhat.com>; Kumar,
>> Rahul R <rahul.r.kumar@intel.com>; Ni, Ray <ray.ni@intel.com>
>> Subject: Re: [edk2-devel] [PATCH v2 4/5] UefiCpuPkg/MpInitLib: Add support
>> for multiple HOBs to MpInitLibInitialize
>>
>>   Hi,
>>
>>> The code looks otherwise OK, but I'm not happy that
>>> WaitLoopExecutionMode (and StartupSignalValue) are replicated over all
>>> the HOBs, just like in v1. IMO, that will only make it harder for others
>>> to understand the code / data structures, and therefore it increases
>>> technical debt.
>>>
>>> I understand that Ray is OK with that, so I won't try to block the
>>> patch, but I'm not comfortable giving it an R-b myself, due to the
>>> increase in technical debt.
>>
>> I can try to address that, but this certainly will be a separate
>> patch.
>>
>> Given that the HOB structure is defined in locally in the library
>> I assume this is considered private and there are no compatibility
>> concerns when changing the structs?
>>
>> Any preference where the fields should be moved to?  PCD?  Separate
>> HOB?  Something else?
>>
>> take care,
>>   Gerd
> 



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

[edk2-devel] [PATCH v2 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 | 44 +++++++++++++++----------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
index f80e00edcff3..8a916a218016 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 = (MAX_UINT16 - 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 (#115670): https://edk2.groups.io/g/devel/message/115670
Mute This Topic: https://groups.io/mt/104472313/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ni, Ray]

> 
> +  MaxCpusPerHob = (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) - sizeof
> (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);

Above formula assumes the maximum HOB length could be 0xFFFF.
But actually the maximum HOB length could be only 0xFFF8 because
PeiCore::PeiCreateHob() contains following logic:

  if (0x10000 - Length <= 0x7) {
    return EFI_INVALID_PARAMETER;
  }
  Length = (UINT16)((Length + 0x7) & (~0x7));

The if-check is to guarantee (Length + 0x7) & ~0x7 doesn't produce value 0
when the Length > 0xFFF8.

So, that means the formula should use 0xFFF8 instead of 0xFFFF (MAX_UINT16).


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

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

[Gerd Hoffmann]

On Wed, Feb 21, 2024 at 03:48:25AM +0000, Ni, Ray wrote:
> > 
> > +  MaxCpusPerHob = (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) - sizeof
> > (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);
> 
> Above formula assumes the maximum HOB length could be 0xFFFF.

Which is IMHO correct.

> But actually the maximum HOB length could be only 0xFFF8 because
> PeiCore::PeiCreateHob() contains following logic:
> 
>   if (0x10000 - Length <= 0x7) {
>     return EFI_INVALID_PARAMETER;
>   }

That Length is the *data* size, the HOB header is not included.

The "- sizeof (EFI_HOB_GUID_TYPE)" in the formula above accounts the
space needed for HOB header and GUID.

take care,
  Gerd



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

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

[Laszlo Ersek]

On 2/21/24 11:24, Gerd Hoffmann wrote:
> On Wed, Feb 21, 2024 at 03:48:25AM +0000, Ni, Ray wrote:
>>>
>>> +  MaxCpusPerHob = (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) - sizeof
>>> (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);
>>
>> Above formula assumes the maximum HOB length could be 0xFFFF.
> 
> Which is IMHO correct.
> 
>> But actually the maximum HOB length could be only 0xFFF8 because
>> PeiCore::PeiCreateHob() contains following logic:
>>
>>   if (0x10000 - Length <= 0x7) {
>>     return EFI_INVALID_PARAMETER;
>>   }
> 
> That Length is the *data* size, the HOB header is not included.
> 
> The "- sizeof (EFI_HOB_GUID_TYPE)" in the formula above accounts the
> space needed for HOB header and GUID.


From the patch:

  MaxCpusPerHob = (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) - sizeof (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);
  ...
  CpusInHob     = MIN (CpuMpData->CpuCount - HobBase, MaxCpusPerHob);
  MpHandOffSize = sizeof (MP_HAND_OFF) + sizeof (PROCESSOR_HAND_OFF) * CpusInHob;
  MpHandOff     = (MP_HAND_OFF *)BuildGuidHob (&mMpHandOffGuid, MpHandOffSize);

Assuming that the division is exact, and that the MIN selects MaxCpusPerHob for CpusInHob, we get:

  MpHandOffSize = sizeof (MP_HAND_OFF) + sizeof (PROCESSOR_HAND_OFF) * (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) - sizeof (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);

the multiplication and the division cancel out (again, assuming exact division):

  MpHandOffSize = sizeof (MP_HAND_OFF) + (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) - sizeof (MP_HAND_OFF));

the sizeof (MP_HAND_OFF) addition and subtraction cancel out:

  MpHandOffSize = MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE);

then we get:

  MpHandOff     = (MP_HAND_OFF *)BuildGuidHob (&mMpHandOffGuid, MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE));

Inside BuildGuidHob() [MdePkg/Library/PeiHobLib/HobLib.c], we get

  DataLength = MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE);

and further

  ASSERT (DataLength <= (0xFFF8 - sizeof (EFI_HOB_GUID_TYPE)));

Substituting for DataLength,

  ASSERT (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) <= (0xFFF8 - sizeof (EFI_HOB_GUID_TYPE)));

which is

  ASSERT (MAX_UINT16  <= 0xFFF8);

which fails.

So, as Ray says, and as I wrote under v1, you need to use 0xFFF8 here, not MAX_UINT16.

... Under v1, I wrote

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

If you want to make "sizeof (EFI_HOB_GUID_TYPE)" -- 24 bytes -- explicit in that subtraction, that is fine, but then we just get back to 0xFFF8:

  CpusPerHob = (0xFFF8 - sizeof (EFI_HOB_GUID_TYPE) - sizeof (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);

Laszlo



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

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

[Laszlo Ersek]

(commenting once more, to explain the actual reason:)

On 2/21/24 11:24, Gerd Hoffmann wrote:
> On Wed, Feb 21, 2024 at 03:48:25AM +0000, Ni, Ray wrote:
>>>
>>> +  MaxCpusPerHob = (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) - sizeof
>>> (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);
>>
>> Above formula assumes the maximum HOB length could be 0xFFFF.
> 
> Which is IMHO correct.

It is not correct:

> 
>> But actually the maximum HOB length could be only 0xFFF8 because
>> PeiCore::PeiCreateHob() contains following logic:
>>
>>   if (0x10000 - Length <= 0x7) {
>>     return EFI_INVALID_PARAMETER;
>>   }
> 
> That Length is the *data* size, the HOB header is not included.
> 
> The "- sizeof (EFI_HOB_GUID_TYPE)" in the formula above accounts the
> space needed for HOB header and GUID.

Yes, but the problem is not that we need the extra space for
EFI_HOB_GUID_TYPE (24 bytes) -- we need the extra space for padding /
alignment.

This is what the PI spec says (v1.8, section "III-4.5.2 HOB Construction
Rules"):

  HOB construction must obey the following rules:

  [...]

  4. All HOBs must be multiples of 8 bytes in length. This requirement
     meets the alignment restrictions of the Itanium® processor family.

  [...]

And if your total *desired* HOB length exceeds 0xFFF8 (i.e., it falls in
[0xFFF9..0xFFFF]), then the required upwards rounding produces 0x1_0000.
But that rounded-up value cannot be expressed in
"EFI_HOB_GENERIC_HEADER.HobLength" -- a UINT16 field.

In short, you've got Itanium to thank for this. :)

Laszlo



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

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

[Laszlo Ersek]

On 2/22/24 02:49, Laszlo Ersek wrote:
> (commenting once more, to explain the actual reason:)
> 
> On 2/21/24 11:24, Gerd Hoffmann wrote:
>> On Wed, Feb 21, 2024 at 03:48:25AM +0000, Ni, Ray wrote:
>>>>
>>>> +  MaxCpusPerHob = (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) - sizeof
>>>> (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);
>>>
>>> Above formula assumes the maximum HOB length could be 0xFFFF.
>>
>> Which is IMHO correct.
> 
> It is not correct:
> 
>>
>>> But actually the maximum HOB length could be only 0xFFF8 because
>>> PeiCore::PeiCreateHob() contains following logic:
>>>
>>>   if (0x10000 - Length <= 0x7) {
>>>     return EFI_INVALID_PARAMETER;
>>>   }
>>
>> That Length is the *data* size, the HOB header is not included.

sorry, I meant to insert here: no; "Length" at this location does
include everything, except the padding.

The BuildGuidHob() library function takes a data length, and calculates
the total HOB length, by adding 24 bytes, for the EFI_HOB_GUID_TYPE header.

InternalPeiCreateHob() (also in "MdePkg/Library/PeiHobLib/HobLib.c")
already takes the total HOB length, and passes it on to the CreateHob()
PEI service -- which Ray quotes above.

The total HOB length cannot exceed 0xFFF8 (due to the rounding up, for
Itanium's sake), so the GUID HOB data length cannot exceed
(0xFFF8-sizeof(EFI_HOB_GUID_TYPE)).

Laszlo

>>
>> The "- sizeof (EFI_HOB_GUID_TYPE)" in the formula above accounts the
>> space needed for HOB header and GUID.
> 
> Yes, but the problem is not that we need the extra space for
> EFI_HOB_GUID_TYPE (24 bytes) -- we need the extra space for padding /
> alignment.
> 
> This is what the PI spec says (v1.8, section "III-4.5.2 HOB Construction
> Rules"):
> 
>   HOB construction must obey the following rules:
> 
>   [...]
> 
>   4. All HOBs must be multiples of 8 bytes in length. This requirement
>      meets the alignment restrictions of the Itanium® processor family.
> 
>   [...]
> 
> And if your total *desired* HOB length exceeds 0xFFF8 (i.e., it falls in
> [0xFFF9..0xFFFF]), then the required upwards rounding produces 0x1_0000.
> But that rounded-up value cannot be expressed in
> "EFI_HOB_GENERIC_HEADER.HobLength" -- a UINT16 field.
> 
> In short, you've got Itanium to thank for this. :)
> 
> Laszlo



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

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

[Laszlo Ersek]

On 2/20/24 18:49, 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.

(1) The mention of the count 128 now seems stale for the code.

> 
> 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..8a916a218016 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 = (MAX_UINT16 - sizeof (EFI_HOB_GUID_TYPE) - sizeof (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);

(2) MAX_UINT16 should be 0xFFF8 instead; see subthread.

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

(3) The conversion looks good otherwise, but I dislike that
StartupSignalValue and WaitLoopExecutionMode get uselessly replicated
over all HOBs. Again, it *increases* technical debt. It's fine to ignore
existent debt (if you can), but adding to it is not right.

Can you file a new TianoCore BZ, for moving these fields to separate
dynamic PCDs, or to a new singleton GUID HOB (containing both fields)?
If you assign that BZ to yourself, and reference it in patches #4 and
#5, I'll be happy to R-b version 3 of the series. (Which is something
I'd like to do.)

Version 3 may be mergeable regardless, of course, if Ray accepts it.

Thanks,
Laszlo



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