[PATCH 0/4] UefiCpuPkg, OvmfPkg: separate PCDs for boot CPU count vs. max CPU count

[PATCH 0/4] UefiCpuPkg, OvmfPkg: separate PCDs for boot CPU count vs. max CPU count

[Laszlo Ersek]

Repo:   https://github.com/lersek/edk2.git
Branch: max_cpus_bz_1515
Ref:    https://bugzilla.tianocore.org/show_bug.cgi?id=1515

UefiCpuPkg/MpInitLib currently lacks support for the following use case:

- time-limited AP enumeration is not reliable on the platform
  (individual AP check-in may take arbitrarily long), and

- APs may finish the wakeup routine, and report in to the BSP, in any
  sequence whatsoever, and

- the number of boot CPUs (which is known in advance) is strictly less
  than the number of maximum CPUs (which is also known in advance).

In the above case, the platform cannot tell UefiCpuPkg/MpInitLib to wait
exactly until all boot APs check in. That is, the platform can't request
that the AP enumeration never time out, but also not wait for too long.

For supporting this use case, the patch series introduces
PcdCpuBootLogicalProcessorNumber to UefiCpuPkg, and makes MpInitLib wait
for exactly that many CPUs (= BSP + APs) to show up during CPU
enumeration.

Working towards VCPU hotplug with OVMF, OvmfPkg/PlatformPei fetches both
the boot and the max CPU counts from QEMU, co-operating with the
following QEMU patch set:

  [qemu-devel] [PATCH 0/4] hw/i386: pass "MachineState.smp.max_cpus" to OVMF
  http://mid.mail-archive.com/20191008105259.5378-1-lersek@redhat.com

and passes them to UefiCpuPkg via PcdCpuBootLogicalProcessorNumber and
PcdCpuMaxLogicalProcessorNumber.

As a result, PcdCpuApInitTimeOutInMicroSeconds becomes irrelevant for,
and unused by, OVMF -- time-limited AP enumeration is never going to be
used.

When OVMF is built with -D SMM_REQUIRE, this patch series is just a
small building block, towards the full VCPU hotplug feature. However,
when OVMF is built without -D SMM_REQUIRE, this series (together with
the counterpart patch set for QEMU) completes the VCPU hotplug feature:
it allows S3 resume to work with VCPUs hot-plugged previously (at OS
runtime, of course).

Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Ray Ni <ray.ni@intel.com>

Thanks
Laszlo

Laszlo Ersek (4):
  UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP
    detection
  OvmfPkg/OvmfXen.dsc: remove PcdCpu* dynamic defaults
  OvmfPkg/IndustryStandard: define FW_CFG_X86_TOPOLOGY structure type
  OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU
    hotplug

 OvmfPkg/Include/IndustryStandard/QemuFwCfg.h  | 22 ++++++
 OvmfPkg/OvmfPkgIa32.dsc                       |  2 +-
 OvmfPkg/OvmfPkgIa32X64.dsc                    |  2 +-
 OvmfPkg/OvmfPkgX64.dsc                        |  2 +-
 OvmfPkg/OvmfXen.dsc                           |  4 -
 OvmfPkg/PlatformPei/Platform.c                | 83 +++++++++++++-------
 OvmfPkg/PlatformPei/PlatformPei.inf           |  2 +-
 UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf |  1 +
 UefiCpuPkg/Library/MpInitLib/MpLib.c          | 42 ++++++----
 UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf |  3 +-
 UefiCpuPkg/UefiCpuPkg.dec                     | 11 +++
 UefiCpuPkg/UefiCpuPkg.uni                     |  4 +
 12 files changed, 125 insertions(+), 53 deletions(-)

-- 
2.19.1.3.g30247aa5d201

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

[Laszlo Ersek]

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

[PATCH 2/4] OvmfPkg/OvmfXen.dsc: remove PcdCpu* dynamic defaults

[Laszlo Ersek]

PcdCpuMaxLogicalProcessorNumber and PcdCpuApInitTimeOutInMicroSeconds are
only referenced in "OvmfPkg/PlatformPei/PlatformPei.inf", and OvmfXen does
not include that module. Remove the unnecessary dynamic PCD defaults from
"OvmfXen.dsc".

Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@arm.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/OvmfXen.dsc | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 5a31f75f05d0..6deafea034c0 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -481,10 +481,6 @@ [PcdsDynamicDefault]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
   gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
 
-  # UefiCpuPkg PCDs related to initial AP bringup and general AP management.
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
-
   # Set memory encryption mask
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
 
-- 
2.19.1.3.g30247aa5d201

[PATCH 3/4] OvmfPkg/IndustryStandard: define FW_CFG_X86_TOPOLOGY structure type

[Laszlo Ersek]

pc-q35-4.2+ and pc-i440fx-4.2+ machine types report CPU topology info in
the "etc/x86-smp-topology" fw_cfg file. Add a structure type for parsing
this blob.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/Include/IndustryStandard/QemuFwCfg.h | 22 ++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/OvmfPkg/Include/IndustryStandard/QemuFwCfg.h b/OvmfPkg/Include/IndustryStandard/QemuFwCfg.h
index 7969aba3ca61..fe410d4a3fc2 100644
--- a/OvmfPkg/Include/IndustryStandard/QemuFwCfg.h
+++ b/OvmfPkg/Include/IndustryStandard/QemuFwCfg.h
@@ -95,4 +95,26 @@ typedef struct {
 } FW_CFG_DMA_ACCESS;
 #pragma pack ()
 
+//
+// Structure describing the VCPU topology on IA32 and X64, in the fw_cfg file
+// named "etc/x86-smp-topology".
+//
+// All fields are little endian.
+//
+// The number of sockets (aka packages) can be calculated by dividing
+// MaxLogicalProcessors by (DiesPerSocket * CoresPerDie * ThreadsPerCore), and
+// rounding up the quotient.
+//
+// APIC ID field widths and offsets can be derived with the standard method
+// described in "Intel(R) 64 Architecture Processor Topology Enumeration"
+// <http://software.intel.com/en-us/articles/intel-64-architecture-processor-topology-enumeration/>.
+//
+#pragma pack (1)
+typedef struct {
+  UINT32 DiesPerSocket;
+  UINT32 CoresPerDie;
+  UINT32 ThreadsPerCore;
+  UINT32 MaxLogicalProcessors;
+} FW_CFG_X86_TOPOLOGY;
+#pragma pack ()
 #endif
-- 
2.19.1.3.g30247aa5d201

[PATCH 4/4] OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug

[Laszlo Ersek]

MaxCpuCountInitialization() currently handles the following options:

(1) QEMU does not report the boot CPU count.

    In this case, PlatformPei makes MpInitLib enumerate APs up to the
    default PcdCpuMaxLogicalProcessorNumber value (64) minus 1, or until
    the default PcdCpuApInitTimeOutInMicroSeconds (50,000) elapses.
    (Whichever is reached first.)

    Time-limited AP enumeration had never been reliable on QEMU/KVM, which
    is why commit 45a70db3c3a5 strated handling case (2) below, in OVMF.

(2) QEMU reports the boot CPU count.

    In this case, PlatformPei sets PcdCpuMaxLogicalProcessorNumber to the
    reported boot CPU count, and PcdCpuApInitTimeOutInMicroSeconds to
    practically "infinity" (MAX_UINT32, ~71 minutes). That causes
    MpInitLib to enumerate exactly the available (boot) APs.

    With CPU hotplug in mind, this method is not good enough. While
    UefiCpuPkg expects PcdCpuMaxLogicalProcessorNumber to include both
    boot (i.e., cold-plugged), and all *potentially* hot-plugged, logical
    processors, the boot CPU count reported by QEMU does not cover the
    second category.

Rewrite MaxCpuCountInitialization() for handling the following cases:

(1) The behavior of case (1) does not change. (No UefiCpuPkg PCDs are set
    to values different from the defaults.)

(2) QEMU reports the boot CPU count, but not the potential maximum CPU
    count.

    In this case, the behavior remains unchanged.

    The way MpInitLib is instructed to do the same differs however: we now
    set the new PcdCpuBootLogicalProcessorNumber to the boot CPU count
    (together with PcdCpuMaxLogicalProcessorNumber).
    PcdCpuApInitTimeOutInMicroSeconds is irrelevant.

(3) QEMU reports both the boot CPU count, and the potential maximum CPU
    count.

    We tell UefiCpuPkg about the potential maximum through
    PcdCpuMaxLogicalProcessorNumber. We also tell MpInitLib the boot CPU
    count for precise and quick AP enumeration, via
    PcdCpuBootLogicalProcessorNumber. PcdCpuApInitTimeOutInMicroSeconds is
    irrelevant.

This patch is a pre-requisite for enabling CPU hotplug with SMM_REQUIRE.
As a side effect, it also enables S3 to work with CPU hotplug at once,
*without* SMM_REQUIRE.

Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien.grall@arm.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>
---
 OvmfPkg/OvmfPkgIa32.dsc             |  2 +-
 OvmfPkg/OvmfPkgIa32X64.dsc          |  2 +-
 OvmfPkg/OvmfPkgX64.dsc              |  2 +-
 OvmfPkg/PlatformPei/PlatformPei.inf |  2 +-
 OvmfPkg/PlatformPei/Platform.c      | 83 +++++++++++++-------
 5 files changed, 60 insertions(+), 31 deletions(-)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 66e944436a69..89b1fc41458d 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -554,7 +554,7 @@ [PcdsDynamicDefault]
 
   # UefiCpuPkg PCDs related to initial AP bringup and general AP management.
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0
 
   # Set memory encryption mask
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 51c2bfb44f14..f978d27a0b55 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -566,7 +566,7 @@ [PcdsDynamicDefault]
 
   # UefiCpuPkg PCDs related to initial AP bringup and general AP management.
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0
 
   # Set memory encryption mask
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index ba7a75884490..04604ed6b35b 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -565,7 +565,7 @@ [PcdsDynamicDefault]
 
   # UefiCpuPkg PCDs related to initial AP bringup and general AP management.
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0
 
   # Set memory encryption mask
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index d9fd9c8f05b3..30eaebdfae63 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -98,7 +98,7 @@ [Pcd]
   gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy
   gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
 
 [FixedPcd]
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 3ba2459872d9..12cec1d60ec1 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -564,43 +564,72 @@ S3Verification (
 
 
 /**
-  Fetch the number of boot CPUs from QEMU and expose it to UefiCpuPkg modules.
-  Set the mMaxCpuCount variable.
+  Fetch the boot CPU and max CPU numbers from QEMU, and expose them to
+  UefiCpuPkg modules. Set the mMaxCpuCount variable.
 **/
 VOID
 MaxCpuCountInitialization (
   VOID
   )
 {
-  UINT16        ProcessorCount;
-  RETURN_STATUS PcdStatus;
+  UINT16        BootCpuCount;
+  RETURN_STATUS Status;
 
+  //
+  // Try to fetch the boot CPU count.
+  //
   QemuFwCfgSelectItem (QemuFwCfgItemSmpCpuCount);
-  ProcessorCount = QemuFwCfgRead16 ();
-  //
-  // If the fw_cfg key or fw_cfg entirely is unavailable, load mMaxCpuCount
-  // from the PCD default. No change to PCDs.
-  //
-  if (ProcessorCount == 0) {
+  BootCpuCount = QemuFwCfgRead16 ();
+  if (BootCpuCount == 0) {
+    //
+    // QEMU doesn't report the boot CPU count. Let MpInitLib count APs up to
+    // (PcdCpuMaxLogicalProcessorNumber-1), or until
+    // PcdCpuApInitTimeOutInMicroSeconds elapses (whichever is reached first).
+    //
+    DEBUG ((DEBUG_WARN, "%a: boot CPU count unavailable\n", __FUNCTION__));
     mMaxCpuCount = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
-    return;
+  } else {
+    //
+    // We will expose BootCpuCount to MpInitLib. MpInitLib will count APs up to
+    // (BootCpuCount-1) precisely, regardless of timeout.
+    //
+    // Now try to fetch topology info.
+    //
+    FIRMWARE_CONFIG_ITEM FwCfgItem;
+    UINTN                FwCfgSize;
+    FW_CFG_X86_TOPOLOGY  Topology;
+
+    Status = QemuFwCfgFindFile ("etc/x86-smp-topology", &FwCfgItem,
+               &FwCfgSize);
+    if (RETURN_ERROR (Status) || FwCfgSize != sizeof Topology) {
+      //
+      // QEMU doesn't report the topology. Assume that the maximum CPU count
+      // equals the boot CPU count (implying no hotplug).
+      //
+      DEBUG ((DEBUG_WARN, "%a: topology unavailable\n", __FUNCTION__));
+      mMaxCpuCount = BootCpuCount;
+    } else {
+      //
+      // Grab the maximum CPU count from the topology info.
+      //
+      QemuFwCfgSelectItem (FwCfgItem);
+      QemuFwCfgReadBytes (FwCfgSize, &Topology);
+      DEBUG ((DEBUG_VERBOSE,
+        "%a: DiesPerSocket=%u CoresPerDie=%u ThreadsPerCore=%u\n",
+        __FUNCTION__, Topology.DiesPerSocket, Topology.CoresPerDie,
+        Topology.ThreadsPerCore));
+      mMaxCpuCount = Topology.MaxLogicalProcessors;
+    }
   }
-  //
-  // Otherwise, set mMaxCpuCount to the value reported by QEMU.
-  //
-  mMaxCpuCount = ProcessorCount;
-  //
-  // Additionally, tell UefiCpuPkg modules (a) the exact number of VCPUs, (b)
-  // to wait, in the initial AP bringup, exactly as long as it takes for all of
-  // the APs to report in. For this, we set the longest representable timeout
-  // (approx. 71 minutes).
-  //
-  PcdStatus = PcdSet32S (PcdCpuMaxLogicalProcessorNumber, ProcessorCount);
-  ASSERT_RETURN_ERROR (PcdStatus);
-  PcdStatus = PcdSet32S (PcdCpuApInitTimeOutInMicroSeconds, MAX_UINT32);
-  ASSERT_RETURN_ERROR (PcdStatus);
-  DEBUG ((DEBUG_INFO, "%a: QEMU reports %d processor(s)\n", __FUNCTION__,
-    ProcessorCount));
+
+  DEBUG ((DEBUG_INFO, "%a: BootCpuCount=%d mMaxCpuCount=%u\n", __FUNCTION__,
+    BootCpuCount, mMaxCpuCount));
+  ASSERT (BootCpuCount <= mMaxCpuCount);
+
+  Status = PcdSet32S (PcdCpuBootLogicalProcessorNumber, BootCpuCount);
+  ASSERT_RETURN_ERROR (Status);
+  Status = PcdSet32S (PcdCpuMaxLogicalProcessorNumber, mMaxCpuCount);
+  ASSERT_RETURN_ERROR (Status);
 }
 
 
-- 
2.19.1.3.g30247aa5d201

Re: [PATCH 0/4] UefiCpuPkg, OvmfPkg: separate PCDs for boot CPU count vs. max CPU count

[Ard Biesheuvel]

On Tue, 8 Oct 2019 at 13:27, Laszlo Ersek <lersek@redhat.com> wrote:
>
> Repo:   https://github.com/lersek/edk2.git
> Branch: max_cpus_bz_1515
> Ref:    https://bugzilla.tianocore.org/show_bug.cgi?id=1515
>
> UefiCpuPkg/MpInitLib currently lacks support for the following use case:
>
> - time-limited AP enumeration is not reliable on the platform
>   (individual AP check-in may take arbitrarily long), and
>
> - APs may finish the wakeup routine, and report in to the BSP, in any
>   sequence whatsoever, and
>
> - the number of boot CPUs (which is known in advance) is strictly less
>   than the number of maximum CPUs (which is also known in advance).
>
> In the above case, the platform cannot tell UefiCpuPkg/MpInitLib to wait
> exactly until all boot APs check in. That is, the platform can't request
> that the AP enumeration never time out, but also not wait for too long.
>
> For supporting this use case, the patch series introduces
> PcdCpuBootLogicalProcessorNumber to UefiCpuPkg, and makes MpInitLib wait
> for exactly that many CPUs (= BSP + APs) to show up during CPU
> enumeration.
>
> Working towards VCPU hotplug with OVMF, OvmfPkg/PlatformPei fetches both
> the boot and the max CPU counts from QEMU, co-operating with the
> following QEMU patch set:
>
>   [qemu-devel] [PATCH 0/4] hw/i386: pass "MachineState.smp.max_cpus" to OVMF
>   http://mid.mail-archive.com/20191008105259.5378-1-lersek@redhat.com
>
> and passes them to UefiCpuPkg via PcdCpuBootLogicalProcessorNumber and
> PcdCpuMaxLogicalProcessorNumber.
>
> As a result, PcdCpuApInitTimeOutInMicroSeconds becomes irrelevant for,
> and unused by, OVMF -- time-limited AP enumeration is never going to be
> used.
>
> When OVMF is built with -D SMM_REQUIRE, this patch series is just a
> small building block, towards the full VCPU hotplug feature. However,
> when OVMF is built without -D SMM_REQUIRE, this series (together with
> the counterpart patch set for QEMU) completes the VCPU hotplug feature:
> it allows S3 resume to work with VCPUs hot-plugged previously (at OS
> runtime, of course).
>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Cc: Ray Ni <ray.ni@intel.com>
>
> Thanks
> Laszlo
>
> Laszlo Ersek (4):
>   UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP
>     detection

Assuming this ^^^ patch gets accepted by the maintainers,

>   OvmfPkg/OvmfXen.dsc: remove PcdCpu* dynamic defaults
>   OvmfPkg/IndustryStandard: define FW_CFG_X86_TOPOLOGY structure type
>   OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU
>     hotplug
>

Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

for the OvmfPkg changes.

Re: [edk2-devel] [PATCH 2/4] OvmfPkg/OvmfXen.dsc: remove PcdCpu* dynamic defaults

[Philippe Mathieu-Daudé]

On 10/8/19 1:27 PM, Laszlo Ersek wrote:
> PcdCpuMaxLogicalProcessorNumber and PcdCpuApInitTimeOutInMicroSeconds are
> only referenced in "OvmfPkg/PlatformPei/PlatformPei.inf", and OvmfXen does
> not include that module. Remove the unnecessary dynamic PCD defaults from
> "OvmfXen.dsc".
> 
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien.grall@arm.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>   OvmfPkg/OvmfXen.dsc | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index 5a31f75f05d0..6deafea034c0 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.dsc
> @@ -481,10 +481,6 @@ [PcdsDynamicDefault]
>     gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
>     gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
>   
> -  # UefiCpuPkg PCDs related to initial AP bringup and general AP management.
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
> -
>     # Set memory encryption mask
>     gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
>   
> 

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Re: [PATCH 4/4] OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug

[Igor Mammedov]

On Tue,  8 Oct 2019 13:27:14 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> MaxCpuCountInitialization() currently handles the following options:
> 
> (1) QEMU does not report the boot CPU count.
> 
>     In this case, PlatformPei makes MpInitLib enumerate APs up to the
>     default PcdCpuMaxLogicalProcessorNumber value (64) minus 1, or until
>     the default PcdCpuApInitTimeOutInMicroSeconds (50,000) elapses.
>     (Whichever is reached first.)
> 
>     Time-limited AP enumeration had never been reliable on QEMU/KVM, which
>     is why commit 45a70db3c3a5 strated handling case (2) below, in OVMF.
> 
> (2) QEMU reports the boot CPU count.
> 
>     In this case, PlatformPei sets PcdCpuMaxLogicalProcessorNumber to the
>     reported boot CPU count, and PcdCpuApInitTimeOutInMicroSeconds to
>     practically "infinity" (MAX_UINT32, ~71 minutes). That causes
>     MpInitLib to enumerate exactly the available (boot) APs.
> 
>     With CPU hotplug in mind, this method is not good enough. While
>     UefiCpuPkg expects PcdCpuMaxLogicalProcessorNumber to include both
>     boot (i.e., cold-plugged), and all *potentially* hot-plugged, logical
>     processors, the boot CPU count reported by QEMU does not cover the
>     second category.

Can you elaborate why it doesn't cover the second category?



> Rewrite MaxCpuCountInitialization() for handling the following cases:
> 
> (1) The behavior of case (1) does not change. (No UefiCpuPkg PCDs are set
>     to values different from the defaults.)
> 
> (2) QEMU reports the boot CPU count, but not the potential maximum CPU
>     count.
> 
>     In this case, the behavior remains unchanged.
> 
>     The way MpInitLib is instructed to do the same differs however: we now
>     set the new PcdCpuBootLogicalProcessorNumber to the boot CPU count
>     (together with PcdCpuMaxLogicalProcessorNumber).
>     PcdCpuApInitTimeOutInMicroSeconds is irrelevant.
> 
> (3) QEMU reports both the boot CPU count, and the potential maximum CPU
>     count.
> 
>     We tell UefiCpuPkg about the potential maximum through
>     PcdCpuMaxLogicalProcessorNumber. We also tell MpInitLib the boot CPU
>     count for precise and quick AP enumeration, via
>     PcdCpuBootLogicalProcessorNumber. PcdCpuApInitTimeOutInMicroSeconds is
>     irrelevant.
What for max cpu count is/will be used?

> 
> This patch is a pre-requisite for enabling CPU hotplug with SMM_REQUIRE.
> As a side effect, it also enables S3 to work with CPU hotplug at once,
> *without* SMM_REQUIRE.
Does OVMF reread boot CPU count on resume from QEMU?


> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Julien Grall <julien.grall@arm.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>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc             |  2 +-
>  OvmfPkg/OvmfPkgIa32X64.dsc          |  2 +-
>  OvmfPkg/OvmfPkgX64.dsc              |  2 +-
>  OvmfPkg/PlatformPei/PlatformPei.inf |  2 +-
>  OvmfPkg/PlatformPei/Platform.c      | 83 +++++++++++++-------
>  5 files changed, 60 insertions(+), 31 deletions(-)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 66e944436a69..89b1fc41458d 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -554,7 +554,7 @@ [PcdsDynamicDefault]
>  
>    # UefiCpuPkg PCDs related to initial AP bringup and general AP management.
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0
>  
>    # Set memory encryption mask
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 51c2bfb44f14..f978d27a0b55 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -566,7 +566,7 @@ [PcdsDynamicDefault]
>  
>    # UefiCpuPkg PCDs related to initial AP bringup and general AP management.
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0
>  
>    # Set memory encryption mask
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index ba7a75884490..04604ed6b35b 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -565,7 +565,7 @@ [PcdsDynamicDefault]
>  
>    # UefiCpuPkg PCDs related to initial AP bringup and general AP management.
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0
>  
>    # Set memory encryption mask
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
> index d9fd9c8f05b3..30eaebdfae63 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -98,7 +98,7 @@ [Pcd]
>    gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
>  
>  [FixedPcd]
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 3ba2459872d9..12cec1d60ec1 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -564,43 +564,72 @@ S3Verification (
>  
>  
>  /**
> -  Fetch the number of boot CPUs from QEMU and expose it to UefiCpuPkg modules.
> -  Set the mMaxCpuCount variable.
> +  Fetch the boot CPU and max CPU numbers from QEMU, and expose them to
> +  UefiCpuPkg modules. Set the mMaxCpuCount variable.
>  **/
>  VOID
>  MaxCpuCountInitialization (
>    VOID
>    )
>  {
> -  UINT16        ProcessorCount;
> -  RETURN_STATUS PcdStatus;
> +  UINT16        BootCpuCount;
> +  RETURN_STATUS Status;
>  
> +  //
> +  // Try to fetch the boot CPU count.
> +  //
>    QemuFwCfgSelectItem (QemuFwCfgItemSmpCpuCount);
> -  ProcessorCount = QemuFwCfgRead16 ();
> -  //
> -  // If the fw_cfg key or fw_cfg entirely is unavailable, load mMaxCpuCount
> -  // from the PCD default. No change to PCDs.
> -  //
> -  if (ProcessorCount == 0) {
> +  BootCpuCount = QemuFwCfgRead16 ();
> +  if (BootCpuCount == 0) {
> +    //
> +    // QEMU doesn't report the boot CPU count. Let MpInitLib count APs up to
> +    // (PcdCpuMaxLogicalProcessorNumber-1), or until
> +    // PcdCpuApInitTimeOutInMicroSeconds elapses (whichever is reached first).
> +    //
> +    DEBUG ((DEBUG_WARN, "%a: boot CPU count unavailable\n", __FUNCTION__));
>      mMaxCpuCount = PcdGet32 (PcdCpuMaxLogicalProcessorNumber);
> -    return;
> +  } else {
> +    //
> +    // We will expose BootCpuCount to MpInitLib. MpInitLib will count APs up to
> +    // (BootCpuCount-1) precisely, regardless of timeout.
> +    //
> +    // Now try to fetch topology info.
> +    //
> +    FIRMWARE_CONFIG_ITEM FwCfgItem;
> +    UINTN                FwCfgSize;
> +    FW_CFG_X86_TOPOLOGY  Topology;
> +
> +    Status = QemuFwCfgFindFile ("etc/x86-smp-topology", &FwCfgItem,
> +               &FwCfgSize);
> +    if (RETURN_ERROR (Status) || FwCfgSize != sizeof Topology) {
> +      //
> +      // QEMU doesn't report the topology. Assume that the maximum CPU count
> +      // equals the boot CPU count (implying no hotplug).
> +      //
> +      DEBUG ((DEBUG_WARN, "%a: topology unavailable\n", __FUNCTION__));
> +      mMaxCpuCount = BootCpuCount;
> +    } else {
> +      //
> +      // Grab the maximum CPU count from the topology info.
> +      //
> +      QemuFwCfgSelectItem (FwCfgItem);
> +      QemuFwCfgReadBytes (FwCfgSize, &Topology);
> +      DEBUG ((DEBUG_VERBOSE,
> +        "%a: DiesPerSocket=%u CoresPerDie=%u ThreadsPerCore=%u\n",
> +        __FUNCTION__, Topology.DiesPerSocket, Topology.CoresPerDie,
> +        Topology.ThreadsPerCore));
> +      mMaxCpuCount = Topology.MaxLogicalProcessors;
> +    }
>    }
> -  //
> -  // Otherwise, set mMaxCpuCount to the value reported by QEMU.
> -  //
> -  mMaxCpuCount = ProcessorCount;
> -  //
> -  // Additionally, tell UefiCpuPkg modules (a) the exact number of VCPUs, (b)
> -  // to wait, in the initial AP bringup, exactly as long as it takes for all of
> -  // the APs to report in. For this, we set the longest representable timeout
> -  // (approx. 71 minutes).
> -  //
> -  PcdStatus = PcdSet32S (PcdCpuMaxLogicalProcessorNumber, ProcessorCount);
> -  ASSERT_RETURN_ERROR (PcdStatus);
> -  PcdStatus = PcdSet32S (PcdCpuApInitTimeOutInMicroSeconds, MAX_UINT32);
> -  ASSERT_RETURN_ERROR (PcdStatus);
> -  DEBUG ((DEBUG_INFO, "%a: QEMU reports %d processor(s)\n", __FUNCTION__,
> -    ProcessorCount));
> +
> +  DEBUG ((DEBUG_INFO, "%a: BootCpuCount=%d mMaxCpuCount=%u\n", __FUNCTION__,
> +    BootCpuCount, mMaxCpuCount));
> +  ASSERT (BootCpuCount <= mMaxCpuCount);
> +
> +  Status = PcdSet32S (PcdCpuBootLogicalProcessorNumber, BootCpuCount);
> +  ASSERT_RETURN_ERROR (Status);
> +  Status = PcdSet32S (PcdCpuMaxLogicalProcessorNumber, mMaxCpuCount);
> +  ASSERT_RETURN_ERROR (Status);
>  }
>  
>  

Re: [PATCH 0/4] UefiCpuPkg, OvmfPkg: separate PCDs for boot CPU count vs. max CPU count

[Laszlo Ersek]

On 10/08/19 13:35, Ard Biesheuvel wrote:
> On Tue, 8 Oct 2019 at 13:27, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: max_cpus_bz_1515
>> Ref:    https://bugzilla.tianocore.org/show_bug.cgi?id=1515
>>
>> UefiCpuPkg/MpInitLib currently lacks support for the following use case:
>>
>> - time-limited AP enumeration is not reliable on the platform
>>   (individual AP check-in may take arbitrarily long), and
>>
>> - APs may finish the wakeup routine, and report in to the BSP, in any
>>   sequence whatsoever, and
>>
>> - the number of boot CPUs (which is known in advance) is strictly less
>>   than the number of maximum CPUs (which is also known in advance).
>>
>> In the above case, the platform cannot tell UefiCpuPkg/MpInitLib to wait
>> exactly until all boot APs check in. That is, the platform can't request
>> that the AP enumeration never time out, but also not wait for too long.
>>
>> For supporting this use case, the patch series introduces
>> PcdCpuBootLogicalProcessorNumber to UefiCpuPkg, and makes MpInitLib wait
>> for exactly that many CPUs (= BSP + APs) to show up during CPU
>> enumeration.
>>
>> Working towards VCPU hotplug with OVMF, OvmfPkg/PlatformPei fetches both
>> the boot and the max CPU counts from QEMU, co-operating with the
>> following QEMU patch set:
>>
>>   [qemu-devel] [PATCH 0/4] hw/i386: pass "MachineState.smp.max_cpus" to OVMF
>>   http://mid.mail-archive.com/20191008105259.5378-1-lersek@redhat.com
>>
>> and passes them to UefiCpuPkg via PcdCpuBootLogicalProcessorNumber and
>> PcdCpuMaxLogicalProcessorNumber.
>>
>> As a result, PcdCpuApInitTimeOutInMicroSeconds becomes irrelevant for,
>> and unused by, OVMF -- time-limited AP enumeration is never going to be
>> used.
>>
>> When OVMF is built with -D SMM_REQUIRE, this patch series is just a
>> small building block, towards the full VCPU hotplug feature. However,
>> when OVMF is built without -D SMM_REQUIRE, this series (together with
>> the counterpart patch set for QEMU) completes the VCPU hotplug feature:
>> it allows S3 resume to work with VCPUs hot-plugged previously (at OS
>> runtime, of course).
>>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Julien Grall <julien.grall@arm.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (4):
>>   UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP
>>     detection
> 
> Assuming this ^^^ patch gets accepted by the maintainers,
> 
>>   OvmfPkg/OvmfXen.dsc: remove PcdCpu* dynamic defaults
>>   OvmfPkg/IndustryStandard: define FW_CFG_X86_TOPOLOGY structure type
>>   OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU
>>     hotplug
>>
> 
> Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> for the OvmfPkg changes.
> 

Thanks!
Laszlo

Re: [edk2-devel] [PATCH 2/4] OvmfPkg/OvmfXen.dsc: remove PcdCpu* dynamic defaults

[Laszlo Ersek]

On 10/08/19 15:11, Philippe Mathieu-Daudé wrote:
> On 10/8/19 1:27 PM, Laszlo Ersek wrote:
>> PcdCpuMaxLogicalProcessorNumber and PcdCpuApInitTimeOutInMicroSeconds are
>> only referenced in "OvmfPkg/PlatformPei/PlatformPei.inf", and OvmfXen
>> does
>> not include that module. Remove the unnecessary dynamic PCD defaults from
>> "OvmfXen.dsc".
>>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Julien Grall <julien.grall@arm.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>   OvmfPkg/OvmfXen.dsc | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
>> index 5a31f75f05d0..6deafea034c0 100644
>> --- a/OvmfPkg/OvmfXen.dsc
>> +++ b/OvmfPkg/OvmfXen.dsc
>> @@ -481,10 +481,6 @@ [PcdsDynamicDefault]
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
>>   -  # UefiCpuPkg PCDs related to initial AP bringup and general AP
>> management.
>> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64
>> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000
>> -
>>     # Set memory encryption mask
>>    
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
>>  
> 
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Thanks!

Re: [PATCH 4/4] OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug

[Laszlo Ersek]

On 10/08/19 17:06, Igor Mammedov wrote:
> On Tue,  8 Oct 2019 13:27:14 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
>
>> MaxCpuCountInitialization() currently handles the following options:
>>
>> (1) QEMU does not report the boot CPU count.
>>
>>     In this case, PlatformPei makes MpInitLib enumerate APs up to the
>>     default PcdCpuMaxLogicalProcessorNumber value (64) minus 1, or until
>>     the default PcdCpuApInitTimeOutInMicroSeconds (50,000) elapses.
>>     (Whichever is reached first.)
>>
>>     Time-limited AP enumeration had never been reliable on QEMU/KVM, which
>>     is why commit 45a70db3c3a5 strated handling case (2) below, in OVMF.
>>
>> (2) QEMU reports the boot CPU count.
>>
>>     In this case, PlatformPei sets PcdCpuMaxLogicalProcessorNumber to the
>>     reported boot CPU count, and PcdCpuApInitTimeOutInMicroSeconds to
>>     practically "infinity" (MAX_UINT32, ~71 minutes). That causes
>>     MpInitLib to enumerate exactly the available (boot) APs.
>>
>>     With CPU hotplug in mind, this method is not good enough. While
>>     UefiCpuPkg expects PcdCpuMaxLogicalProcessorNumber to include both
>>     boot (i.e., cold-plugged), and all *potentially* hot-plugged, logical
>>     processors, the boot CPU count reported by QEMU does not cover the
>>     second category.
>
> Can you elaborate why it doesn't cover the second category?

(Elaborate just in this thread, or elaborate in the commit message?)

The boot CPU count reported by QEMU does not cover all potentially
hot-plugged logical processors ... because it is not supposed to.

Namely, FW_CFG_NB_CPUS exposes "pcms->boot_cpus", and that is not
supposed to cover all potentially hot-plugged CPUs, only the boot-time
CPUs.

Whereas PcdCpuMaxLogicalProcessorNumber needs to include all potentially
hot-plugged CPUs too. For that, we need to expose "ms->smp.max_cpus"
from QEMU.

Does this answer your question?

>> Rewrite MaxCpuCountInitialization() for handling the following cases:
>>
>> (1) The behavior of case (1) does not change. (No UefiCpuPkg PCDs are set
>>     to values different from the defaults.)
>>
>> (2) QEMU reports the boot CPU count, but not the potential maximum CPU
>>     count.
>>
>>     In this case, the behavior remains unchanged.
>>
>>     The way MpInitLib is instructed to do the same differs however: we now
>>     set the new PcdCpuBootLogicalProcessorNumber to the boot CPU count
>>     (together with PcdCpuMaxLogicalProcessorNumber).
>>     PcdCpuApInitTimeOutInMicroSeconds is irrelevant.
>>
>> (3) QEMU reports both the boot CPU count, and the potential maximum CPU
>>     count.
>>
>>     We tell UefiCpuPkg about the potential maximum through
>>     PcdCpuMaxLogicalProcessorNumber. We also tell MpInitLib the boot CPU
>>     count for precise and quick AP enumeration, via
>>     PcdCpuBootLogicalProcessorNumber. PcdCpuApInitTimeOutInMicroSeconds is
>>     irrelevant.
> What for max cpu count is/will be used?

Before this patch:

- "pcms->boot_cpus" determines the boot CPU count for OVMF
- "pcms->boot_cpus" determines the maximum CPU count for OVMF

- when OVMF is built without SMM_REQUIRE:
  - OS-time CPU hotplug works (no firmware involvement)
  - after OS-time CPU hotplug, S3 resume breaks (firmware can't handle
    unexpected CPUs appearing beyond original maximum)

- when OVMF is built with SMM_REQUIRE:
  - OS-time CPU hotplug does not work (yet)

After this patch:

- "pcms->boot_cpus" determines the boot CPU count for OVMF
- "ms->smp.max_cpus" determines the maximum CPU count for OVMF [*]

- when OVMF is built without SMM_REQUIRE:
  - OS-time CPU hotplug works (no firmware involvement)
  - after OS-time CPU hotplug, S3 resume works (any new CPUs are
    within the original maximum, therefore they are expected) [*]

- when OVMF is built with SMM_REQUIRE:
  - OS-time CPU hotplug does not work (yet)

The differences are marked with [*].

>> This patch is a pre-requisite for enabling CPU hotplug with SMM_REQUIRE.
>> As a side effect, it also enables S3 to work with CPU hotplug at once,
>> *without* SMM_REQUIRE.
> Does OVMF reread boot CPU count on resume from QEMU?

Yes, it does.

When a CPU is hotplugged at OS runtime, "pcms->boot_cpus" increases in
QEMU.

Before the patch, the "pcms->boot_cpus" increase causes
PcdCpuMaxLogicalProcessorNumber to increase as well. That breaks S3
resume. PcdCpuMaxLogicalProcessorNumber must not change from initial
boot to S3 resume.

After the patch, the "pcms->boot_cpus" increase does not increase
PcdCpuMaxLogicalProcessorNumber. PcdCpuMaxLogicalProcessorNumber remains
the same (namely, "ms->smp.max_cpus"). Therefore, the CPU structures
allocated during normal boot, for "ms->smp.max_cpus", will accommodate
the CPUs that have been hotplugged prior to S3 resume.

I tested the S3 behavior, plus the values mentioned above are also
logged to the OVMF debug log, by this patch:

> +  DEBUG ((DEBUG_INFO, "%a: BootCpuCount=%d mMaxCpuCount=%u\n", __FUNCTION__,
> +    BootCpuCount, mMaxCpuCount));

This is one libvirt domain XML snippet I've used for testing:

  <vcpu placement='static' current='3'>4</vcpu>
  <vcpus>
    <vcpu id='0' enabled='yes' hotpluggable='no'/>
    <vcpu id='1' enabled='yes' hotpluggable='no'/>
    <vcpu id='2' enabled='yes' hotpluggable='no'/>
    <vcpu id='3' enabled='no' hotpluggable='yes'/>
  </vcpus>
  <os>
    <type arch='x86_64' machine='pc-i440fx-4.2'>hvm</type>
...
  <cpu mode='custom' match='exact' check='partial'>
    <model fallback='allow'>Haswell-noTSX</model>
    <topology sockets='1' cores='2' threads='2'/>
    <feature policy='require' name='vmx'/>
    <feature policy='require' name='pcid'/>
    <feature policy='require' name='spec-ctrl'/>
    <feature policy='require' name='pdpe1gb'/>
  </cpu>

And the command used for testing was:

$ virsh setvcpu GUEST 3 --enable --live

When this is done, the guest kernel dmesg confirms the CPU hotplug, but
it is not onlined automatically. I onlined it via /sys manually in the
guest.

After that point, I exercised S3 suspend / resume. During S3 resume, the
"BootCpuCount" log (from above) increases from 3 to 4 (and the related
edk2 infrastructure messages are in sync). All the while "mMaxCpuCount"
preserves its initial value, 4.

Finally, after S3 resume, I ran "rdmsr -a 0x3a" in the guest, which
printed "5" for all *four* CPUs. This confirms that OVMF has set the
Feature Control MSR on the new VCPU too, during S3 resume.

(The MSR is zeroed on platform reset, and has to be re-configured by
firmware, on every CPU, during S3. It is set to 5 due to the "vmx" CPU
feature in the above domain XML snippet. References:

- https://bugzilla.tianocore.org/show_bug.cgi?id=86
- https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt#confirm-simple-multiprocessing-during-boot

The specific meaning of the MSR is totally irrelevant now, it's just a
good use case for testing multi-processing during S3 resume. For MSR
0x3A (Feature Control), the firmware must execute the wrmsr
instructionon on each CPU, and the "rdmsr -a" Linux userspace command
reads back the MSR on each CPU separately.)

Thanks!
Laszlo

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

[Dong, Eric]

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]
> -=-=-=-=-=-=

Re: [PATCH 4/4] OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug

[Igor Mammedov]

On Tue, 8 Oct 2019 23:12:10 +0200
Laszlo Ersek <lersek@redhat.com> wrote:

> On 10/08/19 17:06, Igor Mammedov wrote:
> > On Tue,  8 Oct 2019 13:27:14 +0200
> > Laszlo Ersek <lersek@redhat.com> wrote:
> >  
> >> MaxCpuCountInitialization() currently handles the following options:
> >>
> >> (1) QEMU does not report the boot CPU count.
> >>
> >>     In this case, PlatformPei makes MpInitLib enumerate APs up to the
> >>     default PcdCpuMaxLogicalProcessorNumber value (64) minus 1, or until
> >>     the default PcdCpuApInitTimeOutInMicroSeconds (50,000) elapses.
> >>     (Whichever is reached first.)
> >>
> >>     Time-limited AP enumeration had never been reliable on QEMU/KVM, which
> >>     is why commit 45a70db3c3a5 strated handling case (2) below, in OVMF.
> >>
> >> (2) QEMU reports the boot CPU count.
> >>
> >>     In this case, PlatformPei sets PcdCpuMaxLogicalProcessorNumber to the
> >>     reported boot CPU count, and PcdCpuApInitTimeOutInMicroSeconds to
> >>     practically "infinity" (MAX_UINT32, ~71 minutes). That causes
> >>     MpInitLib to enumerate exactly the available (boot) APs.
> >>
> >>     With CPU hotplug in mind, this method is not good enough. While
> >>     UefiCpuPkg expects PcdCpuMaxLogicalProcessorNumber to include both
> >>     boot (i.e., cold-plugged), and all *potentially* hot-plugged, logical
> >>     processors, the boot CPU count reported by QEMU does not cover the
> >>     second category.  
> >
> > Can you elaborate why it doesn't cover the second category?  
> 
> (Elaborate just in this thread, or elaborate in the commit message?)
> 
> The boot CPU count reported by QEMU does not cover all potentially
> hot-plugged logical processors ... because it is not supposed to.
> 
> Namely, FW_CFG_NB_CPUS exposes "pcms->boot_cpus", and that is not
> supposed to cover all potentially hot-plugged CPUs, only the boot-time
> CPUs.
'potentially hotplugged' confuses me greatly in this context
(I'm not sure if you talk about hotplugged or not existing yet CPUs).

If it's about possible CPUs limit, I'd use "possible CPUs"
instead (wich includes present and not present CPUs).


> Whereas PcdCpuMaxLogicalProcessorNumber needs to include all potentially
> hot-plugged CPUs too. For that, we need to expose "ms->smp.max_cpus"
> from QEMU.
> 
> Does this answer your question?
You answered question better below

[...]

> When a CPU is hotplugged at OS runtime, "pcms->boot_cpus" increases in
> QEMU. 
> Before the patch, the "pcms->boot_cpus" increase causes
> PcdCpuMaxLogicalProcessorNumber to increase as well. That breaks S3
> resume. PcdCpuMaxLogicalProcessorNumber must not change from initial
> boot to S3 resume.
Probably something along this lines should be put in commit message,
which clearly states a requirement for max_cpus

> 
> After the patch, the "pcms->boot_cpus" increase does not increase
> PcdCpuMaxLogicalProcessorNumber. PcdCpuMaxLogicalProcessorNumber remains
> the same (namely, "ms->smp.max_cpus"). Therefore, the CPU structures
> allocated during normal boot, for "ms->smp.max_cpus", will accommodate
> the CPUs that have been hotplugged prior to S3 resume.
> 
> I tested the S3 behavior, plus the values mentioned above are also
> logged to the OVMF debug log, by this patch:
> 
> > +  DEBUG ((DEBUG_INFO, "%a: BootCpuCount=%d mMaxCpuCount=%u\n", __FUNCTION__,
> > +    BootCpuCount, mMaxCpuCount));  
> 
> This is one libvirt domain XML snippet I've used for testing:
> 
>   <vcpu placement='static' current='3'>4</vcpu>
>   <vcpus>
>     <vcpu id='0' enabled='yes' hotpluggable='no'/>
>     <vcpu id='1' enabled='yes' hotpluggable='no'/>
>     <vcpu id='2' enabled='yes' hotpluggable='no'/>
>     <vcpu id='3' enabled='no' hotpluggable='yes'/>
>   </vcpus>
>   <os>
>     <type arch='x86_64' machine='pc-i440fx-4.2'>hvm</type>
> ...
>   <cpu mode='custom' match='exact' check='partial'>
>     <model fallback='allow'>Haswell-noTSX</model>
>     <topology sockets='1' cores='2' threads='2'/>
>     <feature policy='require' name='vmx'/>
>     <feature policy='require' name='pcid'/>
>     <feature policy='require' name='spec-ctrl'/>
>     <feature policy='require' name='pdpe1gb'/>
>   </cpu>
> 
> And the command used for testing was:
> 
> $ virsh setvcpu GUEST 3 --enable --live
> 
> When this is done, the guest kernel dmesg confirms the CPU hotplug, but
> it is not onlined automatically. I onlined it via /sys manually in the
> guest.
> 
> After that point, I exercised S3 suspend / resume. During S3 resume, the
> "BootCpuCount" log (from above) increases from 3 to 4 (and the related
> edk2 infrastructure messages are in sync). All the while "mMaxCpuCount"
> preserves its initial value, 4.
> 
> Finally, after S3 resume, I ran "rdmsr -a 0x3a" in the guest, which
> printed "5" for all *four* CPUs. This confirms that OVMF has set the
> Feature Control MSR on the new VCPU too, during S3 resume.
> 
> (The MSR is zeroed on platform reset, and has to be re-configured by
> firmware, on every CPU, during S3. It is set to 5 due to the "vmx" CPU
> feature in the above domain XML snippet. References:
> 
> - https://bugzilla.tianocore.org/show_bug.cgi?id=86
> - https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt#confirm-simple-multiprocessing-during-boot
> 
> The specific meaning of the MSR is totally irrelevant now, it's just a
> good use case for testing multi-processing during S3 resume. For MSR
> 0x3A (Feature Control), the firmware must execute the wrmsr
> instructionon on each CPU, and the "rdmsr -a" Linux userspace command
> reads back the MSR on each CPU separately.)
> 
> Thanks!
> Laszlo

Re: [PATCH 4/4] OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug

[Laszlo Ersek]

On 10/09/19 12:23, Igor Mammedov wrote:
> On Tue, 8 Oct 2019 23:12:10 +0200
> Laszlo Ersek <lersek@redhat.com> wrote:
> 
>> On 10/08/19 17:06, Igor Mammedov wrote:
>>> On Tue,  8 Oct 2019 13:27:14 +0200
>>> Laszlo Ersek <lersek@redhat.com> wrote:
>>>  
>>>> MaxCpuCountInitialization() currently handles the following options:
>>>>
>>>> (1) QEMU does not report the boot CPU count.
>>>>
>>>>     In this case, PlatformPei makes MpInitLib enumerate APs up to the
>>>>     default PcdCpuMaxLogicalProcessorNumber value (64) minus 1, or until
>>>>     the default PcdCpuApInitTimeOutInMicroSeconds (50,000) elapses.
>>>>     (Whichever is reached first.)
>>>>
>>>>     Time-limited AP enumeration had never been reliable on QEMU/KVM, which
>>>>     is why commit 45a70db3c3a5 strated handling case (2) below, in OVMF.
>>>>
>>>> (2) QEMU reports the boot CPU count.
>>>>
>>>>     In this case, PlatformPei sets PcdCpuMaxLogicalProcessorNumber to the
>>>>     reported boot CPU count, and PcdCpuApInitTimeOutInMicroSeconds to
>>>>     practically "infinity" (MAX_UINT32, ~71 minutes). That causes
>>>>     MpInitLib to enumerate exactly the available (boot) APs.
>>>>
>>>>     With CPU hotplug in mind, this method is not good enough. While
>>>>     UefiCpuPkg expects PcdCpuMaxLogicalProcessorNumber to include both
>>>>     boot (i.e., cold-plugged), and all *potentially* hot-plugged, logical
>>>>     processors, the boot CPU count reported by QEMU does not cover the
>>>>     second category.  
>>>
>>> Can you elaborate why it doesn't cover the second category?  
>>
>> (Elaborate just in this thread, or elaborate in the commit message?)
>>
>> The boot CPU count reported by QEMU does not cover all potentially
>> hot-plugged logical processors ... because it is not supposed to.
>>
>> Namely, FW_CFG_NB_CPUS exposes "pcms->boot_cpus", and that is not
>> supposed to cover all potentially hot-plugged CPUs, only the boot-time
>> CPUs.
> 'potentially hotplugged' confuses me greatly in this context
> (I'm not sure if you talk about hotplugged or not existing yet CPUs).
> 
> If it's about possible CPUs limit, I'd use "possible CPUs"
> instead (wich includes present and not present CPUs).

Good idea. I was struggling to find the correct term. I realize the
"possible CPUs" term has been used in QEMU forever. I'll update the
commit message.

> 
> 
>> Whereas PcdCpuMaxLogicalProcessorNumber needs to include all potentially
>> hot-plugged CPUs too. For that, we need to expose "ms->smp.max_cpus"
>> from QEMU.
>>
>> Does this answer your question?
> You answered question better below
> 
> [...]
> 
>> When a CPU is hotplugged at OS runtime, "pcms->boot_cpus" increases in
>> QEMU. 
>> Before the patch, the "pcms->boot_cpus" increase causes
>> PcdCpuMaxLogicalProcessorNumber to increase as well. That breaks S3
>> resume. PcdCpuMaxLogicalProcessorNumber must not change from initial
>> boot to S3 resume.
> Probably something along this lines should be put in commit message,
> which clearly states a requirement for max_cpus

OK, I'll work that into the commit message too.

Thanks!
Laszlo

> 
>>
>> After the patch, the "pcms->boot_cpus" increase does not increase
>> PcdCpuMaxLogicalProcessorNumber. PcdCpuMaxLogicalProcessorNumber remains
>> the same (namely, "ms->smp.max_cpus"). Therefore, the CPU structures
>> allocated during normal boot, for "ms->smp.max_cpus", will accommodate
>> the CPUs that have been hotplugged prior to S3 resume.
>>
>> I tested the S3 behavior, plus the values mentioned above are also
>> logged to the OVMF debug log, by this patch:
>>
>>> +  DEBUG ((DEBUG_INFO, "%a: BootCpuCount=%d mMaxCpuCount=%u\n", __FUNCTION__,
>>> +    BootCpuCount, mMaxCpuCount));  
>>
>> This is one libvirt domain XML snippet I've used for testing:
>>
>>   <vcpu placement='static' current='3'>4</vcpu>
>>   <vcpus>
>>     <vcpu id='0' enabled='yes' hotpluggable='no'/>
>>     <vcpu id='1' enabled='yes' hotpluggable='no'/>
>>     <vcpu id='2' enabled='yes' hotpluggable='no'/>
>>     <vcpu id='3' enabled='no' hotpluggable='yes'/>
>>   </vcpus>
>>   <os>
>>     <type arch='x86_64' machine='pc-i440fx-4.2'>hvm</type>
>> ...
>>   <cpu mode='custom' match='exact' check='partial'>
>>     <model fallback='allow'>Haswell-noTSX</model>
>>     <topology sockets='1' cores='2' threads='2'/>
>>     <feature policy='require' name='vmx'/>
>>     <feature policy='require' name='pcid'/>
>>     <feature policy='require' name='spec-ctrl'/>
>>     <feature policy='require' name='pdpe1gb'/>
>>   </cpu>
>>
>> And the command used for testing was:
>>
>> $ virsh setvcpu GUEST 3 --enable --live
>>
>> When this is done, the guest kernel dmesg confirms the CPU hotplug, but
>> it is not onlined automatically. I onlined it via /sys manually in the
>> guest.
>>
>> After that point, I exercised S3 suspend / resume. During S3 resume, the
>> "BootCpuCount" log (from above) increases from 3 to 4 (and the related
>> edk2 infrastructure messages are in sync). All the while "mMaxCpuCount"
>> preserves its initial value, 4.
>>
>> Finally, after S3 resume, I ran "rdmsr -a 0x3a" in the guest, which
>> printed "5" for all *four* CPUs. This confirms that OVMF has set the
>> Feature Control MSR on the new VCPU too, during S3 resume.
>>
>> (The MSR is zeroed on platform reset, and has to be re-configured by
>> firmware, on every CPU, during S3. It is set to 5 due to the "vmx" CPU
>> feature in the above domain XML snippet. References:
>>
>> - https://bugzilla.tianocore.org/show_bug.cgi?id=86
>> - https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt#confirm-simple-multiprocessing-during-boot
>>
>> The specific meaning of the MSR is totally irrelevant now, it's just a
>> good use case for testing multi-processing during S3 resume. For MSR
>> 0x3A (Feature Control), the firmware must execute the wrmsr
>> instructionon on each CPU, and the "rdmsr -a" Linux userspace command
>> reads back the MSR on each CPU separately.)
>>
>> Thanks!
>> Laszlo
> 

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

[Laszlo Ersek]

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]
>> -=-=-=-=-=-=
> 

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

[Ni, Ray]

Laszlo,
Can you add comments in the code you changed to describe the two different behaviors?

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

[Laszlo Ersek]

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

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

[Ni, Ray]

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

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

[Laszlo Ersek]

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