public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Fan Jeff <vanjeff_919@hotmail.com>
To: "Ni, Ruiyu" <ruiyu.ni@intel.com>,
	"Dong, Eric" <eric.dong@intel.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Subject: 答复: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
Date: Tue, 24 Oct 2017 06:27:12 +0000	[thread overview]
Message-ID: <CY1PR19MB0283908A6BE2F9DB735D76BBD7470@CY1PR19MB0283.namprd19.prod.outlook.com> (raw)
In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5BAA35C3@SHSMSX104.ccr.corp.intel.com>

Ray,

MpCpuExchangeInfo was declared as volatile type. It may not be necessary to add volatile for NumApsExecuting.

Jeff

发件人: Ni, Ruiyu<mailto:ruiyu.ni@intel.com>
发送时间: 2017年10月24日 14:03
收件人: Dong, Eric<mailto:eric.dong@intel.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
主题: Re: [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.

You need to have "volatile" for "UINTN  NumApsExecuting;".
Otherwise, compiler may optimize the code to cause below code wait forever:
      while (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) {
        CpuPause();
      }


Thanks/Ray

> -----Original Message-----
> From: Dong, Eric
> Sent: Monday, October 23, 2017 3:23 PM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP
> initialization logic.
>
> Current logic always waiting for a specific value to collect all APs count. This
> logic may caused some platforms cost too much time to wait for time out.
> This patch add new logic to collect APs count. It adds new variable
> NumApsExecuting to detect whether all APs have finished initialization.
> Each AP let NumApsExecuting++ when begin to initialize itself and let
> NumApsExecuting-- when it finish the initialization. BSP base on whether
> NumApsExecuting == 0  to finished the collect AP process.
>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc    |  1 +
>  UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm |  6 ++++++
>  UefiCpuPkg/Library/MpInitLib/MpLib.c           | 20 ++++++++++++++------
>  UefiCpuPkg/Library/MpInitLib/MpLib.h           |  1 +
>  UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc     |  3 ++-
>  UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm  |  6 ++++++
>  6 files changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
> b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
> index 976af1f..bdfe0d3 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
> +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
> @@ -40,4 +40,5 @@ EnableExecuteDisableLocation  equ        LockLocation +
> 30h
>  Cr3Location                   equ        LockLocation + 34h
>  InitFlagLocation              equ        LockLocation + 38h
>  CpuInfoLocation               equ        LockLocation + 3Ch
> +NumApsExecutingLocation       equ        LockLocation + 40h
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> index 1b9c6a6..2b6c27d 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> @@ -86,6 +86,12 @@ Flat32Start:                                   ; protected mode entry
> point
>
>      mov        esi, ebx
>
> +    ; Increment the number of APs executing here as early as possible
> +    ; This is decremented in C code when AP is finished executing
> +    mov        edi, esi
> +    add        edi, NumApsExecutingLocation
> +    lock inc   dword [edi]
> +
>      mov         edi, esi
>      add         edi, EnableExecuteDisableLocation
>      cmp         byte [edi], 0
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index db923c9..48f930b 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -662,6 +662,7 @@ ApWakeupFunction (
>      // AP finished executing C code
>      //
>      InterlockedIncrement ((UINT32 *) &CpuMpData->FinishedCount);
> +    InterlockedDecrement ((UINT32 *)
> + &CpuMpData->MpCpuExchangeInfo->NumApsExecuting);
>
>      //
>      // Place AP is specified loop mode
> @@ -765,6 +766,7 @@ FillExchangeInfoData (
>
>    ExchangeInfo->CFunction       = (UINTN) ApWakeupFunction;
>    ExchangeInfo->ApIndex         = 0;
> +  ExchangeInfo->NumApsExecuting = 0;
>    ExchangeInfo->InitFlag        = (UINTN) CpuMpData->InitFlag;
>    ExchangeInfo->CpuInfo         = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData-
> >CpuInfoInHob;
>    ExchangeInfo->CpuMpData       = CpuMpData;
> @@ -934,13 +936,19 @@ WakeUpAP (
>      }
>      if (CpuMpData->InitFlag == ApInitConfig) {
>        //
> -      // Wait for all potential APs waken up in one specified period
> +      // Wait for one potential AP waken up in one specified period
>        //
> -      TimedWaitForApFinish (
> -        CpuMpData,
> -        PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
> -        PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
> -        );
> +      if (CpuMpData->CpuCount == 0) {
> +        TimedWaitForApFinish (
> +          CpuMpData,
> +          PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
> +          PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
> +          );
> +      }
> +
> +      while (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) {
> +        CpuPause();
> +      }
>      } else {
>        //
>        // Wait all APs waken up if this is not the 1st broadcast of SIPI diff --git
> a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index e41d2db..d13d5c0 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -176,6 +176,7 @@ typedef struct {
>    UINTN                 Cr3;
>    UINTN                 InitFlag;
>    CPU_INFO_IN_HOB       *CpuInfo;
> +  UINTN                 NumApsExecuting;
>    CPU_MP_DATA           *CpuMpData;
>    UINTN                 InitializeFloatingPointUnitsAddress;
>  } MP_CPU_EXCHANGE_INFO;
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> index 114f4e0..d255ca5 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> @@ -40,5 +40,6 @@ EnableExecuteDisableLocation  equ        LockLocation +
> 5Ch
>  Cr3Location                   equ        LockLocation + 64h
>  InitFlagLocation              equ        LockLocation + 6Ch
>  CpuInfoLocation               equ        LockLocation + 74h
> -InitializeFloatingPointUnitsAddress equ  LockLocation + 84h
> +NumApsExecutingLocation       equ        LockLocation + 7Ch
> +InitializeFloatingPointUnitsAddress equ  LockLocation + 8Ch
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> index 4ada649..21d2786 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> @@ -124,6 +124,12 @@ LongModeStart:
>      cmp        qword [edi], 1       ; ApInitConfig
>      jnz        GetApicId
>
> +    ; Increment the number of APs executing here as early as possible
> +    ; This is decremented in C code when AP is finished executing
> +    mov        edi, esi
> +    add        edi, NumApsExecutingLocation
> +    lock inc   dword [edi]
> +
>      ; AP init
>      mov        edi, esi
>      add        edi, LockLocation
> --
> 2.7.0.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


  reply	other threads:[~2017-10-24  6:23 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-10-23  7:22 [Patch 0/2] Enhance collect AP Count logic Eric Dong
2017-10-23  7:22 ` [Patch 1/2] UefiCpuPkg/MpInitLib: Change AP Index variable name Eric Dong
2017-10-24  6:00   ` Ni, Ruiyu
2017-10-23  7:22 ` [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic Eric Dong
2017-10-24  6:02   ` Ni, Ruiyu
2017-10-24  6:27     ` Fan Jeff [this message]
2017-10-24  7:18       ` Ni, Ruiyu
2017-10-24  7:32         ` 答复: " Fan Jeff
2017-10-24 10:15   ` Laszlo Ersek
2017-10-24 14:24     ` 答复: " Fan Jeff
2017-10-24 16:29       ` Laszlo Ersek
2017-10-24 15:23     ` Dong, Eric
2017-10-24 15:40       ` Dong, Eric
2017-10-24 17:40       ` Laszlo Ersek
2017-10-24 22:30         ` Brian J. Johnson
2017-10-25  5:35           ` 答复: " Fan Jeff
2017-10-25  5:32         ` Fan Jeff
2017-10-25  5:42         ` Dong, Eric
2017-10-25 15:07           ` Laszlo Ersek
2017-10-26  1:13             ` Dong, Eric
2017-10-26 20:48               ` Brian J. Johnson
2017-10-27  1:31                 ` Dong, Eric
2017-10-24  7:31 ` 答复: [Patch 0/2] Enhance collect AP Count logic Fan Jeff

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CY1PR19MB0283908A6BE2F9DB735D76BBD7470@CY1PR19MB0283.namprd19.prod.outlook.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox