1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH 0/4] UefiCpuPkg, OvmfPkg: separate PCDs for boot CPU count vs. max CPU count
  4. View patch

[edk2-devel] [PATCH 1/4] UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP detection

Laszlo Ersek posted 4 patches 6 years, 4 months ago
[edk2-devel] [PATCH 1/4] UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP detection
Posted by Laszlo Ersek 6 years, 4 months ago 
If a platform boots with a CPU topology that is not fully populated (that
is, the boot CPU count is smaller than PcdCpuMaxLogicalProcessorNumber),
then the platform cannot use the "fast AP detection" logic added in commit
6e1987f19af7. Said logic depends on the boot CPU count being equal to
PcdCpuMaxLogicalProcessorNumber.

The platform may not be able to use the variant added in commit
0594ec417c89 either, for different reasons; see for example commit
861218740d6d.

Allow platforms to specify the exact boot CPU count, independently of
PcdCpuMaxLogicalProcessorNumber.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Ray Ni <ray.ni@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 UefiCpuPkg/UefiCpuPkg.dec                     | 11 +++++
 UefiCpuPkg/UefiCpuPkg.uni                     |  4 ++
 UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  1 +
 UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |  3 +-
 UefiCpuPkg/Library/MpInitLib/MpLib.c          | 42 ++++++++++++--------
 5 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index 031a2ccd680a..d6b33fd9b465 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -227,6 +227,17 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
   ## Specifies timeout value in microseconds for the BSP to detect all APs for the first time.
   # @Prompt Timeout for the BSP to detect all APs for the first time.
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000|UINT32|0x00000004
+  ## Specifies the number of Logical Processors that are available in the
+  #  preboot environment after platform reset, including BSP and APs. Possible
+  #  values:<BR><BR>
+  #  zero (default) - This PCD is ignored, and
+  #                   PcdCpuApInitTimeOutInMicroSeconds limits the initial AP
+  #                   detection by the BSP.<BR>
+  #  nonzero        - PcdCpuApInitTimeOutInMicroSeconds is ignored. The initial
+  #                   AP detection finishes when the detected CPU count (BSP
+  #                   plus APs) reaches the value of this PCD.<BR>
+  # @Prompt Number of Logical Processors available after platform reset.
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0|UINT32|0x00000008
   ## Specifies the base address of the first microcode Patch in the microcode Region.
   # @Prompt Microcode Region base address.
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress|0x0|UINT64|0x00000005
diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
index fbf768072668..a7e279c5cb14 100644
--- a/UefiCpuPkg/UefiCpuPkg.uni
+++ b/UefiCpuPkg/UefiCpuPkg.uni
@@ -37,6 +37,10 @@
 
 #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApInitTimeOutInMicroSeconds_HELP  #language en-US "Specifies timeout value in microseconds for the BSP to detect all APs for the first time."
 
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuBootLogicalProcessorNumber_PROMPT  #language en-US "Number of Logical Processors available after platform reset."
+
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuBootLogicalProcessorNumber_HELP  #language en-US "Specifies the number of Logical Processors that are available in the preboot environment after platform reset, including BSP and APs."
+
 #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuMicrocodePatchAddress_PROMPT  #language en-US "Microcode Region base address."
 
 #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuMicrocodePatchAddress_HELP  #language en-US "Specifies the base address of the first microcode Patch in the microcode Region."
diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
index 37b3f64e578a..cd912ab0c5ee 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
@@ -61,6 +61,7 @@ [Guids]
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber       ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds      ## SOMETIMES_CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                      ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress            ## CONSUMES
diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
index 82b77b63ea87..1538185ef99a 100644
--- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
+++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
@@ -53,7 +53,8 @@ [LibraryClasses]
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ## CONSUMES
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds      ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber       ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds      ## SOMETIMES_CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                      ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress            ## CONSUMES
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ## CONSUMES
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index d6f84c6f45c0..f1bf8a7ba7cf 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1044,24 +1044,32 @@ WakeUpAP (
       SendInitSipiSipiAllExcludingSelf ((UINT32) ExchangeInfo->BufferStart);
     }
     if (CpuMpData->InitFlag == ApInitConfig) {
-      //
-      // Here support two methods to collect AP count through adjust
-      // PcdCpuApInitTimeOutInMicroSeconds values.
-      //
-      // one way is set a value to just let the first AP to start the
-      // initialization, then through the later while loop to wait all Aps
-      // finsh the initialization.
-      // The other way is set a value to let all APs finished the initialzation.
-      // In this case, the later while loop is useless.
-      //
-      TimedWaitForApFinish (
-        CpuMpData,
-        PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
-        PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
-        );
+      if (PcdGet32 (PcdCpuBootLogicalProcessorNumber) > 0) {
+        TimedWaitForApFinish (
+          CpuMpData,
+          PcdGet32 (PcdCpuBootLogicalProcessorNumber) - 1,
+          MAX_UINT32 // approx. 71 minutes
+          );
+      } else {
+        //
+        // Here support two methods to collect AP count through adjust
+        // PcdCpuApInitTimeOutInMicroSeconds values.
+        //
+        // one way is set a value to just let the first AP to start the
+        // initialization, then through the later while loop to wait all Aps
+        // finsh the initialization.
+        // The other way is set a value to let all APs finished the
+        // initialzation. In this case, the later while loop is useless.
+        //
+        TimedWaitForApFinish (
+          CpuMpData,
+          PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
+          PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
+          );
 
-      while (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) {
-        CpuPause();
+        while (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) {
+          CpuPause();
+        }
       }
     } else {
       //
-- 
2.19.1.3.g30247aa5d201



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

View/Reply Online (#48563): https://edk2.groups.io/g/devel/message/48563
Mute This Topic: https://groups.io/mt/34441228/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP detection
Posted by Dong, Eric 6 years, 4 months ago 
Hi Laszlo,

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Tuesday, October 8, 2019 7:27 PM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Dong, Eric <eric.dong@intel.com>; Igor Mammedov
> <imammedo@redhat.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [edk2-devel] [PATCH 1/4] UefiCpuPkg/MpInitLib: honor the platform's
> boot CPU count in AP detection
> 
> If a platform boots with a CPU topology that is not fully populated (that
> is, the boot CPU count is smaller than PcdCpuMaxLogicalProcessorNumber),
> then the platform cannot use the "fast AP detection" logic added in commit
> 6e1987f19af7. Said logic depends on the boot CPU count being equal to
> PcdCpuMaxLogicalProcessorNumber.
> 
> The platform may not be able to use the variant added in commit
> 0594ec417c89 either, for different reasons; see for example commit
> 861218740d6d.
> 
> Allow platforms to specify the exact boot CPU count, independently of
> PcdCpuMaxLogicalProcessorNumber.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  UefiCpuPkg/UefiCpuPkg.dec                     | 11 +++++
>  UefiCpuPkg/UefiCpuPkg.uni                     |  4 ++
>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  1 +
>  UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |  3 +-
>  UefiCpuPkg/Library/MpInitLib/MpLib.c          | 42 ++++++++++++--------
>  5 files changed, 43 insertions(+), 18 deletions(-)
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index 031a2ccd680a..d6b33fd9b465 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -227,6 +227,17 @@ [PcdsFixedAtBuild, PcdsPatchableInModule,
> PcdsDynamic, PcdsDynamicEx]
>    ## Specifies timeout value in microseconds for the BSP to detect all APs for
> the first time.
>    # @Prompt Timeout for the BSP to detect all APs for the first time.
> 
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000|UI
> NT32|0x00000004
> +  ## Specifies the number of Logical Processors that are available in the
> +  #  preboot environment after platform reset, including BSP and APs. Possible
> +  #  values:<BR><BR>
> +  #  zero (default) - This PCD is ignored, and
> +  #                   PcdCpuApInitTimeOutInMicroSeconds limits the initial AP
> +  #                   detection by the BSP.<BR>
> +  #  nonzero        - PcdCpuApInitTimeOutInMicroSeconds is ignored. The initial
> +  #                   AP detection finishes when the detected CPU count (BSP
> +  #                   plus APs) reaches the value of this PCD.<BR>
> +  # @Prompt Number of Logical Processors available after platform reset.
> +
> gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0|UINT32
> |0x00000008
>    ## Specifies the base address of the first microcode Patch in the microcode
> Region.
>    # @Prompt Microcode Region base address.
> 
> gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress|0x0|UINT64|0x
> 00000005
> diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
> index fbf768072668..a7e279c5cb14 100644
> --- a/UefiCpuPkg/UefiCpuPkg.uni
> +++ b/UefiCpuPkg/UefiCpuPkg.uni
> @@ -37,6 +37,10 @@
> 
>  #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApInitTimeOutInMicroSeconds_HEL
> P  #language en-US "Specifies timeout value in microseconds for the BSP to
> detect all APs for the first time."
> 
> +#string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuBootLogicalProcessorNumber_PRO
> MPT  #language en-US "Number of Logical Processors available after platform
> reset."
> +
> +#string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuBootLogicalProcessorNumber_HELP
> #language en-US "Specifies the number of Logical Processors that are available
> in the preboot environment after platform reset, including BSP and APs."
> +
>  #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuMicrocodePatchAddress_PROMPT
> #language en-US "Microcode Region base address."
> 
>  #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuMicrocodePatchAddress_HELP
> #language en-US "Specifies the base address of the first microcode Patch in
> the microcode Region."
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> index 37b3f64e578a..cd912ab0c5ee 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
> @@ -61,6 +61,7 @@ [Guids]
> 
>  [Pcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ##
> CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber       ##
> CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds      ##
> SOMETIMES_CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                      ## CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress            ##
> CONSUMES
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> index 82b77b63ea87..1538185ef99a 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
> @@ -53,7 +53,8 @@ [LibraryClasses]
> 
>  [Pcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ##
> CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds      ##
> CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber       ##
> CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds      ##
> SOMETIMES_CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                      ## CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress            ##
> CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ##
> CONSUMES
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index d6f84c6f45c0..f1bf8a7ba7cf 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -1044,24 +1044,32 @@ WakeUpAP (
>        SendInitSipiSipiAllExcludingSelf ((UINT32) ExchangeInfo->BufferStart);
>      }
>      if (CpuMpData->InitFlag == ApInitConfig) {
> -      //
> -      // Here support two methods to collect AP count through adjust
> -      // PcdCpuApInitTimeOutInMicroSeconds values.
> -      //
> -      // one way is set a value to just let the first AP to start the
> -      // initialization, then through the later while loop to wait all Aps
> -      // finsh the initialization.
> -      // The other way is set a value to let all APs finished the initialzation.
> -      // In this case, the later while loop is useless.
> -      //
> -      TimedWaitForApFinish (
> -        CpuMpData,
> -        PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
> -        PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
> -        );
> +      if (PcdGet32 (PcdCpuBootLogicalProcessorNumber) > 0) {
> +        TimedWaitForApFinish (
> +          CpuMpData,
> +          PcdGet32 (PcdCpuBootLogicalProcessorNumber) - 1,
> +          MAX_UINT32 // approx. 71 minutes

Do you think if the PcdCpuBootLogicalProcessorNumber is not zero, driver should always wait for all the processors specified by PcdCpuBootLogicalProcessorNumber ready? I just have little concern about the MAX_UINT32 usage. I not sure whether there has a case that some processors can't wake up?

Thanks,
Eric

> +          );
> +      } else {
> +        //
> +        // Here support two methods to collect AP count through adjust
> +        // PcdCpuApInitTimeOutInMicroSeconds values.
> +        //
> +        // one way is set a value to just let the first AP to start the
> +        // initialization, then through the later while loop to wait all Aps
> +        // finsh the initialization.
> +        // The other way is set a value to let all APs finished the
> +        // initialzation. In this case, the later while loop is useless.
> +        //
> +        TimedWaitForApFinish (
> +          CpuMpData,
> +          PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
> +          PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
> +          );
> 
> -      while (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) {
> -        CpuPause();
> +        while (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) {
> +          CpuPause();
> +        }
>        }
>      } else {
>        //
> --
> 2.19.1.3.g30247aa5d201
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> 
> View/Reply Online (#48563): https://edk2.groups.io/g/devel/message/48563
> Mute This Topic: https://groups.io/mt/34441228/1768733
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [eric.dong@intel.com]
> -=-=-=-=-=-=


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

View/Reply Online (#48616): https://edk2.groups.io/g/devel/message/48616
Mute This Topic: https://groups.io/mt/34441228/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP detection
Posted by Laszlo Ersek 6 years, 4 months ago 
On 10/09/19 02:57, Dong, Eric wrote:
> Hi Laszlo,
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>> Laszlo Ersek
>> Sent: Tuesday, October 8, 2019 7:27 PM
>> To: edk2-devel-groups-io <devel@edk2.groups.io>
>> Cc: Dong, Eric <eric.dong@intel.com>; Igor Mammedov
>> <imammedo@redhat.com>; Ni, Ray <ray.ni@intel.com>
>> Subject: [edk2-devel] [PATCH 1/4] UefiCpuPkg/MpInitLib: honor the platform's
>> boot CPU count in AP detection
>>
>> If a platform boots with a CPU topology that is not fully populated (that
>> is, the boot CPU count is smaller than PcdCpuMaxLogicalProcessorNumber),
>> then the platform cannot use the "fast AP detection" logic added in commit
>> 6e1987f19af7. Said logic depends on the boot CPU count being equal to
>> PcdCpuMaxLogicalProcessorNumber.
>>
>> The platform may not be able to use the variant added in commit
>> 0594ec417c89 either, for different reasons; see for example commit
>> 861218740d6d.
>>
>> Allow platforms to specify the exact boot CPU count, independently of
>> PcdCpuMaxLogicalProcessorNumber.
>>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>  UefiCpuPkg/UefiCpuPkg.dec                     | 11 +++++
>>  UefiCpuPkg/UefiCpuPkg.uni                     |  4 ++
>>  UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  1 +
>>  UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |  3 +-
>>  UefiCpuPkg/Library/MpInitLib/MpLib.c          | 42 ++++++++++++--------
>>  5 files changed, 43 insertions(+), 18 deletions(-)
>>
>> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
>> index 031a2ccd680a..d6b33fd9b465 100644
>> --- a/UefiCpuPkg/UefiCpuPkg.dec
>> +++ b/UefiCpuPkg/UefiCpuPkg.dec
>> @@ -227,6 +227,17 @@ [PcdsFixedAtBuild, PcdsPatchableInModule,
>> PcdsDynamic, PcdsDynamicEx]
>>    ## Specifies timeout value in microseconds for the BSP to detect all APs for
>> the first time.
>>    # @Prompt Timeout for the BSP to detect all APs for the first time.
>>
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000|UI
>> NT32|0x00000004
>> +  ## Specifies the number of Logical Processors that are available in the
>> +  #  preboot environment after platform reset, including BSP and APs. Possible
>> +  #  values:<BR><BR>
>> +  #  zero (default) - This PCD is ignored, and
>> +  #                   PcdCpuApInitTimeOutInMicroSeconds limits the initial AP
>> +  #                   detection by the BSP.<BR>
>> +  #  nonzero        - PcdCpuApInitTimeOutInMicroSeconds is ignored. The initial
>> +  #                   AP detection finishes when the detected CPU count (BSP
>> +  #                   plus APs) reaches the value of this PCD.<BR>
>> +  # @Prompt Number of Logical Processors available after platform reset.
>> +
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0|UINT32
>> |0x00000008
>>    ## Specifies the base address of the first microcode Patch in the microcode
>> Region.
>>    # @Prompt Microcode Region base address.
>>
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress|0x0|UINT64|0x
>> 00000005
>> diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
>> index fbf768072668..a7e279c5cb14 100644
>> --- a/UefiCpuPkg/UefiCpuPkg.uni
>> +++ b/UefiCpuPkg/UefiCpuPkg.uni
>> @@ -37,6 +37,10 @@
>>
>>  #string
>> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApInitTimeOutInMicroSeconds_HEL
>> P  #language en-US "Specifies timeout value in microseconds for the BSP to
>> detect all APs for the first time."
>>
>> +#string
>> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuBootLogicalProcessorNumber_PRO
>> MPT  #language en-US "Number of Logical Processors available after platform
>> reset."
>> +
>> +#string
>> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuBootLogicalProcessorNumber_HELP
>> #language en-US "Specifies the number of Logical Processors that are available
>> in the preboot environment after platform reset, including BSP and APs."
>> +
>>  #string
>> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuMicrocodePatchAddress_PROMPT
>> #language en-US "Microcode Region base address."
>>
>>  #string
>> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuMicrocodePatchAddress_HELP
>> #language en-US "Specifies the base address of the first microcode Patch in
>> the microcode Region."
>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>> b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>> index 37b3f64e578a..cd912ab0c5ee 100644
>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>> @@ -61,6 +61,7 @@ [Guids]
>>
>>  [Pcd]
>>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ##
>> CONSUMES
>> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber       ##
>> CONSUMES
>>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds      ##
>> SOMETIMES_CONSUMES
>>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                      ## CONSUMES
>>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress            ##
>> CONSUMES
>> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
>> b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
>> index 82b77b63ea87..1538185ef99a 100644
>> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
>> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
>> @@ -53,7 +53,8 @@ [LibraryClasses]
>>
>>  [Pcd]
>>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber        ##
>> CONSUMES
>> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds      ##
>> CONSUMES
>> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber       ##
>> CONSUMES
>> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds      ##
>> SOMETIMES_CONSUMES
>>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize                      ## CONSUMES
>>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress            ##
>> CONSUMES
>>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize         ##
>> CONSUMES
>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
>> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>> index d6f84c6f45c0..f1bf8a7ba7cf 100644
>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>> @@ -1044,24 +1044,32 @@ WakeUpAP (
>>        SendInitSipiSipiAllExcludingSelf ((UINT32) ExchangeInfo->BufferStart);
>>      }
>>      if (CpuMpData->InitFlag == ApInitConfig) {
>> -      //
>> -      // Here support two methods to collect AP count through adjust
>> -      // PcdCpuApInitTimeOutInMicroSeconds values.
>> -      //
>> -      // one way is set a value to just let the first AP to start the
>> -      // initialization, then through the later while loop to wait all Aps
>> -      // finsh the initialization.
>> -      // The other way is set a value to let all APs finished the initialzation.
>> -      // In this case, the later while loop is useless.
>> -      //
>> -      TimedWaitForApFinish (
>> -        CpuMpData,
>> -        PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
>> -        PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
>> -        );
>> +      if (PcdGet32 (PcdCpuBootLogicalProcessorNumber) > 0) {
>> +        TimedWaitForApFinish (
>> +          CpuMpData,
>> +          PcdGet32 (PcdCpuBootLogicalProcessorNumber) - 1,
>> +          MAX_UINT32 // approx. 71 minutes
> 
> Do you think if the PcdCpuBootLogicalProcessorNumber is not zero, driver should always wait for all the processors specified by PcdCpuBootLogicalProcessorNumber ready?

Yes, it absolutely must.

Control must never advance past this point if at least one of the CPUs
announced in PcdCpuBootLogicalProcessorNumber fails to check in,
regardless of time limit.

> I just have little concern about the MAX_UINT32 usage. I not sure whether there has a case that some processors can't wake up?

In fact, I would have preferred "absolute infinity" over MAX_UINT32
here; MAX_UINT32 (~71 minutes) is just a substitute that's "good enough"
in practice.

So, to confirm: if a platform sets PcdCpuBootLogicalProcessorNumber to a
nonzero value, then the platform *must not boot* at all, if at least one
processor (from the BSP and APs together) fails to wake, from that number.

Speaking generally, if you encounter a timeout situation -- you give up
waiting for the result of a particular action that you initiated --, you
don't know what the end result is going to be. The action may never
finish, or it may very well finish *after* you stop waiting. This is a
general characteristic of all timeouts.

In this specific case, hanging the boot forever (= failing to boot) is
*much* better than having a stray processor wake up after we stop
waiting, and then run amok in no man's land.

It is my explicit goal to remove the timeout condition from this logic,
and to make it block *forever* if at least one CPU fails to check in.

If this policy is not suitable for a platform, then it should not set
the new PCD to a nonzero value. (The default value is zero.)

For OVMF running on QEMU/KVM, this is the right policy however. If one
of your VCPUs fails to check in, then the virtualization stack (QEMU
and/or KVM) under OVMF is busted, and the guest should *not* continue
booting. Continuing would only lead to more misery down the line.

Thanks,
Laszlo


> 
> Thanks,
> Eric
> 
>> +          );
>> +      } else {
>> +        //
>> +        // Here support two methods to collect AP count through adjust
>> +        // PcdCpuApInitTimeOutInMicroSeconds values.
>> +        //
>> +        // one way is set a value to just let the first AP to start the
>> +        // initialization, then through the later while loop to wait all Aps
>> +        // finsh the initialization.
>> +        // The other way is set a value to let all APs finished the
>> +        // initialzation. In this case, the later while loop is useless.
>> +        //
>> +        TimedWaitForApFinish (
>> +          CpuMpData,
>> +          PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1,
>> +          PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds)
>> +          );
>>
>> -      while (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) {
>> -        CpuPause();
>> +        while (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) {
>> +          CpuPause();
>> +        }
>>        }
>>      } else {
>>        //
>> --
>> 2.19.1.3.g30247aa5d201
>>
>>
>>
>> -=-=-=-=-=-=
>> Groups.io Links: You receive all messages sent to this group.
>>
>> View/Reply Online (#48563): https://edk2.groups.io/g/devel/message/48563
>> Mute This Topic: https://groups.io/mt/34441228/1768733
>> Group Owner: devel+owner@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub  [eric.dong@intel.com]
>> -=-=-=-=-=-=
> 


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

View/Reply Online (#48663): https://edk2.groups.io/g/devel/message/48663
Mute This Topic: https://groups.io/mt/34441228/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP detection
Posted by Ni, Ray 6 years, 4 months ago 
Laszlo,
Can you add comments in the code you changed to describe the two different behaviors?

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

View/Reply Online (#48685): https://edk2.groups.io/g/devel/message/48685
Mute This Topic: https://groups.io/mt/34441228/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP detection
Posted by Laszlo Ersek 6 years, 4 months ago 
On 10/10/19 04:52, Ni, Ray wrote:
> Laszlo,
> Can you add comments in the code you changed to describe the two different behaviors?

It's described in the DEC file, near the PCD:

+  #  nonzero        - PcdCpuApInitTimeOutInMicroSeconds is ignored. The initial
+  #                   AP detection finishes when the detected CPU count (BSP
+  #                   plus APs) reaches the value of this PCD.<BR>

(1) Do you want me to repeat this explanation in the code as well?

(2) Do you want me to emphasize in the explanation that, if at least one processor fails to check in, the boot is going to block forever, by design?

(3) Emphasize this in the DEC file only, in the code only, or in both?

Thanks
Laszlo

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

View/Reply Online (#48707): https://edk2.groups.io/g/devel/message/48707
Mute This Topic: https://groups.io/mt/34441228/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP detection
Posted by Laszlo Ersek 6 years, 4 months ago 
On 10/10/19 09:38, Laszlo Ersek wrote:
> On 10/10/19 04:52, Ni, Ray wrote:
>> Laszlo,
>> Can you add comments in the code you changed to describe the two different behaviors?
> 
> It's described in the DEC file, near the PCD:
> 
> +  #  nonzero        - PcdCpuApInitTimeOutInMicroSeconds is ignored. The initial
> +  #                   AP detection finishes when the detected CPU count (BSP
> +  #                   plus APs) reaches the value of this PCD.<BR>
> 
> (1) Do you want me to repeat this explanation in the code as well?
> 
> (2) Do you want me to emphasize in the explanation that, if at least one processor fails to check in, the boot is going to block forever, by design?
> 
> (3) Emphasize this in the DEC file only, in the code only, or in both?

... I think I've figured it out, let me send a v2 of just this patch.

(The rest of the series (for OvmfPkg) will likely be dropped, due to an
alternative proposal from Igor. The UefiCpuPkg facility will remain the
same however.)

Thanks!
Laszlo

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

View/Reply Online (#48711): https://edk2.groups.io/g/devel/message/48711
Mute This Topic: https://groups.io/mt/34441228/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP detection
Posted by Ni, Ray 6 years, 4 months ago 
Laszlo,

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, October 10, 2019 3:39 PM
> To: Ni, Ray <ray.ni@intel.com>; Dong, Eric <eric.dong@intel.com>;
> devel@edk2.groups.io
> Cc: Igor Mammedov <imammedo@redhat.com>
> Subject: Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg/MpInitLib: honor the
> platform's boot CPU count in AP detection
> 
> On 10/10/19 04:52, Ni, Ray wrote:
> > Laszlo,
> > Can you add comments in the code you changed to describe the two
> different behaviors?
> 
> It's described in the DEC file, near the PCD:
> 
> +  #  nonzero        - PcdCpuApInitTimeOutInMicroSeconds is ignored. The
> initial
> +  #                   AP detection finishes when the detected CPU count (BSP
> +  #                   plus APs) reaches the value of this PCD.<BR>
> 
> (1) Do you want me to repeat this explanation in the code as well?
Yes, please😊

> 
> (2) Do you want me to emphasize in the explanation that, if at least one
> processor fails to check in, the boot is going to block forever, by design?

Yes, please😊.

> 
> (3) Emphasize this in the DEC file only, in the code only, or in both?

Code and DEC please.

> 
> Thanks
> Laszlo

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

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.