[edk2-devel] [PATCH v2 3/3] OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug
Philippe Mathieu-Daudé
philmd at redhat.com
Thu Oct 24 15:33:39 UTC 2019
On 10/23/19 12:15 AM, Laszlo Ersek wrote:
> MaxCpuCountInitialization() currently handles the following options:
>
> (1) QEMU does not report the boot CPU count (FW_CFG_NB_CPUS is 0)
>
> 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 (FW_CFG_NB_CPUS is nonzero)
>
> In this case, PlatformPei sets
>
> - PcdCpuMaxLogicalProcessorNumber to the reported boot CPU count
> (FW_CFG_NB_CPUS, which exports "PCMachineState.boot_cpus"),
>
> - and PcdCpuApInitTimeOutInMicroSeconds to practically "infinity"
> (MAX_UINT32, ~71 minutes).
>
> That causes MpInitLib to enumerate exactly the present (boot) APs.
>
> With CPU hotplug in mind, this method is not good enough. Because,
> using QEMU terminology, UefiCpuPkg expects
> PcdCpuMaxLogicalProcessorNumber to provide the "possible CPUs" count
> ("MachineState.smp.max_cpus"), which includes present and not present
> CPUs both (with not present CPUs being subject for hot-plugging).
> FW_CFG_NB_CPUS does not include not present CPUs.
>
> 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 ("PCMachineState.boot_cpus", via
> FW_CFG_NB_CPUS), but not the possible CPUs count
> ("MachineState.smp.max_cpus").
>
> 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
> (while continuing to set PcdCpuMaxLogicalProcessorNumber identically).
> PcdCpuApInitTimeOutInMicroSeconds becomes irrelevant.
>
> (3) QEMU reports both the boot CPU count ("PCMachineState.boot_cpus", via
> FW_CFG_NB_CPUS), and the possible CPUs count
> ("MachineState.smp.max_cpus").
>
> We tell UefiCpuPkg about the possible CPUs count through
> PcdCpuMaxLogicalProcessorNumber. We also tell MpInitLib the boot CPU
> count for precise and quick AP enumeration, via
> PcdCpuBootLogicalProcessorNumber. PcdCpuApInitTimeOutInMicroSeconds is
> irrelevant again.
>
> This patch is a pre-requisite for enabling CPU hotplug with SMM_REQUIRE.
> As a side effect, the patch also enables S3 to work with CPU hotplug at
> once, *without* SMM_REQUIRE.
>
> (Without the patch, S3 resume fails, if a CPU is hot-plugged at OS
> runtime, prior to suspend: the FW_CFG_NB_CPUS increase seen during resume
> causes PcdCpuMaxLogicalProcessorNumber to increase as well, which is not
> permitted.
>
> With the patch, PcdCpuMaxLogicalProcessorNumber stays the same, namely
> "MachineState.smp.max_cpus". Therefore, the CPU structures allocated
> during normal boot can accommodate the CPUs at S3 resume that have been
> hotplugged prior to S3 suspend.)
>
> Cc: Anthony Perard <anthony.perard at citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
> Cc: Igor Mammedov <imammedo at redhat.com>
> Cc: Jordan Justen <jordan.l.justen at intel.com>
> Cc: Julien Grall <julien.grall at arm.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515
> Signed-off-by: Laszlo Ersek <lersek at redhat.com>
> ---
>
> Notes:
> v2:
>
> - use "possible CPUs" term in the code and the commit message [Igor]
>
> - add details about S3 to the commit message [Igor]
>
> - use QEMU's existent CPU hotplug register block, rather than a new
> named file in fw_cfg [Igor]
>
> - tested on QEMU v2.6.2 (legacy-only CPU hotplug register block), v2.7.1
> (modern register block, but buggy fw_cfg), v2.8.1.1 (no QEMU issues),
> v4.0.0 (no QEMU issues)
>
> OvmfPkg/OvmfPkgIa32.dsc | 2 +-
> OvmfPkg/OvmfPkgIa32X64.dsc | 2 +-
> OvmfPkg/OvmfPkgX64.dsc | 2 +-
> OvmfPkg/PlatformPei/PlatformPei.inf | 2 +-
> OvmfPkg/PlatformPei/Platform.c | 172 +++++++++++++++++---
> 5 files changed, 151 insertions(+), 29 deletions(-)
>
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 4301e7821902..68d8a9fb9655 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -553,11 +553,11 @@ [PcdsDynamicDefault]
> gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
>
> # 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
>
> !if $(SMM_REQUIRE) == TRUE
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 803fd74ae8e4..e5a6260b6088 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -565,11 +565,11 @@ [PcdsDynamicDefault]
> gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
>
> # 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
>
> !if $(SMM_REQUIRE) == TRUE
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 5dbd1b793a90..f5d904945103 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -564,11 +564,11 @@ [PcdsDynamicDefault]
> gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
> gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
>
> # 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
>
> !if $(SMM_REQUIRE) == TRUE
> 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
> @@ -96,11 +96,11 @@ [Pcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
> gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPolicy
> gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress
> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
> - gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds
> + gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber
> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
>
> [FixedPcd]
> gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 3ba2459872d9..e5e8581752b5 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -28,11 +28,14 @@
> #include <Library/QemuFwCfgLib.h>
> #include <Library/QemuFwCfgS3Lib.h>
> #include <Library/ResourcePublicationLib.h>
> #include <Guid/MemoryTypeInformation.h>
> #include <Ppi/MasterBootMode.h>
> +#include <IndustryStandard/I440FxPiix4.h>
> #include <IndustryStandard/Pci22.h>
> +#include <IndustryStandard/Q35MchIch9.h>
> +#include <IndustryStandard/QemuCpuHotplug.h>
> #include <OvmfPlatforms.h>
>
> #include "Platform.h"
> #include "Cmos.h"
>
> @@ -562,47 +565,165 @@ S3Verification (
> #endif
> }
>
>
> /**
> - Fetch the number of boot CPUs from QEMU and expose it to UefiCpuPkg modules.
> - Set the mMaxCpuCount variable.
> + Fetch the boot CPU count and the possible CPU count from QEMU, and expose
> + them to UefiCpuPkg modules. Set the mMaxCpuCount variable.
> **/
> VOID
> MaxCpuCountInitialization (
> VOID
> )
> {
> - UINT16 ProcessorCount;
> + UINT16 BootCpuCount;
> RETURN_STATUS PcdStatus;
>
> + //
> + // 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. (BootCpuCount == 0) will 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 the possible CPU count.
> + //
> + UINTN CpuHpBase;
> + UINT32 CmdData2;
> +
> + CpuHpBase = ((mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) ?
> + ICH9_CPU_HOTPLUG_BASE : PIIX4_CPU_HOTPLUG_BASE);
> +
> + //
> + // If only legacy mode is available in the CPU hotplug register block, or
> + // the register block is completely missing, then the writes below are
> + // no-ops.
> + //
> + // 1. Switch the hotplug register block to modern mode.
> + //
> + IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, 0);
> + //
> + // 2. Select a valid CPU for deterministic reading of
> + // QEMU_CPUHP_R_CMD_DATA2.
> + //
> + // CPU#0 is always valid; it is the always present and non-removable
> + // BSP.
> + //
> + IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, 0);
> + //
> + // 3. Send a command after which QEMU_CPUHP_R_CMD_DATA2 is specified to
> + // read as zero, and which does not invalidate the selector. (The
> + // selector may change, but it must not become invalid.)
> + //
> + // Send QEMU_CPUHP_CMD_GET_PENDING, as it will prove useful later.
> + //
> + IoWrite8 (CpuHpBase + QEMU_CPUHP_W_CMD, QEMU_CPUHP_CMD_GET_PENDING);
> + //
> + // 4. Read QEMU_CPUHP_R_CMD_DATA2.
> + //
> + // If the register block is entirely missing, then this is an unassigned
> + // IO read, returning all-bits-one.
> + //
> + // If only legacy mode is available, then bit#0 stands for CPU#0 in the
> + // "CPU present bitmap". CPU#0 is always present.
> + //
> + // Otherwise, QEMU_CPUHP_R_CMD_DATA2 is either still reserved (returning
> + // all-bits-zero), or it is specified to read as zero after the above
> + // steps. Both cases confirm modern mode.
> + //
> + CmdData2 = IoRead32 (CpuHpBase + QEMU_CPUHP_R_CMD_DATA2);
> + DEBUG ((DEBUG_VERBOSE, "%a: CmdData2=0x%x\n", __FUNCTION__, CmdData2));
> + if (CmdData2 != 0) {
> + //
> + // QEMU doesn't support the modern CPU hotplug interface. Assume that the
> + // possible CPU count equals the boot CPU count (precluding hotplug).
> + //
> + DEBUG ((DEBUG_WARN, "%a: modern CPU hotplug interface unavailable\n",
> + __FUNCTION__));
> + mMaxCpuCount = BootCpuCount;
> + } else {
> + //
> + // Grab the possible CPU count from the modern CPU hotplug interface.
> + //
> + UINT32 Present, Possible, Selected;
> +
> + Present = 0;
> + Possible = 0;
> +
> + //
> + // We've sent QEMU_CPUHP_CMD_GET_PENDING last; this ensures
> + // QEMU_CPUHP_RW_CMD_DATA can now be read usefully. However,
> + // QEMU_CPUHP_CMD_GET_PENDING may have selected a CPU with actual pending
> + // hotplug events; therefore, select CPU#0 forcibly.
> + //
> + IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Possible);
Nitpicking, I find this easier to read (matches the comment):
IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, 0);
Rest very well documented, thanks.
Reviewed-by: Philippe Mathieu-Daude <philmd at redhat.com>
> +
> + do {
> + UINT8 CpuStatus;
> +
> + //
> + // Read the status of the currently selected CPU. This will help with a
> + // sanity check against "BootCpuCount".
> + //
> + CpuStatus = IoRead8 (CpuHpBase + QEMU_CPUHP_R_CPU_STAT);
> + if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) != 0) {
> + ++Present;
> + }
> + //
> + // Attempt to select the next CPU.
> + //
> + ++Possible;
> + IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Possible);
> + //
> + // If the selection is successful, then the following read will return
> + // the selector (which we know is positive at this point). Otherwise,
> + // the read will return 0.
> + //
> + Selected = IoRead32 (CpuHpBase + QEMU_CPUHP_RW_CMD_DATA);
> + ASSERT (Selected == Possible || Selected == 0);
> + } while (Selected > 0);
> +
> + //
> + // Sanity check: fw_cfg and the modern CPU hotplug interface should
> + // return the same boot CPU count.
> + //
> + if (BootCpuCount != Present) {
> + DEBUG ((DEBUG_WARN, "%a: QEMU v2.7 reset bug: BootCpuCount=%d "
> + "Present=%u\n", __FUNCTION__, BootCpuCount, Present));
> + //
> + // The handling of QemuFwCfgItemSmpCpuCount, across CPU hotplug plus
> + // platform reset (including S3), was corrected in QEMU commit
> + // e3cadac073a9 ("pc: fix FW_CFG_NB_CPUS to account for -device added
> + // CPUs", 2016-11-16), part of release v2.8.0.
> + //
> + BootCpuCount = (UINT16)Present;
> + }
> +
> + mMaxCpuCount = Possible;
> + }
> }
> - //
> - // 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);
> +
> + DEBUG ((DEBUG_INFO, "%a: BootCpuCount=%d mMaxCpuCount=%u\n", __FUNCTION__,
> + BootCpuCount, mMaxCpuCount));
> + ASSERT (BootCpuCount <= mMaxCpuCount);
> +
> + PcdStatus = PcdSet32S (PcdCpuBootLogicalProcessorNumber, BootCpuCount);
> ASSERT_RETURN_ERROR (PcdStatus);
> - PcdStatus = PcdSet32S (PcdCpuApInitTimeOutInMicroSeconds, MAX_UINT32);
> + PcdStatus = PcdSet32S (PcdCpuMaxLogicalProcessorNumber, mMaxCpuCount);
> ASSERT_RETURN_ERROR (PcdStatus);
> - DEBUG ((DEBUG_INFO, "%a: QEMU reports %d processor(s)\n", __FUNCTION__,
> - ProcessorCount));
> }
>
>
> /**
> Perform Platform PEI initialization.
> @@ -636,17 +757,18 @@ InitializePlatform (
> }
>
> S3Verification ();
> BootModeInitialization ();
> AddressWidthInitialization ();
> - MaxCpuCountInitialization ();
>
> //
> // Query Host Bridge DID
> //
> mHostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
>
> + MaxCpuCountInitialization ();
> +
> if (FeaturePcdGet (PcdSmmSmramRequire)) {
> Q35TsegMbytesInitialization ();
> }
>
> PublishPeiMemory ();
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#49429): https://edk2.groups.io/g/devel/message/49429
Mute This Topic: https://groups.io/mt/36465055/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list