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:ler...@redhat.com]
Sent: Tuesday, October 24, 2017 6:16 PM
To: Dong, Eric <eric.d...@intel.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu...@intel.com>; Paolo Bonzini <pbonz...@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
