Re: [Patch V3 3/5] UefiCpuPkg: Create MpHandOff.

["Yuanhao Xie" <yuanhao.xie@intel.com>]

Hi Ray,

Please check V4 patch series.

Thanks for the feedbacks.
Yuanhao


> 
> The SaveCpuMpData() function was updated to construct the MP_HAND_OFF 
> Hob. Additionally, the function introduced the MP_HAND_OFF_SIGNAL, 
> which solely served the purpose of awakening the APs and transitioning 
> their context from PEI to DXE. The WaitLoopExecutionMode field 
> indicated whether the bit mode of PEI matched that of DXE. Both of 
> them were filled only if the ApLoopMode was not ApInHltLoop. In the 
> case of ApLoopMode, it remained necessary

1. "ApLoopMode" -> "ApInHltMode"?
2. can you update commit message to highlight that this patch still sends INIT-SIPI-SIPI even AP is wait in run-loop or mwait loop?
A: I updated commit message in patch V4.
> +
> +/**
> +  Get pointer to CPU MP Data structure from GUIDed HOB.
> +
> +  @param[in] CpuMpData  The pointer to CPU MP Data structure.
> +**/
> +VOID
> +AmdSevUpdateCpuMpData (
> +  IN CPU_MP_DATA  *CpuMpData
> +  )
> +{
> +  CPU_MP_DATA  *OldCpuMpData;
> +
> +  OldCpuMpData = GetCpuMpDataFromGuidedHob ();
> +
> +  OldCpuMpData->NewCpuMpData = CpuMpData; }

3. Can you please move this function implementation into MpInitLib/AmdSev.c?
A: This function is added in AmdSev.c


> +//
> +// To trigger the start-up signal, BSP writes the specified // 
> +StartupSignalValue to the StartupSignalAddress of each processor.
> +// This address is monitored by the APs, and as soon as they receive 
> +// the value that matches the MP_HAND_OFF_SIGNAL, they will wake up 
> +// and switch the context from PEI to DXE phase.
> +//
> +#define MP_HAND_OFF_SIGNAL  SIGNATURE_32 ('M', 'P', 'H', 'O')
4. It's ok to define the signal signature here.

> +#define MP_HANDOFF_GUID \
> +  { \
> +    0x11e2bd88, 0xed38, 0x4abd, {0xa3, 0x99, 0x21, 0xf2, 0x5f, 0xd0, 
> +0x7a, 0x60 }
> \
> +  }

5. can you please move this GUID definition to MpInitLib/MpHandOff.h?
A: MP_HANDOFF_GUID  and 2 structure definitions in below are put in MpInitLib/MpHandOff in patch V4.


> +//
> +// The information required to transfer from the PEI phase to the // 
> +DXE phase is contained within the MP_HAND_OFF and
> PROCESSOR_HAND_OFF.
> +// If the SizeOfPointer (WaitLoopExecutionMode) of both phases are 
> +equal, // and the APs is not in halt mode, // then the APs can be 
> +awakened by triggering the start-up // signal, rather than using 
> +INIT-SIPI-SIPI.
> +// To trigger the start-up signal, BSP writes the specified // 
> +StartupSignalValue to the StartupSignalAddress of each processor.
> +// This address is monitored by the APs.
> +//
> +typedef struct {
> +  UINT32    ApicId;
> +  UINT32    Health;
> +  UINT64    StartupSignalAddress;
> +  UINT64    StartupProcedureAddress;
> +} PROCESSOR_HAND_OFF;
> +
> +typedef struct {
> +  //
> +  // The ProcessorIndex indicates the range of processors. If it is 
> +set to 0, it
> signifies
> +  // processors from 0 to CpuCount - 1. Multiple instances in the HOB 
> + list
> describe
> +  // processors from ProcessorIndex to ProcessorIndex + CpuCount - 1.
> +  //
> +  UINT32                ProcessorIndex;
> +  UINT32                CpuCount;
> +  UINT32                WaitLoopExecutionMode;
> +  UINT32                StartupSignalValue;
> +  PROCESSOR_HAND_OFF    Info[];
> +} MP_HAND_OFF;

6. can you please move the above 2 structure definitions to MpInitLib/MpHandOff.h?

> +
>  //
>  // Data structure for microcode patch information  // @@ -347,6 
> +391,7 @@ typedef
>    );
> 
>  extern EFI_GUID  mCpuInitMpLibHobGuid;
> +extern EFI_GUID  mMpHandOffGuid;
> 
>  /**
>    Assembly code to place AP into safe loop mode.
> @@ -452,6 +497,17 @@ SaveCpuMpData (
>    IN CPU_MP_DATA  *CpuMpData
>    );
> 
> +/**
> +  This function Get BspNumber.
> +
> +  @param[in] MpHandOff        Pointer to MpHandOff
> +  @return                     BspNumber
> +**/
> +UINT32
> +GetBspNumber (
> +  IN CONST MP_HAND_OFF  *MpHandOff
> +  );

7. It seems above "GetBspNumber" prototype declaration can be removed. Can you confirm?
A: I agree. GetBspNumber is removed in patch V4.


> +
>  /**
>    Get available system memory below 1MB by specified size.
> 
> @@ -652,6 +708,16 @@ EnableDisableApWorker (
>    IN  UINT32   *HealthFlag OPTIONAL
>    );
> 
> +/**
> +  Get pointer to MP_HAND_OFF GUIDed HOB.
> +
> +  @return  The pointer to MP_HAND_OFF structure.
> +**/
> +MP_HAND_OFF *
> +GetMpHandOffHob (
> +  VOID
> +  );
> +

8. It seems above "GetMpHandOffHob" prototype declaration can be removed if you implement it in front of
    MpInitLibInitialize() in MpLib.c. can you confirm?
A: I agree. GetMpHandOffHob is removed in patch V4.