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


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

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

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);
>  }
>  
>  


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

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

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

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

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

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


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

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

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
> 


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

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