Re: [edk2] [PATCH 4/6] UefiCpuPkg/CpuDxe: Allow BSP to continue once max APs are awake

[Laszlo Ersek]

On 10/29/15 02:32, Jordan Justen wrote:
> Since the CpuDxe has a fixed maximum number of APs that are controlled
> with PcdCpuMaxLogicalProcessorNumber, if the maximum number of APs are
> awake, then the BSP should be able to continue.
> 
> Since the APs may be updating the mMpSystemData.NumberOfProcessors
> during this early init phase, we use SynchronizationLib to update this
> variable on the AP and to read it on the BSP.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jordan Justen 
> Cc: Jeff Fan 
> Cc: Laszlo Ersek 
> ---
>  UefiCpuPkg/CpuDxe/CpuMp.c | 15 ---
>  UefiCpuPkg/CpuDxe/CpuMp.h |  2 +-
>  2 files changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c
> index 6a22b9d..1094292 100644
> --- a/UefiCpuPkg/CpuDxe/CpuMp.c
> +++ b/UefiCpuPkg/CpuDxe/CpuMp.c
> @@ -1474,7 +1474,7 @@ ApEntryPointInC (
>  // Store the Stack address, when reset the AP, We can found the original 
> address.
>  //
>  mMpSystemData.CpuDatas[mMpSystemData.NumberOfProcessors].TopOfStack = 
> TopOfApStack;
> -mMpSystemData.NumberOfProcessors++;
> +InterlockedIncrement ();
>  mMpSystemData.NumberOfEnabledProcessors++;
>} else {
>  Status = WhoAmI (, );
> @@ -1749,9 +1749,18 @@ InitializeMpSupport (
>  StartApsStackless ();
>  
>  //
> -// Wait for APs to arrive at the ApEntryPoint routine
> +// Wait for all APs to start
>  //
> -MicroSecondDelay (PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds));
> +Timeout = 0;
> +do {
> +  if (InterlockedCompareExchange32 (
> +, 0, 0) >=
> +  gMaxLogicalProcessorNumber) {
> +break;
> +  }

I suggest to add a CpuPause() call here. I think it might help on
physical platforms too, but it definitely helps on hypervisors / PCPUs
that support Pause Loop Exiting.

The CpuPause() call will likely make each iteration of this loop to take
longer, but gPollInterval below is a lower bound anyway.

Anyway, this is just an idea. With or without CpuPause():

Reviewed-by: Laszlo Ersek 

Thanks
Laszlo

> +  MicroSecondDelay (gPollInterval);
> +  Timeout += gPollInterval;
> +} while (Timeout <= PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds));
>}
>  
>DEBUG ((DEBUG_INFO, "Detect CPU count: %d\n", 
> mMpSystemData.NumberOfProcessors));
> diff --git a/UefiCpuPkg/CpuDxe/CpuMp.h b/UefiCpuPkg/CpuDxe/CpuMp.h
> index 503f3ae..ab7a7f5 100644
> --- a/UefiCpuPkg/CpuDxe/CpuMp.h
> +++ b/UefiCpuPkg/CpuDxe/CpuMp.h
> @@ -114,7 +114,7 @@ typedef struct {
>  **/
>  typedef struct {
>CPU_DATA_BLOCK  *CpuDatas;
> -  UINTN   NumberOfProcessors;
> +  UINT32  NumberOfProcessors;
>UINTN   NumberOfEnabledProcessors;
>  
>EFI_AP_PROCEDUREProcedure;
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

[edk2] [PATCH 4/6] UefiCpuPkg/CpuDxe: Allow BSP to continue once max APs are awake

[Jordan Justen]

Since the CpuDxe has a fixed maximum number of APs that are controlled
with PcdCpuMaxLogicalProcessorNumber, if the maximum number of APs are
awake, then the BSP should be able to continue.

Since the APs may be updating the mMpSystemData.NumberOfProcessors
during this early init phase, we use SynchronizationLib to update this
variable on the AP and to read it on the BSP.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jordan Justen 
Cc: Jeff Fan 
Cc: Laszlo Ersek 
---
 UefiCpuPkg/CpuDxe/CpuMp.c | 15 ---
 UefiCpuPkg/CpuDxe/CpuMp.h |  2 +-
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c
index 6a22b9d..1094292 100644
--- a/UefiCpuPkg/CpuDxe/CpuMp.c
+++ b/UefiCpuPkg/CpuDxe/CpuMp.c
@@ -1474,7 +1474,7 @@ ApEntryPointInC (
 // Store the Stack address, when reset the AP, We can found the original 
address.
 //
 mMpSystemData.CpuDatas[mMpSystemData.NumberOfProcessors].TopOfStack = 
TopOfApStack;
-mMpSystemData.NumberOfProcessors++;
+InterlockedIncrement ();
 mMpSystemData.NumberOfEnabledProcessors++;
   } else {
 Status = WhoAmI (, );
@@ -1749,9 +1749,18 @@ InitializeMpSupport (
 StartApsStackless ();
 
 //
-// Wait for APs to arrive at the ApEntryPoint routine
+// Wait for all APs to start
 //
-MicroSecondDelay (PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds));
+Timeout = 0;
+do {
+  if (InterlockedCompareExchange32 (
+, 0, 0) >=
+  gMaxLogicalProcessorNumber) {
+break;
+  }
+  MicroSecondDelay (gPollInterval);
+  Timeout += gPollInterval;
+} while (Timeout <= PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds));
   }
 
   DEBUG ((DEBUG_INFO, "Detect CPU count: %d\n", 
mMpSystemData.NumberOfProcessors));
diff --git a/UefiCpuPkg/CpuDxe/CpuMp.h b/UefiCpuPkg/CpuDxe/CpuMp.h
index 503f3ae..ab7a7f5 100644
--- a/UefiCpuPkg/CpuDxe/CpuMp.h
+++ b/UefiCpuPkg/CpuDxe/CpuMp.h
@@ -114,7 +114,7 @@ typedef struct {
 **/
 typedef struct {
   CPU_DATA_BLOCK  *CpuDatas;
-  UINTN   NumberOfProcessors;
+  UINT32  NumberOfProcessors;
   UINTN   NumberOfEnabledProcessors;
 
   EFI_AP_PROCEDUREProcedure;
-- 
2.5.1

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel