From: "Dong, Eric" <eric.dong@intel.com>
To: "Dong, Eric" <eric.dong@intel.com>,
Laszlo Ersek <lersek@redhat.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Ni, Ruiyu" <ruiyu.ni@intel.com>, Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic.
Date: Tue, 24 Oct 2017 15:40:26 +0000 [thread overview]
Message-ID: <ED077930C258884BBCB450DB737E66224AA3DAB9@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <ED077930C258884BBCB450DB737E66224AA3DA63@shsmsx102.ccr.corp.intel.com>
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
next prev parent reply other threads:[~2017-10-24 15:36 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-23 7:22 [Patch 0/2] Enhance collect AP Count logic Eric Dong
2017-10-23 7:22 ` [Patch 1/2] UefiCpuPkg/MpInitLib: Change AP Index variable name Eric Dong
2017-10-24 6:00 ` Ni, Ruiyu
2017-10-23 7:22 ` [Patch 2/2] UefiCpuPkg/MpInitLib: Enhance waiting for AP initialization logic Eric Dong
2017-10-24 6:02 ` Ni, Ruiyu
2017-10-24 6:27 ` 答复: " Fan Jeff
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 [this message]
2017-10-24 17:40 ` Laszlo Ersek
2017-10-24 22:30 ` Brian J. Johnson
2017-10-25 5:35 ` 答复: " Fan Jeff
2017-10-25 5:32 ` Fan Jeff
2017-10-25 5:42 ` Dong, Eric
2017-10-25 15:07 ` Laszlo Ersek
2017-10-26 1:13 ` Dong, Eric
2017-10-26 20:48 ` Brian J. Johnson
2017-10-27 1:31 ` Dong, Eric
2017-10-24 7:31 ` 答复: [Patch 0/2] Enhance collect AP Count logic Fan Jeff
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ED077930C258884BBCB450DB737E66224AA3DAB9@shsmsx102.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox