public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [Patch 0/2] Enhance collect AP Count logic
@ 2017-10-23  7:22 Eric Dong
  2017-10-23  7:22 ` [Patch 1/2] UefiCpuPkg/MpInitLib: Change AP Index variable name Eric Dong
                   ` (2 more replies)
  0 siblings, 3 replies; 23+ messages in thread
From: Eric Dong @ 2017-10-23  7:22 UTC (permalink / raw)
  To: edk2-devel

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 series 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. 

Because current code already use NumApsExecuting variable, so add another
patch to change the variable name for the current code for better 
understanding.


Eric Dong (2):
  UefiCpuPkg/MpInitLib: Change AP Index variable name.
  UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.

 UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc    |  3 ++-
 UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 10 ++++++++--
 UefiCpuPkg/Library/MpInitLib/MpLib.c           | 24 ++++++++++++++++--------
 UefiCpuPkg/Library/MpInitLib/MpLib.h           |  3 ++-
 UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc     |  5 +++--
 UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm  | 10 ++++++++--
 6 files changed, 39 insertions(+), 16 deletions(-)

-- 
2.7.0.windows.1



^ permalink raw reply	[flat|nested] 23+ messages in thread

* [Patch 1/2] UefiCpuPkg/MpInitLib: Change AP Index variable name.
  2017-10-23  7:22 [Patch 0/2] Enhance collect AP Count logic Eric Dong
@ 2017-10-23  7:22 ` 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  7:31 ` 答复: [Patch 0/2] Enhance collect AP Count logic Fan Jeff
  2 siblings, 1 reply; 23+ messages in thread
From: Eric Dong @ 2017-10-23  7:22 UTC (permalink / raw)
  To: edk2-devel; +Cc: Ruiyu Ni

Original AP index variable name not well express the meaning
of the variable. Also this name is better used in later patch.
So change the variable name for better understanding.

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    | 2 +-
 UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 4 ++--
 UefiCpuPkg/Library/MpInitLib/MpLib.c           | 6 +++---
 UefiCpuPkg/Library/MpInitLib/MpLib.h           | 2 +-
 UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc     | 2 +-
 UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm  | 4 ++--
 6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
index 6276230..976af1f 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
@@ -33,7 +33,7 @@ GdtrLocation                  equ        LockLocation + 10h
 IdtrLocation                  equ        LockLocation + 16h
 BufferStartLocation           equ        LockLocation + 1Ch
 ModeOffsetLocation            equ        LockLocation + 20h
-NumApsExecutingLocation       equ        LockLocation + 24h
+ApIndexLocation               equ        LockLocation + 24h
 CodeSegmentLocation           equ        LockLocation + 28h
 DataSegmentLocation           equ        LockLocation + 2Ch
 EnableExecuteDisableLocation  equ        LockLocation + 30h
diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
index 52363e6..1b9c6a6 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
@@ -130,7 +130,7 @@ TestLock:
     jz         TestLock
 
     mov        ecx, esi
-    add        ecx, NumApsExecutingLocation
+    add        ecx, ApIndexLocation
     inc        dword [ecx]
     mov        ebx, [ecx]
 
@@ -200,7 +200,7 @@ CProcedureInvoke:
     mov        eax, ASM_PFX(InitializeFloatingPointUnits)
     call       eax               ; Call assembly function to initialize FPU per UEFI spec
 
-    push       ebx               ; Push NumApsExecuting
+    push       ebx               ; Push ApIndex
     mov        eax, esi
     add        eax, LockLocation
     push       eax               ; push address of exchange info data buffer
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index f3ee6d4..db923c9 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -542,7 +542,7 @@ VOID
 EFIAPI
 ApWakeupFunction (
   IN MP_CPU_EXCHANGE_INFO      *ExchangeInfo,
-  IN UINTN                     NumApsExecuting
+  IN UINTN                     ApIndex
   )
 {
   CPU_MP_DATA                *CpuMpData;
@@ -574,7 +574,7 @@ ApWakeupFunction (
       // Add CPU number
       //
       InterlockedIncrement ((UINT32 *) &CpuMpData->CpuCount);
-      ProcessorNumber = NumApsExecuting;
+      ProcessorNumber = ApIndex;
       //
       // This is first time AP wakeup, get BIST information from AP stack
       //
@@ -764,7 +764,7 @@ FillExchangeInfoData (
   ExchangeInfo->Cr3             = AsmReadCr3 ();
 
   ExchangeInfo->CFunction       = (UINTN) ApWakeupFunction;
-  ExchangeInfo->NumApsExecuting = 0;
+  ExchangeInfo->ApIndex         = 0;
   ExchangeInfo->InitFlag        = (UINTN) CpuMpData->InitFlag;
   ExchangeInfo->CpuInfo         = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob;
   ExchangeInfo->CpuMpData       = CpuMpData;
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 84ae24f..e41d2db 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -169,7 +169,7 @@ typedef struct {
   IA32_DESCRIPTOR       IdtrProfile;
   UINTN                 BufferStart;
   UINTN                 ModeOffset;
-  UINTN                 NumApsExecuting;
+  UINTN                 ApIndex;
   UINTN                 CodeSegment;
   UINTN                 DataSegment;
   UINTN                 EnableExecuteDisable;
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
index 5b2529b..114f4e0 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
@@ -33,7 +33,7 @@ GdtrLocation                  equ        LockLocation + 20h
 IdtrLocation                  equ        LockLocation + 2Ah
 BufferStartLocation           equ        LockLocation + 34h
 ModeOffsetLocation            equ        LockLocation + 3Ch
-NumApsExecutingLocation       equ        LockLocation + 44h
+ApIndexLocation               equ        LockLocation + 44h
 CodeSegmentLocation           equ        LockLocation + 4Ch
 DataSegmentLocation           equ        LockLocation + 54h
 EnableExecuteDisableLocation  equ        LockLocation + 5Ch
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 0b14a53..4ada649 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -134,7 +134,7 @@ TestLock:
     cmp        rax, NotVacantFlag
     jz         TestLock
 
-    lea        ecx, [esi + NumApsExecutingLocation]
+    lea        ecx, [esi + ApIndexLocation]
     inc        dword [ecx]
     mov        ebx, [ecx]
 
@@ -206,7 +206,7 @@ CProcedureInvoke:
     call       rax               ; Call assembly function to initialize FPU per UEFI spec
     add        rsp, 20h
 
-    mov        edx, ebx          ; edx is NumApsExecuting
+    mov        edx, ebx          ; edx is ApIndex
     mov        ecx, esi
     add        ecx, LockLocation ; rcx is address of exchange info data buffer
 
-- 
2.7.0.windows.1



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
  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-23  7:22 ` Eric Dong
  2017-10-24  6:02   ` Ni, Ruiyu
  2017-10-24 10:15   ` Laszlo Ersek
  2017-10-24  7:31 ` 答复: [Patch 0/2] Enhance collect AP Count logic Fan Jeff
  2 siblings, 2 replies; 23+ messages in thread
From: Eric Dong @ 2017-10-23  7:22 UTC (permalink / raw)
  To: edk2-devel; +Cc: Ruiyu Ni

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



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [Patch 1/2] UefiCpuPkg/MpInitLib: Change AP Index variable name.
  2017-10-23  7:22 ` [Patch 1/2] UefiCpuPkg/MpInitLib: Change AP Index variable name Eric Dong
@ 2017-10-24  6:00   ` Ni, Ruiyu
  0 siblings, 0 replies; 23+ messages in thread
From: Ni, Ruiyu @ 2017-10-24  6:00 UTC (permalink / raw)
  To: Dong, Eric, edk2-devel@lists.01.org

Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>

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 1/2] UefiCpuPkg/MpInitLib: Change AP Index variable name.
> 
> Original AP index variable name not well express the meaning of the variable.
> Also this name is better used in later patch.
> So change the variable name for better understanding.
> 
> 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    | 2 +-
>  UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 4 ++--
>  UefiCpuPkg/Library/MpInitLib/MpLib.c           | 6 +++---
>  UefiCpuPkg/Library/MpInitLib/MpLib.h           | 2 +-
>  UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc     | 2 +-
>  UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm  | 4 ++--
>  6 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
> b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
> index 6276230..976af1f 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
> +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
> @@ -33,7 +33,7 @@ GdtrLocation                  equ        LockLocation + 10h
>  IdtrLocation                  equ        LockLocation + 16h
>  BufferStartLocation           equ        LockLocation + 1Ch
>  ModeOffsetLocation            equ        LockLocation + 20h
> -NumApsExecutingLocation       equ        LockLocation + 24h
> +ApIndexLocation               equ        LockLocation + 24h
>  CodeSegmentLocation           equ        LockLocation + 28h
>  DataSegmentLocation           equ        LockLocation + 2Ch
>  EnableExecuteDisableLocation  equ        LockLocation + 30h
> diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> index 52363e6..1b9c6a6 100644
> --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
> @@ -130,7 +130,7 @@ TestLock:
>      jz         TestLock
> 
>      mov        ecx, esi
> -    add        ecx, NumApsExecutingLocation
> +    add        ecx, ApIndexLocation
>      inc        dword [ecx]
>      mov        ebx, [ecx]
> 
> @@ -200,7 +200,7 @@ CProcedureInvoke:
>      mov        eax, ASM_PFX(InitializeFloatingPointUnits)
>      call       eax               ; Call assembly function to initialize FPU per UEFI spec
> 
> -    push       ebx               ; Push NumApsExecuting
> +    push       ebx               ; Push ApIndex
>      mov        eax, esi
>      add        eax, LockLocation
>      push       eax               ; push address of exchange info data buffer
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index f3ee6d4..db923c9 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -542,7 +542,7 @@ VOID
>  EFIAPI
>  ApWakeupFunction (
>    IN MP_CPU_EXCHANGE_INFO      *ExchangeInfo,
> -  IN UINTN                     NumApsExecuting
> +  IN UINTN                     ApIndex
>    )
>  {
>    CPU_MP_DATA                *CpuMpData;
> @@ -574,7 +574,7 @@ ApWakeupFunction (
>        // Add CPU number
>        //
>        InterlockedIncrement ((UINT32 *) &CpuMpData->CpuCount);
> -      ProcessorNumber = NumApsExecuting;
> +      ProcessorNumber = ApIndex;
>        //
>        // This is first time AP wakeup, get BIST information from AP stack
>        //
> @@ -764,7 +764,7 @@ FillExchangeInfoData (
>    ExchangeInfo->Cr3             = AsmReadCr3 ();
> 
>    ExchangeInfo->CFunction       = (UINTN) ApWakeupFunction;
> -  ExchangeInfo->NumApsExecuting = 0;
> +  ExchangeInfo->ApIndex         = 0;
>    ExchangeInfo->InitFlag        = (UINTN) CpuMpData->InitFlag;
>    ExchangeInfo->CpuInfo         = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData-
> >CpuInfoInHob;
>    ExchangeInfo->CpuMpData       = CpuMpData;
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> index 84ae24f..e41d2db 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
> @@ -169,7 +169,7 @@ typedef struct {
>    IA32_DESCRIPTOR       IdtrProfile;
>    UINTN                 BufferStart;
>    UINTN                 ModeOffset;
> -  UINTN                 NumApsExecuting;
> +  UINTN                 ApIndex;
>    UINTN                 CodeSegment;
>    UINTN                 DataSegment;
>    UINTN                 EnableExecuteDisable;
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> index 5b2529b..114f4e0 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
> @@ -33,7 +33,7 @@ GdtrLocation                  equ        LockLocation + 20h
>  IdtrLocation                  equ        LockLocation + 2Ah
>  BufferStartLocation           equ        LockLocation + 34h
>  ModeOffsetLocation            equ        LockLocation + 3Ch
> -NumApsExecutingLocation       equ        LockLocation + 44h
> +ApIndexLocation               equ        LockLocation + 44h
>  CodeSegmentLocation           equ        LockLocation + 4Ch
>  DataSegmentLocation           equ        LockLocation + 54h
>  EnableExecuteDisableLocation  equ        LockLocation + 5Ch
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> index 0b14a53..4ada649 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> @@ -134,7 +134,7 @@ TestLock:
>      cmp        rax, NotVacantFlag
>      jz         TestLock
> 
> -    lea        ecx, [esi + NumApsExecutingLocation]
> +    lea        ecx, [esi + ApIndexLocation]
>      inc        dword [ecx]
>      mov        ebx, [ecx]
> 
> @@ -206,7 +206,7 @@ CProcedureInvoke:
>      call       rax               ; Call assembly function to initialize FPU per UEFI spec
>      add        rsp, 20h
> 
> -    mov        edx, ebx          ; edx is NumApsExecuting
> +    mov        edx, ebx          ; edx is ApIndex
>      mov        ecx, esi
>      add        ecx, LockLocation ; rcx is address of exchange info data buffer
> 
> --
> 2.7.0.windows.1



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
  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
  2017-10-24 10:15   ` Laszlo Ersek
  1 sibling, 1 reply; 23+ messages in thread
From: Ni, Ruiyu @ 2017-10-24  6:02 UTC (permalink / raw)
  To: Dong, Eric, edk2-devel@lists.01.org

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



^ permalink raw reply	[flat|nested] 23+ messages in thread

* 答复: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
  2017-10-24  6:02   ` Ni, Ruiyu
@ 2017-10-24  6:27     ` Fan Jeff
  2017-10-24  7:18       ` Ni, Ruiyu
  0 siblings, 1 reply; 23+ messages in thread
From: Fan Jeff @ 2017-10-24  6:27 UTC (permalink / raw)
  To: Ni, Ruiyu, Dong, Eric, edk2-devel@lists.01.org

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
  2017-10-24  6:27     ` 答复: " Fan Jeff
@ 2017-10-24  7:18       ` Ni, Ruiyu
  2017-10-24  7:32         ` 答复: " Fan Jeff
  0 siblings, 1 reply; 23+ messages in thread
From: Ni, Ruiyu @ 2017-10-24  7:18 UTC (permalink / raw)
  To: Fan Jeff, Dong, Eric, edk2-devel@lists.01.org

Jeff,

I see. Thanks for mentioning that😊
Reviewed-by: Ruiyu Ni <Ruiyu.ni@intel.com>

Do you mind to also give a r-b?


Thanks/Ray

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Fan Jeff
> Sent: Tuesday, October 24, 2017 2:27 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-
> devel@lists.01.org
> Subject: [edk2] 答复: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for
> AP initialization logic.
> 
> 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
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

^ permalink raw reply	[flat|nested] 23+ messages in thread

* 答复: [Patch 0/2] Enhance collect AP Count logic
  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-23  7:22 ` [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic Eric Dong
@ 2017-10-24  7:31 ` Fan Jeff
  2 siblings, 0 replies; 23+ messages in thread
From: Fan Jeff @ 2017-10-24  7:31 UTC (permalink / raw)
  To: Eric Dong, edk2-devel@lists.01.org

Reviewed-by: Jeff Fan <vanjeff_919@hotmail.com>

发件人: Eric Dong<mailto:eric.dong@intel.com>
发送时间: 2017年10月23日 15:22
收件人: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
主题: [edk2] [Patch 0/2] Enhance collect AP Count 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 series 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.

Because current code already use NumApsExecuting variable, so add another
patch to change the variable name for the current code for better
understanding.


Eric Dong (2):
  UefiCpuPkg/MpInitLib: Change AP Index variable name.
  UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.

 UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc    |  3 ++-
 UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 10 ++++++++--
 UefiCpuPkg/Library/MpInitLib/MpLib.c           | 24 ++++++++++++++++--------
 UefiCpuPkg/Library/MpInitLib/MpLib.h           |  3 ++-
 UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc     |  5 +++--
 UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm  | 10 ++++++++--
 6 files changed, 39 insertions(+), 16 deletions(-)

--
2.7.0.windows.1

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* 答复: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
  2017-10-24  7:18       ` Ni, Ruiyu
@ 2017-10-24  7:32         ` Fan Jeff
  0 siblings, 0 replies; 23+ messages in thread
From: Fan Jeff @ 2017-10-24  7:32 UTC (permalink / raw)
  To: Ni, Ruiyu, Dong, Eric, edk2-devel@lists.01.org

Sure. Done.

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

Jeff,

I see. Thanks for mentioning that😊
Reviewed-by: Ruiyu Ni <Ruiyu.ni@intel.com>

Do you mind to also give a r-b?


Thanks/Ray

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Fan Jeff
> Sent: Tuesday, October 24, 2017 2:27 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; edk2-
> devel@lists.01.org
> Subject: [edk2] 答复: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for
> AP initialization logic.
>
> 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
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
  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 10:15   ` Laszlo Ersek
  2017-10-24 14:24     ` 答复: " Fan Jeff
  2017-10-24 15:23     ` Dong, Eric
  1 sibling, 2 replies; 23+ messages in thread
From: Laszlo Ersek @ 2017-10-24 10:15 UTC (permalink / raw)
  To: Eric Dong, edk2-devel; +Cc: Ruiyu Ni, Paolo Bonzini

CC Paolo

On 10/23/17 09:22, Eric Dong wrote:
> 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(-)

I was out of office yesterday, and did not get a chance to comment on
this patch.

In a virtualization guest, I see the following problem with the patch:

VCPUs (virtual CPUs) may not be scheduled by the hypervisor similarly to
how physical CPUs behave on a physical board. It is possible that a VCPU
starts up and even finishes its initialization routine before another
VCPU starts running at all.

Therefore the locked NumApsExecuting counter may hit zero, even multiple
times, before all APs have finished initializing.

In OVMF, we query QEMU about the exact number of virtual processors, in
PlatformPei. So OVMF configures the logical processor count in advance
that MpInitLib has to wait for. Correspondingly, we also set the timeout
to "infinity".

Please see the MaxCpuCountInitialization() function in following commit:

  https://github.com/tianocore/edk2/commit/45a70db3c3a59

In the past, we used to have AP initialization problems in OVMF due to
the VCPU scheduling artifacts I mention above. After commit
45a70db3c3a59, things have been stable; it would be nice to keep that
working.

Please note that simply testing this patch on my end is not sufficient.
The AP init problems we used to face were sporadic and also specific to
the virtualization host systems (i.e., dependent on the physical
hardware and the host kernel).

Furthermore:

> 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)
> +          );
> +      }

I don't understand this change. The new comment says,

  Wait for *one* potential AP waken up in one specified period

However, the second parameter of TimedWaitForApFinish(), namely
"FinishedApLimit", gets the same value as before:

  PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1

It means that all of the (possible) APs are waited-for, just the same as
before.

In other words, I think the patch does not correctly implement what the
commit message says -- and for QEMU / OVMF, that's actually a good thing
at the moment, because a correct implementation of the description would
likely break on QEMU.

Thanks
Laszlo

> +
> +      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
> 



^ permalink raw reply	[flat|nested] 23+ messages in thread

* 答复: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
  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
  1 sibling, 1 reply; 23+ messages in thread
From: Fan Jeff @ 2017-10-24 14:24 UTC (permalink / raw)
  To: Laszlo Ersek, Eric Dong, edk2-devel@lists.01.org; +Cc: Ruiyu Ni, Paolo Bonzini

Laszlo,

How about to skip the condition check on CpuCount?
-   if (CpuMpData->CpuCount == 0) {
        TimedWaitForApFinish (
          CpuMpData,
          PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
          PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
          );
-      }

For OVMF platform, it still keep the exiting behavior on bigger PcdCpuApInitTimeOutInMicroSeconds value and actual PcdCpuMaxLogicalProcessorNumber value.
For real platform, it could set the small PcdCpuApInitTimeOutInMicroSeconds value and get big performance improvement by the latter code logic.

Jeff

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

CC Paolo

On 10/23/17 09:22, Eric Dong wrote:
> 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(-)

I was out of office yesterday, and did not get a chance to comment on
this patch.

In a virtualization guest, I see the following problem with the patch:

VCPUs (virtual CPUs) may not be scheduled by the hypervisor similarly to
how physical CPUs behave on a physical board. It is possible that a VCPU
starts up and even finishes its initialization routine before another
VCPU starts running at all.

Therefore the locked NumApsExecuting counter may hit zero, even multiple
times, before all APs have finished initializing.

In OVMF, we query QEMU about the exact number of virtual processors, in
PlatformPei. So OVMF configures the logical processor count in advance
that MpInitLib has to wait for. Correspondingly, we also set the timeout
to "infinity".

Please see the MaxCpuCountInitialization() function in following commit:

  https://github.com/tianocore/edk2/commit/45a70db3c3a59

In the past, we used to have AP initialization problems in OVMF due to
the VCPU scheduling artifacts I mention above. After commit
45a70db3c3a59, things have been stable; it would be nice to keep that
working.

Please note that simply testing this patch on my end is not sufficient.
The AP init problems we used to face were sporadic and also specific to
the virtualization host systems (i.e., dependent on the physical
hardware and the host kernel).

Furthermore:

> 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)
> +          );
> +      }

I don't understand this change. The new comment says,

  Wait for *one* potential AP waken up in one specified period

However, the second parameter of TimedWaitForApFinish(), namely
"FinishedApLimit", gets the same value as before:

  PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1

It means that all of the (possible) APs are waited-for, just the same as
before.

In other words, I think the patch does not correctly implement what the
commit message says -- and for QEMU / OVMF, that's actually a good thing
at the moment, because a correct implementation of the description would
likely break on QEMU.

Thanks
Laszlo

> +
> +      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
>

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
  2017-10-24 10:15   ` Laszlo Ersek
  2017-10-24 14:24     ` 答复: " Fan Jeff
@ 2017-10-24 15:23     ` Dong, Eric
  2017-10-24 15:40       ` Dong, Eric
  2017-10-24 17:40       ` Laszlo Ersek
  1 sibling, 2 replies; 23+ messages in thread
From: Dong, Eric @ 2017-10-24 15:23 UTC (permalink / raw)
  To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Paolo Bonzini

Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, October 24, 2017 6:16 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for
> AP initialization logic.
> 
> CC Paolo
> 
> On 10/23/17 09:22, Eric Dong wrote:
> > 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(-)
> 
> I was out of office yesterday, and did not get a chance to comment on this
> patch.
> 
> In a virtualization guest, I see the following problem with the patch:
> 
> VCPUs (virtual CPUs) may not be scheduled by the hypervisor similarly to how
> physical CPUs behave on a physical board. It is possible that a VCPU starts up
> and even finishes its initialization routine before another VCPU starts running
> at all.
> 
> Therefore the locked NumApsExecuting counter may hit zero, even multiple
> times, before all APs have finished initializing.
> 
> In OVMF, we query QEMU about the exact number of virtual processors, in
> PlatformPei. So OVMF configures the logical processor count in advance that
> MpInitLib has to wait for. Correspondingly, we also set the timeout to
> "infinity".
> 
> Please see the MaxCpuCountInitialization() function in following commit:
> 
>   https://github.com/tianocore/edk2/commit/45a70db3c3a59
> 
> In the past, we used to have AP initialization problems in OVMF due to the
> VCPU scheduling artifacts I mention above. After commit 45a70db3c3a59,
> things have been stable; it would be nice to keep that working.
> 
> Please note that simply testing this patch on my end is not sufficient.
> The AP init problems we used to face were sporadic and also specific to the
> virtualization host systems (i.e., dependent on the physical hardware and the
> host kernel).
> 
> Furthermore:

[[Eric]] Will comments later if needed, need some investigation first.

> 
> > 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)
> > +          );
> > +      }
> 
> I don't understand this change. The new comment says,
> 
>   Wait for *one* potential AP waken up in one specified period
> 
> However, the second parameter of TimedWaitForApFinish(), namely
> "FinishedApLimit", gets the same value as before:
> 
>   PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1
> 
> It means that all of the (possible) APs are waited-for, just the same as before.

[[Eric]] This patch changes the collect AP count logic, original solution always waits for a specific time to let all APs start up. If the input CPU number (PcdGet32(PcdCpuMaxLogicalProcessorNumber) - 1) have been found or after a specific time(PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)). BSP will not wait anymore and use  CpuMpData->CpuCount as the found AP count.

New logic also wait for a specific time, but this time is smaller than the original one. It just wait for the first AP(any AP) begin to do the initialization( do CpuMpData->MpCpuExchangeInfo->NumApsExecuting++ means it begin to do the initialization). When Ap finishes initialization, it will do CpuMpData->MpCpuExchangeInfo->NumApsExecuting--. So after BSP waits for a specific time at first, it just needs to check whether CpuMpData->MpCpuExchangeInfo->NumApsExecuting == 0 to know whether all Aps have finished initialization. Here we still use the original PCD (PcdCpuApInitTimeOutInMicroSeconds) for the new time value. 

When one AP do the initialization, it will also do CpuMpData->CpuCount++. So here I check whether CpuMpData->CpuCount != 0 to know whether APs already begin to do the initialization. If yes, I not need to do the time out waiting anymore, just check the CpuMpData->MpCpuExchangeInfo->NumApsExecuting to know whether all Aps have finished initialization.

> 
> In other words, I think the patch does not correctly implement what the
> commit message says -- and for QEMU / OVMF, that's actually a good thing
> at the moment, because a correct implementation of the description would
> likely break on QEMU.
> 
> Thanks
> Laszlo
> 
> > +
> > +      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
> >


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
  2017-10-24 15:23     ` Dong, Eric
@ 2017-10-24 15:40       ` Dong, Eric
  2017-10-24 17:40       ` Laszlo Ersek
  1 sibling, 0 replies; 23+ messages in thread
From: Dong, Eric @ 2017-10-24 15:40 UTC (permalink / raw)
  To: Dong, Eric, Laszlo Ersek, edk2-devel@lists.01.org
  Cc: Ni, Ruiyu, Paolo Bonzini

Laszlo,

Add more comments for TimedWaitForApFinish function in mail.

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Dong, Eric
> Sent: Tuesday, October 24, 2017 11:24 PM
> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for
> AP initialization logic.
> 
> Laszlo,
> 
> > -----Original Message-----
> > From: Laszlo Ersek [mailto:lersek@redhat.com]
> > Sent: Tuesday, October 24, 2017 6:16 PM
> > To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
> > <pbonzini@redhat.com>
> > Subject: Re: [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting
> > for AP initialization logic.
> >
> > CC Paolo
> >
> > On 10/23/17 09:22, Eric Dong wrote:
> > > 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(-)
> >
> > I was out of office yesterday, and did not get a chance to comment on
> > this patch.
> >
> > In a virtualization guest, I see the following problem with the patch:
> >
> > VCPUs (virtual CPUs) may not be scheduled by the hypervisor similarly
> > to how physical CPUs behave on a physical board. It is possible that a
> > VCPU starts up and even finishes its initialization routine before
> > another VCPU starts running at all.
> >
> > Therefore the locked NumApsExecuting counter may hit zero, even
> > multiple times, before all APs have finished initializing.
> >
> > In OVMF, we query QEMU about the exact number of virtual processors,
> > in PlatformPei. So OVMF configures the logical processor count in
> > advance that MpInitLib has to wait for. Correspondingly, we also set
> > the timeout to "infinity".
> >
> > Please see the MaxCpuCountInitialization() function in following commit:
> >
> >   https://github.com/tianocore/edk2/commit/45a70db3c3a59
> >
> > In the past, we used to have AP initialization problems in OVMF due to
> > the VCPU scheduling artifacts I mention above. After commit
> > 45a70db3c3a59, things have been stable; it would be nice to keep that
> working.
> >
> > Please note that simply testing this patch on my end is not sufficient.
> > The AP init problems we used to face were sporadic and also specific
> > to the virtualization host systems (i.e., dependent on the physical
> > hardware and the host kernel).
> >
> > Furthermore:
> 
> [[Eric]] Will comments later if needed, need some investigation first.
> 
> >
> > > 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)
> > > +          );
> > > +      }
> >
> > I don't understand this change. The new comment says,
> >
> >   Wait for *one* potential AP waken up in one specified period
> >
> > However, the second parameter of TimedWaitForApFinish(), namely
> > "FinishedApLimit", gets the same value as before:
> >
> >   PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1
> >
> > It means that all of the (possible) APs are waited-for, just the same as
> before.
> 

[[Eric]] We still use the original PCD and not change the value, because this value need to tune on different platforms. So we think it's no need to change the default value for this PCD. 

For TimedWaitForApFinish, it will return in two cases, one is the specified CPU number (PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,) have been found, the other is the specified time value(PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)) has reached.  I think with old solution, this function will return when it found the specified CPU numbers (except for the cpu hotplug case, which the value may bigger than the actual CPU numbers). With new solution, after update PCD value base on platform, it will return when the specific time out is reach.

> [[Eric]] This patch changes the collect AP count logic, original solution always
> waits for a specific time to let all APs start up. If the input CPU number
> (PcdGet32(PcdCpuMaxLogicalProcessorNumber) - 1) have been found or
> after a specific time(PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)). BSP
> will not wait anymore and use  CpuMpData->CpuCount as the found AP
> count.
> 
> New logic also wait for a specific time, but this time is smaller than the
> original one. It just wait for the first AP(any AP) begin to do the
> initialization( do CpuMpData->MpCpuExchangeInfo->NumApsExecuting++
> means it begin to do the initialization). When Ap finishes initialization, it will
> do CpuMpData->MpCpuExchangeInfo->NumApsExecuting--. So after BSP
> waits for a specific time at first, it just needs to check whether CpuMpData-
> >MpCpuExchangeInfo->NumApsExecuting == 0 to know whether all Aps have
> finished initialization. Here we still use the original PCD
> (PcdCpuApInitTimeOutInMicroSeconds) for the new time value.
> 
> When one AP do the initialization, it will also do CpuMpData->CpuCount++.
> So here I check whether CpuMpData->CpuCount != 0 to know whether APs
> already begin to do the initialization. If yes, I not need to do the time out
> waiting anymore, just check the CpuMpData->MpCpuExchangeInfo-
> >NumApsExecuting to know whether all Aps have finished initialization.
> 
> >
> > In other words, I think the patch does not correctly implement what
> > the commit message says -- and for QEMU / OVMF, that's actually a good
> > thing at the moment, because a correct implementation of the
> > description would likely break on QEMU.
> >
> > Thanks
> > Laszlo
> >
> > > +
> > > +      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
> > >
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: 答复: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
  2017-10-24 14:24     ` 答复: " Fan Jeff
@ 2017-10-24 16:29       ` Laszlo Ersek
  0 siblings, 0 replies; 23+ messages in thread
From: Laszlo Ersek @ 2017-10-24 16:29 UTC (permalink / raw)
  To: Fan Jeff, Eric Dong, edk2-devel@lists.01.org; +Cc: Ruiyu Ni, Paolo Bonzini

Hi Jeff,

On 10/24/17 16:24, Fan Jeff wrote:
> Laszlo,
> 
> How about to skip the condition check on CpuCount?
> -   if (CpuMpData->CpuCount == 0) {
>         TimedWaitForApFinish (
>           CpuMpData,
>           PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
>           PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
>           );
> -      }
> 
> For OVMF platform, it still keep the exiting behavior on bigger PcdCpuApInitTimeOutInMicroSeconds value and actual PcdCpuMaxLogicalProcessorNumber value.
> For real platform, it could set the small PcdCpuApInitTimeOutInMicroSeconds value and get big performance improvement by the latter code logic.

That could work; yes.

But, I still don't understand the new comment above the code; "Wait for
one potential AP waken up in one specified period". The FinishedApLimit
argument is not 1, generally.

... Does the comment mean that, if one AP has already woken up, then we
won't wait at all, because "CpuMpData->CpuCount" will be larger than 0?
(If this is the case, then it's a problem for QEMU.)

I'm about to check Eric's response as well.

Thanks!
Laszlo



> 
> Jeff
> 
> 发件人: Laszlo Ersek<mailto:lersek@redhat.com>
> 发送时间: 2017年10月24日 18:15
> 收件人: Eric Dong<mailto:eric.dong@intel.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> 抄送: Ruiyu Ni<mailto:ruiyu.ni@intel.com>; Paolo Bonzini<mailto:pbonzini@redhat.com>
> 主题: Re: [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
> 
> CC Paolo
> 
> On 10/23/17 09:22, Eric Dong wrote:
>> 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(-)
> 
> I was out of office yesterday, and did not get a chance to comment on
> this patch.
> 
> In a virtualization guest, I see the following problem with the patch:
> 
> VCPUs (virtual CPUs) may not be scheduled by the hypervisor similarly to
> how physical CPUs behave on a physical board. It is possible that a VCPU
> starts up and even finishes its initialization routine before another
> VCPU starts running at all.
> 
> Therefore the locked NumApsExecuting counter may hit zero, even multiple
> times, before all APs have finished initializing.
> 
> In OVMF, we query QEMU about the exact number of virtual processors, in
> PlatformPei. So OVMF configures the logical processor count in advance
> that MpInitLib has to wait for. Correspondingly, we also set the timeout
> to "infinity".
> 
> Please see the MaxCpuCountInitialization() function in following commit:
> 
>   https://github.com/tianocore/edk2/commit/45a70db3c3a59
> 
> In the past, we used to have AP initialization problems in OVMF due to
> the VCPU scheduling artifacts I mention above. After commit
> 45a70db3c3a59, things have been stable; it would be nice to keep that
> working.
> 
> Please note that simply testing this patch on my end is not sufficient.
> The AP init problems we used to face were sporadic and also specific to
> the virtualization host systems (i.e., dependent on the physical
> hardware and the host kernel).
> 
> Furthermore:
> 
>> 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)
>> +          );
>> +      }
> 
> I don't understand this change. The new comment says,
> 
>   Wait for *one* potential AP waken up in one specified period
> 
> However, the second parameter of TimedWaitForApFinish(), namely
> "FinishedApLimit", gets the same value as before:
> 
>   PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1
> 
> It means that all of the (possible) APs are waited-for, just the same as
> before.
> 
> In other words, I think the patch does not correctly implement what the
> commit message says -- and for QEMU / OVMF, that's actually a good thing
> at the moment, because a correct implementation of the description would
> likely break on QEMU.
> 
> Thanks
> Laszlo
> 
>> +
>> +      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
>>
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
  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
                           ` (2 more replies)
  1 sibling, 3 replies; 23+ messages in thread
From: Laszlo Ersek @ 2017-10-24 17:40 UTC (permalink / raw)
  To: Dong, Eric, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Paolo Bonzini, Jeff Fan

Hi Eric,

On 10/24/17 17:23, Dong, Eric wrote:
> Laszlo,
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Tuesday, October 24, 2017 6:16 PM
>> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini <pbonzini@redhat.com>
>> Subject: Re: [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for
>> AP initialization logic.
>>
>> CC Paolo
>>
>> On 10/23/17 09:22, Eric Dong wrote:

>>> 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)
>>> +          );
>>> +      }
>>
>> I don't understand this change. The new comment says,
>>
>>   Wait for *one* potential AP waken up in one specified period
>>
>> However, the second parameter of TimedWaitForApFinish(), namely
>> "FinishedApLimit", gets the same value as before:
>>
>>   PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1
>>
>> It means that all of the (possible) APs are waited-for, just the same
>> as before.
>
> [[Eric]] This patch changes the collect AP count logic, original
> solution always waits for a specific time to let all APs start up. If
> the input CPU number (PcdGet32(PcdCpuMaxLogicalProcessorNumber) - 1)
> have been found or after a specific time(PcdGet32
> (PcdCpuApInitTimeOutInMicroSeconds)). BSP will not wait anymore and use
> CpuMpData->CpuCount as the found AP count.
>
> New logic also wait for a specific time, but this time is smaller than
> the original one. It just wait for the first AP(any AP) begin to do the
> initialization( do CpuMpData->MpCpuExchangeInfo->NumApsExecuting++ means
> it begin to do the initialization). When Ap finishes initialization, it
> will do CpuMpData->MpCpuExchangeInfo->NumApsExecuting--. So after BSP
> waits for a specific time at first, it just needs to check whether
> CpuMpData->MpCpuExchangeInfo->NumApsExecuting == 0 to know whether all
> Aps have finished initialization. Here we still use the original PCD
> (PcdCpuApInitTimeOutInMicroSeconds) for the new time value.
>
> When one AP do the initialization, it will also do
> CpuMpData->CpuCount++. So here I check whether CpuMpData->CpuCount != 0
> to know whether APs already begin to do the initialization. If yes, I
> not need to do the time out waiting anymore, just check the
> CpuMpData->MpCpuExchangeInfo->NumApsExecuting to know whether all Aps
> have finished initialization.

Thanks for the explanation.

The "NumApsExecuting" increment / decrement logic in this patch expects
that the APs work as follows:

(1) After the INIT-SIPI-SIPI, there may be a delay before the APs start
running. During this delay, the BSP may or may not reach the code in
question. The (CpuMpData->CpuCount != 0) check is supposed to take this
into account.

(2) After at least one AP has started running, the logic expects
"NumApsExecuting" to monotonically grow for a while, and then to
monotonically decrease, back to zero. For example, if we have 1 BSP and
7 APs, the BSP logic more or less expects the following values in
"NumApsExecuting":

1; 2; 3; 4; 5; 6; 7;
6; 5; 4; 3; 2; 1; 0


While this may be a valid expectation for physical processors (which
actually run in parallel, in the physical world), in a virtual machine,
it is not guaranteed. Dependent on hypervisor scheduling artifacts, it
is possible that, say, three APs start up *and finish* before the
remaining four APs start up *at all*. The INIT-SIPI-SIPI marks all APs
for execution / scheduling in the hypervisor, yes, but the actual code
execution may commence a lot later. For example, the BSP may witness the
following series of values in "NumApsExecuting":

1; 2; 3;
2; 1; 0;
1; 2; 3; 4;
3; 2; 1; 0

and the BSP could think that there are 3 APs only, when it sees the
first 0 value.


Now, let me get back to the use case that actually matters to OVMF and
QEMU. As I mentioned earlier, OVMF knows from QEMU the exact number of
APs. If there is a way for OVMF to tell MpInitLib to wait for this exact
number of APs -- no matter how long it takes --, then that's what I
would like to use.

Please see the original discussion around OVMF commit 45a70db3c3a59:

* In version 1, I introduced a new PCD called

  PcdCpuKnownLogicalProcessorNumber

and I modified MpInitLib to wait for this AP number, ignoring timeout
completely, if the PCD was set:

  https://lists.01.org/pipermail/edk2-devel/2016-November/005076.html

However, Jeff suggested to use the preexistent PCD
"PcdCpuMaxLogicalProcessorNumber" for this purpose:

  https://lists.01.org/pipermail/edk2-devel/2016-November/005092.html

* In version 2, I used the PCD suggested by Jeff, but I also introduced
a new special value for the timeout. Timeout=0 would mean "infinity":

  https://lists.01.org/pipermail/edk2-devel/2016-November/005209.html

Jeff didn't like the special value, and suggested that OVMF simply use
MAX_UINT32 microseconds (approx. 71 minutes) instead of "infinity":

  https://lists.01.org/pipermail/edk2-devel/2016-November/005266.html

* In v3, I implemented that, and that was pushed as:

  - UefiCpuPkg commit 6e1987f19af7 ("UefiCpuPkg/MpInitLib: wait no
    longer than necessary for initial AP startup", 2016-11-24)

  - and OvmfPkg commit 45a70db3c3a5 ("OvmfPkg/PlatformPei: take VCPU
    count from QEMU and configure MpInitLib", 2016-11-24).


So, again, the use case that I would like to cover is:

* the exact number of APs is known at boot, to OvmfPkg/PlatformPei,

* after the very first INIT-SIPI-SIPI, MpInitLib should wait for exactly
  this number of APs to "report in", regardless of:

  - how long it takes,

  - in what order / sequence the APs report in. (Again, please remember
    that some APs may complete the initialization before other APs
    execute their very first instruction.)

* Preferably, the case should be handled when the processor count grows
  from cold boot to S3 resume (VCPU hotplug). TianoCore BZ#251 used to
  track this use case separately.

(

Jeff closed BZ#251 today, with the argument that commit 0594ec417c89
(this patch) finds the CPU count dynamically anyway, so a platform can
simply set "PcdCpuMaxLogicalProcessorNumber" to the maximum possible value.

This argument does not work in a virtual machine, because commit
0594ec417c89 (this patch) may in fact not find the VCPU count correctly
-- in a VM, (NumApsExecuting==0) does not imply that all APs have finished.

)

Thanks!
Laszlo


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
  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
  2 siblings, 1 reply; 23+ messages in thread
From: Brian J. Johnson @ 2017-10-24 22:30 UTC (permalink / raw)
  To: Laszlo Ersek, Dong, Eric, edk2-devel@lists.01.org
  Cc: Ni, Ruiyu, Paolo Bonzini

On 10/24/2017 12:40 PM, Laszlo Ersek wrote:
> Hi Eric,
> 
> On 10/24/17 17:23, Dong, Eric wrote:
>> Laszlo,
>>
>>> -----Original Message-----
>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>> Sent: Tuesday, October 24, 2017 6:16 PM
>>> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini <pbonzini@redhat.com>
>>> Subject: Re: [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for
>>> AP initialization logic.
>>>
>>> CC Paolo
>>>
>>> On 10/23/17 09:22, Eric Dong wrote:
> 
>>>> 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)
>>>> +          );
>>>> +      }
>>>
>>> I don't understand this change. The new comment says,
>>>
>>>    Wait for *one* potential AP waken up in one specified period
>>>
>>> However, the second parameter of TimedWaitForApFinish(), namely
>>> "FinishedApLimit", gets the same value as before:
>>>
>>>    PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1
>>>
>>> It means that all of the (possible) APs are waited-for, just the same
>>> as before.
>>
>> [[Eric]] This patch changes the collect AP count logic, original
>> solution always waits for a specific time to let all APs start up. If
>> the input CPU number (PcdGet32(PcdCpuMaxLogicalProcessorNumber) - 1)
>> have been found or after a specific time(PcdGet32
>> (PcdCpuApInitTimeOutInMicroSeconds)). BSP will not wait anymore and use
>> CpuMpData->CpuCount as the found AP count.
>>
>> New logic also wait for a specific time, but this time is smaller than
>> the original one. It just wait for the first AP(any AP) begin to do the
>> initialization( do CpuMpData->MpCpuExchangeInfo->NumApsExecuting++ means
>> it begin to do the initialization). When Ap finishes initialization, it
>> will do CpuMpData->MpCpuExchangeInfo->NumApsExecuting--. So after BSP
>> waits for a specific time at first, it just needs to check whether
>> CpuMpData->MpCpuExchangeInfo->NumApsExecuting == 0 to know whether all
>> Aps have finished initialization. Here we still use the original PCD
>> (PcdCpuApInitTimeOutInMicroSeconds) for the new time value.
>>
>> When one AP do the initialization, it will also do
>> CpuMpData->CpuCount++. So here I check whether CpuMpData->CpuCount != 0
>> to know whether APs already begin to do the initialization. If yes, I
>> not need to do the time out waiting anymore, just check the
>> CpuMpData->MpCpuExchangeInfo->NumApsExecuting to know whether all Aps
>> have finished initialization.
> 
> Thanks for the explanation.
> 
> The "NumApsExecuting" increment / decrement logic in this patch expects
> that the APs work as follows:
> 
> (1) After the INIT-SIPI-SIPI, there may be a delay before the APs start
> running. During this delay, the BSP may or may not reach the code in
> question. The (CpuMpData->CpuCount != 0) check is supposed to take this
> into account.
> 
> (2) After at least one AP has started running, the logic expects
> "NumApsExecuting" to monotonically grow for a while, and then to
> monotonically decrease, back to zero. For example, if we have 1 BSP and
> 7 APs, the BSP logic more or less expects the following values in
> "NumApsExecuting":
> 
> 1; 2; 3; 4; 5; 6; 7;
> 6; 5; 4; 3; 2; 1; 0
> 
> 
> While this may be a valid expectation for physical processors (which
> actually run in parallel, in the physical world), in a virtual machine,
> it is not guaranteed. Dependent on hypervisor scheduling artifacts, it
> is possible that, say, three APs start up *and finish* before the
> remaining four APs start up *at all*. The INIT-SIPI-SIPI marks all APs
> for execution / scheduling in the hypervisor, yes, but the actual code
> execution may commence a lot later. For example, the BSP may witness the
> following series of values in "NumApsExecuting":
> 
> 1; 2; 3;
> 2; 1; 0;
> 1; 2; 3; 4;
> 3; 2; 1; 0
> 
> and the BSP could think that there are 3 APs only, when it sees the
> first 0 value.
> 

Eric,

I agree with Laszlo -- this patch introduces race conditions and very 
machine-specific behavior assumptions.  That's dangerous at best, and 
can easily break machines which are perfectly valid (such as VMs and 
node-controller based scalable systems) but don't meet the implicit 
assumptions.

I'd rather see each AP get tracked individually.  For example, have each 
AP set a bit in a global bitmap when it starts executing, and set a bit 
in another global bitmap when it completes.  Then the BSP can compare 
the bitmaps to determine how many APs are running.  That also provides 
good debug information on exactly which APs start running, and exactly 
which ones fail to complete.  Otherwise that's quite difficult to determine.

Thanks,
Brian Johnson

> 
> Now, let me get back to the use case that actually matters to OVMF and
> QEMU. As I mentioned earlier, OVMF knows from QEMU the exact number of
> APs. If there is a way for OVMF to tell MpInitLib to wait for this exact
> number of APs -- no matter how long it takes --, then that's what I
> would like to use.
> 
> Please see the original discussion around OVMF commit 45a70db3c3a59:
> 
> * In version 1, I introduced a new PCD called
> 
>    PcdCpuKnownLogicalProcessorNumber
> 
> and I modified MpInitLib to wait for this AP number, ignoring timeout
> completely, if the PCD was set:
> 
>    https://lists.01.org/pipermail/edk2-devel/2016-November/005076.html
> 
> However, Jeff suggested to use the preexistent PCD
> "PcdCpuMaxLogicalProcessorNumber" for this purpose:
> 
>    https://lists.01.org/pipermail/edk2-devel/2016-November/005092.html
> 
> * In version 2, I used the PCD suggested by Jeff, but I also introduced
> a new special value for the timeout. Timeout=0 would mean "infinity":
> 
>    https://lists.01.org/pipermail/edk2-devel/2016-November/005209.html
> 
> Jeff didn't like the special value, and suggested that OVMF simply use
> MAX_UINT32 microseconds (approx. 71 minutes) instead of "infinity":
> 
>    https://lists.01.org/pipermail/edk2-devel/2016-November/005266.html
> 
> * In v3, I implemented that, and that was pushed as:
> 
>    - UefiCpuPkg commit 6e1987f19af7 ("UefiCpuPkg/MpInitLib: wait no
>      longer than necessary for initial AP startup", 2016-11-24)
> 
>    - and OvmfPkg commit 45a70db3c3a5 ("OvmfPkg/PlatformPei: take VCPU
>      count from QEMU and configure MpInitLib", 2016-11-24).
> 
> 
> So, again, the use case that I would like to cover is:
> 
> * the exact number of APs is known at boot, to OvmfPkg/PlatformPei,
> 
> * after the very first INIT-SIPI-SIPI, MpInitLib should wait for exactly
>    this number of APs to "report in", regardless of:
> 
>    - how long it takes,
> 
>    - in what order / sequence the APs report in. (Again, please remember
>      that some APs may complete the initialization before other APs
>      execute their very first instruction.)
> 
> * Preferably, the case should be handled when the processor count grows
>    from cold boot to S3 resume (VCPU hotplug). TianoCore BZ#251 used to
>    track this use case separately.
> 
> (
> 
> Jeff closed BZ#251 today, with the argument that commit 0594ec417c89
> (this patch) finds the CPU count dynamically anyway, so a platform can
> simply set "PcdCpuMaxLogicalProcessorNumber" to the maximum possible value.
> 
> This argument does not work in a virtual machine, because commit
> 0594ec417c89 (this patch) may in fact not find the VCPU count correctly
> -- in a VM, (NumApsExecuting==0) does not imply that all APs have finished.
> 
> )
> 
> Thanks!
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 


-- 

                                                 Brian

--------------------------------------------------------------------

    "I don't believe personal letters sent bulk rate."
                                            -- me


^ permalink raw reply	[flat|nested] 23+ messages in thread

* 答复: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
  2017-10-24 17:40       ` Laszlo Ersek
  2017-10-24 22:30         ` Brian J. Johnson
@ 2017-10-25  5:32         ` Fan Jeff
  2017-10-25  5:42         ` Dong, Eric
  2 siblings, 0 replies; 23+ messages in thread
From: Fan Jeff @ 2017-10-25  5:32 UTC (permalink / raw)
  To: Laszlo Ersek, Dong, Eric, edk2-devel@lists.01.org
  Cc: Ni, Ruiyu, Paolo Bonzini

Laszlo,

I agree BZ#251 can not be closed if commit 0594ec417c89 does not work for OVMF.

I will reopen it.

Jeff

发件人: Laszlo Ersek<mailto:lersek@redhat.com>
发送时间: 2017年10月25日 1:40
收件人: Dong, Eric<mailto:eric.dong@intel.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
抄送: Ni, Ruiyu<mailto:ruiyu.ni@intel.com>; Paolo Bonzini<mailto:pbonzini@redhat.com>; Jeff Fan<mailto:vanjeff_919@hotmail.com>
主题: Re: [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.

Hi Eric,

On 10/24/17 17:23, Dong, Eric wrote:
> Laszlo,
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Tuesday, October 24, 2017 6:16 PM
>> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini <pbonzini@redhat.com>
>> Subject: Re: [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for
>> AP initialization logic.
>>
>> CC Paolo
>>
>> On 10/23/17 09:22, Eric Dong wrote:

>>> 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)
>>> +          );
>>> +      }
>>
>> I don't understand this change. The new comment says,
>>
>>   Wait for *one* potential AP waken up in one specified period
>>
>> However, the second parameter of TimedWaitForApFinish(), namely
>> "FinishedApLimit", gets the same value as before:
>>
>>   PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1
>>
>> It means that all of the (possible) APs are waited-for, just the same
>> as before.
>
> [[Eric]] This patch changes the collect AP count logic, original
> solution always waits for a specific time to let all APs start up. If
> the input CPU number (PcdGet32(PcdCpuMaxLogicalProcessorNumber) - 1)
> have been found or after a specific time(PcdGet32
> (PcdCpuApInitTimeOutInMicroSeconds)). BSP will not wait anymore and use
> CpuMpData->CpuCount as the found AP count.
>
> New logic also wait for a specific time, but this time is smaller than
> the original one. It just wait for the first AP(any AP) begin to do the
> initialization( do CpuMpData->MpCpuExchangeInfo->NumApsExecuting++ means
> it begin to do the initialization). When Ap finishes initialization, it
> will do CpuMpData->MpCpuExchangeInfo->NumApsExecuting--. So after BSP
> waits for a specific time at first, it just needs to check whether
> CpuMpData->MpCpuExchangeInfo->NumApsExecuting == 0 to know whether all
> Aps have finished initialization. Here we still use the original PCD
> (PcdCpuApInitTimeOutInMicroSeconds) for the new time value.
>
> When one AP do the initialization, it will also do
> CpuMpData->CpuCount++. So here I check whether CpuMpData->CpuCount != 0
> to know whether APs already begin to do the initialization. If yes, I
> not need to do the time out waiting anymore, just check the
> CpuMpData->MpCpuExchangeInfo->NumApsExecuting to know whether all Aps
> have finished initialization.

Thanks for the explanation.

The "NumApsExecuting" increment / decrement logic in this patch expects
that the APs work as follows:

(1) After the INIT-SIPI-SIPI, there may be a delay before the APs start
running. During this delay, the BSP may or may not reach the code in
question. The (CpuMpData->CpuCount != 0) check is supposed to take this
into account.

(2) After at least one AP has started running, the logic expects
"NumApsExecuting" to monotonically grow for a while, and then to
monotonically decrease, back to zero. For example, if we have 1 BSP and
7 APs, the BSP logic more or less expects the following values in
"NumApsExecuting":

1; 2; 3; 4; 5; 6; 7;
6; 5; 4; 3; 2; 1; 0


While this may be a valid expectation for physical processors (which
actually run in parallel, in the physical world), in a virtual machine,
it is not guaranteed. Dependent on hypervisor scheduling artifacts, it
is possible that, say, three APs start up *and finish* before the
remaining four APs start up *at all*. The INIT-SIPI-SIPI marks all APs
for execution / scheduling in the hypervisor, yes, but the actual code
execution may commence a lot later. For example, the BSP may witness the
following series of values in "NumApsExecuting":

1; 2; 3;
2; 1; 0;
1; 2; 3; 4;
3; 2; 1; 0

and the BSP could think that there are 3 APs only, when it sees the
first 0 value.


Now, let me get back to the use case that actually matters to OVMF and
QEMU. As I mentioned earlier, OVMF knows from QEMU the exact number of
APs. If there is a way for OVMF to tell MpInitLib to wait for this exact
number of APs -- no matter how long it takes --, then that's what I
would like to use.

Please see the original discussion around OVMF commit 45a70db3c3a59:

* In version 1, I introduced a new PCD called

  PcdCpuKnownLogicalProcessorNumber

and I modified MpInitLib to wait for this AP number, ignoring timeout
completely, if the PCD was set:

  https://lists.01.org/pipermail/edk2-devel/2016-November/005076.html

However, Jeff suggested to use the preexistent PCD
"PcdCpuMaxLogicalProcessorNumber" for this purpose:

  https://lists.01.org/pipermail/edk2-devel/2016-November/005092.html

* In version 2, I used the PCD suggested by Jeff, but I also introduced
a new special value for the timeout. Timeout=0 would mean "infinity":

  https://lists.01.org/pipermail/edk2-devel/2016-November/005209.html

Jeff didn't like the special value, and suggested that OVMF simply use
MAX_UINT32 microseconds (approx. 71 minutes) instead of "infinity":

  https://lists.01.org/pipermail/edk2-devel/2016-November/005266.html

* In v3, I implemented that, and that was pushed as:

  - UefiCpuPkg commit 6e1987f19af7 ("UefiCpuPkg/MpInitLib: wait no
    longer than necessary for initial AP startup", 2016-11-24)

  - and OvmfPkg commit 45a70db3c3a5 ("OvmfPkg/PlatformPei: take VCPU
    count from QEMU and configure MpInitLib", 2016-11-24).


So, again, the use case that I would like to cover is:

* the exact number of APs is known at boot, to OvmfPkg/PlatformPei,

* after the very first INIT-SIPI-SIPI, MpInitLib should wait for exactly
  this number of APs to "report in", regardless of:

  - how long it takes,

  - in what order / sequence the APs report in. (Again, please remember
    that some APs may complete the initialization before other APs
    execute their very first instruction.)

* Preferably, the case should be handled when the processor count grows
  from cold boot to S3 resume (VCPU hotplug). TianoCore BZ#251 used to
  track this use case separately.

(

Jeff closed BZ#251 today, with the argument that commit 0594ec417c89
(this patch) finds the CPU count dynamically anyway, so a platform can
simply set "PcdCpuMaxLogicalProcessorNumber" to the maximum possible value.

This argument does not work in a virtual machine, because commit
0594ec417c89 (this patch) may in fact not find the VCPU count correctly
-- in a VM, (NumApsExecuting==0) does not imply that all APs have finished.

)

Thanks!
Laszlo


^ permalink raw reply	[flat|nested] 23+ messages in thread

* 答复: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
  2017-10-24 22:30         ` Brian J. Johnson
@ 2017-10-25  5:35           ` Fan Jeff
  0 siblings, 0 replies; 23+ messages in thread
From: Fan Jeff @ 2017-10-25  5:35 UTC (permalink / raw)
  To: Brian J. Johnson, Laszlo Ersek, Dong, Eric,
	edk2-devel@lists.01.org
  Cc: Ni, Ruiyu, Paolo Bonzini

Johnson,

For OVMF case, we knew the actual processor number.
But for most real platform cases, we have no this knowledge on the first time sending broadcast INIT-SIPI-SIPI.

Jeff

发件人: Brian J. Johnson<mailto:brian.johnson@hpe.com>
发送时间: 2017年10月25日 6:30
收件人: Laszlo Ersek<mailto:lersek@redhat.com>; Dong, Eric<mailto:eric.dong@intel.com>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
抄送: Ni, Ruiyu<mailto:ruiyu.ni@intel.com>; Paolo Bonzini<mailto:pbonzini@redhat.com>
主题: Re: [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.

On 10/24/2017 12:40 PM, Laszlo Ersek wrote:
> Hi Eric,
>
> On 10/24/17 17:23, Dong, Eric wrote:
>> Laszlo,
>>
>>> -----Original Message-----
>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>> Sent: Tuesday, October 24, 2017 6:16 PM
>>> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini <pbonzini@redhat.com>
>>> Subject: Re: [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for
>>> AP initialization logic.
>>>
>>> CC Paolo
>>>
>>> On 10/23/17 09:22, Eric Dong wrote:
>
>>>> 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)
>>>> +          );
>>>> +      }
>>>
>>> I don't understand this change. The new comment says,
>>>
>>>    Wait for *one* potential AP waken up in one specified period
>>>
>>> However, the second parameter of TimedWaitForApFinish(), namely
>>> "FinishedApLimit", gets the same value as before:
>>>
>>>    PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1
>>>
>>> It means that all of the (possible) APs are waited-for, just the same
>>> as before.
>>
>> [[Eric]] This patch changes the collect AP count logic, original
>> solution always waits for a specific time to let all APs start up. If
>> the input CPU number (PcdGet32(PcdCpuMaxLogicalProcessorNumber) - 1)
>> have been found or after a specific time(PcdGet32
>> (PcdCpuApInitTimeOutInMicroSeconds)). BSP will not wait anymore and use
>> CpuMpData->CpuCount as the found AP count.
>>
>> New logic also wait for a specific time, but this time is smaller than
>> the original one. It just wait for the first AP(any AP) begin to do the
>> initialization( do CpuMpData->MpCpuExchangeInfo->NumApsExecuting++ means
>> it begin to do the initialization). When Ap finishes initialization, it
>> will do CpuMpData->MpCpuExchangeInfo->NumApsExecuting--. So after BSP
>> waits for a specific time at first, it just needs to check whether
>> CpuMpData->MpCpuExchangeInfo->NumApsExecuting == 0 to know whether all
>> Aps have finished initialization. Here we still use the original PCD
>> (PcdCpuApInitTimeOutInMicroSeconds) for the new time value.
>>
>> When one AP do the initialization, it will also do
>> CpuMpData->CpuCount++. So here I check whether CpuMpData->CpuCount != 0
>> to know whether APs already begin to do the initialization. If yes, I
>> not need to do the time out waiting anymore, just check the
>> CpuMpData->MpCpuExchangeInfo->NumApsExecuting to know whether all Aps
>> have finished initialization.
>
> Thanks for the explanation.
>
> The "NumApsExecuting" increment / decrement logic in this patch expects
> that the APs work as follows:
>
> (1) After the INIT-SIPI-SIPI, there may be a delay before the APs start
> running. During this delay, the BSP may or may not reach the code in
> question. The (CpuMpData->CpuCount != 0) check is supposed to take this
> into account.
>
> (2) After at least one AP has started running, the logic expects
> "NumApsExecuting" to monotonically grow for a while, and then to
> monotonically decrease, back to zero. For example, if we have 1 BSP and
> 7 APs, the BSP logic more or less expects the following values in
> "NumApsExecuting":
>
> 1; 2; 3; 4; 5; 6; 7;
> 6; 5; 4; 3; 2; 1; 0
>
>
> While this may be a valid expectation for physical processors (which
> actually run in parallel, in the physical world), in a virtual machine,
> it is not guaranteed. Dependent on hypervisor scheduling artifacts, it
> is possible that, say, three APs start up *and finish* before the
> remaining four APs start up *at all*. The INIT-SIPI-SIPI marks all APs
> for execution / scheduling in the hypervisor, yes, but the actual code
> execution may commence a lot later. For example, the BSP may witness the
> following series of values in "NumApsExecuting":
>
> 1; 2; 3;
> 2; 1; 0;
> 1; 2; 3; 4;
> 3; 2; 1; 0
>
> and the BSP could think that there are 3 APs only, when it sees the
> first 0 value.
>

Eric,

I agree with Laszlo -- this patch introduces race conditions and very
machine-specific behavior assumptions.  That's dangerous at best, and
can easily break machines which are perfectly valid (such as VMs and
node-controller based scalable systems) but don't meet the implicit
assumptions.

I'd rather see each AP get tracked individually.  For example, have each
AP set a bit in a global bitmap when it starts executing, and set a bit
in another global bitmap when it completes.  Then the BSP can compare
the bitmaps to determine how many APs are running.  That also provides
good debug information on exactly which APs start running, and exactly
which ones fail to complete.  Otherwise that's quite difficult to determine.

Thanks,
Brian Johnson

>
> Now, let me get back to the use case that actually matters to OVMF and
> QEMU. As I mentioned earlier, OVMF knows from QEMU the exact number of
> APs. If there is a way for OVMF to tell MpInitLib to wait for this exact
> number of APs -- no matter how long it takes --, then that's what I
> would like to use.
>
> Please see the original discussion around OVMF commit 45a70db3c3a59:
>
> * In version 1, I introduced a new PCD called
>
>    PcdCpuKnownLogicalProcessorNumber
>
> and I modified MpInitLib to wait for this AP number, ignoring timeout
> completely, if the PCD was set:
>
>    https://lists.01.org/pipermail/edk2-devel/2016-November/005076.html
>
> However, Jeff suggested to use the preexistent PCD
> "PcdCpuMaxLogicalProcessorNumber" for this purpose:
>
>    https://lists.01.org/pipermail/edk2-devel/2016-November/005092.html
>
> * In version 2, I used the PCD suggested by Jeff, but I also introduced
> a new special value for the timeout. Timeout=0 would mean "infinity":
>
>    https://lists.01.org/pipermail/edk2-devel/2016-November/005209.html
>
> Jeff didn't like the special value, and suggested that OVMF simply use
> MAX_UINT32 microseconds (approx. 71 minutes) instead of "infinity":
>
>    https://lists.01.org/pipermail/edk2-devel/2016-November/005266.html
>
> * In v3, I implemented that, and that was pushed as:
>
>    - UefiCpuPkg commit 6e1987f19af7 ("UefiCpuPkg/MpInitLib: wait no
>      longer than necessary for initial AP startup", 2016-11-24)
>
>    - and OvmfPkg commit 45a70db3c3a5 ("OvmfPkg/PlatformPei: take VCPU
>      count from QEMU and configure MpInitLib", 2016-11-24).
>
>
> So, again, the use case that I would like to cover is:
>
> * the exact number of APs is known at boot, to OvmfPkg/PlatformPei,
>
> * after the very first INIT-SIPI-SIPI, MpInitLib should wait for exactly
>    this number of APs to "report in", regardless of:
>
>    - how long it takes,
>
>    - in what order / sequence the APs report in. (Again, please remember
>      that some APs may complete the initialization before other APs
>      execute their very first instruction.)
>
> * Preferably, the case should be handled when the processor count grows
>    from cold boot to S3 resume (VCPU hotplug). TianoCore BZ#251 used to
>    track this use case separately.
>
> (
>
> Jeff closed BZ#251 today, with the argument that commit 0594ec417c89
> (this patch) finds the CPU count dynamically anyway, so a platform can
> simply set "PcdCpuMaxLogicalProcessorNumber" to the maximum possible value.
>
> This argument does not work in a virtual machine, because commit
> 0594ec417c89 (this patch) may in fact not find the VCPU count correctly
> -- in a VM, (NumApsExecuting==0) does not imply that all APs have finished.
>
> )
>
> Thanks!
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>


--

                                                 Brian

--------------------------------------------------------------------

    "I don't believe personal letters sent bulk rate."
                                            -- me
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
  2017-10-24 17:40       ` Laszlo Ersek
  2017-10-24 22:30         ` Brian J. Johnson
  2017-10-25  5:32         ` Fan Jeff
@ 2017-10-25  5:42         ` Dong, Eric
  2017-10-25 15:07           ` Laszlo Ersek
  2 siblings, 1 reply; 23+ messages in thread
From: Dong, Eric @ 2017-10-25  5:42 UTC (permalink / raw)
  To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Paolo Bonzini, Jeff Fan

Hi Laszlo,

I think I have clear about your raised issues for Ovmf platform. In this case, I think your platform not suit for this code change.  How about I do below change based on the new code:

-      if (CpuMpData->CpuCount == 0) {
        TimedWaitForApFinish (
          CpuMpData,
          PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
          PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
          );
-      }

After this change, for Ovmf can still set PcdCpuApInitTimeOutInMicroSeconds to MAX_UINT32 to keep old solution. For the real platform, it can set a small value to follow the new solution. For this new change, it just wait one more PcdCpuApInitTimeOutInMicroSeconds timeout, should not have performance impact (This time may also been cost in later while (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) loop) or have litter performance impact (not cover by the later loop).

Thanks,
Eric

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, October 25, 2017 1:40 AM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini <pbonzini@redhat.com>;
> Jeff Fan <vanjeff_919@hotmail.com>
> Subject: Re: [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for
> AP initialization logic.
> 
> Hi Eric,
> 
> On 10/24/17 17:23, Dong, Eric wrote:
> > Laszlo,
> >
> >> -----Original Message-----
> >> From: Laszlo Ersek [mailto:lersek@redhat.com]
> >> Sent: Tuesday, October 24, 2017 6:16 PM
> >> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
> >> <pbonzini@redhat.com>
> >> Subject: Re: [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting
> >> for AP initialization logic.
> >>
> >> CC Paolo
> >>
> >> On 10/23/17 09:22, Eric Dong wrote:
> 
> >>> 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)
> >>> +          );
> >>> +      }
> >>
> >> I don't understand this change. The new comment says,
> >>
> >>   Wait for *one* potential AP waken up in one specified period
> >>
> >> However, the second parameter of TimedWaitForApFinish(), namely
> >> "FinishedApLimit", gets the same value as before:
> >>
> >>   PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1
> >>
> >> It means that all of the (possible) APs are waited-for, just the same
> >> as before.
> >
> > [[Eric]] This patch changes the collect AP count logic, original
> > solution always waits for a specific time to let all APs start up. If
> > the input CPU number (PcdGet32(PcdCpuMaxLogicalProcessorNumber) - 1)
> > have been found or after a specific time(PcdGet32
> > (PcdCpuApInitTimeOutInMicroSeconds)). BSP will not wait anymore and
> > use
> > CpuMpData->CpuCount as the found AP count.
> >
> > New logic also wait for a specific time, but this time is smaller than
> > the original one. It just wait for the first AP(any AP) begin to do
> > the initialization( do CpuMpData->MpCpuExchangeInfo-
> >NumApsExecuting++
> > means it begin to do the initialization). When Ap finishes
> > initialization, it will do
> > CpuMpData->MpCpuExchangeInfo->NumApsExecuting--. So after BSP
> waits
> > for a specific time at first, it just needs to check whether
> > CpuMpData->MpCpuExchangeInfo->NumApsExecuting == 0 to know
> whether all
> > Aps have finished initialization. Here we still use the original PCD
> > (PcdCpuApInitTimeOutInMicroSeconds) for the new time value.
> >
> > When one AP do the initialization, it will also do
> > CpuMpData->CpuCount++. So here I check whether CpuMpData-
> >CpuCount !=
> > CpuMpData->CpuCount++0
> > to know whether APs already begin to do the initialization. If yes, I
> > not need to do the time out waiting anymore, just check the
> > CpuMpData->MpCpuExchangeInfo->NumApsExecuting to know whether
> all Aps
> > have finished initialization.
> 
> Thanks for the explanation.
> 
> The "NumApsExecuting" increment / decrement logic in this patch expects
> that the APs work as follows:
> 
> (1) After the INIT-SIPI-SIPI, there may be a delay before the APs start
> running. During this delay, the BSP may or may not reach the code in
> question. The (CpuMpData->CpuCount != 0) check is supposed to take this
> into account.
> 
> (2) After at least one AP has started running, the logic expects
> "NumApsExecuting" to monotonically grow for a while, and then to
> monotonically decrease, back to zero. For example, if we have 1 BSP and
> 7 APs, the BSP logic more or less expects the following values in
> "NumApsExecuting":
> 
> 1; 2; 3; 4; 5; 6; 7;
> 6; 5; 4; 3; 2; 1; 0
> 
> 
> While this may be a valid expectation for physical processors (which actually
> run in parallel, in the physical world), in a virtual machine, it is not guaranteed.
> Dependent on hypervisor scheduling artifacts, it is possible that, say, three
> APs start up *and finish* before the remaining four APs start up *at all*. The
> INIT-SIPI-SIPI marks all APs for execution / scheduling in the hypervisor, yes,
> but the actual code execution may commence a lot later. For example, the
> BSP may witness the following series of values in "NumApsExecuting":
> 
> 1; 2; 3;
> 2; 1; 0;
> 1; 2; 3; 4;
> 3; 2; 1; 0
> 
> and the BSP could think that there are 3 APs only, when it sees the first 0
> value.
> 
> 
> Now, let me get back to the use case that actually matters to OVMF and
> QEMU. As I mentioned earlier, OVMF knows from QEMU the exact number
> of APs. If there is a way for OVMF to tell MpInitLib to wait for this exact
> number of APs -- no matter how long it takes --, then that's what I would like
> to use.
> 
> Please see the original discussion around OVMF commit 45a70db3c3a59:
> 
> * In version 1, I introduced a new PCD called
> 
>   PcdCpuKnownLogicalProcessorNumber
> 
> and I modified MpInitLib to wait for this AP number, ignoring timeout
> completely, if the PCD was set:
> 
>   https://lists.01.org/pipermail/edk2-devel/2016-November/005076.html
> 
> However, Jeff suggested to use the preexistent PCD
> "PcdCpuMaxLogicalProcessorNumber" for this purpose:
> 
>   https://lists.01.org/pipermail/edk2-devel/2016-November/005092.html
> 
> * In version 2, I used the PCD suggested by Jeff, but I also introduced a new
> special value for the timeout. Timeout=0 would mean "infinity":
> 
>   https://lists.01.org/pipermail/edk2-devel/2016-November/005209.html
> 
> Jeff didn't like the special value, and suggested that OVMF simply use
> MAX_UINT32 microseconds (approx. 71 minutes) instead of "infinity":
> 
>   https://lists.01.org/pipermail/edk2-devel/2016-November/005266.html
> 
> * In v3, I implemented that, and that was pushed as:
> 
>   - UefiCpuPkg commit 6e1987f19af7 ("UefiCpuPkg/MpInitLib: wait no
>     longer than necessary for initial AP startup", 2016-11-24)
> 
>   - and OvmfPkg commit 45a70db3c3a5 ("OvmfPkg/PlatformPei: take VCPU
>     count from QEMU and configure MpInitLib", 2016-11-24).
> 
> 
> So, again, the use case that I would like to cover is:
> 
> * the exact number of APs is known at boot, to OvmfPkg/PlatformPei,
> 
> * after the very first INIT-SIPI-SIPI, MpInitLib should wait for exactly
>   this number of APs to "report in", regardless of:
> 
>   - how long it takes,
> 
>   - in what order / sequence the APs report in. (Again, please remember
>     that some APs may complete the initialization before other APs
>     execute their very first instruction.)
> 
> * Preferably, the case should be handled when the processor count grows
>   from cold boot to S3 resume (VCPU hotplug). TianoCore BZ#251 used to
>   track this use case separately.
> 
> (
> 
> Jeff closed BZ#251 today, with the argument that commit 0594ec417c89 (this
> patch) finds the CPU count dynamically anyway, so a platform can simply set
> "PcdCpuMaxLogicalProcessorNumber" to the maximum possible value.
> 
> This argument does not work in a virtual machine, because commit
> 0594ec417c89 (this patch) may in fact not find the VCPU count correctly
> -- in a VM, (NumApsExecuting==0) does not imply that all APs have finished.
> 
> )
> 
> Thanks!
> Laszlo

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
  2017-10-25  5:42         ` Dong, Eric
@ 2017-10-25 15:07           ` Laszlo Ersek
  2017-10-26  1:13             ` Dong, Eric
  0 siblings, 1 reply; 23+ messages in thread
From: Laszlo Ersek @ 2017-10-25 15:07 UTC (permalink / raw)
  To: Dong, Eric, edk2-devel@lists.01.org
  Cc: Ni, Ruiyu, Paolo Bonzini, Jeff Fan, Brian J. Johnson

Hi Eric,

On 10/25/17 07:42, Dong, Eric wrote:
> Hi Laszlo,
> 
> I think I have clear about your raised issues for Ovmf platform. In this case, I think your platform not suit for this code change.  How about I do below change based on the new code:
> 
> -      if (CpuMpData->CpuCount == 0) {
>         TimedWaitForApFinish (
>           CpuMpData,
>           PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
>           PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
>           );
> -      }
> 
> After this change, for Ovmf can still set
> PcdCpuApInitTimeOutInMicroSeconds to MAX_UINT32 to keep old solution.
> For the real platform, it can set a small value to follow the new
> solution. For this new change, it just wait one more
> PcdCpuApInitTimeOutInMicroSeconds timeout, should not have
> performance impact (This time may also been cost in later while
> (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) loop) or have
> litter performance impact (not cover by the later loop).
The suggested change will work for OVMF, thanks!

(Just please don't forget to un-indent the TimedWaitForApFinish() call
that will become unconditional again.)

--o--

Do you think Brian's concern should be investigated as well (perhaps in
a separate Bugzilla entry)?

Because, I believe, the scheduling pattern that I described earlier
could be possible on physical platforms as well, at least in theory:

>> (2) After at least one AP has started running, the logic expects
>> "NumApsExecuting" to monotonically grow for a while, and then to
>> monotonically decrease, back to zero. For example, if we have 1 BSP and
>> 7 APs, the BSP logic more or less expects the following values in
>> "NumApsExecuting":
>>
>> 1; 2; 3; 4; 5; 6; 7;
>> 6; 5; 4; 3; 2; 1; 0
>>
>>
>> While this may be a valid expectation for physical processors (which actually
>> run in parallel, in the physical world), in a virtual machine, it is not guaranteed.
>> Dependent on hypervisor scheduling artifacts, it is possible that, say, three
>> APs start up *and finish* before the remaining four APs start up *at all*. The
>> INIT-SIPI-SIPI marks all APs for execution / scheduling in the hypervisor, yes,
>> but the actual code execution may commence a lot later. For example, the
>> BSP may witness the following series of values in "NumApsExecuting":
>>
>> 1; 2; 3;
>> 2; 1; 0;
>> 1; 2; 3; 4;
>> 3; 2; 1; 0
>>
>> and the BSP could think that there are 3 APs only, when it sees the first 0
>> value.

I don't know if such a pattern is "likely", "unlikely", or "extremely
unlikely" on physical platforms. I think the pattern is "theoretically
possible" at least.

I suggest that we go ahead with the small patch for the OVMF use case
first, and then open a new BZ if Brian thinks there's a real safety
problem with the code.

... I should note that, with the small patch that's going to fix the
OVMF use case, the previous behavior will be restored for *all*
platforms that continue using their previous PCD values.

The cumulative effect of commit 0594ec417c89 and the upcoming patch will
be that a platform may significantly decrease
"PcdCpuApInitTimeOutInMicroSeconds", if it chooses to, and then the
"NumApsExecuting" loop will take the role of the preexisting
TimedWaitForApFinish() call. If a platform doesn't want that, then it
should keep its "PcdCpuApInitTimeOutInMicroSeconds" as high as before,
and then any implementation details in the "NumApsExecuting" loop will
be irrelevant.

Thanks!
Laszlo


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
  2017-10-25 15:07           ` Laszlo Ersek
@ 2017-10-26  1:13             ` Dong, Eric
  2017-10-26 20:48               ` Brian J. Johnson
  0 siblings, 1 reply; 23+ messages in thread
From: Dong, Eric @ 2017-10-26  1:13 UTC (permalink / raw)
  To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Paolo Bonzini

Laszlo,


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Wednesday, October 25, 2017 11:08 PM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for
> AP initialization logic.
> 
> Hi Eric,
> 
> On 10/25/17 07:42, Dong, Eric wrote:
> > Hi Laszlo,
> >
> > I think I have clear about your raised issues for Ovmf platform. In this case, I
> think your platform not suit for this code change.  How about I do below
> change based on the new code:
> >
> > -      if (CpuMpData->CpuCount == 0) {
> >         TimedWaitForApFinish (
> >           CpuMpData,
> >           PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
> >           PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
> >           );
> > -      }
> >
> > After this change, for Ovmf can still set
> > PcdCpuApInitTimeOutInMicroSeconds to MAX_UINT32 to keep old
> solution.
> > For the real platform, it can set a small value to follow the new
> > solution. For this new change, it just wait one more
> > PcdCpuApInitTimeOutInMicroSeconds timeout, should not have
> performance
> > impact (This time may also been cost in later while
> > (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) loop) or have
> > litter performance impact (not cover by the later loop).
> The suggested change will work for OVMF, thanks!
> 
> (Just please don't forget to un-indent the TimedWaitForApFinish() call that
> will become unconditional again.)
> 
> --o--
> 
> Do you think Brian's concern should be investigated as well (perhaps in a
> separate Bugzilla entry)?

As Jeff has mentioned, only Ovmf can know the exist Ap numbers before 
send the Init-sipi-sipi.  So we don't know how many bits needed for this bitmap.
In case we can create the bitmap, we still don't know when all Aps have been
 found(If the begin map bit value same as finish map bit value, we still can't get 
the conclusion that all Aps have been found, because maybe other Aps not
started yet).

Thanks,
Eric
> 
> Because, I believe, the scheduling pattern that I described earlier could be
> possible on physical platforms as well, at least in theory:
> 
> >> (2) After at least one AP has started running, the logic expects
> >> "NumApsExecuting" to monotonically grow for a while, and then to
> >> monotonically decrease, back to zero. For example, if we have 1 BSP
> >> and
> >> 7 APs, the BSP logic more or less expects the following values in
> >> "NumApsExecuting":
> >>
> >> 1; 2; 3; 4; 5; 6; 7;
> >> 6; 5; 4; 3; 2; 1; 0
> >>
> >>
> >> While this may be a valid expectation for physical processors (which
> >> actually run in parallel, in the physical world), in a virtual machine, it is not
> guaranteed.
> >> Dependent on hypervisor scheduling artifacts, it is possible that,
> >> say, three APs start up *and finish* before the remaining four APs
> >> start up *at all*. The INIT-SIPI-SIPI marks all APs for execution /
> >> scheduling in the hypervisor, yes, but the actual code execution may
> >> commence a lot later. For example, the BSP may witness the following
> series of values in "NumApsExecuting":
> >>
> >> 1; 2; 3;
> >> 2; 1; 0;
> >> 1; 2; 3; 4;
> >> 3; 2; 1; 0
> >>
> >> and the BSP could think that there are 3 APs only, when it sees the
> >> first 0 value.
> 
> I don't know if such a pattern is "likely", "unlikely", or "extremely unlikely" on
> physical platforms. I think the pattern is "theoretically possible" at least.
> 
> I suggest that we go ahead with the small patch for the OVMF use case first,
> and then open a new BZ if Brian thinks there's a real safety problem with the
> code.
> 

We also notice this theoretical problem when we implement this change, but 
We think this is rarely to happen on physical platforms. We think the value for 
the change is much bigger than not do this change because of this theoretical 
problem.

Thanks,
Eric
> ... I should note that, with the small patch that's going to fix the OVMF use
> case, the previous behavior will be restored for *all* platforms that continue
> using their previous PCD values.
> 
> The cumulative effect of commit 0594ec417c89 and the upcoming patch will
> be that a platform may significantly decrease
> "PcdCpuApInitTimeOutInMicroSeconds", if it chooses to, and then the
> "NumApsExecuting" loop will take the role of the preexisting
> TimedWaitForApFinish() call. If a platform doesn't want that, then it should
> keep its "PcdCpuApInitTimeOutInMicroSeconds" as high as before, and then
> any implementation details in the "NumApsExecuting" loop will be irrelevant.
> 
> Thanks!
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
  2017-10-26  1:13             ` Dong, Eric
@ 2017-10-26 20:48               ` Brian J. Johnson
  2017-10-27  1:31                 ` Dong, Eric
  0 siblings, 1 reply; 23+ messages in thread
From: Brian J. Johnson @ 2017-10-26 20:48 UTC (permalink / raw)
  To: Dong, Eric, Laszlo Ersek, edk2-devel@lists.01.org
  Cc: Ni, Ruiyu, Paolo Bonzini

On 10/25/2017 08:13 PM, Dong, Eric wrote:
> Laszlo,
> 
> 
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> Sent: Wednesday, October 25, 2017 11:08 PM
>> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini <pbonzini@redhat.com>
>> Subject: Re: [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for
>> AP initialization logic.
>>
>> Hi Eric,
>>
>> On 10/25/17 07:42, Dong, Eric wrote:
>>> Hi Laszlo,
>>>
>>> I think I have clear about your raised issues for Ovmf platform. In this case, I
>> think your platform not suit for this code change.  How about I do below
>> change based on the new code:
>>>
>>> -      if (CpuMpData->CpuCount == 0) {
>>>          TimedWaitForApFinish (
>>>            CpuMpData,
>>>            PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
>>>            PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
>>>            );
>>> -      }
>>>
>>> After this change, for Ovmf can still set
>>> PcdCpuApInitTimeOutInMicroSeconds to MAX_UINT32 to keep old
>> solution.
>>> For the real platform, it can set a small value to follow the new
>>> solution. For this new change, it just wait one more
>>> PcdCpuApInitTimeOutInMicroSeconds timeout, should not have
>> performance
>>> impact (This time may also been cost in later while
>>> (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) loop) or have
>>> litter performance impact (not cover by the later loop).
>> The suggested change will work for OVMF, thanks!
>>
>> (Just please don't forget to un-indent the TimedWaitForApFinish() call that
>> will become unconditional again.)
>>
>> --o--
>>
>> Do you think Brian's concern should be investigated as well (perhaps in a
>> separate Bugzilla entry)?
> 
> As Jeff has mentioned, only Ovmf can know the exist Ap numbers before
> send the Init-sipi-sipi.  So we don't know how many bits needed for this bitmap.
> In case we can create the bitmap, we still don't know when all Aps have been
>   found(If the begin map bit value same as finish map bit value, we still can't get
> the conclusion that all Aps have been found, because maybe other Aps not
> started yet).
> 

Right, physical platforms don't usually know the number of CPUs up 
front, so they still need a timeout.  That's unavoidable.  But we've 
seen cases where interrupts aren't getting delivered reliably, so not 
all the expected CPUs start up (based on the core counts in the physical 
sockets, as known by the developing engineer, not the firmware.)  To 
debug this failure, it's very useful to have a list of exactly which 
CPUs did start.

Similarly, we've seen cases where a CPU starts, but crashes due to h/w 
or s/w bugs before signaling the BSP that it has finished.  With only a 
counter to indicate how many CPUs are still running, it's impossible to 
tell which CPUs failed.  That makes debugging much more difficult.

The bitmaps would need to be sized according to the maximum number of 
CPUs supported by the platform (or the maximum APIC ID, depending on how 
they are indexed), like other data structures in the MpCpu code.

Bitmaps aren't the only possible implementation of course....  My point 
is that it's useful to have a list of which CPUs started executing, and 
which finished.

> Thanks,
> Eric
>>
>> Because, I believe, the scheduling pattern that I described earlier could be
>> possible on physical platforms as well, at least in theory:
>>
>>>> (2) After at least one AP has started running, the logic expects
>>>> "NumApsExecuting" to monotonically grow for a while, and then to
>>>> monotonically decrease, back to zero. For example, if we have 1 BSP
>>>> and
>>>> 7 APs, the BSP logic more or less expects the following values in
>>>> "NumApsExecuting":
>>>>
>>>> 1; 2; 3; 4; 5; 6; 7;
>>>> 6; 5; 4; 3; 2; 1; 0
>>>>
>>>>
>>>> While this may be a valid expectation for physical processors (which
>>>> actually run in parallel, in the physical world), in a virtual machine, it is not
>> guaranteed.
>>>> Dependent on hypervisor scheduling artifacts, it is possible that,
>>>> say, three APs start up *and finish* before the remaining four APs
>>>> start up *at all*. The INIT-SIPI-SIPI marks all APs for execution /
>>>> scheduling in the hypervisor, yes, but the actual code execution may
>>>> commence a lot later. For example, the BSP may witness the following
>> series of values in "NumApsExecuting":
>>>>
>>>> 1; 2; 3;
>>>> 2; 1; 0;
>>>> 1; 2; 3; 4;
>>>> 3; 2; 1; 0
>>>>
>>>> and the BSP could think that there are 3 APs only, when it sees the
>>>> first 0 value.
>>
>> I don't know if such a pattern is "likely", "unlikely", or "extremely unlikely" on
>> physical platforms. I think the pattern is "theoretically possible" at least.
>>
>> I suggest that we go ahead with the small patch for the OVMF use case first,
>> and then open a new BZ if Brian thinks there's a real safety problem with the
>> code.
>>
> 
> We also notice this theoretical problem when we implement this change, but
> We think this is rarely to happen on physical platforms. We think the value for
> the change is much bigger than not do this change because of this theoretical
> problem.

I strongly disagree.  Any chance for it to happen on physical platforms 
is too large of a chance.  There's simply too much variance between 
physical platforms to make assumptions about interrupt delivery and the 
interleaving of atomic operations among dozens (or thousands!) of CPUs.

With the latest change from Eric above, we can restore the old behavior 
by setting PcdCpuApInitTimeOutInMicroSeconds large enough to cover all 
APs.  So the new behavior is just an optimization which a platform can 
enable if its timing characteristics are suitable for it.  That should work.

Thanks,
Brian

> 
> Thanks,
> Eric
>> ... I should note that, with the small patch that's going to fix the OVMF use
>> case, the previous behavior will be restored for *all* platforms that continue
>> using their previous PCD values.
>>
>> The cumulative effect of commit 0594ec417c89 and the upcoming patch will
>> be that a platform may significantly decrease
>> "PcdCpuApInitTimeOutInMicroSeconds", if it chooses to, and then the
>> "NumApsExecuting" loop will take the role of the preexisting
>> TimedWaitForApFinish() call. If a platform doesn't want that, then it should
>> keep its "PcdCpuApInitTimeOutInMicroSeconds" as high as before, and then
>> any implementation details in the "NumApsExecuting" loop will be irrelevant.
>>
>> Thanks!
>> Laszlo
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 


-- 

                                                 Brian

--------------------------------------------------------------------

   "This isn't a design, it's a hack."
                                            -- Stephen Gunn


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
  2017-10-26 20:48               ` Brian J. Johnson
@ 2017-10-27  1:31                 ` Dong, Eric
  0 siblings, 0 replies; 23+ messages in thread
From: Dong, Eric @ 2017-10-27  1:31 UTC (permalink / raw)
  To: Brian J. Johnson, Laszlo Ersek, edk2-devel@lists.01.org
  Cc: Ni, Ruiyu, Paolo Bonzini

Brian,

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Brian J. Johnson
> Sent: Friday, October 27, 2017 4:48 AM
> To: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini <pbonzini@redhat.com>
> Subject: Re: [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for
> AP initialization logic.
> 
> On 10/25/2017 08:13 PM, Dong, Eric wrote:
> > Laszlo,
> >
> >
> >> -----Original Message-----
> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
> >> Of Laszlo Ersek
> >> Sent: Wednesday, October 25, 2017 11:08 PM
> >> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini
> >> <pbonzini@redhat.com>
> >> Subject: Re: [edk2] [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting
> >> for AP initialization logic.
> >>
> >> Hi Eric,
> >>
> >> On 10/25/17 07:42, Dong, Eric wrote:
> >>> Hi Laszlo,
> >>>
> >>> I think I have clear about your raised issues for Ovmf platform. In
> >>> this case, I
> >> think your platform not suit for this code change.  How about I do
> >> below change based on the new code:
> >>>
> >>> -      if (CpuMpData->CpuCount == 0) {
> >>>          TimedWaitForApFinish (
> >>>            CpuMpData,
> >>>            PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
> >>>            PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
> >>>            );
> >>> -      }
> >>>
> >>> After this change, for Ovmf can still set
> >>> PcdCpuApInitTimeOutInMicroSeconds to MAX_UINT32 to keep old
> >> solution.
> >>> For the real platform, it can set a small value to follow the new
> >>> solution. For this new change, it just wait one more
> >>> PcdCpuApInitTimeOutInMicroSeconds timeout, should not have
> >> performance
> >>> impact (This time may also been cost in later while
> >>> (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) loop) or
> have
> >>> litter performance impact (not cover by the later loop).
> >> The suggested change will work for OVMF, thanks!
> >>
> >> (Just please don't forget to un-indent the TimedWaitForApFinish()
> >> call that will become unconditional again.)
> >>
> >> --o--
> >>
> >> Do you think Brian's concern should be investigated as well (perhaps
> >> in a separate Bugzilla entry)?
> >
> > As Jeff has mentioned, only Ovmf can know the exist Ap numbers before
> > send the Init-sipi-sipi.  So we don't know how many bits needed for this
> bitmap.
> > In case we can create the bitmap, we still don't know when all Aps have
> been
> >   found(If the begin map bit value same as finish map bit value, we
> > still can't get the conclusion that all Aps have been found, because
> > maybe other Aps not started yet).
> >
> 
> Right, physical platforms don't usually know the number of CPUs up front, so
> they still need a timeout.  That's unavoidable.  But we've seen cases where
> interrupts aren't getting delivered reliably, so not all the expected CPUs start
> up (based on the core counts in the physical sockets, as known by the
> developing engineer, not the firmware.)  To debug this failure, it's very
> useful to have a list of exactly which CPUs did start.
> 
> Similarly, we've seen cases where a CPU starts, but crashes due to h/w or
> s/w bugs before signaling the BSP that it has finished.  With only a counter to
> indicate how many CPUs are still running, it's impossible to tell which CPUs
> failed.  That makes debugging much more difficult.
> 
> The bitmaps would need to be sized according to the maximum number of
> CPUs supported by the platform (or the maximum APIC ID, depending on
> how they are indexed), like other data structures in the MpCpu code.
> 
> Bitmaps aren't the only possible implementation of course....  My point is
> that it's useful to have a list of which CPUs started executing, and which
> finished.
> 

Seems this is not a must have item for this patch related issue. I think you can submit
A bugz for this debug feature.

Thanks,
Eric

> > Thanks,
> > Eric
> >>
> >> Because, I believe, the scheduling pattern that I described earlier
> >> could be possible on physical platforms as well, at least in theory:
> >>
> >>>> (2) After at least one AP has started running, the logic expects
> >>>> "NumApsExecuting" to monotonically grow for a while, and then to
> >>>> monotonically decrease, back to zero. For example, if we have 1 BSP
> >>>> and
> >>>> 7 APs, the BSP logic more or less expects the following values in
> >>>> "NumApsExecuting":
> >>>>
> >>>> 1; 2; 3; 4; 5; 6; 7;
> >>>> 6; 5; 4; 3; 2; 1; 0
> >>>>
> >>>>
> >>>> While this may be a valid expectation for physical processors
> >>>> (which actually run in parallel, in the physical world), in a
> >>>> virtual machine, it is not
> >> guaranteed.
> >>>> Dependent on hypervisor scheduling artifacts, it is possible that,
> >>>> say, three APs start up *and finish* before the remaining four APs
> >>>> start up *at all*. The INIT-SIPI-SIPI marks all APs for execution /
> >>>> scheduling in the hypervisor, yes, but the actual code execution
> >>>> may commence a lot later. For example, the BSP may witness the
> >>>> following
> >> series of values in "NumApsExecuting":
> >>>>
> >>>> 1; 2; 3;
> >>>> 2; 1; 0;
> >>>> 1; 2; 3; 4;
> >>>> 3; 2; 1; 0
> >>>>
> >>>> and the BSP could think that there are 3 APs only, when it sees the
> >>>> first 0 value.
> >>
> >> I don't know if such a pattern is "likely", "unlikely", or "extremely
> >> unlikely" on physical platforms. I think the pattern is "theoretically
> possible" at least.
> >>
> >> I suggest that we go ahead with the small patch for the OVMF use case
> >> first, and then open a new BZ if Brian thinks there's a real safety
> >> problem with the code.
> >>
> >
> > We also notice this theoretical problem when we implement this change,
> > but We think this is rarely to happen on physical platforms. We think
> > the value for the change is much bigger than not do this change
> > because of this theoretical problem.
> 
> I strongly disagree.  Any chance for it to happen on physical platforms is too
> large of a chance.  There's simply too much variance between physical
> platforms to make assumptions about interrupt delivery and the interleaving
> of atomic operations among dozens (or thousands!) of CPUs.
> 
> With the latest change from Eric above, we can restore the old behavior by
> setting PcdCpuApInitTimeOutInMicroSeconds large enough to cover all APs.
> So the new behavior is just an optimization which a platform can enable if its
> timing characteristics are suitable for it.  That should work.
> 
> Thanks,
> Brian
> 
> >
> > Thanks,
> > Eric
> >> ... I should note that, with the small patch that's going to fix the
> >> OVMF use case, the previous behavior will be restored for *all*
> >> platforms that continue using their previous PCD values.
> >>
> >> The cumulative effect of commit 0594ec417c89 and the upcoming patch
> >> will be that a platform may significantly decrease
> >> "PcdCpuApInitTimeOutInMicroSeconds", if it chooses to, and then the
> >> "NumApsExecuting" loop will take the role of the preexisting
> >> TimedWaitForApFinish() call. If a platform doesn't want that, then it
> >> should keep its "PcdCpuApInitTimeOutInMicroSeconds" as high as
> >> before, and then any implementation details in the "NumApsExecuting"
> loop will be irrelevant.
> >>
> >> Thanks!
> >> Laszlo
> >> _______________________________________________
> >> edk2-devel mailing list
> >> edk2-devel@lists.01.org
> >> https://lists.01.org/mailman/listinfo/edk2-devel
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> >
> 
> 
> --
> 
>                                                  Brian
> 
> --------------------------------------------------------------------
> 
>    "This isn't a design, it's a hack."
>                                             -- Stephen Gunn
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2017-10-27  1:27 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
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
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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox