* [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