Re: [edk2] [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs.
> +if (GetApState (CpuData) == CpuStateDisabled && !WakeUpDisabledAps) {
> + continue;
> +}
> +
> CpuData->ApFunction = (UINTN) Procedure;
> CpuData->ApFunctionArgument = (UINTN) ProcedureArgument;
> SetApState (CpuData, CpuStateReady);
Eric, can you add more comments here to say ALL AP will be woke up by
INIT-SIPI-SIPI,
but the AP procedure will be skipped when AP state is not Ready.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs.
With that, Reviewed-by: Ruiyu Ni
Thanks/Ray
> -Original Message-
> From: Ni, Ruiyu
> Sent: Thursday, July 26, 2018 4:36 PM
> To: Dong, Eric ; edk2-devel@lists.01.org
> Cc: Laszlo Ersek
> Subject: RE: [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP when
> call
> StartAllAPs.
>
> > +if (GetApState (CpuData) == CpuStateDisabled && !WakeUpDisabledAps)
> {
> > + continue;
> > +}
> > +
> > CpuData->ApFunction = (UINTN) Procedure;
> > CpuData->ApFunctionArgument = (UINTN) ProcedureArgument;
> > SetApState (CpuData, CpuStateReady);
>
> Eric, can you add more comments here to say ALL AP will be woke up by INIT-
> SIPI-SIPI, but the AP procedure will be skipped when AP state is not Ready.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs.
Hi Laszlo,
> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, July 25, 2018 8:12 PM
> To: Dong, Eric ; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu
> Subject: Re: [edk2] [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP
> when call StartAllAPs.
>
> Hi Eric,
>
> On 07/25/18 09:50, Eric Dong wrote:
> > Base on UEFI spec requirement, StartAllAPs function should not use the
> > APs which has been disabled before. This patch just change current
> > code to follow this rule.
> >
> > V3 changes:
> > Only called by StartUpAllAps, WakeUpAp will not wake up the disabled
> > APs, in other cases also need to include the disabled APs, such as
> > CpuDxe driver start up and ChangeApLoopCallback function.
> >
> > Cc: Laszlo Ersek
> > Cc: Ruiyu Ni
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong
> > ---
> > UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 2 +-
> > UefiCpuPkg/Library/MpInitLib/MpLib.c| 27 +--
> > UefiCpuPkg/Library/MpInitLib/MpLib.h| 4 +++-
> > 3 files changed, 21 insertions(+), 12 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > index d82e9aea45..c17daa0fc0 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > @@ -306,7 +306,7 @@ MpInitChangeApLoopCallback (
> >CpuMpData->PmCodeSegment = GetProtectedModeCS ();
> >CpuMpData->ApLoopMode = PcdGet8 (PcdCpuApLoopMode);
> >mNumberToFinish = CpuMpData->CpuCount - 1;
> > - WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL);
> > + WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL, TRUE);
> >while (mNumberToFinish > 0) {
> > CpuPause ();
> >}
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > index 0e57cc86bf..1a81062a3f 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > @@ -470,7 +470,7 @@ CollectProcessorCount (
> >//
> >CpuMpData->InitFlag = ApInitConfig;
> >CpuMpData->X2ApicEnable = FALSE;
> > - WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL);
> > + WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL, TRUE);
> >CpuMpData->InitFlag = ApInitDone;
> >ASSERT (CpuMpData->CpuCount <= PcdGet32
> (PcdCpuMaxLogicalProcessorNumber));
> >//
> > @@ -491,7 +491,7 @@ CollectProcessorCount (
> > //
> > // Wakeup all APs to enable x2APIC mode
> > //
> > -WakeUpAP (CpuMpData, TRUE, 0, ApFuncEnableX2Apic, NULL);
> > +WakeUpAP (CpuMpData, TRUE, 0, ApFuncEnableX2Apic, NULL, TRUE);
> > //
> > // Wait for all known APs finished
> > //
> > @@ -969,6 +969,7 @@ FreeResetVector (
> >@param[in] ProcessorNumberThe handle number of specified
> processor
> >@param[in] Procedure The function to be invoked by AP
> >@param[in] ProcedureArgument The argument to be passed into AP
> > function
> > + @param[in] WakeUpDisabledAps Whether need to wake up disabled APs
> in broadcast mode.
> > **/
> > VOID
> > WakeUpAP (
> > @@ -976,7 +977,8 @@ WakeUpAP (
> >IN BOOLEAN Broadcast,
> >IN UINTN ProcessorNumber,
> >IN EFI_AP_PROCEDURE Procedure, OPTIONAL
> > - IN VOID *ProcedureArgument OPTIONAL
> > + IN VOID *ProcedureArgument, OPTIONAL
> > + IN BOOLEAN WakeUpDisabledAps
> >)
> > {
> >volatile MP_CPU_EXCHANGE_INFO*ExchangeInfo;
> > @@ -1010,6 +1012,10 @@ WakeUpAP (
> > for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
> >if (Index != CpuMpData->BspNumber) {
> > CpuData = >CpuData[Index];
> > +if (GetApState (CpuData) == CpuStateDisabled
> && !WakeUpDisabledAps) {
> > + continue;
> > +}
> > +
> > CpuData->ApFunction = (UINTN) Procedure;
> > CpuData->ApFunctionArgument = (UINTN) ProcedureArgument;
> > SetApState (CpuData, CpuStateReady);
>
> I think something *might* be missing from the patch here, but I'm not sure.
>
> Namely, is it possible that we can reach WakeUpAP() with:
>
> (Broadcast && WakeUpDisabledAps)
>
> *after* some APs have been disabled?
>
Re: [edk2] [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs.
Hi Eric,
On 07/25/18 09:50, Eric Dong wrote:
> Base on UEFI spec requirement, StartAllAPs function should not
> use the APs which has been disabled before. This patch just
> change current code to follow this rule.
>
> V3 changes:
> Only called by StartUpAllAps, WakeUpAp will not wake up
> the disabled APs, in other cases also need to include the
> disabled APs, such as CpuDxe driver start up and
> ChangeApLoopCallback function.
>
> Cc: Laszlo Ersek
> Cc: Ruiyu Ni
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong
> ---
> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 2 +-
> UefiCpuPkg/Library/MpInitLib/MpLib.c| 27 +--
> UefiCpuPkg/Library/MpInitLib/MpLib.h| 4 +++-
> 3 files changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index d82e9aea45..c17daa0fc0 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -306,7 +306,7 @@ MpInitChangeApLoopCallback (
>CpuMpData->PmCodeSegment = GetProtectedModeCS ();
>CpuMpData->ApLoopMode = PcdGet8 (PcdCpuApLoopMode);
>mNumberToFinish = CpuMpData->CpuCount - 1;
> - WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL);
> + WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL, TRUE);
>while (mNumberToFinish > 0) {
> CpuPause ();
>}
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 0e57cc86bf..1a81062a3f 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -470,7 +470,7 @@ CollectProcessorCount (
>//
>CpuMpData->InitFlag = ApInitConfig;
>CpuMpData->X2ApicEnable = FALSE;
> - WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL);
> + WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL, TRUE);
>CpuMpData->InitFlag = ApInitDone;
>ASSERT (CpuMpData->CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
>//
> @@ -491,7 +491,7 @@ CollectProcessorCount (
> //
> // Wakeup all APs to enable x2APIC mode
> //
> -WakeUpAP (CpuMpData, TRUE, 0, ApFuncEnableX2Apic, NULL);
> +WakeUpAP (CpuMpData, TRUE, 0, ApFuncEnableX2Apic, NULL, TRUE);
> //
> // Wait for all known APs finished
> //
> @@ -969,6 +969,7 @@ FreeResetVector (
>@param[in] ProcessorNumberThe handle number of specified processor
>@param[in] Procedure The function to be invoked by AP
>@param[in] ProcedureArgument The argument to be passed into AP function
> + @param[in] WakeUpDisabledAps Whether need to wake up disabled APs in
> broadcast mode.
> **/
> VOID
> WakeUpAP (
> @@ -976,7 +977,8 @@ WakeUpAP (
>IN BOOLEAN Broadcast,
>IN UINTN ProcessorNumber,
>IN EFI_AP_PROCEDURE Procedure, OPTIONAL
> - IN VOID *ProcedureArgument OPTIONAL
> + IN VOID *ProcedureArgument, OPTIONAL
> + IN BOOLEAN WakeUpDisabledAps
>)
> {
>volatile MP_CPU_EXCHANGE_INFO*ExchangeInfo;
> @@ -1010,6 +1012,10 @@ WakeUpAP (
> for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
>if (Index != CpuMpData->BspNumber) {
> CpuData = >CpuData[Index];
> +if (GetApState (CpuData) == CpuStateDisabled && !WakeUpDisabledAps) {
> + continue;
> +}
> +
> CpuData->ApFunction = (UINTN) Procedure;
> CpuData->ApFunctionArgument = (UINTN) ProcedureArgument;
> SetApState (CpuData, CpuStateReady);
I think something *might* be missing from the patch here, but I'm not sure.
Namely, is it possible that we can reach WakeUpAP() with:
(Broadcast && WakeUpDisabledAps)
*after* some APs have been disabled?
Because, in that case, we set the AP state from Disabled to Ready (and
the AP will perform the Procedure as necessary) -- however, once the AP
is done, we do not seem to re-set its state to Disabled.
Is that correct?
I tried to collect all the WakeUpAp() calls that pass TRUE for both
booleans mentioned above. Their callers are as follows:
- MpInitLibInitialize()
- CollectProcessorCount()
- MpInitChangeApLoopCallback()
>From these three, MpInitLibInitialize() and CollectProcessorCount() run
before the protocol or PPI user has any chance to disable a CPU, so it
looks impossible to reach the problematic code path I'm describing from
those functions.
What about MpInitChangeApLoopCallback()?... Hm, that function is only
called as a notification function for:
- the exit-boot-services event, and
- the legacy boot event
After which the MP protocol is unusable anyway, so it doesn't matter if
we don't flip the AP status back to Disabled.
OK, so this patch looks correct to me; can you please update the commit
message:
"WakeUpAP() is called with (Broadcast && WakeUpDisabledAps) from
MpInitLibInitialize(), CollectProcessorCount()
[edk2] [Patch v3 3/3] UefiCpuPkg/MpInitLib: Not use disabled AP when call StartAllAPs.
Base on UEFI spec requirement, StartAllAPs function should not
use the APs which has been disabled before. This patch just
change current code to follow this rule.
V3 changes:
Only called by StartUpAllAps, WakeUpAp will not wake up
the disabled APs, in other cases also need to include the
disabled APs, such as CpuDxe driver start up and
ChangeApLoopCallback function.
Cc: Laszlo Ersek
Cc: Ruiyu Ni
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong
---
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 2 +-
UefiCpuPkg/Library/MpInitLib/MpLib.c| 27 +--
UefiCpuPkg/Library/MpInitLib/MpLib.h| 4 +++-
3 files changed, 21 insertions(+), 12 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index d82e9aea45..c17daa0fc0 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -306,7 +306,7 @@ MpInitChangeApLoopCallback (
CpuMpData->PmCodeSegment = GetProtectedModeCS ();
CpuMpData->ApLoopMode = PcdGet8 (PcdCpuApLoopMode);
mNumberToFinish = CpuMpData->CpuCount - 1;
- WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL);
+ WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL, TRUE);
while (mNumberToFinish > 0) {
CpuPause ();
}
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 0e57cc86bf..1a81062a3f 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -470,7 +470,7 @@ CollectProcessorCount (
//
CpuMpData->InitFlag = ApInitConfig;
CpuMpData->X2ApicEnable = FALSE;
- WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL);
+ WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL, TRUE);
CpuMpData->InitFlag = ApInitDone;
ASSERT (CpuMpData->CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber));
//
@@ -491,7 +491,7 @@ CollectProcessorCount (
//
// Wakeup all APs to enable x2APIC mode
//
-WakeUpAP (CpuMpData, TRUE, 0, ApFuncEnableX2Apic, NULL);
+WakeUpAP (CpuMpData, TRUE, 0, ApFuncEnableX2Apic, NULL, TRUE);
//
// Wait for all known APs finished
//
@@ -969,6 +969,7 @@ FreeResetVector (
@param[in] ProcessorNumberThe handle number of specified processor
@param[in] Procedure The function to be invoked by AP
@param[in] ProcedureArgument The argument to be passed into AP function
+ @param[in] WakeUpDisabledAps Whether need to wake up disabled APs in
broadcast mode.
**/
VOID
WakeUpAP (
@@ -976,7 +977,8 @@ WakeUpAP (
IN BOOLEAN Broadcast,
IN UINTN ProcessorNumber,
IN EFI_AP_PROCEDURE Procedure, OPTIONAL
- IN VOID *ProcedureArgument OPTIONAL
+ IN VOID *ProcedureArgument, OPTIONAL
+ IN BOOLEAN WakeUpDisabledAps
)
{
volatile MP_CPU_EXCHANGE_INFO*ExchangeInfo;
@@ -1010,6 +1012,10 @@ WakeUpAP (
for (Index = 0; Index < CpuMpData->CpuCount; Index++) {
if (Index != CpuMpData->BspNumber) {
CpuData = >CpuData[Index];
+if (GetApState (CpuData) == CpuStateDisabled && !WakeUpDisabledAps) {
+ continue;
+}
+
CpuData->ApFunction = (UINTN) Procedure;
CpuData->ApFunctionArgument = (UINTN) ProcedureArgument;
SetApState (CpuData, CpuStateReady);
@@ -1289,7 +1295,7 @@ ResetProcessorToIdleState (
CpuMpData = GetCpuMpData ();
CpuMpData->InitFlag = ApInitReconfig;
- WakeUpAP (CpuMpData, FALSE, ProcessorNumber, NULL, NULL);
+ WakeUpAP (CpuMpData, FALSE, ProcessorNumber, NULL, NULL, TRUE);
while (CpuMpData->FinishedCount < 1) {
CpuPause ();
}
@@ -1439,7 +1445,8 @@ CheckAllAPs (
FALSE,
(UINT32) NextProcessorNumber,
CpuMpData->Procedure,
-CpuMpData->ProcArguments
+CpuMpData->ProcArguments,
+TRUE
);
}
}
@@ -1711,7 +1718,7 @@ MpInitLibInitialize (
//
// Wakeup APs to do some AP initialize sync
//
- WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData);
+ WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE);
//
// Wait for all APs finished initialization
//
@@ -1906,7 +1913,7 @@ SwitchBSPWorker (
//
// Need to wakeUp AP (future BSP).
//
- WakeUpAP (CpuMpData, FALSE, ProcessorNumber, FutureBSPProc, CpuMpData);
+ WakeUpAP (CpuMpData, FALSE, ProcessorNumber, FutureBSPProc, CpuMpData, TRUE);
AsmExchangeRole (>BSPInfo, >APInfo);
@@ -2240,14 +2247,14 @@ StartupAllAPsWorker (
CpuMpData->WaitEvent = WaitEvent;
if (!SingleThread) {
-WakeUpAP (CpuMpData, TRUE, 0, Procedure, ProcedureArgument);
+WakeUpAP (CpuMpData, TRUE, 0, Procedure, ProcedureArgument, FALSE);
} else {
for (ProcessorNumber = 0; ProcessorNumber < ProcessorCount;
ProcessorNumber++) {
if
