From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web09.1924.1571782574318235105 for ; Tue, 22 Oct 2019 15:16:14 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=PlzW4rPc; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1571782573; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Z2N28qrl/ObfShQ4Q54n0HmaIEj09YUgJZyHx2tj73I=; b=PlzW4rPcetPoTwSuF+qfZtEaeYscQ2JaikUuA7pnfZLOi5xJkJu/SpZpDmjsHg3rhRDc4+ QBXbNVBZqiI1OSS/weD67GqvrlazChCtgtr6BciYcwEB7/80jllaUEQFEUUPaO1+UvMwL0 6xbVCzL0W0sQboLfli0UofbWWTzz63Q= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-314-BrcDDQnSPEaNDeYi0hk0kA-1; Tue, 22 Oct 2019 18:16:05 -0400 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id BBB35800D49; Tue, 22 Oct 2019 22:16:03 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (unknown [10.36.118.54]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3DD635DAAF; Tue, 22 Oct 2019 22:16:02 +0000 (UTC) From: "Laszlo Ersek" To: edk2-devel-groups-io Cc: Anthony Perard , Ard Biesheuvel , Igor Mammedov , Jordan Justen , Julien Grall Subject: [PATCH v2 3/3] OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug Date: Wed, 23 Oct 2019 00:15:54 +0200 Message-Id: <20191022221554.14963-4-lersek@redhat.com> In-Reply-To: <20191022221554.14963-1-lersek@redhat.com> References: <20191022221554.14963-1-lersek@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-MC-Unique: BrcDDQnSPEaNDeYi0hk0kA-1 X-Mimecast-Spam-Score: 0 Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable 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 Cc: Ard Biesheuvel Cc: Igor Mammedov Cc: Jordan Justen Cc: Julien Grall Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1515 Signed-off-by: Laszlo Ersek --- Notes: v2: =20 - use "possible CPUs" term in the code and the commit message [Igor] =20 - add details about S3 to the commit message [Igor] =20 - use QEMU's existent CPU hotplug register block, rather than a new named file in fw_cfg [Igor] =20 - 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 =20 # UefiCpuPkg PCDs related to initial AP bringup and general AP managemen= t. gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64 - gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000 + gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0 =20 # Set memory encryption mask gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0 =20 !if $(SMM_REQUIRE) =3D=3D 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 =20 # UefiCpuPkg PCDs related to initial AP bringup and general AP managemen= t. gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64 - gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000 + gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0 =20 # Set memory encryption mask gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0 =20 !if $(SMM_REQUIRE) =3D=3D 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 =20 # UefiCpuPkg PCDs related to initial AP bringup and general AP managemen= t. gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64 - gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000 + gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0 =20 # Set memory encryption mask gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0 =20 !if $(SMM_REQUIRE) =3D=3D TRUE diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/Plat= formPei.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 =20 [FixedPcd] gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress =20 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 #include #include #include #include +#include #include +#include +#include #include =20 #include "Platform.h" #include "Cmos.h" =20 @@ -562,47 +565,165 @@ S3Verification ( #endif } =20 =20 /** - Fetch the number of boot CPUs from QEMU and expose it to UefiCpuPkg modu= les. - Set the mMaxCpuCount variable. + Fetch the boot CPU count and the possible CPU count from QEMU, and expos= e + them to UefiCpuPkg modules. Set the mMaxCpuCount variable. **/ VOID MaxCpuCountInitialization ( VOID ) { - UINT16 ProcessorCount; + UINT16 BootCpuCount; RETURN_STATUS PcdStatus; =20 + // + // Try to fetch the boot CPU count. + // QemuFwCfgSelectItem (QemuFwCfgItemSmpCpuCount); - ProcessorCount =3D QemuFwCfgRead16 (); - // - // If the fw_cfg key or fw_cfg entirely is unavailable, load mMaxCpuCoun= t - // from the PCD default. No change to PCDs. - // - if (ProcessorCount =3D=3D 0) { + BootCpuCount =3D QemuFwCfgRead16 (); + if (BootCpuCount =3D=3D 0) { + // + // QEMU doesn't report the boot CPU count. (BootCpuCount =3D=3D 0) wil= l let + // MpInitLib count APs up to (PcdCpuMaxLogicalProcessorNumber - 1), or + // until PcdCpuApInitTimeOutInMicroSeconds elapses (whichever is reach= ed + // first). + // + DEBUG ((DEBUG_WARN, "%a: boot CPU count unavailable\n", __FUNCTION__))= ; mMaxCpuCount =3D 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 =3D ((mHostBridgeDevId =3D=3D 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-removabl= e + // BSP. + // + IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, 0); + // + // 3. Send a command after which QEMU_CPUHP_R_CMD_DATA2 is specified t= o + // 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 unass= igned + // 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 (retu= rning + // all-bits-zero), or it is specified to read as zero after the abo= ve + // steps. Both cases confirm modern mode. + // + CmdData2 =3D IoRead32 (CpuHpBase + QEMU_CPUHP_R_CMD_DATA2); + DEBUG ((DEBUG_VERBOSE, "%a: CmdData2=3D0x%x\n", __FUNCTION__, CmdData2= )); + if (CmdData2 !=3D 0) { + // + // QEMU doesn't support the modern CPU hotplug interface. Assume tha= t the + // possible CPU count equals the boot CPU count (precluding hotplug)= . + // + DEBUG ((DEBUG_WARN, "%a: modern CPU hotplug interface unavailable\n"= , + __FUNCTION__)); + mMaxCpuCount =3D BootCpuCount; + } else { + // + // Grab the possible CPU count from the modern CPU hotplug interface= . + // + UINT32 Present, Possible, Selected; + + Present =3D 0; + Possible =3D 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 pe= nding + // hotplug events; therefore, select CPU#0 forcibly. + // + IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Possible); + + do { + UINT8 CpuStatus; + + // + // Read the status of the currently selected CPU. This will help w= ith a + // sanity check against "BootCpuCount". + // + CpuStatus =3D IoRead8 (CpuHpBase + QEMU_CPUHP_R_CPU_STAT); + if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) !=3D 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 re= turn + // the selector (which we know is positive at this point). Otherwi= se, + // the read will return 0. + // + Selected =3D IoRead32 (CpuHpBase + QEMU_CPUHP_RW_CMD_DATA); + ASSERT (Selected =3D=3D Possible || Selected =3D=3D 0); + } while (Selected > 0); + + // + // Sanity check: fw_cfg and the modern CPU hotplug interface should + // return the same boot CPU count. + // + if (BootCpuCount !=3D Present) { + DEBUG ((DEBUG_WARN, "%a: QEMU v2.7 reset bug: BootCpuCount=3D%d " + "Present=3D%u\n", __FUNCTION__, BootCpuCount, Present)); + // + // The handling of QemuFwCfgItemSmpCpuCount, across CPU hotplug pl= us + // platform reset (including S3), was corrected in QEMU commit + // e3cadac073a9 ("pc: fix FW_CFG_NB_CPUS to account for -device ad= ded + // CPUs", 2016-11-16), part of release v2.8.0. + // + BootCpuCount =3D (UINT16)Present; + } + + mMaxCpuCount =3D Possible; + } } - // - // Otherwise, set mMaxCpuCount to the value reported by QEMU. - // - mMaxCpuCount =3D 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 a= ll of - // the APs to report in. For this, we set the longest representable time= out - // (approx. 71 minutes). - // - PcdStatus =3D PcdSet32S (PcdCpuMaxLogicalProcessorNumber, ProcessorCount= ); + + DEBUG ((DEBUG_INFO, "%a: BootCpuCount=3D%d mMaxCpuCount=3D%u\n", __FUNCT= ION__, + BootCpuCount, mMaxCpuCount)); + ASSERT (BootCpuCount <=3D mMaxCpuCount); + + PcdStatus =3D PcdSet32S (PcdCpuBootLogicalProcessorNumber, BootCpuCount)= ; ASSERT_RETURN_ERROR (PcdStatus); - PcdStatus =3D PcdSet32S (PcdCpuApInitTimeOutInMicroSeconds, MAX_UINT32); + PcdStatus =3D PcdSet32S (PcdCpuMaxLogicalProcessorNumber, mMaxCpuCount); ASSERT_RETURN_ERROR (PcdStatus); - DEBUG ((DEBUG_INFO, "%a: QEMU reports %d processor(s)\n", __FUNCTION__, - ProcessorCount)); } =20 =20 /** Perform Platform PEI initialization. @@ -636,17 +757,18 @@ InitializePlatform ( } =20 S3Verification (); BootModeInitialization (); AddressWidthInitialization (); - MaxCpuCountInitialization (); =20 // // Query Host Bridge DID // mHostBridgeDevId =3D PciRead16 (OVMF_HOSTBRIDGE_DID); =20 + MaxCpuCountInitialization (); + if (FeaturePcdGet (PcdSmmSmramRequire)) { Q35TsegMbytesInitialization (); } =20 PublishPeiMemory (); --=20 2.19.1.3.g30247aa5d201