[PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval

[PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval

[Wu, Hao A]

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.

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

Unitest done:
OS boot successfully.

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>
---
 UefiCpuPkg/UefiCpuPkg.dec                     | 6 ++++++
 UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 2 +-
 UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       | 3 +--
 UefiCpuPkg/UefiCpuPkg.uni                     | 5 ++++-
 4 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index e91dc68cbe..06a3d52060 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 milliseconds for the status check
+  #  of APs for StartupAllAPs() and StartupThisAP() executed in non-blocking
+  #  mode in DXE phase.
+  # @Prompt Periodic interval value in milliseconds for AP status check in DXE.
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval|100|UINT64|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..05e004dc3c 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
@@ -69,5 +69,5 @@ [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ## SOMETIMES_CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval            ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                  ## CONSUMES
-
diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index a987c32109..683547dc99 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,7 @@ InitMpGlobalData (
   Status = gBS->SetTimer (
                   mCheckAllApsEvent,
                   TimerPeriodic,
-                  AP_CHECK_INTERVAL
+                  PcdGet64 (PcdCpuApStatusCheckInterval)
                   );
   ASSERT_EFI_ERROR (Status);
 
diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
index c0d6ed5136..08d9b45f76 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_PcdCpuApStatusCheckInterval_PROMPT  #language en-US "Periodic interval value in milliseconds for AP status check in DXE.\n"
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckInterval_HELP    #language en-US "Periodic interval value in milliseconds for the status check of APs for StartupAllAPs() and StartupThisAP() executed in non-blocking mode in DXE phase.\n"
-- 
2.12.0.windows.1

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

[Zeng, Star]

The code logic is good to me.
Only minor concern, do we really need the PCD to be UINT64 type? :)

Thanks,
Star

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Wu, Hao A
> Sent: Tuesday, March 24, 2020 2:33 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: [edk2-devel] [PATCH v2] 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.
> 
> For least impact, the default value of the new PCD will be the same with the
> current interval value, which is 100 milliseconds.
> 
> Unitest done:
> OS boot successfully.
> 
> 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>
> ---
>  UefiCpuPkg/UefiCpuPkg.dec                     | 6 ++++++
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 2 +-
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       | 3 +--
>  UefiCpuPkg/UefiCpuPkg.uni                     | 5 ++++-
>  4 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec index
> e91dc68cbe..06a3d52060 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 milliseconds for the
> + status check  #  of APs for StartupAllAPs() and StartupThisAP()
> + executed in non-blocking  #  mode in DXE phase.
> +  # @Prompt Periodic interval value in milliseconds for AP status check in
> DXE.
> +
> +
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval|100|UINT64|0x
> 000
> + 0001E
> +
>  [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..05e004dc3c 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -69,5 +69,5 @@ [Pcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ##
> CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ##
> CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ##
> SOMETIMES_CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval            ##
> CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                  ##
> CONSUMES
> -
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index a987c32109..683547dc99 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,7 @@ InitMpGlobalData (
>    Status = gBS->SetTimer (
>                    mCheckAllApsEvent,
>                    TimerPeriodic,
> -                  AP_CHECK_INTERVAL
> +                  PcdGet64 (PcdCpuApStatusCheckInterval)
>                    );
>    ASSERT_EFI_ERROR (Status);
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni index
> c0d6ed5136..08d9b45f76 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_PcdCpuApStatusCheckInterval_PROMPT
> #language en-US "Periodic interval value in milliseconds for AP status check in
> DXE.\n"
> +#string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckInterval_HELP
> #language en-US "Periodic interval value in milliseconds for the status check
> of APs for StartupAllAPs() and StartupThisAP() executed in non-blocking
> mode in DXE phase.\n"
> --
> 2.12.0.windows.1
> 
> 
> 

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

[Wu, Hao A]

> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, March 24, 2020 3:02 PM
> To: devel@edk2.groups.io; Wu, Hao A
> Cc: Dong, Eric; Ni, Ray; Laszlo Ersek; Kinney, Michael D; Brian J . Johnson; Zeng,
> Star
> Subject: RE: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to
> control AP status check interval
> 
> The code logic is good to me.
> Only minor concern, do we really need the PCD to be UINT64 type? :)


Hi Star,

I am thinking the UINT64 type fits with the parameter for the SetTimer()
service in EFI_BOOT_SERVICES when preparing the patch.

If there is concern, I can switch it to other type.

Best Regards,
Hao Wu


> 
> Thanks,
> Star
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> > Wu, Hao A
> > Sent: Tuesday, March 24, 2020 2:33 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: [edk2-devel] [PATCH v2] 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.
> >
> > For least impact, the default value of the new PCD will be the same with the
> > current interval value, which is 100 milliseconds.
> >
> > Unitest done:
> > OS boot successfully.
> >
> > 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>
> > ---
> >  UefiCpuPkg/UefiCpuPkg.dec                     | 6 ++++++
> >  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 2 +-
> >  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       | 3 +--
> >  UefiCpuPkg/UefiCpuPkg.uni                     | 5 ++++-
> >  4 files changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec index
> > e91dc68cbe..06a3d52060 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 milliseconds for the
> > + status check  #  of APs for StartupAllAPs() and StartupThisAP()
> > + executed in non-blocking  #  mode in DXE phase.
> > +  # @Prompt Periodic interval value in milliseconds for AP status check in
> > DXE.
> > +
> > +
> > gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval|100|UINT64|0x
> > 000
> > + 0001E
> > +
> >  [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..05e004dc3c 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > @@ -69,5 +69,5 @@ [Pcd]
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ##
> > CONSUMES
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ##
> > CONSUMES
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ##
> > SOMETIMES_CONSUMES
> > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval            ##
> > CONSUMES
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                  ##
> > CONSUMES
> > -
> > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > index a987c32109..683547dc99 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,7 @@ InitMpGlobalData (
> >    Status = gBS->SetTimer (
> >                    mCheckAllApsEvent,
> >                    TimerPeriodic,
> > -                  AP_CHECK_INTERVAL
> > +                  PcdGet64 (PcdCpuApStatusCheckInterval)
> >                    );
> >    ASSERT_EFI_ERROR (Status);
> >
> > diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni index
> > c0d6ed5136..08d9b45f76 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_PcdCpuApStatusCheckInterval_PROMPT
> > #language en-US "Periodic interval value in milliseconds for AP status check in
> > DXE.\n"
> > +#string
> > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckInterval_HELP
> > #language en-US "Periodic interval value in milliseconds for the status check
> > of APs for StartupAllAPs() and StartupThisAP() executed in non-blocking
> > mode in DXE phase.\n"
> > --
> > 2.12.0.windows.1
> >
> >
> > 

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

[Zeng, Star]

> -----Original Message-----
> From: Wu, Hao A
> Sent: Tuesday, March 24, 2020 3:12 PM
> To: Zeng, Star <star.zeng@intel.com>; devel@edk2.groups.io
> Cc: 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>; Brian J . Johnson <brian.johnson@hpe.com>
> Subject: RE: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to
> control AP status check interval
> 
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Tuesday, March 24, 2020 3:02 PM
> > To: devel@edk2.groups.io; Wu, Hao A
> > Cc: Dong, Eric; Ni, Ray; Laszlo Ersek; Kinney, Michael D; Brian J .
> > Johnson; Zeng, Star
> > Subject: RE: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD
> > to control AP status check interval
> >
> > The code logic is good to me.
> > Only minor concern, do we really need the PCD to be UINT64 type? :)
> 
> 
> Hi Star,
> 
> I am thinking the UINT64 type fits with the parameter for the SetTimer()
> service in EFI_BOOT_SERVICES when preparing the patch.
> 
> If there is concern, I can switch it to other type.

Got your good point.
For me, UINT32 (FFFFFFFFh = 4294967295d = 4294967.295 second = 71582.78825 minutes ~= 1193.0464708333333333333333333333 hours) should be totally enough.
I do not insist on that if others think UINT64 is ok.

Thanks,
Star

> 
> Best Regards,
> Hao Wu
> 
> 
> >
> > Thanks,
> > Star
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf
> > > Of Wu, Hao A
> > > Sent: Tuesday, March 24, 2020 2:33 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: [edk2-devel] [PATCH v2] 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.
> > >
> > > For least impact, the default value of the new PCD will be the same
> > > with the current interval value, which is 100 milliseconds.
> > >
> > > Unitest done:
> > > OS boot successfully.
> > >
> > > 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>
> > > ---
> > >  UefiCpuPkg/UefiCpuPkg.dec                     | 6 ++++++
> > >  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 2 +-
> > >  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       | 3 +--
> > >  UefiCpuPkg/UefiCpuPkg.uni                     | 5 ++++-
> > >  4 files changed, 12 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> > > index
> > > e91dc68cbe..06a3d52060 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 milliseconds for the
> > > + status check  #  of APs for StartupAllAPs() and StartupThisAP()
> > > + executed in non-blocking  #  mode in DXE phase.
> > > +  # @Prompt Periodic interval value in milliseconds for AP status
> > > + check in
> > > DXE.
> > > +
> > > +
> > >
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval|100|UINT64|0x
> > > 000
> > > + 0001E
> > > +
> > >  [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..05e004dc3c 100644
> > > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > > @@ -69,5 +69,5 @@ [Pcd]
> > >    gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ##
> > > CONSUMES
> > >    gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ##
> > > CONSUMES
> > >    gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ##
> > > SOMETIMES_CONSUMES
> > > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval            ##
> > > CONSUMES
> > >    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                  ##
> > > CONSUMES
> > > -
> > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > index a987c32109..683547dc99 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,7 @@ InitMpGlobalData (
> > >    Status = gBS->SetTimer (
> > >                    mCheckAllApsEvent,
> > >                    TimerPeriodic,
> > > -                  AP_CHECK_INTERVAL
> > > +                  PcdGet64 (PcdCpuApStatusCheckInterval)
> > >                    );
> > >    ASSERT_EFI_ERROR (Status);
> > >
> > > diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
> > > index
> > > c0d6ed5136..08d9b45f76 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_PcdCpuApStatusCheckInterval_PROMPT
> > > #language en-US "Periodic interval value in milliseconds for AP
> > > status check in DXE.\n"
> > > +#string
> > > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckInterval_HELP
> > > #language en-US "Periodic interval value in milliseconds for the
> > > status check of APs for StartupAllAPs() and StartupThisAP() executed
> > > in non-blocking mode in DXE phase.\n"
> > > --
> > > 2.12.0.windows.1
> > >
> > >
> > > 

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

[Ni, Ray]

>From the perspective of PCD producer, I agree with Star to use UINT32.
Internal implementation can be changed to use other polling methods which don't require UINT64 data type.

> -----Original Message-----
> From: Zeng, Star <star.zeng@intel.com>
> Sent: Tuesday, March 24, 2020 3:29 PM
> To: Wu, Hao A <hao.a.wu@intel.com>; devel@edk2.groups.io
> Cc: 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>; Brian J . Johnson <brian.johnson@hpe.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval
> 
> > -----Original Message-----
> > From: Wu, Hao A
> > Sent: Tuesday, March 24, 2020 3:12 PM
> > To: Zeng, Star <star.zeng@intel.com>; devel@edk2.groups.io
> > Cc: 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>; Brian J . Johnson <brian.johnson@hpe.com>
> > Subject: RE: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to
> > control AP status check interval
> >
> > > -----Original Message-----
> > > From: Zeng, Star
> > > Sent: Tuesday, March 24, 2020 3:02 PM
> > > To: devel@edk2.groups.io; Wu, Hao A
> > > Cc: Dong, Eric; Ni, Ray; Laszlo Ersek; Kinney, Michael D; Brian J .
> > > Johnson; Zeng, Star
> > > Subject: RE: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD
> > > to control AP status check interval
> > >
> > > The code logic is good to me.
> > > Only minor concern, do we really need the PCD to be UINT64 type? :)
> >
> >
> > Hi Star,
> >
> > I am thinking the UINT64 type fits with the parameter for the SetTimer()
> > service in EFI_BOOT_SERVICES when preparing the patch.
> >
> > If there is concern, I can switch it to other type.
> 
> Got your good point.
> For me, UINT32 (FFFFFFFFh = 4294967295d = 4294967.295 second = 71582.78825 minutes ~=
> 1193.0464708333333333333333333333 hours) should be totally enough.
> I do not insist on that if others think UINT64 is ok.
> 
> Thanks,
> Star
> 
> >
> > Best Regards,
> > Hao Wu
> >
> >
> > >
> > > Thanks,
> > > Star
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf
> > > > Of Wu, Hao A
> > > > Sent: Tuesday, March 24, 2020 2:33 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: [edk2-devel] [PATCH v2] 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.
> > > >
> > > > For least impact, the default value of the new PCD will be the same
> > > > with the current interval value, which is 100 milliseconds.
> > > >
> > > > Unitest done:
> > > > OS boot successfully.
> > > >
> > > > 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>
> > > > ---
> > > >  UefiCpuPkg/UefiCpuPkg.dec                     | 6 ++++++
> > > >  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 2 +-
> > > >  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       | 3 +--
> > > >  UefiCpuPkg/UefiCpuPkg.uni                     | 5 ++++-
> > > >  4 files changed, 12 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> > > > index
> > > > e91dc68cbe..06a3d52060 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 milliseconds for the
> > > > + status check  #  of APs for StartupAllAPs() and StartupThisAP()
> > > > + executed in non-blocking  #  mode in DXE phase.
> > > > +  # @Prompt Periodic interval value in milliseconds for AP status
> > > > + check in
> > > > DXE.
> > > > +
> > > > +
> > > >
> > gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval|100|UINT64|0x
> > > > 000
> > > > + 0001E
> > > > +
> > > >  [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..05e004dc3c 100644
> > > > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > > > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > > > @@ -69,5 +69,5 @@ [Pcd]
> > > >    gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ##
> > > > CONSUMES
> > > >    gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode                       ##
> > > > CONSUMES
> > > >    gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate                   ##
> > > > SOMETIMES_CONSUMES
> > > > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval            ##
> > > > CONSUMES
> > > >    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                  ##
> > > > CONSUMES
> > > > -
> > > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > > b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > > index a987c32109..683547dc99 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,7 @@ InitMpGlobalData (
> > > >    Status = gBS->SetTimer (
> > > >                    mCheckAllApsEvent,
> > > >                    TimerPeriodic,
> > > > -                  AP_CHECK_INTERVAL
> > > > +                  PcdGet64 (PcdCpuApStatusCheckInterval)
> > > >                    );
> > > >    ASSERT_EFI_ERROR (Status);
> > > >
> > > > diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
> > > > index
> > > > c0d6ed5136..08d9b45f76 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_PcdCpuApStatusCheckInterval_PROMPT
> > > > #language en-US "Periodic interval value in milliseconds for AP
> > > > status check in DXE.\n"
> > > > +#string
> > > > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckInterval_HELP
> > > > #language en-US "Periodic interval value in milliseconds for the
> > > > status check of APs for StartupAllAPs() and StartupThisAP() executed
> > > > in non-blocking mode in DXE phase.\n"
> > > > --
> > > > 2.12.0.windows.1
> > > >
> > > >
> > > > 

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

[Michael D Kinney]

How was the milliseconds units selected?

We have other PCDs that provide timer intervals in 
100ns unit and my microseconds.  I would prefer we
be consistent so platform developers do not have
to keep track of so many different units for time
intervals.

Mike

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Tuesday, March 24, 2020 12:53 AM
> To: Zeng, Star <star.zeng@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Brian J . Johnson
> <brian.johnson@hpe.com>
> Subject: RE: [edk2-devel] [PATCH v2]
> UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status
> check interval
> 
> From the perspective of PCD producer, I agree with Star
> to use UINT32.
> Internal implementation can be changed to use other
> polling methods which don't require UINT64 data type.
> 
> > -----Original Message-----
> > From: Zeng, Star <star.zeng@intel.com>
> > Sent: Tuesday, March 24, 2020 3:29 PM
> > To: Wu, Hao A <hao.a.wu@intel.com>;
> devel@edk2.groups.io
> > Cc: 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>; Brian J . Johnson
> <brian.johnson@hpe.com>; Zeng, Star
> <star.zeng@intel.com>
> > Subject: RE: [edk2-devel] [PATCH v2]
> UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status
> check interval
> >
> > > -----Original Message-----
> > > From: Wu, Hao A
> > > Sent: Tuesday, March 24, 2020 3:12 PM
> > > To: Zeng, Star <star.zeng@intel.com>;
> devel@edk2.groups.io
> > > Cc: 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>; Brian J . Johnson
> <brian.johnson@hpe.com>
> > > Subject: RE: [edk2-devel] [PATCH v2]
> UefiCpuPkg/MpInitLib DXE: Add PCD to
> > > control AP status check interval
> > >
> > > > -----Original Message-----
> > > > From: Zeng, Star
> > > > Sent: Tuesday, March 24, 2020 3:02 PM
> > > > To: devel@edk2.groups.io; Wu, Hao A
> > > > Cc: Dong, Eric; Ni, Ray; Laszlo Ersek; Kinney,
> Michael D; Brian J .
> > > > Johnson; Zeng, Star
> > > > Subject: RE: [edk2-devel] [PATCH v2]
> UefiCpuPkg/MpInitLib DXE: Add PCD
> > > > to control AP status check interval
> > > >
> > > > The code logic is good to me.
> > > > Only minor concern, do we really need the PCD to
> be UINT64 type? :)
> > >
> > >
> > > Hi Star,
> > >
> > > I am thinking the UINT64 type fits with the
> parameter for the SetTimer()
> > > service in EFI_BOOT_SERVICES when preparing the
> patch.
> > >
> > > If there is concern, I can switch it to other type.
> >
> > Got your good point.
> > For me, UINT32 (FFFFFFFFh = 4294967295d = 4294967.295
> second = 71582.78825 minutes ~=
> > 1193.0464708333333333333333333333 hours) should be
> totally enough.
> > I do not insist on that if others think UINT64 is ok.
> >
> > Thanks,
> > Star
> >
> > >
> > > Best Regards,
> > > Hao Wu
> > >
> > >
> > > >
> > > > Thanks,
> > > > Star
> > > >
> > > > > -----Original Message-----
> > > > > From: devel@edk2.groups.io
> [mailto:devel@edk2.groups.io] On Behalf
> > > > > Of Wu, Hao A
> > > > > Sent: Tuesday, March 24, 2020 2:33 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: [edk2-devel] [PATCH v2]
> 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/DxeRegisterCp
> uFeaturesLib.
> > > > > 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.
> > > > >
> > > > > For least impact, the default value of the new
> PCD will be the same
> > > > > with the current interval value, which is 100
> milliseconds.
> > > > >
> > > > > Unitest done:
> > > > > OS boot successfully.
> > > > >
> > > > > 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>
> > > > > ---
> > > > >  UefiCpuPkg/UefiCpuPkg.dec
> | 6 ++++++
> > > > >  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> | 2 +-
> > > > >  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> | 3 +--
> > > > >  UefiCpuPkg/UefiCpuPkg.uni
> | 5 ++++-
> > > > >  4 files changed, 12 insertions(+), 4
> deletions(-)
> > > > >
> > > > > diff --git a/UefiCpuPkg/UefiCpuPkg.dec
> b/UefiCpuPkg/UefiCpuPkg.dec
> > > > > index
> > > > > e91dc68cbe..06a3d52060 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.PcdCpuCoreCrystalClockFrequen
> cy|24000000|
> > > > > UINT64|0x32132113
> > > > >
> > > > > +  ## Specifies the periodic interval value in
> milliseconds for the
> > > > > + status check  #  of APs for StartupAllAPs()
> and StartupThisAP()
> > > > > + executed in non-blocking  #  mode in DXE
> phase.
> > > > > +  # @Prompt Periodic interval value in
> milliseconds for AP status
> > > > > + check in
> > > > > DXE.
> > > > > +
> > > > > +
> > > > >
> > >
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval|1
> 00|UINT64|0x
> > > > > 000
> > > > > + 0001E
> > > > > +
> > > > >  [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..05e004dc3c 100644
> > > > > ---
> a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > > > > +++
> b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > > > > @@ -69,5 +69,5 @@ [Pcd]
> > > > >
> gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSiz
> e         ##
> > > > > CONSUMES
> > > > >    gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode
> ##
> > > > > CONSUMES
> > > > >
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate
> ##
> > > > > SOMETIMES_CONSUMES
> > > > > +
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval
> ##
> > > > > CONSUMES
> > > > >
> gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
> ##
> > > > > CONSUMES
> > > > > -
> > > > > diff --git
> a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > > > b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > > > index a987c32109..683547dc99 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,7 @@ InitMpGlobalData (
> > > > >    Status = gBS->SetTimer (
> > > > >                    mCheckAllApsEvent,
> > > > >                    TimerPeriodic,
> > > > > -                  AP_CHECK_INTERVAL
> > > > > +                  PcdGet64
> (PcdCpuApStatusCheckInterval)
> > > > >                    );
> > > > >    ASSERT_EFI_ERROR (Status);
> > > > >
> > > > > diff --git a/UefiCpuPkg/UefiCpuPkg.uni
> b/UefiCpuPkg/UefiCpuPkg.uni
> > > > > index
> > > > > c0d6ed5136..08d9b45f76 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_PcdCpuSmmMpTokenCountPerC
> hunk_PR
> > > > > OMPT  #language en-US "Specify the count of pre
> allocated SMM MP
> > > > > tokens per chunk.\n"
> > > > >
> > > > >  #string
> > > > >
> > >
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerC
> hunk_HE
> > > > > LP    #language en-US "This value used to
> specify the count of pre
> > > allocated
> > > > > SMM MP tokens per chunk.\n"
> > > > > +
> > > > > +#string
> > > > >
> > >
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckInterv
> al_PROMPT
> > > > > #language en-US "Periodic interval value in
> milliseconds for AP
> > > > > status check in DXE.\n"
> > > > > +#string
> > > > >
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckInterv
> al_HELP
> > > > > #language en-US "Periodic interval value in
> milliseconds for the
> > > > > status check of APs for StartupAllAPs() and
> StartupThisAP() executed
> > > > > in non-blocking mode in DXE phase.\n"
> > > > > --
> > > > > 2.12.0.windows.1
> > > > >
> > > > >
> > > > > 

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

[Ni, Ray]

Mike, Hao,
I searched all edk2 code using regex pattern "Pcd.*Timeout.*", "Pcd.*Timer.*", "Pcd.*Delay.*"
and found below PCDs:

gEfiMdePkgTokenSpaceGuid.PcdSpinLockTimeout: in tick unit depending on the TimerLib used
gEfiMdePkgTokenSpaceGuid.PcdUsbTransferTimeoutValue: in 1ms unit
gPcAtChipsetPkgTokenSpaceGuid.PcdHpetDefaultTimerPeriod: in 100ns unit

gEfiMdeModulePkgTokenSpaceGuid.PcdSataSpinUpDelayInSecForRecoveryPath: in 1s unit
gUefiCpuPkgTokenSpaceGuid.PcdCpuInitIpiDelayInMicroSeconds: in 1us unit
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout: in 1us unit
gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdUsbXhciDebugDetectTimeout: in 1us unit

1 uses timer ticks as unit.
1 uses as unit.
1 uses ms as unit.
3 use us as unit.
1 uses 100ns as unit.

UefiCpuPkg uses us consistently as the unit.
So how about using us for this PCD as well?
And don't forget to name the PCD as "PcdCpuApStatusCheckIntervalInMicroSeconds" so developers who set the PCD can be aware by reading the name.

Thanks,
Ray

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Tuesday, March 24, 2020 11:59 PM
> To: Ni, Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Brian J . Johnson <brian.johnson@hpe.com>
> Subject: RE: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status check interval
> 
> How was the milliseconds units selected?
> 
> We have other PCDs that provide timer intervals in
> 100ns unit and my microseconds.  I would prefer we
> be consistent so platform developers do not have
> to keep track of so many different units for time
> intervals.
> 
> Mike
> 
> > -----Original Message-----
> > From: Ni, Ray <ray.ni@intel.com>
> > Sent: Tuesday, March 24, 2020 12:53 AM
> > To: Zeng, Star <star.zeng@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; devel@edk2.groups.io
> > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Brian J . Johnson
> > <brian.johnson@hpe.com>
> > Subject: RE: [edk2-devel] [PATCH v2]
> > UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status
> > check interval
> >
> > From the perspective of PCD producer, I agree with Star
> > to use UINT32.
> > Internal implementation can be changed to use other
> > polling methods which don't require UINT64 data type.
> >
> > > -----Original Message-----
> > > From: Zeng, Star <star.zeng@intel.com>
> > > Sent: Tuesday, March 24, 2020 3:29 PM
> > > To: Wu, Hao A <hao.a.wu@intel.com>;
> > devel@edk2.groups.io
> > > Cc: 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>; Brian J . Johnson
> > <brian.johnson@hpe.com>; Zeng, Star
> > <star.zeng@intel.com>
> > > Subject: RE: [edk2-devel] [PATCH v2]
> > UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status
> > check interval
> > >
> > > > -----Original Message-----
> > > > From: Wu, Hao A
> > > > Sent: Tuesday, March 24, 2020 3:12 PM
> > > > To: Zeng, Star <star.zeng@intel.com>;
> > devel@edk2.groups.io
> > > > Cc: 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>; Brian J . Johnson
> > <brian.johnson@hpe.com>
> > > > Subject: RE: [edk2-devel] [PATCH v2]
> > UefiCpuPkg/MpInitLib DXE: Add PCD to
> > > > control AP status check interval
> > > >
> > > > > -----Original Message-----
> > > > > From: Zeng, Star
> > > > > Sent: Tuesday, March 24, 2020 3:02 PM
> > > > > To: devel@edk2.groups.io; Wu, Hao A
> > > > > Cc: Dong, Eric; Ni, Ray; Laszlo Ersek; Kinney,
> > Michael D; Brian J .
> > > > > Johnson; Zeng, Star
> > > > > Subject: RE: [edk2-devel] [PATCH v2]
> > UefiCpuPkg/MpInitLib DXE: Add PCD
> > > > > to control AP status check interval
> > > > >
> > > > > The code logic is good to me.
> > > > > Only minor concern, do we really need the PCD to
> > be UINT64 type? :)
> > > >
> > > >
> > > > Hi Star,
> > > >
> > > > I am thinking the UINT64 type fits with the
> > parameter for the SetTimer()
> > > > service in EFI_BOOT_SERVICES when preparing the
> > patch.
> > > >
> > > > If there is concern, I can switch it to other type.
> > >
> > > Got your good point.
> > > For me, UINT32 (FFFFFFFFh = 4294967295d = 4294967.295
> > second = 71582.78825 minutes ~=
> > > 1193.0464708333333333333333333333 hours) should be
> > totally enough.
> > > I do not insist on that if others think UINT64 is ok.
> > >
> > > Thanks,
> > > Star
> > >
> > > >
> > > > Best Regards,
> > > > Hao Wu
> > > >
> > > >
> > > > >
> > > > > Thanks,
> > > > > Star
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: devel@edk2.groups.io
> > [mailto:devel@edk2.groups.io] On Behalf
> > > > > > Of Wu, Hao A
> > > > > > Sent: Tuesday, March 24, 2020 2:33 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: [edk2-devel] [PATCH v2]
> > 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/DxeRegisterCp
> > uFeaturesLib.
> > > > > > 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.
> > > > > >
> > > > > > For least impact, the default value of the new
> > PCD will be the same
> > > > > > with the current interval value, which is 100
> > milliseconds.
> > > > > >
> > > > > > Unitest done:
> > > > > > OS boot successfully.
> > > > > >
> > > > > > 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>
> > > > > > ---
> > > > > >  UefiCpuPkg/UefiCpuPkg.dec
> > | 6 ++++++
> > > > > >  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > | 2 +-
> > > > > >  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > | 3 +--
> > > > > >  UefiCpuPkg/UefiCpuPkg.uni
> > | 5 ++++-
> > > > > >  4 files changed, 12 insertions(+), 4
> > deletions(-)
> > > > > >
> > > > > > diff --git a/UefiCpuPkg/UefiCpuPkg.dec
> > b/UefiCpuPkg/UefiCpuPkg.dec
> > > > > > index
> > > > > > e91dc68cbe..06a3d52060 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.PcdCpuCoreCrystalClockFrequen
> > cy|24000000|
> > > > > > UINT64|0x32132113
> > > > > >
> > > > > > +  ## Specifies the periodic interval value in
> > milliseconds for the
> > > > > > + status check  #  of APs for StartupAllAPs()
> > and StartupThisAP()
> > > > > > + executed in non-blocking  #  mode in DXE
> > phase.
> > > > > > +  # @Prompt Periodic interval value in
> > milliseconds for AP status
> > > > > > + check in
> > > > > > DXE.
> > > > > > +
> > > > > > +
> > > > > >
> > > >
> > gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval|1
> > 00|UINT64|0x
> > > > > > 000
> > > > > > + 0001E
> > > > > > +
> > > > > >  [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..05e004dc3c 100644
> > > > > > ---
> > a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > > > > > +++
> > b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > > > > > @@ -69,5 +69,5 @@ [Pcd]
> > > > > >
> > gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSiz
> > e         ##
> > > > > > CONSUMES
> > > > > >    gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode
> > ##
> > > > > > CONSUMES
> > > > > >
> > gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate
> > ##
> > > > > > SOMETIMES_CONSUMES
> > > > > > +
> > gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval
> > ##
> > > > > > CONSUMES
> > > > > >
> > gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
> > ##
> > > > > > CONSUMES
> > > > > > -
> > > > > > diff --git
> > a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > > > > b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > > > > index a987c32109..683547dc99 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,7 @@ InitMpGlobalData (
> > > > > >    Status = gBS->SetTimer (
> > > > > >                    mCheckAllApsEvent,
> > > > > >                    TimerPeriodic,
> > > > > > -                  AP_CHECK_INTERVAL
> > > > > > +                  PcdGet64
> > (PcdCpuApStatusCheckInterval)
> > > > > >                    );
> > > > > >    ASSERT_EFI_ERROR (Status);
> > > > > >
> > > > > > diff --git a/UefiCpuPkg/UefiCpuPkg.uni
> > b/UefiCpuPkg/UefiCpuPkg.uni
> > > > > > index
> > > > > > c0d6ed5136..08d9b45f76 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_PcdCpuSmmMpTokenCountPerC
> > hunk_PR
> > > > > > OMPT  #language en-US "Specify the count of pre
> > allocated SMM MP
> > > > > > tokens per chunk.\n"
> > > > > >
> > > > > >  #string
> > > > > >
> > > >
> > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerC
> > hunk_HE
> > > > > > LP    #language en-US "This value used to
> > specify the count of pre
> > > > allocated
> > > > > > SMM MP tokens per chunk.\n"
> > > > > > +
> > > > > > +#string
> > > > > >
> > > >
> > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckInterv
> > al_PROMPT
> > > > > > #language en-US "Periodic interval value in
> > milliseconds for AP
> > > > > > status check in DXE.\n"
> > > > > > +#string
> > > > > >
> > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckInterv
> > al_HELP
> > > > > > #language en-US "Periodic interval value in
> > milliseconds for the
> > > > > > status check of APs for StartupAllAPs() and
> > StartupThisAP() executed
> > > > > > in non-blocking mode in DXE phase.\n"
> > > > > > --
> > > > > > 2.12.0.windows.1
> > > > > >
> > > > > >
> > > > > > 

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

[Wu, Hao A]

> -----Original Message-----
> From: Ni, Ray
> Sent: Wednesday, March 25, 2020 8:46 AM
> To: Kinney, Michael D; Zeng, Star; Wu, Hao A; devel@edk2.groups.io
> Cc: Dong, Eric; Laszlo Ersek; Brian J . Johnson
> Subject: RE: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to
> control AP status check interval
> 
> Mike, Hao,
> I searched all edk2 code using regex pattern "Pcd.*Timeout.*", "Pcd.*Timer.*",
> "Pcd.*Delay.*"
> and found below PCDs:
> 
> gEfiMdePkgTokenSpaceGuid.PcdSpinLockTimeout: in tick unit depending on the
> TimerLib used
> gEfiMdePkgTokenSpaceGuid.PcdUsbTransferTimeoutValue: in 1ms unit
> gPcAtChipsetPkgTokenSpaceGuid.PcdHpetDefaultTimerPeriod: in 100ns unit
> 
> gEfiMdeModulePkgTokenSpaceGuid.PcdSataSpinUpDelayInSecForRecoveryPat
> h: in 1s unit
> gUefiCpuPkgTokenSpaceGuid.PcdCpuInitIpiDelayInMicroSeconds: in 1us unit
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout: in 1us unit
> gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdUsbXhciDebugDetectTimeout: in
> 1us unit
> 
> 1 uses timer ticks as unit.
> 1 uses as unit.
> 1 uses ms as unit.
> 3 use us as unit.
> 1 uses 100ns as unit.
> 
> UefiCpuPkg uses us consistently as the unit.
> So how about using us for this PCD as well?
> And don't forget to name the PCD as
> "PcdCpuApStatusCheckIntervalInMicroSeconds" so developers who set the PCD
> can be aware by reading the name.


Hello Ray,

Agree with your suggestion.
I will follow up with this approach and provide a new version of the patch.

Best Regards,
Hao Wu


> 
> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > Sent: Tuesday, March 24, 2020 11:59 PM
> > To: Ni, Ray <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>;
> > devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> Brian J . Johnson <brian.johnson@hpe.com>
> > Subject: RE: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to
> control AP status check interval
> >
> > How was the milliseconds units selected?
> >
> > We have other PCDs that provide timer intervals in
> > 100ns unit and my microseconds.  I would prefer we
> > be consistent so platform developers do not have
> > to keep track of so many different units for time
> > intervals.
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: Ni, Ray <ray.ni@intel.com>
> > > Sent: Tuesday, March 24, 2020 12:53 AM
> > > To: Zeng, Star <star.zeng@intel.com>; Wu, Hao A
> > > <hao.a.wu@intel.com>; devel@edk2.groups.io
> > > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek
> > > <lersek@redhat.com>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>; Brian J . Johnson
> > > <brian.johnson@hpe.com>
> > > Subject: RE: [edk2-devel] [PATCH v2]
> > > UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status
> > > check interval
> > >
> > > From the perspective of PCD producer, I agree with Star
> > > to use UINT32.
> > > Internal implementation can be changed to use other
> > > polling methods which don't require UINT64 data type.
> > >
> > > > -----Original Message-----
> > > > From: Zeng, Star <star.zeng@intel.com>
> > > > Sent: Tuesday, March 24, 2020 3:29 PM
> > > > To: Wu, Hao A <hao.a.wu@intel.com>;
> > > devel@edk2.groups.io
> > > > Cc: 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>; Brian J . Johnson
> > > <brian.johnson@hpe.com>; Zeng, Star
> > > <star.zeng@intel.com>
> > > > Subject: RE: [edk2-devel] [PATCH v2]
> > > UefiCpuPkg/MpInitLib DXE: Add PCD to control AP status
> > > check interval
> > > >
> > > > > -----Original Message-----
> > > > > From: Wu, Hao A
> > > > > Sent: Tuesday, March 24, 2020 3:12 PM
> > > > > To: Zeng, Star <star.zeng@intel.com>;
> > > devel@edk2.groups.io
> > > > > Cc: 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>; Brian J . Johnson
> > > <brian.johnson@hpe.com>
> > > > > Subject: RE: [edk2-devel] [PATCH v2]
> > > UefiCpuPkg/MpInitLib DXE: Add PCD to
> > > > > control AP status check interval
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Zeng, Star
> > > > > > Sent: Tuesday, March 24, 2020 3:02 PM
> > > > > > To: devel@edk2.groups.io; Wu, Hao A
> > > > > > Cc: Dong, Eric; Ni, Ray; Laszlo Ersek; Kinney,
> > > Michael D; Brian J .
> > > > > > Johnson; Zeng, Star
> > > > > > Subject: RE: [edk2-devel] [PATCH v2]
> > > UefiCpuPkg/MpInitLib DXE: Add PCD
> > > > > > to control AP status check interval
> > > > > >
> > > > > > The code logic is good to me.
> > > > > > Only minor concern, do we really need the PCD to
> > > be UINT64 type? :)
> > > > >
> > > > >
> > > > > Hi Star,
> > > > >
> > > > > I am thinking the UINT64 type fits with the
> > > parameter for the SetTimer()
> > > > > service in EFI_BOOT_SERVICES when preparing the
> > > patch.
> > > > >
> > > > > If there is concern, I can switch it to other type.
> > > >
> > > > Got your good point.
> > > > For me, UINT32 (FFFFFFFFh = 4294967295d = 4294967.295
> > > second = 71582.78825 minutes ~=
> > > > 1193.0464708333333333333333333333 hours) should be
> > > totally enough.
> > > > I do not insist on that if others think UINT64 is ok.
> > > >
> > > > Thanks,
> > > > Star
> > > >
> > > > >
> > > > > Best Regards,
> > > > > Hao Wu
> > > > >
> > > > >
> > > > > >
> > > > > > Thanks,
> > > > > > Star
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: devel@edk2.groups.io
> > > [mailto:devel@edk2.groups.io] On Behalf
> > > > > > > Of Wu, Hao A
> > > > > > > Sent: Tuesday, March 24, 2020 2:33 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: [edk2-devel] [PATCH v2]
> > > 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/DxeRegisterCp
> > > uFeaturesLib.
> > > > > > > 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.
> > > > > > >
> > > > > > > For least impact, the default value of the new
> > > PCD will be the same
> > > > > > > with the current interval value, which is 100
> > > milliseconds.
> > > > > > >
> > > > > > > Unitest done:
> > > > > > > OS boot successfully.
> > > > > > >
> > > > > > > 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>
> > > > > > > ---
> > > > > > >  UefiCpuPkg/UefiCpuPkg.dec
> > > | 6 ++++++
> > > > > > >  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > > | 2 +-
> > > > > > >  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > | 3 +--
> > > > > > >  UefiCpuPkg/UefiCpuPkg.uni
> > > | 5 ++++-
> > > > > > >  4 files changed, 12 insertions(+), 4
> > > deletions(-)
> > > > > > >
> > > > > > > diff --git a/UefiCpuPkg/UefiCpuPkg.dec
> > > b/UefiCpuPkg/UefiCpuPkg.dec
> > > > > > > index
> > > > > > > e91dc68cbe..06a3d52060 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.PcdCpuCoreCrystalClockFrequen
> > > cy|24000000|
> > > > > > > UINT64|0x32132113
> > > > > > >
> > > > > > > +  ## Specifies the periodic interval value in
> > > milliseconds for the
> > > > > > > + status check  #  of APs for StartupAllAPs()
> > > and StartupThisAP()
> > > > > > > + executed in non-blocking  #  mode in DXE
> > > phase.
> > > > > > > +  # @Prompt Periodic interval value in
> > > milliseconds for AP status
> > > > > > > + check in
> > > > > > > DXE.
> > > > > > > +
> > > > > > > +
> > > > > > >
> > > > >
> > > gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval|1
> > > 00|UINT64|0x
> > > > > > > 000
> > > > > > > + 0001E
> > > > > > > +
> > > > > > >  [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..05e004dc3c 100644
> > > > > > > ---
> > > a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > > > > > > +++
> > > b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> > > > > > > @@ -69,5 +69,5 @@ [Pcd]
> > > > > > >
> > > gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSiz
> > > e         ##
> > > > > > > CONSUMES
> > > > > > >    gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode
> > > ##
> > > > > > > CONSUMES
> > > > > > >
> > > gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate
> > > ##
> > > > > > > SOMETIMES_CONSUMES
> > > > > > > +
> > > gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval
> > > ##
> > > > > > > CONSUMES
> > > > > > >
> > > gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
> > > ##
> > > > > > > CONSUMES
> > > > > > > -
> > > > > > > diff --git
> > > a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > > > > > b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> > > > > > > index a987c32109..683547dc99 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,7 @@ InitMpGlobalData (
> > > > > > >    Status = gBS->SetTimer (
> > > > > > >                    mCheckAllApsEvent,
> > > > > > >                    TimerPeriodic,
> > > > > > > -                  AP_CHECK_INTERVAL
> > > > > > > +                  PcdGet64
> > > (PcdCpuApStatusCheckInterval)
> > > > > > >                    );
> > > > > > >    ASSERT_EFI_ERROR (Status);
> > > > > > >
> > > > > > > diff --git a/UefiCpuPkg/UefiCpuPkg.uni
> > > b/UefiCpuPkg/UefiCpuPkg.uni
> > > > > > > index
> > > > > > > c0d6ed5136..08d9b45f76 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_PcdCpuSmmMpTokenCountPerC
> > > hunk_PR
> > > > > > > OMPT  #language en-US "Specify the count of pre
> > > allocated SMM MP
> > > > > > > tokens per chunk.\n"
> > > > > > >
> > > > > > >  #string
> > > > > > >
> > > > >
> > > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuSmmMpTokenCountPerC
> > > hunk_HE
> > > > > > > LP    #language en-US "This value used to
> > > specify the count of pre
> > > > > allocated
> > > > > > > SMM MP tokens per chunk.\n"
> > > > > > > +
> > > > > > > +#string
> > > > > > >
> > > > >
> > > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckInterv
> > > al_PROMPT
> > > > > > > #language en-US "Periodic interval value in
> > > milliseconds for AP
> > > > > > > status check in DXE.\n"
> > > > > > > +#string
> > > > > > >
> > > STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApStatusCheckInterv
> > > al_HELP
> > > > > > > #language en-US "Periodic interval value in
> > > milliseconds for the
> > > > > > > status check of APs for StartupAllAPs() and
> > > StartupThisAP() executed
> > > > > > > in non-blocking mode in DXE phase.\n"
> > > > > > > --
> > > > > > > 2.12.0.windows.1
> > > > > > >
> > > > > > >
> > > > > > > 

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

[Laszlo Ersek]

On 03/24/20 16:59, Kinney, Michael D wrote:
> How was the milliseconds units selected?

I suggested msecs for continuity with the pre-patch unit:

#define  AP_CHECK_INTERVAL     (EFI_TIMER_PERIOD_MILLISECONDS (100))

> We have other PCDs that provide timer intervals in 
> 100ns unit and my microseconds.  I would prefer we
> be consistent so platform developers do not have
> to keep track of so many different units for time
> intervals.

Sounds OK to me.

What's more: your suggestion points at a bug in the v2 patch. The patch replaces the argument for the gBS->SetTimer() service's "TriggerTime" parameter, namely:

   Status = gBS->SetTimer (
                   mCheckAllApsEvent,
                   TimerPeriodic,
-                  AP_CHECK_INTERVAL
+                  PcdGet64 (PcdCpuApStatusCheckInterval)
                   );

But that parameter is taken in units of 100ns, per spec.

Pre-patch, AP_CHECK_INTERVAL takes that into account:

#define EFI_TIMER_PERIOD_MILLISECONDS(Milliseconds) MultU64x32((UINT64)(Milliseconds), 10000)

but post-patch, the PCD does not:

+  gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval|100|UINT64|0x0000001E

So this patch would shorten the interval by a divisor of 10000 at once, namely from (100 * 10000) * 100ns, to 100 * 100ns.

The fix is to update the PCD comments according to your remark, and to change the default PCD value to (100 * 10000) = 1000,000.

Thanks,
Laszlo

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

[Wu, Hao A]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, March 25, 2020 9:37 AM
> To: Kinney, Michael D; Ni, Ray; Zeng, Star; Wu, Hao A; devel@edk2.groups.io
> Cc: Dong, Eric; Brian J . Johnson
> Subject: Re: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to
> control AP status check interval
> 
> On 03/24/20 16:59, Kinney, Michael D wrote:
> > How was the milliseconds units selected?
> 
> I suggested msecs for continuity with the pre-patch unit:
> 
> #define  AP_CHECK_INTERVAL     (EFI_TIMER_PERIOD_MILLISECONDS (100))
> 
> > We have other PCDs that provide timer intervals in
> > 100ns unit and my microseconds.  I would prefer we
> > be consistent so platform developers do not have
> > to keep track of so many different units for time
> > intervals.
> 
> Sounds OK to me.
> 
> What's more: your suggestion points at a bug in the v2 patch. The patch replaces
> the argument for the gBS->SetTimer() service's "TriggerTime" parameter,
> namely:
> 
>    Status = gBS->SetTimer (
>                    mCheckAllApsEvent,
>                    TimerPeriodic,
> -                  AP_CHECK_INTERVAL
> +                  PcdGet64 (PcdCpuApStatusCheckInterval)
>                    );
> 
> But that parameter is taken in units of 100ns, per spec.
> 
> Pre-patch, AP_CHECK_INTERVAL takes that into account:
> 
> #define EFI_TIMER_PERIOD_MILLISECONDS(Milliseconds)
> MultU64x32((UINT64)(Milliseconds), 10000)
> 
> but post-patch, the PCD does not:
> 
> +
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckInterval|100|UINT64|0x0
> 000001E
> 
> So this patch would shorten the interval by a divisor of 10000 at once, namely
> from (100 * 10000) * 100ns, to 100 * 100ns.
> 
> The fix is to update the PCD comments according to your remark, and to change
> the default PCD value to (100 * 10000) = 1000,000.


Hello Laszlo,

Thanks for the catch. I will address the bug introduced in the V2 patch.

One thing to confirm, as Ray mentioned that the UefiCpuPkg uses microseconds(us)
as unit for timeout/interval values, so how about using the below definition
for the proposed PCD:

gUefiCpuPkgTokenSpaceGuid.PcdCpuApStatusCheckIntervalInMicroseconds|100000|UINT32|0x0000001E

And in the C code use the macro 'EFI_TIMER_PERIOD_MICROSECONDS' to convert the
PCD value to the 'SetTimer' parameter?

Best Regards,
Hao Wu


> 
> Thanks,
> Laszlo

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

[Laszlo Ersek]

On 03/25/20 02:24, Wu, Hao A wrote:
>> -----Original Message-----
>> From: Ni, Ray
>> Sent: Wednesday, March 25, 2020 8:46 AM
>> To: Kinney, Michael D; Zeng, Star; Wu, Hao A; devel@edk2.groups.io
>> Cc: Dong, Eric; Laszlo Ersek; Brian J . Johnson
>> Subject: RE: [edk2-devel] [PATCH v2] UefiCpuPkg/MpInitLib DXE: Add PCD to
>> control AP status check interval
>>
>> Mike, Hao,
>> I searched all edk2 code using regex pattern "Pcd.*Timeout.*", "Pcd.*Timer.*",
>> "Pcd.*Delay.*"
>> and found below PCDs:
>>
>> gEfiMdePkgTokenSpaceGuid.PcdSpinLockTimeout: in tick unit depending on the
>> TimerLib used
>> gEfiMdePkgTokenSpaceGuid.PcdUsbTransferTimeoutValue: in 1ms unit
>> gPcAtChipsetPkgTokenSpaceGuid.PcdHpetDefaultTimerPeriod: in 100ns unit
>>
>> gEfiMdeModulePkgTokenSpaceGuid.PcdSataSpinUpDelayInSecForRecoveryPat
>> h: in 1s unit
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuInitIpiDelayInMicroSeconds: in 1us unit
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout: in 1us unit
>> gEfiSourceLevelDebugPkgTokenSpaceGuid.PcdUsbXhciDebugDetectTimeout: in
>> 1us unit
>>
>> 1 uses timer ticks as unit.
>> 1 uses as unit.
>> 1 uses ms as unit.
>> 3 use us as unit.
>> 1 uses 100ns as unit.
>>
>> UefiCpuPkg uses us consistently as the unit.
>> So how about using us for this PCD as well?
>> And don't forget to name the PCD as
>> "PcdCpuApStatusCheckIntervalInMicroSeconds" so developers who set the PCD
>> can be aware by reading the name.
> 
> 
> Hello Ray,
> 
> Agree with your suggestion.
> I will follow up with this approach and provide a new version of the patch.

Works for me, too.

Thanks!
Laszlo