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

["Laszlo Ersek" <lersek@redhat.com>]

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