* [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
* 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
* [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 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 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 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 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 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 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
* 答复: [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
* 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
* 答复: [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
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