Re: [edk2-devel] [PATCH 1/1] UefiCpuPkg/MpInitLib: add struct MP_HAND_OFF_CONFIG

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

On 2/27/24 12:41, Gerd Hoffmann wrote:
> Move the WaitLoopExecutionMode and StartupSignalValue fields to a
> separate HOB with the new struct.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpHandOff.h | 13 ++++++--
>  UefiCpuPkg/Library/MpInitLib/MpLib.h     |  3 +-
>  UefiCpuPkg/Library/MpInitLib/MpLib.c     | 38 ++++++++++++++++++++----
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c  | 34 +++++++++++++--------
>  4 files changed, 66 insertions(+), 22 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpHandOff.h b/UefiCpuPkg/Library/MpInitLib/MpHandOff.h
> index 77854d6a81f8..ae93b7e3d7c9 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpHandOff.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpHandOff.h
> @@ -15,7 +15,13 @@
>      0x11e2bd88, 0xed38, 0x4abd, {0xa3, 0x99, 0x21, 0xf2, 0x5f, 0xd0, 0x7a, 0x60 } \
>    }
>  
> +#define MP_HANDOFF_CONFIG_GUID \
> +  { \
> +    0xdabbd793, 0x7b46, 0x4144, {0x8a, 0xd4, 0x10, 0x1c, 0x7c, 0x08, 0xeb, 0xfa } \
> +  }
> +
>  extern EFI_GUID  mMpHandOffGuid;
> +extern EFI_GUID  mMpHandOffConfigGuid;
>  
>  //
>  // The information required to transfer from the PEI phase to the
> @@ -43,8 +49,11 @@ typedef struct {
>    //
>    UINT32                ProcessorIndex;
>    UINT32                CpuCount;
> -  UINT32                WaitLoopExecutionMode;
> -  UINT32                StartupSignalValue;
>    PROCESSOR_HAND_OFF    Info[];
>  } MP_HAND_OFF;
> +
> +typedef struct {
> +  UINT32    WaitLoopExecutionMode;
> +  UINT32    StartupSignalValue;
> +} MP_HAND_OFF_CONFIG;
>  #endif
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 3a7b9896cff4..d26035559f22 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -482,7 +482,8 @@ GetWakeupBuffer (
>  **/
>  VOID
>  SwitchApContext (
> -  IN CONST MP_HAND_OFF  *FirstMpHandOff
> +  IN CONST MP_HAND_OFF_CONFIG  *MpHandOffConfig,
> +  IN CONST MP_HAND_OFF         *FirstMpHandOff
>    );
>  
>  /**
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 8c186211fb38..a22bcfc0bc30 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -15,6 +15,7 @@
>  
>  EFI_GUID  mCpuInitMpLibHobGuid = CPU_INIT_MP_LIB_HOB_GUID;
>  EFI_GUID  mMpHandOffGuid       = MP_HANDOFF_GUID;
> +EFI_GUID  mMpHandOffConfigGuid = MP_HANDOFF_CONFIG_GUID;
>  
>  /**
>    Save the volatile registers required to be restored following INIT IPI.
> @@ -1935,11 +1936,13 @@ GetBspNumber (
>    This procedure allows the AP to switch to another section of
>    memory and continue its loop there.
>  
> -  @param[in] FirstMpHandOff  Pointer to first MP hand-off HOB body.
> +  @param[in] MpHandOffConfig  Pointer to MP hand-off config HOB body.
> +  @param[in] FirstMpHandOff   Pointer to first MP hand-off HOB body.
>  **/
>  VOID
>  SwitchApContext (
> -  IN CONST MP_HAND_OFF  *FirstMpHandOff
> +  IN CONST MP_HAND_OFF_CONFIG  *MpHandOffConfig,
> +  IN CONST MP_HAND_OFF         *FirstMpHandOff
>    )
>  {
>    UINTN              Index;
> @@ -1955,7 +1958,7 @@ SwitchApContext (
>      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;
> +        *(UINT32 *)(UINTN)MpHandOff->Info[Index].StartupSignalAddress   = MpHandOffConfig->StartupSignalValue;
>        }
>      }
>    }
> @@ -1975,6 +1978,26 @@ SwitchApContext (
>    }
>  }
>  
> +/**
> +  Get pointer to MP_HAND_OFF_CONFIG GUIDed HOB body.
> +
> +  @return  The pointer to MP_HAND_OFF_CONFIG structure.
> +**/
> +MP_HAND_OFF_CONFIG *
> +GetMpHandOffConfigHob (
> +  VOID
> +  )
> +{
> +  EFI_HOB_GUID_TYPE  *GuidHob;
> +
> +  GuidHob = GetFirstGuidHob (&mMpHandOffConfigGuid);
> +  if (GuidHob == NULL) {
> +    return NULL;
> +  }
> +
> +  return (MP_HAND_OFF_CONFIG *)GET_GUID_HOB_DATA (GuidHob);
> +}
> +
>  /**
>    Get pointer to next MP_HAND_OFF GUIDed HOB body.
>  
> @@ -2022,6 +2045,7 @@ MpInitLibInitialize (
>    VOID
>    )
>  {
> +  MP_HAND_OFF_CONFIG       *MpHandOffConfig;
>    MP_HAND_OFF              *FirstMpHandOff;
>    MP_HAND_OFF              *MpHandOff;
>    CPU_INFO_IN_HOB          *CpuInfoInHob;
> @@ -2239,13 +2263,15 @@ MpInitLibInitialize (
>        }
>      }
>  
> +    MpHandOffConfig = GetMpHandOffConfigHob ();
> +    ASSERT (MpHandOffConfig != NULL);

So, in connection to the other subthread

Re: [edk2-devel] CodeQL Analysis in edk2
msgid: <80abb140-9a9c-43b8-ba0b-d8ea631d9051@linux.microsoft.com>
https://edk2.groups.io/g/devel/message/116054

I suggest replacing this with:

    if (MpHandOffConfig == NULL) {
      DEBUG ((
        DEBUG_ERROR,
        "%a: at least one MpHandOff HOB, but no MpHandOffConfig HOB\n",
        __func__
      ));
      ASSERT (MpHandOffConfig != NULL);
      CpuDeadLoop ();
    }

(We should use a generic "panic" here; but for now, CpuDeadLoop() should
do. There are two prior examples for that in MpInitLib already.)

With that update:

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

Thanks
Laszlo


>      DEBUG ((
>        DEBUG_INFO,
>        "FirstMpHandOff->WaitLoopExecutionMode: %04d, sizeof (VOID *): %04d\n",
> -      FirstMpHandOff->WaitLoopExecutionMode,
> +      MpHandOffConfig->WaitLoopExecutionMode,
>        sizeof (VOID *)
>        ));
> -    if (FirstMpHandOff->WaitLoopExecutionMode == sizeof (VOID *)) {
> +    if (MpHandOffConfig->WaitLoopExecutionMode == sizeof (VOID *)) {
>        ASSERT (CpuMpData->ApLoopMode != ApInHltLoop);
>  
>        CpuMpData->FinishedCount                        = 0;
> @@ -2261,7 +2287,7 @@ MpInitLibInitialize (
>        // enables the APs to switch to a different memory section and continue their
>        // looping process there.
>        //
> -      SwitchApContext (FirstMpHandOff);
> +      SwitchApContext (MpHandOffConfig, FirstMpHandOff);
>        //
>        // Wait for all APs finished initialization
>        //
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index ec1aa666923d..4d3acb491f36 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> @@ -126,14 +126,15 @@ SaveCpuMpData (
>    IN CPU_MP_DATA  *CpuMpData
>    )
>  {
> -  UINT32           MaxCpusPerHob;
> -  UINT32           CpusInHob;
> -  UINT64           Data64;
> -  UINT32           Index;
> -  UINT32           HobBase;
> -  CPU_INFO_IN_HOB  *CpuInfoInHob;
> -  MP_HAND_OFF      *MpHandOff;
> -  UINTN            MpHandOffSize;
> +  UINT32              MaxCpusPerHob;
> +  UINT32              CpusInHob;
> +  UINT64              Data64;
> +  UINT32              Index;
> +  UINT32              HobBase;
> +  CPU_INFO_IN_HOB     *CpuInfoInHob;
> +  MP_HAND_OFF         *MpHandOff;
> +  MP_HAND_OFF_CONFIG  MpHandOffConfig;
> +  UINTN               MpHandOffSize;
>  
>    MaxCpusPerHob = (0xFFF8 - sizeof (EFI_HOB_GUID_TYPE) - sizeof (MP_HAND_OFF)) / sizeof (PROCESSOR_HAND_OFF);
>  
> @@ -155,11 +156,6 @@ SaveCpuMpData (
>  
>        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;
> @@ -170,6 +166,18 @@ SaveCpuMpData (
>      }
>    }
>  
> +  ZeroMem (&MpHandOffConfig, sizeof (MpHandOffConfig));
> +  if (CpuMpData->ApLoopMode != ApInHltLoop) {
> +    MpHandOffConfig.StartupSignalValue    = MP_HAND_OFF_SIGNAL;
> +    MpHandOffConfig.WaitLoopExecutionMode = sizeof (VOID *);
> +  }
> +
> +  BuildGuidDataHob (
> +    &mMpHandOffConfigGuid,
> +    (VOID *)&MpHandOffConfig,
> +    sizeof (MpHandOffConfig)
> +    );
> +
>    //
>    // Build location of CPU MP DATA buffer in HOB
>    //



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