[edk2-devel] [PATCH v4] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval

Wu, Hao A posted 1 patch 4 years ago
Failed in applying to current master (apply log)
UefiCpuPkg/UefiCpuPkg.dec                     |  6 ++++++
UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 20 ++++++++++----------
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       |  5 +++--
UefiCpuPkg/UefiCpuPkg.uni                     |  5 ++++-
4 files changed, 23 insertions(+), 13 deletions(-)
[edk2-devel] [PATCH v4] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval
Posted by Wu, Hao A 4 years ago
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2627

The commit will introduce a static PCD to specify the periodic interval
for checking the AP status when MP services StartupAllAPs() and
StartupThisAP() are being executed in a non-blocking manner. Or in other
words, specifies the interval for callback function CheckApsStatus().

The purpose is to provide the platform owners with the ability to choose
the proper interval value to trigger CheckApsStatus() according to:
A) The number of processors in the system;
B) How MP services (StartupAllAPs & StartupThisAP) being used.

Setting the PCD to a small value means the AP status check callback will
be trigerred more frequently, it can benefit the performance for the case
when the BSP uses WaitForEvent() or uses CheckEvent() in a loop to wait
for AP(s) to complete the task, especially when the task can be finished
considerably fast on AP(s).

An example is within function CpuFeaturesInitialize() under
UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c,
where BSP will perform the same task with APs and requires all the
processors to finish the task before BSP proceeds to its next task.

Setting the PCD to a big value, on the other hand, can reduce the impact
on BSP by the time being consumed in CheckApsStatus(), especially when the
number of processors is huge so that the time consumed in CheckApsStatus()
is not negligible.

The type of the PCD is UINT32, which means the maximum possible interval
value can be set to:
4,294,967,295 microseconds = 4,295 seconds = 71.58 minutes = 1.19 hours
which should be sufficient for usage.

For least impact, the default value of the new PCD will be the same with
the current interval value. It will be set to 100,000 microseconds, which
is 100 milliseconds.

Unitest done:
A) OS boot successfully;
B) Use debug message to confirm the 'TriggerTime' parameter for the
   'SetTimer' service is the same before & after this patch.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Brian J. Johnson <brian.johnson@hpe.com>
Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
---

Notes:
 V4
 Avoiding introducing a local variable in InitMpGlobalData().

 V3
 A) Use microseconds, instead of milliseconds as the interval unit;
 B) Use UINT32, instead of UINT64, for the PCD type;
 C) Address the bug that incorrect 'TriggerTime' parameter was passed into
    the 'SetTimer' service call in V2 patch

 V2
 Introduce a PCD to specify the AP status check interval.


 UefiCpuPkg/UefiCpuPkg.dec                     |  6 ++++++
 UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 20 ++++++++++----------
 UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       |  5 +++--
 UefiCpuPkg/UefiCpuPkg.uni                     |  5 ++++-
 4 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index e91dc68cbe..762badf5d2 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -230,6 +230,12 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
   # @Prompt This PCD is the nominal frequency of the core crystal clock in Hz as is CPUID Leaf 0x15:ECX
   gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency|24000000|UINT64|0x32132113
 
+  ## Specifies the periodic interval value in microseconds for the status check
+  #  of APs for StartupAllAPs() and StartupThisAP() executed in non-blocking
+  #  mode in DXE phase.
+  # @Prompt Periodic interval value in microseconds for AP status check in DXE.
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds|100000|UINT32|0x0000001E
+
 [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   ## Specifies max supported number of Logical Processors.
   # @Prompt Configure max supported number of Logical Processors
diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
index 45aaa179ff..a51a9ec1d2 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
@@ -61,13 +61,13 @@ [Guids]
   gEdkiiMicrocodePatchHobGuid                   ## SOMETIMES_CONSUMES  ## HOB
 
 [Pcd]
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber       ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds      ## SOMETIMES_CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                      ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress            ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ## SOMETIMES_CONSUMES
-  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                  ## CONSUMES
-
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber            ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber           ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds          ## SOMETIMES_CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                          ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress                ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize             ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                           ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                       ## SOMETIMES_CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds  ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                      ## CONSUMES
diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index a987c32109..56b776d3d8 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -15,7 +15,6 @@
 
 #include <Protocol/Timer.h>
 
-#define  AP_CHECK_INTERVAL     (EFI_TIMER_PERIOD_MILLISECONDS (100))
 #define  AP_SAFE_STACK_SIZE    128
 
 CPU_MP_DATA      *mCpuMpData = NULL;
@@ -451,7 +450,9 @@ InitMpGlobalData (
   Status = gBS->SetTimer (
                   mCheckAllApsEvent,
                   TimerPeriodic,
-                  AP_CHECK_INTERVAL
+                  EFI_TIMER_PERIOD_MICROSECONDS (
+                    PcdGet32 (PcdCpuApStatusCheckIntervalInMicroSeconds)
+                    )
                   );
   ASSERT_EFI_ERROR (Status);
 
diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
index c0d6ed5136..1780dfdc12 100644
--- a/UefiCpuPkg/UefiCpuPkg.uni
+++ b/UefiCpuPkg/UefiCpuPkg.uni
@@ -3,7 +3,7 @@
 //
 // This Package provides UEFI compatible CPU modules and libraries.
 //
-// Copyright (c) 2007 - 2015, Intel Corporation. All rights reserved.<BR>
+// Copyright (c) 2007 - 2020, Intel Corporation. All rights reserved.<BR>
 //
 // SPDX-License-Identifier: BSD-2-Clause-Patent
 //
@@ -275,3 +275,6 @@
 #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_PROMPT  #language en-US "Specify the count of pre allocated SMM MP tokens per chunk.\n"
 
 #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_HELP    #language en-US "This value used to specify the count of pre allocated SMM MP tokens per chunk.\n"
+
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckIntervalInMicroSeconds_PROMPT  #language en-US "Periodic interval value in microseconds for AP status check in DXE.\n"
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckIntervalInMicroSeconds_HELP    #language en-US "Periodic interval value in microseconds for the status check of APs for StartupAllAPs() and StartupThisAP() executed in non-blocking mode in DXE phase.\n"
-- 
2.12.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56250): https://edk2.groups.io/g/devel/message/56250
Mute This Topic: https://groups.io/mt/72535222/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v4] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval
Posted by Zeng, Star 4 years ago
Reviewed-by: Star Zeng <star.zeng@intel.com>

> -----Original Message-----
> From: Wu, Hao A
> Sent: Wednesday, March 25, 2020 1:07 PM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni,
> Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>; Brian J .
> Johnson <brian.johnson@hpe.com>
> Subject: [PATCH v4] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status
> check interval
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2627
> 
> The commit will introduce a static PCD to specify the periodic interval for
> checking the AP status when MP services StartupAllAPs() and
> StartupThisAP() are being executed in a non-blocking manner. Or in other
> words, specifies the interval for callback function CheckApsStatus().
> 
> The purpose is to provide the platform owners with the ability to choose the
> proper interval value to trigger CheckApsStatus() according to:
> A) The number of processors in the system;
> B) How MP services (StartupAllAPs & StartupThisAP) being used.
> 
> Setting the PCD to a small value means the AP status check callback will be
> trigerred more frequently, it can benefit the performance for the case when
> the BSP uses WaitForEvent() or uses CheckEvent() in a loop to wait for AP(s)
> to complete the task, especially when the task can be finished considerably
> fast on AP(s).
> 
> An example is within function CpuFeaturesInitialize() under
> UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c,
> where BSP will perform the same task with APs and requires all the
> processors to finish the task before BSP proceeds to its next task.
> 
> Setting the PCD to a big value, on the other hand, can reduce the impact on
> BSP by the time being consumed in CheckApsStatus(), especially when the
> number of processors is huge so that the time consumed in CheckApsStatus()
> is not negligible.
> 
> The type of the PCD is UINT32, which means the maximum possible interval
> value can be set to:
> 4,294,967,295 microseconds = 4,295 seconds = 71.58 minutes = 1.19 hours
> which should be sufficient for usage.
> 
> For least impact, the default value of the new PCD will be the same with the
> current interval value. It will be set to 100,000 microseconds, which is 100
> milliseconds.
> 
> Unitest done:
> A) OS boot successfully;
> B) Use debug message to confirm the 'TriggerTime' parameter for the
>    'SetTimer' service is the same before & after this patch.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Brian J. Johnson <brian.johnson@hpe.com>
> Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> ---
> 
> Notes:
>  V4
>  Avoiding introducing a local variable in InitMpGlobalData().
> 
>  V3
>  A) Use microseconds, instead of milliseconds as the interval unit;
>  B) Use UINT32, instead of UINT64, for the PCD type;
>  C) Address the bug that incorrect 'TriggerTime' parameter was passed into
>     the 'SetTimer' service call in V2 patch
> 
>  V2
>  Introduce a PCD to specify the AP status check interval.
> 
> 
>  UefiCpuPkg/UefiCpuPkg.dec                     |  6 ++++++
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 20 ++++++++++----------
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       |  5 +++--
>  UefiCpuPkg/UefiCpuPkg.uni                     |  5 ++++-
>  4 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec index
> e91dc68cbe..762badf5d2 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -230,6 +230,12 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>    # @Prompt This PCD is the nominal frequency of the core crystal clock in Hz
> as is CPUID Leaf 0x15:ECX
> 
> gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency|24000000|
> UINT64|0x32132113
> 
> +  ## Specifies the periodic interval value in microseconds for the
> + status check  #  of APs for StartupAllAPs() and StartupThisAP()
> + executed in non-blocking  #  mode in DXE phase.
> +  # @Prompt Periodic interval value in microseconds for AP status check in
> DXE.
> +
> +
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds|
> 10
> + 0000|UINT32|0x0000001E
> +
>  [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>    ## Specifies max supported number of Logical Processors.
>    # @Prompt Configure max supported number of Logical Processors diff --git
> a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> index 45aaa179ff..a51a9ec1d2 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -61,13 +61,13 @@ [Guids]
>    gEdkiiMicrocodePatchHobGuid                   ## SOMETIMES_CONSUMES  ##
> HOB
> 
>  [Pcd]
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ##
> CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber       ##
> CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds      ##
> SOMETIMES_CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                      ##
> CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress            ##
> CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ##
> CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ##
> CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ##
> SOMETIMES_CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                  ##
> CONSUMES
> -
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
> ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber
> ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds
> ## SOMETIMES_CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                          ##
> CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress                ##
> CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize             ##
> CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                           ##
> CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                       ##
> SOMETIMES_CONSUMES
> +
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds
> ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                      ##
> CONSUMES
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index a987c32109..56b776d3d8 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -15,7 +15,6 @@
> 
>  #include <Protocol/Timer.h>
> 
> -#define  AP_CHECK_INTERVAL     (EFI_TIMER_PERIOD_MILLISECONDS (100))
>  #define  AP_SAFE_STACK_SIZE    128
> 
>  CPU_MP_DATA      *mCpuMpData = NULL;
> @@ -451,7 +450,9 @@ InitMpGlobalData (
>    Status = gBS->SetTimer (
>                    mCheckAllApsEvent,
>                    TimerPeriodic,
> -                  AP_CHECK_INTERVAL
> +                  EFI_TIMER_PERIOD_MICROSECONDS (
> +                    PcdGet32 (PcdCpuApStatusCheckIntervalInMicroSeconds)
> +                    )
>                    );
>    ASSERT_EFI_ERROR (Status);
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni index
> c0d6ed5136..1780dfdc12 100644
> --- a/UefiCpuPkg/UefiCpuPkg.uni
> +++ b/UefiCpuPkg/UefiCpuPkg.uni
> @@ -3,7 +3,7 @@
>  //
>  // This Package provides UEFI compatible CPU modules and libraries.
>  //
> -// Copyright (c) 2007 - 2015, Intel Corporation. All rights reserved.<BR>
> +// Copyright (c) 2007 - 2020, Intel Corporation. All rights
> +reserved.<BR>
>  //
>  // SPDX-License-Identifier: BSD-2-Clause-Patent  // @@ -275,3 +275,6 @@
> #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_PR
> OMPT  #language en-US "Specify the count of pre allocated SMM MP tokens
> per chunk.\n"
> 
>  #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_HE
> LP    #language en-US "This value used to specify the count of pre allocated
> SMM MP tokens per chunk.\n"
> +
> +#string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckIntervalInMicroSec
> onds_PROMPT  #language en-US "Periodic interval value in microseconds for
> AP status check in DXE.\n"
> +#string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckIntervalInMicroSec
> onds_HELP    #language en-US "Periodic interval value in microseconds for
> the status check of APs for StartupAllAPs() and StartupThisAP() executed in
> non-blocking mode in DXE phase.\n"
> --
> 2.12.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56253): https://edk2.groups.io/g/devel/message/56253
Mute This Topic: https://groups.io/mt/72535222/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v4] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval
Posted by Ni, Ray 4 years ago
Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Wednesday, March 25, 2020 1:07 PM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Zeng, Star <star.zeng@intel.com>; Brian J .
> Johnson <brian.johnson@hpe.com>
> Subject: [PATCH v4] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2627
> 
> The commit will introduce a static PCD to specify the periodic interval
> for checking the AP status when MP services StartupAllAPs() and
> StartupThisAP() are being executed in a non-blocking manner. Or in other
> words, specifies the interval for callback function CheckApsStatus().
> 
> The purpose is to provide the platform owners with the ability to choose
> the proper interval value to trigger CheckApsStatus() according to:
> A) The number of processors in the system;
> B) How MP services (StartupAllAPs & StartupThisAP) being used.
> 
> Setting the PCD to a small value means the AP status check callback will
> be trigerred more frequently, it can benefit the performance for the case
> when the BSP uses WaitForEvent() or uses CheckEvent() in a loop to wait
> for AP(s) to complete the task, especially when the task can be finished
> considerably fast on AP(s).
> 
> An example is within function CpuFeaturesInitialize() under
> UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c,
> where BSP will perform the same task with APs and requires all the
> processors to finish the task before BSP proceeds to its next task.
> 
> Setting the PCD to a big value, on the other hand, can reduce the impact
> on BSP by the time being consumed in CheckApsStatus(), especially when the
> number of processors is huge so that the time consumed in CheckApsStatus()
> is not negligible.
> 
> The type of the PCD is UINT32, which means the maximum possible interval
> value can be set to:
> 4,294,967,295 microseconds = 4,295 seconds = 71.58 minutes = 1.19 hours
> which should be sufficient for usage.
> 
> For least impact, the default value of the new PCD will be the same with
> the current interval value. It will be set to 100,000 microseconds, which
> is 100 milliseconds.
> 
> Unitest done:
> A) OS boot successfully;
> B) Use debug message to confirm the 'TriggerTime' parameter for the
>    'SetTimer' service is the same before & after this patch.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Brian J. Johnson <brian.johnson@hpe.com>
> Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> ---
> 
> Notes:
>  V4
>  Avoiding introducing a local variable in InitMpGlobalData().
> 
>  V3
>  A) Use microseconds, instead of milliseconds as the interval unit;
>  B) Use UINT32, instead of UINT64, for the PCD type;
>  C) Address the bug that incorrect 'TriggerTime' parameter was passed into
>     the 'SetTimer' service call in V2 patch
> 
>  V2
>  Introduce a PCD to specify the AP status check interval.
> 
> 
>  UefiCpuPkg/UefiCpuPkg.dec                     |  6 ++++++
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 20 ++++++++++----------
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       |  5 +++--
>  UefiCpuPkg/UefiCpuPkg.uni                     |  5 ++++-
>  4 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index e91dc68cbe..762badf5d2 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -230,6 +230,12 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>    # @Prompt This PCD is the nominal frequency of the core crystal clock in Hz as is CPUID Leaf 0x15:ECX
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency|24000000|UINT64|0x32132113
> 
> +  ## Specifies the periodic interval value in microseconds for the status check
> +  #  of APs for StartupAllAPs() and StartupThisAP() executed in non-blocking
> +  #  mode in DXE phase.
> +  # @Prompt Periodic interval value in microseconds for AP status check in DXE.
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds|100000|UINT32|0x0000001E
> +
>  [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>    ## Specifies max supported number of Logical Processors.
>    # @Prompt Configure max supported number of Logical Processors
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> index 45aaa179ff..a51a9ec1d2 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -61,13 +61,13 @@ [Guids]
>    gEdkiiMicrocodePatchHobGuid                   ## SOMETIMES_CONSUMES  ## HOB
> 
>  [Pcd]
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber       ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds      ## SOMETIMES_CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                      ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress            ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ## SOMETIMES_CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                  ## CONSUMES
> -
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber            ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber           ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds          ## SOMETIMES_CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                          ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress                ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize             ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                           ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                       ## SOMETIMES_CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds  ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                      ## CONSUMES
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index a987c32109..56b776d3d8 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -15,7 +15,6 @@
> 
>  #include <Protocol/Timer.h>
> 
> -#define  AP_CHECK_INTERVAL     (EFI_TIMER_PERIOD_MILLISECONDS (100))
>  #define  AP_SAFE_STACK_SIZE    128
> 
>  CPU_MP_DATA      *mCpuMpData = NULL;
> @@ -451,7 +450,9 @@ InitMpGlobalData (
>    Status = gBS->SetTimer (
>                    mCheckAllApsEvent,
>                    TimerPeriodic,
> -                  AP_CHECK_INTERVAL
> +                  EFI_TIMER_PERIOD_MICROSECONDS (
> +                    PcdGet32 (PcdCpuApStatusCheckIntervalInMicroSeconds)
> +                    )
>                    );
>    ASSERT_EFI_ERROR (Status);
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
> index c0d6ed5136..1780dfdc12 100644
> --- a/UefiCpuPkg/UefiCpuPkg.uni
> +++ b/UefiCpuPkg/UefiCpuPkg.uni
> @@ -3,7 +3,7 @@
>  //
>  // This Package provides UEFI compatible CPU modules and libraries.
>  //
> -// Copyright (c) 2007 - 2015, Intel Corporation. All rights reserved.<BR>
> +// Copyright (c) 2007 - 2020, Intel Corporation. All rights reserved.<BR>
>  //
>  // SPDX-License-Identifier: BSD-2-Clause-Patent
>  //
> @@ -275,3 +275,6 @@
>  #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_PROMPT  #language en-US "Specify the
> count of pre allocated SMM MP tokens per chunk.\n"
> 
>  #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_HELP    #language en-US "This value used
> to specify the count of pre allocated SMM MP tokens per chunk.\n"
> +
> +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckIntervalInMicroSeconds_PROMPT  #language en-US
> "Periodic interval value in microseconds for AP status check in DXE.\n"
> +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckIntervalInMicroSeconds_HELP    #language en-US
> "Periodic interval value in microseconds for the status check of APs for StartupAllAPs() and StartupThisAP() executed in
> non-blocking mode in DXE phase.\n"
> --
> 2.12.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56254): https://edk2.groups.io/g/devel/message/56254
Mute This Topic: https://groups.io/mt/72535222/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v4] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval
Posted by Laszlo Ersek 4 years ago
On 03/25/20 06:07, Wu, Hao A wrote:
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2627
> 
> The commit will introduce a static PCD to specify the periodic interval
> for checking the AP status when MP services StartupAllAPs() and
> StartupThisAP() are being executed in a non-blocking manner. Or in other
> words, specifies the interval for callback function CheckApsStatus().
> 
> The purpose is to provide the platform owners with the ability to choose
> the proper interval value to trigger CheckApsStatus() according to:
> A) The number of processors in the system;
> B) How MP services (StartupAllAPs & StartupThisAP) being used.
> 
> Setting the PCD to a small value means the AP status check callback will
> be trigerred more frequently, it can benefit the performance for the case
> when the BSP uses WaitForEvent() or uses CheckEvent() in a loop to wait
> for AP(s) to complete the task, especially when the task can be finished
> considerably fast on AP(s).
> 
> An example is within function CpuFeaturesInitialize() under
> UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c,
> where BSP will perform the same task with APs and requires all the
> processors to finish the task before BSP proceeds to its next task.
> 
> Setting the PCD to a big value, on the other hand, can reduce the impact
> on BSP by the time being consumed in CheckApsStatus(), especially when the
> number of processors is huge so that the time consumed in CheckApsStatus()
> is not negligible.
> 
> The type of the PCD is UINT32, which means the maximum possible interval
> value can be set to:
> 4,294,967,295 microseconds = 4,295 seconds = 71.58 minutes = 1.19 hours
> which should be sufficient for usage.
> 
> For least impact, the default value of the new PCD will be the same with
> the current interval value. It will be set to 100,000 microseconds, which
> is 100 milliseconds.
> 
> Unitest done:
> A) OS boot successfully;
> B) Use debug message to confirm the 'TriggerTime' parameter for the
>    'SetTimer' service is the same before & after this patch.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Brian J. Johnson <brian.johnson@hpe.com>
> Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> ---
> 
> Notes:
>  V4
>  Avoiding introducing a local variable in InitMpGlobalData().
> 
>  V3
>  A) Use microseconds, instead of milliseconds as the interval unit;
>  B) Use UINT32, instead of UINT64, for the PCD type;
>  C) Address the bug that incorrect 'TriggerTime' parameter was passed into
>     the 'SetTimer' service call in V2 patch
> 
>  V2
>  Introduce a PCD to specify the AP status check interval.
> 
> 
>  UefiCpuPkg/UefiCpuPkg.dec                     |  6 ++++++
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 20 ++++++++++----------
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       |  5 +++--
>  UefiCpuPkg/UefiCpuPkg.uni                     |  5 ++++-
>  4 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index e91dc68cbe..762badf5d2 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -230,6 +230,12 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>    # @Prompt This PCD is the nominal frequency of the core crystal clock in Hz as is CPUID Leaf 0x15:ECX
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency|24000000|UINT64|0x32132113
>  
> +  ## Specifies the periodic interval value in microseconds for the status check
> +  #  of APs for StartupAllAPs() and StartupThisAP() executed in non-blocking
> +  #  mode in DXE phase.
> +  # @Prompt Periodic interval value in microseconds for AP status check in DXE.
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds|100000|UINT32|0x0000001E
> +
>  [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>    ## Specifies max supported number of Logical Processors.
>    # @Prompt Configure max supported number of Logical Processors
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> index 45aaa179ff..a51a9ec1d2 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -61,13 +61,13 @@ [Guids]
>    gEdkiiMicrocodePatchHobGuid                   ## SOMETIMES_CONSUMES  ## HOB
>  
>  [Pcd]
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber       ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds      ## SOMETIMES_CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                      ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress            ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ## SOMETIMES_CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                  ## CONSUMES
> -
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber            ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber           ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds          ## SOMETIMES_CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                          ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress                ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize             ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                           ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                       ## SOMETIMES_CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds  ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                      ## CONSUMES
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index a987c32109..56b776d3d8 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -15,7 +15,6 @@
>  
>  #include <Protocol/Timer.h>
>  
> -#define  AP_CHECK_INTERVAL     (EFI_TIMER_PERIOD_MILLISECONDS (100))
>  #define  AP_SAFE_STACK_SIZE    128
>  
>  CPU_MP_DATA      *mCpuMpData = NULL;
> @@ -451,7 +450,9 @@ InitMpGlobalData (
>    Status = gBS->SetTimer (
>                    mCheckAllApsEvent,
>                    TimerPeriodic,
> -                  AP_CHECK_INTERVAL
> +                  EFI_TIMER_PERIOD_MICROSECONDS (
> +                    PcdGet32 (PcdCpuApStatusCheckIntervalInMicroSeconds)
> +                    )
>                    );
>    ASSERT_EFI_ERROR (Status);
>  
> diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
> index c0d6ed5136..1780dfdc12 100644
> --- a/UefiCpuPkg/UefiCpuPkg.uni
> +++ b/UefiCpuPkg/UefiCpuPkg.uni
> @@ -3,7 +3,7 @@
>  //
>  // This Package provides UEFI compatible CPU modules and libraries.
>  //
> -// Copyright (c) 2007 - 2015, Intel Corporation. All rights reserved.<BR>
> +// Copyright (c) 2007 - 2020, Intel Corporation. All rights reserved.<BR>
>  //
>  // SPDX-License-Identifier: BSD-2-Clause-Patent
>  //
> @@ -275,3 +275,6 @@
>  #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_PROMPT  #language en-US "Specify the count of pre allocated SMM MP tokens per chunk.\n"
>  
>  #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_HELP    #language en-US "This value used to specify the count of pre allocated SMM MP tokens per chunk.\n"
> +
> +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckIntervalInMicroSeconds_PROMPT  #language en-US "Periodic interval value in microseconds for AP status check in DXE.\n"
> +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckIntervalInMicroSeconds_HELP    #language en-US "Periodic interval value in microseconds for the status check of APs for StartupAllAPs() and StartupThisAP() executed in non-blocking mode in DXE phase.\n"
> 

For this patch:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

I'm happy if this patch is pushed, as-is.

Independently, I have found that MpInitLib, despite using several PCDs,
does not list the PcdLib class in the [LibraryClasses] section, and
never #include's <Library/PcdLib.h>  either.

(The [LibraryClasses] observation applies to both INF files, that is,
PEI and DXE alike.)

Things happen to work in practice because both the #include and the lib
class dependency must be inherited from other headers / lib instances.
But it would be prudent to spell out the PcdLib dependency, especially
because some of the subject PCDs are used with the dynamic access
method, in practice (for example by OvmfPkg) -- meaning that the actual
PCD library APIs are exercised.

Hao, can you post a followup patch for that, or do you prefer that we
only enter a BZ ticket at the moment?

Thanks!
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56339): https://edk2.groups.io/g/devel/message/56339
Mute This Topic: https://groups.io/mt/72535222/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v4] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval
Posted by Wu, Hao A 4 years ago
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, March 26, 2020 3:24 AM
> To: devel@edk2.groups.io; Wu, Hao A
> Cc: Dong, Eric; Ni, Ray; Kinney, Michael D; Zeng, Star; Brian J . Johnson
> Subject: Re: [edk2-devel] [PATCH v4] UefiCpuPkg/MpInitLib DXE: Add PCD to
> control AP status check interval
> 
> On 03/25/20 06:07, Wu, Hao A wrote:
> > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2627
> >
> > The commit will introduce a static PCD to specify the periodic interval
> > for checking the AP status when MP services StartupAllAPs() and
> > StartupThisAP() are being executed in a non-blocking manner. Or in other
> > words, specifies the interval for callback function CheckApsStatus().
> >
> > The purpose is to provide the platform owners with the ability to choose
> > the proper interval value to trigger CheckApsStatus() according to:
> > A) The number of processors in the system;
> > B) How MP services (StartupAllAPs & StartupThisAP) being used.
> >
> > Setting the PCD to a small value means the AP status check callback will
> > be trigerred more frequently, it can benefit the performance for the case
> > when the BSP uses WaitForEvent() or uses CheckEvent() in a loop to wait
> > for AP(s) to complete the task, especially when the task can be finished
> > considerably fast on AP(s).
> >
> > An example is within function CpuFeaturesInitialize() under
> > UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c,
> > where BSP will perform the same task with APs and requires all the
> > processors to finish the task before BSP proceeds to its next task.
> >
> > Setting the PCD to a big value, on the other hand, can reduce the impact
> > on BSP by the time being consumed in CheckApsStatus(), especially when the
> > number of processors is huge so that the time consumed in CheckApsStatus()
> > is not negligible.
> >
> > The type of the PCD is UINT32, which means the maximum possible interval
> > value can be set to:
> > 4,294,967,295 microseconds = 4,295 seconds = 71.58 minutes = 1.19 hours
> > which should be sufficient for usage.
> >
> > For least impact, the default value of the new PCD will be the same with
> > the current interval value. It will be set to 100,000 microseconds, which
> > is 100 milliseconds.
> >
> > Unitest done:
> > A) OS boot successfully;
> > B) Use debug message to confirm the 'TriggerTime' parameter for the
> >    'SetTimer' service is the same before & after this patch.
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Brian J. Johnson <brian.johnson@hpe.com>
> > Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> > ---
> >
> > Notes:
> >  V4
> >  Avoiding introducing a local variable in InitMpGlobalData().
> >
> >  V3
> >  A) Use microseconds, instead of milliseconds as the interval unit;
> >  B) Use UINT32, instead of UINT64, for the PCD type;
> >  C) Address the bug that incorrect 'TriggerTime' parameter was passed into
> >     the 'SetTimer' service call in V2 patch
> >
> >  V2
> >  Introduce a PCD to specify the AP status check interval.
> >
> >
> >  UefiCpuPkg/UefiCpuPkg.dec                     |  6 ++++++
> >  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 20 ++++++++++----------
> >  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       |  5 +++--
> >  UefiCpuPkg/UefiCpuPkg.uni                     |  5 ++++-
> >  4 files changed, 23 insertions(+), 13 deletions(-)
> >
> > diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> > index e91dc68cbe..762badf5d2 100644
> > --- a/UefiCpuPkg/UefiCpuPkg.dec
> > +++ b/UefiCpuPkg/UefiCpuPkg.dec
> > @@ -230,6 +230,12 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
> >    # @Prompt This PCD is the nominal frequency of the core crystal clock in Hz
> as is CPUID Leaf 0x15:ECX
> >
> gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency|24000000|U
> INT64|0x32132113
> >
> > +  ## Specifies the periodic interval value in microseconds for the status check
> > +  #  of APs for StartupAllAPs() and StartupThisAP() executed in non-blocking
> > +  #  mode in DXE phase.
> > +  # @Prompt Periodic interval value in microseconds for AP status check in
> DXE.
> > +
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds|1
> 00000|UINT32|0x0000001E
> > +
> >  [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
> >    ## Specifies max supported number of Logical Processors.
> >    # @Prompt Configure max supported number of Logical Processors
> > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > index 45aaa179ff..a51a9ec1d2 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > @@ -61,13 +61,13 @@ [Guids]
> >    gEdkiiMicrocodePatchHobGuid                   ## SOMETIMES_CONSUMES  ##
> HOB
> >
> >  [Pcd]
> > -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ##
> CONSUMES
> > -  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber       ##
> CONSUMES
> > -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds      ##
> SOMETIMES_CONSUMES
> > -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                      ## CONSUMES
> > -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress            ##
> CONSUMES
> > -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ##
> CONSUMES
> > -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ##
> CONSUMES
> > -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ##
> SOMETIMES_CONSUMES
> > -  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                  ##
> CONSUMES
> > -
> > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber            ##
> CONSUMES
> > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber           ##
> CONSUMES
> > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds
> ## SOMETIMES_CONSUMES
> > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                          ##
> CONSUMES
> > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress                ##
> CONSUMES
> > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize             ##
> CONSUMES
> > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                           ##
> CONSUMES
> > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                       ##
> SOMETIMES_CONSUMES
> > +
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds
> ## CONSUMES
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                      ##
> CONSUMES
> > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > index a987c32109..56b776d3d8 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > @@ -15,7 +15,6 @@
> >
> >  #include <Protocol/Timer.h>
> >
> > -#define  AP_CHECK_INTERVAL     (EFI_TIMER_PERIOD_MILLISECONDS (100))
> >  #define  AP_SAFE_STACK_SIZE    128
> >
> >  CPU_MP_DATA      *mCpuMpData = NULL;
> > @@ -451,7 +450,9 @@ InitMpGlobalData (
> >    Status = gBS->SetTimer (
> >                    mCheckAllApsEvent,
> >                    TimerPeriodic,
> > -                  AP_CHECK_INTERVAL
> > +                  EFI_TIMER_PERIOD_MICROSECONDS (
> > +                    PcdGet32 (PcdCpuApStatusCheckIntervalInMicroSeconds)
> > +                    )
> >                    );
> >    ASSERT_EFI_ERROR (Status);
> >
> > diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
> > index c0d6ed5136..1780dfdc12 100644
> > --- a/UefiCpuPkg/UefiCpuPkg.uni
> > +++ b/UefiCpuPkg/UefiCpuPkg.uni
> > @@ -3,7 +3,7 @@
> >  //
> >  // This Package provides UEFI compatible CPU modules and libraries.
> >  //
> > -// Copyright (c) 2007 - 2015, Intel Corporation. All rights reserved.<BR>
> > +// Copyright (c) 2007 - 2020, Intel Corporation. All rights reserved.<BR>
> >  //
> >  // SPDX-License-Identifier: BSD-2-Clause-Patent
> >  //
> > @@ -275,3 +275,6 @@
> >  #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_PRO
> MPT  #language en-US "Specify the count of pre allocated SMM MP tokens per
> chunk.\n"
> >
> >  #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_HEL
> P    #language en-US "This value used to specify the count of pre allocated SMM
> MP tokens per chunk.\n"
> > +
> > +#string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckIntervalInMicroSecon
> ds_PROMPT  #language en-US "Periodic interval value in microseconds for AP
> status check in DXE.\n"
> > +#string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckIntervalInMicroSecon
> ds_HELP    #language en-US "Periodic interval value in microseconds for the
> status check of APs for StartupAllAPs() and StartupThisAP() executed in non-
> blocking mode in DXE phase.\n"
> >
> 
> For this patch:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> I'm happy if this patch is pushed, as-is.


Thanks for the review effort.


> 
> Independently, I have found that MpInitLib, despite using several PCDs,
> does not list the PcdLib class in the [LibraryClasses] section, and
> never #include's <Library/PcdLib.h>  either.
> 
> (The [LibraryClasses] observation applies to both INF files, that is,
> PEI and DXE alike.)
> 
> Things happen to work in practice because both the #include and the lib
> class dependency must be inherited from other headers / lib instances.
> But it would be prudent to spell out the PcdLib dependency, especially
> because some of the subject PCDs are used with the dynamic access
> method, in practice (for example by OvmfPkg) -- meaning that the actual
> PCD library APIs are exercised.
> 
> Hao, can you post a followup patch for that, or do you prefer that we
> only enter a BZ ticket at the moment?


I have filed a BZ tracker for the issue you mentioned:
https://bugzilla.tianocore.org/show_bug.cgi?id=2632

Let us address this in a separate series.

Best Regards,
Hao Wu


> 
> Thanks!
> Laszlo
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56349): https://edk2.groups.io/g/devel/message/56349
Mute This Topic: https://groups.io/mt/72535222/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v4] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval
Posted by Wu, Hao A 4 years ago
> -----Original Message-----
> From: Wu, Hao A
> Sent: Wednesday, March 25, 2020 1:07 PM
> To: devel@edk2.groups.io
> Cc: Wu, Hao A; Dong, Eric; Ni, Ray; Laszlo Ersek; Kinney, Michael D; Zeng, Star;
> Brian J . Johnson
> Subject: [PATCH v4] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status
> check interval
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2627


Thanks all.
Patch pushed via commit a1c35ff312.

Best Regards,
Hao Wu


> 
> The commit will introduce a static PCD to specify the periodic interval
> for checking the AP status when MP services StartupAllAPs() and
> StartupThisAP() are being executed in a non-blocking manner. Or in other
> words, specifies the interval for callback function CheckApsStatus().
> 
> The purpose is to provide the platform owners with the ability to choose
> the proper interval value to trigger CheckApsStatus() according to:
> A) The number of processors in the system;
> B) How MP services (StartupAllAPs & StartupThisAP) being used.
> 
> Setting the PCD to a small value means the AP status check callback will
> be trigerred more frequently, it can benefit the performance for the case
> when the BSP uses WaitForEvent() or uses CheckEvent() in a loop to wait
> for AP(s) to complete the task, especially when the task can be finished
> considerably fast on AP(s).
> 
> An example is within function CpuFeaturesInitialize() under
> UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c,
> where BSP will perform the same task with APs and requires all the
> processors to finish the task before BSP proceeds to its next task.
> 
> Setting the PCD to a big value, on the other hand, can reduce the impact
> on BSP by the time being consumed in CheckApsStatus(), especially when the
> number of processors is huge so that the time consumed in CheckApsStatus()
> is not negligible.
> 
> The type of the PCD is UINT32, which means the maximum possible interval
> value can be set to:
> 4,294,967,295 microseconds = 4,295 seconds = 71.58 minutes = 1.19 hours
> which should be sufficient for usage.
> 
> For least impact, the default value of the new PCD will be the same with
> the current interval value. It will be set to 100,000 microseconds, which
> is 100 milliseconds.
> 
> Unitest done:
> A) OS boot successfully;
> B) Use debug message to confirm the 'TriggerTime' parameter for the
>    'SetTimer' service is the same before & after this patch.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Brian J. Johnson <brian.johnson@hpe.com>
> Signed-off-by: Hao A Wu <hao.a.wu@intel.com>
> ---
> 
> Notes:
>  V4
>  Avoiding introducing a local variable in InitMpGlobalData().
> 
>  V3
>  A) Use microseconds, instead of milliseconds as the interval unit;
>  B) Use UINT32, instead of UINT64, for the PCD type;
>  C) Address the bug that incorrect 'TriggerTime' parameter was passed into
>     the 'SetTimer' service call in V2 patch
> 
>  V2
>  Introduce a PCD to specify the AP status check interval.
> 
> 
>  UefiCpuPkg/UefiCpuPkg.dec                     |  6 ++++++
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 20 ++++++++++----------
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       |  5 +++--
>  UefiCpuPkg/UefiCpuPkg.uni                     |  5 ++++-
>  4 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index e91dc68cbe..762badf5d2 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -230,6 +230,12 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
>    # @Prompt This PCD is the nominal frequency of the core crystal clock in Hz as
> is CPUID Leaf 0x15:ECX
> 
> gUefiCpuPkgTokenSpaceGuid.PcdCpuCoreCrystalClockFrequency|24000000|U
> INT64|0x32132113
> 
> +  ## Specifies the periodic interval value in microseconds for the status check
> +  #  of APs for StartupAllAPs() and StartupThisAP() executed in non-blocking
> +  #  mode in DXE phase.
> +  # @Prompt Periodic interval value in microseconds for AP status check in DXE.
> +
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds|1
> 00000|UINT32|0x0000001E
> +
>  [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>    ## Specifies max supported number of Logical Processors.
>    # @Prompt Configure max supported number of Logical Processors
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> index 45aaa179ff..a51a9ec1d2 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -61,13 +61,13 @@ [Guids]
>    gEdkiiMicrocodePatchHobGuid                   ## SOMETIMES_CONSUMES  ## HOB
> 
>  [Pcd]
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ##
> CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber       ##
> CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds      ##
> SOMETIMES_CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                      ## CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress            ##
> CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ##
> CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ##
> CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ##
> SOMETIMES_CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                  ##
> CONSUMES
> -
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber            ##
> CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber           ##
> CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds          ##
> SOMETIMES_CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                          ##
> CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress                ##
> CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize             ##
> CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                           ##
> CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                       ##
> SOMETIMES_CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroSeconds
> ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                      ##
> CONSUMES
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index a987c32109..56b776d3d8 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -15,7 +15,6 @@
> 
>  #include <Protocol/Timer.h>
> 
> -#define  AP_CHECK_INTERVAL     (EFI_TIMER_PERIOD_MILLISECONDS (100))
>  #define  AP_SAFE_STACK_SIZE    128
> 
>  CPU_MP_DATA      *mCpuMpData = NULL;
> @@ -451,7 +450,9 @@ InitMpGlobalData (
>    Status = gBS->SetTimer (
>                    mCheckAllApsEvent,
>                    TimerPeriodic,
> -                  AP_CHECK_INTERVAL
> +                  EFI_TIMER_PERIOD_MICROSECONDS (
> +                    PcdGet32 (PcdCpuApStatusCheckIntervalInMicroSeconds)
> +                    )
>                    );
>    ASSERT_EFI_ERROR (Status);
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
> index c0d6ed5136..1780dfdc12 100644
> --- a/UefiCpuPkg/UefiCpuPkg.uni
> +++ b/UefiCpuPkg/UefiCpuPkg.uni
> @@ -3,7 +3,7 @@
>  //
>  // This Package provides UEFI compatible CPU modules and libraries.
>  //
> -// Copyright (c) 2007 - 2015, Intel Corporation. All rights reserved.<BR>
> +// Copyright (c) 2007 - 2020, Intel Corporation. All rights reserved.<BR>
>  //
>  // SPDX-License-Identifier: BSD-2-Clause-Patent
>  //
> @@ -275,3 +275,6 @@
>  #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_PRO
> MPT  #language en-US "Specify the count of pre allocated SMM MP tokens per
> chunk.\n"
> 
>  #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerChunk_HEL
> P    #language en-US "This value used to specify the count of pre allocated SMM
> MP tokens per chunk.\n"
> +
> +#string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckIntervalInMicroSecon
> ds_PROMPT  #language en-US "Periodic interval value in microseconds for AP
> status check in DXE.\n"
> +#string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckIntervalInMicroSecon
> ds_HELP    #language en-US "Periodic interval value in microseconds for the
> status check of APs for StartupAllAPs() and StartupThisAP() executed in non-
> blocking mode in DXE phase.\n"
> --
> 2.12.0.windows.1


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#56432): https://edk2.groups.io/g/devel/message/56432
Mute This Topic: https://groups.io/mt/72535222/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-