From: "Igor Mammedov" <imammedo@redhat.com>
To: Laszlo Ersek <lersek@redhat.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>,
Anthony Perard <anthony.perard@citrix.com>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,
Eric Dong <eric.dong@intel.com>,
Jordan Justen <jordan.l.justen@intel.com>,
Julien Grall <julien.grall@arm.com>, Ray Ni <ray.ni@intel.com>
Subject: Re: [PATCH 4/4] OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug
Date: Tue, 8 Oct 2019 17:06:55 +0200 [thread overview]
Message-ID: <20191008170655.3c0362a1@redhat.com> (raw)
In-Reply-To: <20191008112714.6215-5-lersek@redhat.com>
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);
> }
>
>
next prev parent reply other threads:[~2019-10-08 15:06 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-08 11:27 [PATCH 0/4] UefiCpuPkg, OvmfPkg: separate PCDs for boot CPU count vs. max CPU count Laszlo Ersek
2019-10-08 11:27 ` [PATCH 1/4] UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP detection Laszlo Ersek
2019-10-09 0:57 ` [edk2-devel] " Dong, Eric
2019-10-09 14:55 ` Laszlo Ersek
2019-10-10 2:52 ` Ni, Ray
2019-10-10 7:38 ` Laszlo Ersek
2019-10-10 7:50 ` Ni, Ray
2019-10-10 8:28 ` Laszlo Ersek
2019-10-08 11:27 ` [PATCH 2/4] OvmfPkg/OvmfXen.dsc: remove PcdCpu* dynamic defaults Laszlo Ersek
2019-10-08 13:11 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-10-08 20:27 ` Laszlo Ersek
2019-10-08 11:27 ` [PATCH 3/4] OvmfPkg/IndustryStandard: define FW_CFG_X86_TOPOLOGY structure type Laszlo Ersek
2019-10-08 11:27 ` [PATCH 4/4] OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug Laszlo Ersek
2019-10-08 15:06 ` Igor Mammedov [this message]
2019-10-08 21:12 ` Laszlo Ersek
2019-10-09 10:23 ` Igor Mammedov
2019-10-09 14:43 ` Laszlo Ersek
2019-10-08 11:35 ` [PATCH 0/4] UefiCpuPkg, OvmfPkg: separate PCDs for boot CPU count vs. max CPU count Ard Biesheuvel
2019-10-08 20:27 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191008170655.3c0362a1@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox