Re: [PATCH v2 3/5] UefiCpuPkg/DxeMpLib: Make sure APs in safe loop code

[Laszlo Ersek <lersek@redhat.com>]

On 11/25/16 07:03, Jeff Fan wrote:
> Add one semaphore to make sure BSP to wait till all APs run in AP safe loop
> code.
> 
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jeff Fan <jeff.fan@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c        | 8 +++++++-
>  UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 4 +++-
>  UefiCpuPkg/Library/MpInitLib/MpLib.h           | 3 ++-
>  UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm  | 4 +++-
>  4 files changed, 15 insertions(+), 4 deletions(-)

Looks good to me.

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

Thanks
Laszlo

> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index 5a3b02c..8f5074b 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -27,6 +27,7 @@ EFI_EVENT        mLegacyBootEvent = NULL;
>  volatile BOOLEAN mStopCheckAllApsStatus = TRUE;
>  VOID             *mReservedApLoopFunc = NULL;
>  UINTN            mReservedTopOfApStack;
> +volatile UINT32  mNumberToFinish = 0;
>  
>  /**
>    Get the pointer to CPU MP Data structure.
> @@ -253,7 +254,8 @@ RelocateApLoop (
>      MwaitSupport,
>      CpuMpData->ApTargetCState,
>      CpuMpData->PmCodeSegment,
> -    mReservedTopOfApStack - ProcessorNumber * AP_SAFE_STACK_SIZE
> +    mReservedTopOfApStack - ProcessorNumber * AP_SAFE_STACK_SIZE,
> +    (UINTN) &mNumberToFinish
>      );
>    //
>    // It should never reach here
> @@ -282,7 +284,11 @@ MpInitChangeApLoopCallback (
>    CpuMpData->SaveRestoreFlag = TRUE;
>    CpuMpData->PmCodeSegment = GetProtectedModeCS ();
>    CpuMpData->ApLoopMode = PcdGet8 (PcdCpuApLoopMode);
> +  mNumberToFinish = CpuMpData->CpuCount - 1;
>    WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL);
> +  while (mNumberToFinish > 0) {
> +    CpuPause ();
> +  }
>    DEBUG ((DEBUG_INFO, "%a() done!\n", __FUNCTION__));
>  }
>  
> diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> index 7ab136b..a48f0bc 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> @@ -215,7 +215,7 @@ CProcedureInvoke:
>  RendezvousFunnelProcEnd:
>  
>  ;-------------------------------------------------------------------------------------
> -;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack);
> +;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack, CountTofinish);
>  ;-------------------------------------------------------------------------------------
>  global ASM_PFX(AsmRelocateApLoop)
>  ASM_PFX(AsmRelocateApLoop):
> @@ -227,6 +227,8 @@ AsmRelocateApLoopStart:
>      mov        ebp, esp
>      mov        ebx, [eax + 8]      ; ApTargetCState
>      mov        ecx, [eax + 4]      ; MwaitSupport
> +    mov        eax, [eax + 20]     ; CountTofinish
> +    lock dec   dword [eax]         ; (*CountTofinish)--
>      cmp        cl,  1              ; Check mwait-monitor support
>      jnz        HltLoop
>  MwaitLoop:
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index e6dea18..49305ad 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -251,7 +251,8 @@ VOID
>    IN BOOLEAN                 MwaitSupport,
>    IN UINTN                   ApTargetCState,
>    IN UINTN                   PmCodeSegment,
> -  IN UINTN                   TopOfApStack
> +  IN UINTN                   TopOfApStack,
> +  IN UINTN                   NumberToFinish
>    );
>  
>  /**
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> index 7869970..f8f4712 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> @@ -222,11 +222,13 @@ CProcedureInvoke:
>  RendezvousFunnelProcEnd:
>  
>  ;-------------------------------------------------------------------------------------
> -;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack);
> +;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack, CountTofinish);
>  ;-------------------------------------------------------------------------------------
>  global ASM_PFX(AsmRelocateApLoop)
>  ASM_PFX(AsmRelocateApLoop):
>  AsmRelocateApLoopStart:
> +    mov        rax, [rsp + 40]   ; CountTofinish
> +    lock dec   dword [rax]       ; (*CountTofinish)--
>      mov        rsp, r9
>      push       rcx
>      push       rdx
>