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.web10.2218.1571992171937709559 for ; Fri, 25 Oct 2019 01:29:32 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=ApxklQsU; 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=1571992171; 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=iY2+5ovCDsamDW5eM6unOQw4txgr3kyt6p3c8Xm9aPs=; b=ApxklQsUsvLXRREPx23j2fApHdBmutnUwuThQtYFM2JH/xnxLpQLiJ5Jn758Ni7XvefIYT cTeHkF9LblL7Y4b3nY+/XtGhAMiiL09NE5Z81z9hVyfcvxJlcAGYpdhB92Xp/70OhYQYRj nacTX/1NWBzaUCMI4vrhM/jv/KZwZ/o= 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-372-musPg5-lOVmn_4FIfzAvtA-1; Fri, 25 Oct 2019 04:29:26 -0400 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B2C461800E00; Fri, 25 Oct 2019 08:29:25 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-99.ams2.redhat.com [10.36.116.99]) by smtp.corp.redhat.com (Postfix) with ESMTP id 876EA194B6; Fri, 25 Oct 2019 08:29:21 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 3/3] OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug To: =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , devel@edk2.groups.io Cc: Anthony Perard , Ard Biesheuvel , Igor Mammedov , Jordan Justen , Julien Grall References: <20191022221554.14963-1-lersek@redhat.com> <20191022221554.14963-4-lersek@redhat.com> <9826409d-43ff-f432-f894-9b5c2f9d5a2a@redhat.com> From: "Laszlo Ersek" Message-ID: <9145bf25-41fb-efb2-6a52-5bf44ce378e8@redhat.com> Date: Fri, 25 Oct 2019 10:29:20 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <9826409d-43ff-f432-f894-9b5c2f9d5a2a@redhat.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-MC-Unique: musPg5-lOVmn_4FIfzAvtA-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable On 10/24/19 17:33, Philippe Mathieu-Daud=E9 wrote: > 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) >> >> =A0=A0=A0=A0 In this case, PlatformPei makes MpInitLib enumerate APs up = to the >> =A0=A0=A0=A0 default PcdCpuMaxLogicalProcessorNumber value (64) minus 1,= or until >> =A0=A0=A0=A0 the default PcdCpuApInitTimeOutInMicroSeconds (50,000) elap= ses. >> =A0=A0=A0=A0 (Whichever is reached first.) >> >> =A0=A0=A0=A0 Time-limited AP enumeration had never been reliable on QEMU= /KVM, >> which >> =A0=A0=A0=A0 is why commit 45a70db3c3a5 strated handling case (2) below,= in OVMF. >> >> (2) QEMU reports the boot CPU count (FW_CFG_NB_CPUS is nonzero) >> >> =A0=A0=A0=A0 In this case, PlatformPei sets >> >> =A0=A0=A0=A0 - PcdCpuMaxLogicalProcessorNumber to the reported boot CPU = count >> =A0=A0=A0=A0=A0=A0 (FW_CFG_NB_CPUS, which exports "PCMachineState.boot_c= pus"), >> >> =A0=A0=A0=A0 - and PcdCpuApInitTimeOutInMicroSeconds to practically "inf= inity" >> =A0=A0=A0=A0=A0=A0 (MAX_UINT32, ~71 minutes). >> >> =A0=A0=A0=A0 That causes MpInitLib to enumerate exactly the present (boo= t) APs. >> >> =A0=A0=A0=A0 With CPU hotplug in mind, this method is not good enough. B= ecause, >> =A0=A0=A0=A0 using QEMU terminology, UefiCpuPkg expects >> =A0=A0=A0=A0 PcdCpuMaxLogicalProcessorNumber to provide the "possible CP= Us" count >> =A0=A0=A0=A0 ("MachineState.smp.max_cpus"), which includes present and n= ot >> present >> =A0=A0=A0=A0 CPUs both (with not present CPUs being subject for hot-plug= ging). >> =A0=A0=A0=A0 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 se= t >> =A0=A0=A0=A0 to values different from the defaults.) >> >> (2) QEMU reports the boot CPU count ("PCMachineState.boot_cpus", via >> =A0=A0=A0=A0 FW_CFG_NB_CPUS), but not the possible CPUs count >> =A0=A0=A0=A0 ("MachineState.smp.max_cpus"). >> >> =A0=A0=A0=A0 In this case, the behavior remains unchanged. >> >> =A0=A0=A0=A0 The way MpInitLib is instructed to do the same differs howe= ver: >> we now >> =A0=A0=A0=A0 set the new PcdCpuBootLogicalProcessorNumber to the boot CP= U count >> =A0=A0=A0=A0 (while continuing to set PcdCpuMaxLogicalProcessorNumber >> identically). >> =A0=A0=A0=A0 PcdCpuApInitTimeOutInMicroSeconds becomes irrelevant. >> >> (3) QEMU reports both the boot CPU count ("PCMachineState.boot_cpus", vi= a >> =A0=A0=A0=A0 FW_CFG_NB_CPUS), and the possible CPUs count >> =A0=A0=A0=A0 ("MachineState.smp.max_cpus"). >> >> =A0=A0=A0=A0 We tell UefiCpuPkg about the possible CPUs count through >> =A0=A0=A0=A0 PcdCpuMaxLogicalProcessorNumber. We also tell MpInitLib the= boot CPU >> =A0=A0=A0=A0 count for precise and quick AP enumeration, via >> =A0=A0=A0=A0 PcdCpuBootLogicalProcessorNumber. >> PcdCpuApInitTimeOutInMicroSeconds is >> =A0=A0=A0=A0 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 resum= e >> 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: >> =A0=A0=A0=A0 v2: >> =A0=A0=A0=A0 =A0=A0=A0=A0 - use "possible CPUs" term in the code and the= commit >> message [Igor] >> =A0=A0=A0=A0 =A0=A0=A0=A0 - add details about S3 to the commit message [= Igor] >> =A0=A0=A0=A0 =A0=A0=A0=A0 - use QEMU's existent CPU hotplug register blo= ck, rather >> than a new >> =A0=A0=A0=A0=A0=A0 named file in fw_cfg [Igor] >> =A0=A0=A0=A0 =A0=A0=A0=A0 - tested on QEMU v2.6.2 (legacy-only CPU hotpl= ug register >> block), v2.7.1 >> =A0=A0=A0=A0=A0=A0 (modern register block, but buggy fw_cfg), v2.8.1.1 (= no QEMU >> issues), >> =A0=A0=A0=A0=A0=A0 v4.0.0 (no QEMU issues) >> >> =A0 OvmfPkg/OvmfPkgIa32.dsc=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 |=A0=A0 = 2 +- >> =A0 OvmfPkg/OvmfPkgIa32X64.dsc=A0=A0=A0=A0=A0=A0=A0=A0=A0 |=A0=A0 2 +- >> =A0 OvmfPkg/OvmfPkgX64.dsc=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 |=A0= =A0 2 +- >> =A0 OvmfPkg/PlatformPei/PlatformPei.inf |=A0=A0 2 +- >> =A0 OvmfPkg/PlatformPei/Platform.c=A0=A0=A0=A0=A0 | 172 ++++++++++++++++= +--- >> =A0 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] >> =A0=A0=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE >> =A0=A0=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE >> =A0 =A0=A0=A0 # UefiCpuPkg PCDs related to initial AP bringup and genera= l AP >> management. >> =A0=A0=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64 >> -=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000 >> +=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0 >> =A0 =A0=A0=A0 # Set memory encryption mask >> =A0=A0=A0 >> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0 >> =A0 =A0 !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] >> =A0=A0=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE >> =A0=A0=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE >> =A0 =A0=A0=A0 # UefiCpuPkg PCDs related to initial AP bringup and genera= l AP >> management. >> =A0=A0=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64 >> -=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000 >> +=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0 >> =A0 =A0=A0=A0 # Set memory encryption mask >> =A0=A0=A0 >> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0 >> =A0 =A0 !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] >> =A0=A0=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE >> =A0=A0=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE >> =A0 =A0=A0=A0 # UefiCpuPkg PCDs related to initial AP bringup and genera= l AP >> management. >> =A0=A0=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64 >> -=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000 >> +=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0 >> =A0 =A0=A0=A0 # Set memory encryption mask >> =A0=A0=A0 >> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0 >> =A0 =A0 !if $(SMM_REQUIRE) =3D=3D 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] >> =A0=A0=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable >> =A0=A0=A0 gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOr= Mask >> =A0=A0=A0 gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificationPol= icy >> =A0=A0=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress >> =A0=A0=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber >> -=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds >> +=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber >> =A0=A0=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize >> =A0 =A0 [FixedPcd] >> =A0=A0=A0 gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress >> =A0 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 @@ >> =A0 #include >> =A0 #include >> =A0 #include >> =A0 #include >> =A0 #include >> +#include >> =A0 #include >> +#include >> +#include >> =A0 #include >> =A0 =A0 #include "Platform.h" >> =A0 #include "Cmos.h" >> =A0 @@ -562,47 +565,165 @@ S3Verification ( >> =A0 #endif >> =A0 } >> =A0 =A0 =A0 /** >> -=A0 Fetch the number of boot CPUs from QEMU and expose it to UefiCpuPkg >> modules. >> -=A0 Set the mMaxCpuCount variable. >> +=A0 Fetch the boot CPU count and the possible CPU count from QEMU, and >> expose >> +=A0 them to UefiCpuPkg modules. Set the mMaxCpuCount variable. >> =A0 **/ >> =A0 VOID >> =A0 MaxCpuCountInitialization ( >> =A0=A0=A0 VOID >> =A0=A0=A0 ) >> =A0 { >> -=A0 UINT16=A0=A0=A0=A0=A0=A0=A0 ProcessorCount; >> +=A0 UINT16=A0=A0=A0=A0=A0=A0=A0 BootCpuCount; >> =A0=A0=A0 RETURN_STATUS PcdStatus; >> =A0 +=A0 // >> +=A0 // Try to fetch the boot CPU count. >> +=A0 // >> =A0=A0=A0 QemuFwCfgSelectItem (QemuFwCfgItemSmpCpuCount); >> -=A0 ProcessorCount =3D QemuFwCfgRead16 (); >> -=A0 // >> -=A0 // If the fw_cfg key or fw_cfg entirely is unavailable, load >> mMaxCpuCount >> -=A0 // from the PCD default. No change to PCDs. >> -=A0 // >> -=A0 if (ProcessorCount =3D=3D 0) { >> +=A0 BootCpuCount =3D QemuFwCfgRead16 (); >> +=A0 if (BootCpuCount =3D=3D 0) { >> +=A0=A0=A0 // >> +=A0=A0=A0 // QEMU doesn't report the boot CPU count. (BootCpuCount =3D= =3D 0) >> will let >> +=A0=A0=A0 // MpInitLib count APs up to (PcdCpuMaxLogicalProcessorNumber= - >> 1), or >> +=A0=A0=A0 // until PcdCpuApInitTimeOutInMicroSeconds elapses (whichever= is >> reached >> +=A0=A0=A0 // first). >> +=A0=A0=A0 // >> +=A0=A0=A0 DEBUG ((DEBUG_WARN, "%a: boot CPU count unavailable\n", >> __FUNCTION__)); >> =A0=A0=A0=A0=A0 mMaxCpuCount =3D PcdGet32 (PcdCpuMaxLogicalProcessorNumb= er); >> -=A0=A0=A0 return; >> +=A0 } else { >> +=A0=A0=A0 // >> +=A0=A0=A0 // We will expose BootCpuCount to MpInitLib. MpInitLib will c= ount >> APs up to >> +=A0=A0=A0 // (BootCpuCount - 1) precisely, regardless of timeout. >> +=A0=A0=A0 // >> +=A0=A0=A0 // Now try to fetch the possible CPU count. >> +=A0=A0=A0 // >> +=A0=A0=A0 UINTN CpuHpBase; >> +=A0=A0=A0 UINT32 CmdData2; >> + >> +=A0=A0=A0 CpuHpBase =3D ((mHostBridgeDevId =3D=3D INTEL_Q35_MCH_DEVICE_= ID) ? >> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 ICH9_CPU_HOTPLUG_BASE = : PIIX4_CPU_HOTPLUG_BASE); >> + >> +=A0=A0=A0 // >> +=A0=A0=A0 // If only legacy mode is available in the CPU hotplug regist= er >> block, or >> +=A0=A0=A0 // the register block is completely missing, then the writes >> below are >> +=A0=A0=A0 // no-ops. >> +=A0=A0=A0 // >> +=A0=A0=A0 // 1. Switch the hotplug register block to modern mode. >> +=A0=A0=A0 // >> +=A0=A0=A0 IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, 0); >> +=A0=A0=A0 // >> +=A0=A0=A0 // 2. Select a valid CPU for deterministic reading of >> +=A0=A0=A0 //=A0=A0=A0 QEMU_CPUHP_R_CMD_DATA2. >> +=A0=A0=A0 // >> +=A0=A0=A0 //=A0=A0=A0 CPU#0 is always valid; it is the always present a= nd >> non-removable >> +=A0=A0=A0 //=A0=A0=A0 BSP. >> +=A0=A0=A0 // >> +=A0=A0=A0 IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, 0); >> +=A0=A0=A0 // >> +=A0=A0=A0 // 3. Send a command after which QEMU_CPUHP_R_CMD_DATA2 is >> specified to >> +=A0=A0=A0 //=A0=A0=A0 read as zero, and which does not invalidate the s= elector. (The >> +=A0=A0=A0 //=A0=A0=A0 selector may change, but it must not become inval= id.) >> +=A0=A0=A0 // >> +=A0=A0=A0 //=A0=A0=A0 Send QEMU_CPUHP_CMD_GET_PENDING, as it will prove= useful >> later. >> +=A0=A0=A0 // >> +=A0=A0=A0 IoWrite8 (CpuHpBase + QEMU_CPUHP_W_CMD, QEMU_CPUHP_CMD_GET_PE= NDING); >> +=A0=A0=A0 // >> +=A0=A0=A0 // 4. Read QEMU_CPUHP_R_CMD_DATA2. >> +=A0=A0=A0 // >> +=A0=A0=A0 //=A0=A0=A0 If the register block is entirely missing, then t= his is an >> unassigned >> +=A0=A0=A0 //=A0=A0=A0 IO read, returning all-bits-one. >> +=A0=A0=A0 // >> +=A0=A0=A0 //=A0=A0=A0 If only legacy mode is available, then bit#0 stan= ds for >> CPU#0 in the >> +=A0=A0=A0 //=A0=A0=A0 "CPU present bitmap". CPU#0 is always present. >> +=A0=A0=A0 // >> +=A0=A0=A0 //=A0=A0=A0 Otherwise, QEMU_CPUHP_R_CMD_DATA2 is either still= reserved >> (returning >> +=A0=A0=A0 //=A0=A0=A0 all-bits-zero), or it is specified to read as zer= o after >> the above >> +=A0=A0=A0 //=A0=A0=A0 steps. Both cases confirm modern mode. >> +=A0=A0=A0 // >> +=A0=A0=A0 CmdData2 =3D IoRead32 (CpuHpBase + QEMU_CPUHP_R_CMD_DATA2); >> +=A0=A0=A0 DEBUG ((DEBUG_VERBOSE, "%a: CmdData2=3D0x%x\n", __FUNCTION__, >> CmdData2)); >> +=A0=A0=A0 if (CmdData2 !=3D 0) { >> +=A0=A0=A0=A0=A0 // >> +=A0=A0=A0=A0=A0 // QEMU doesn't support the modern CPU hotplug interfac= e. >> Assume that the >> +=A0=A0=A0=A0=A0 // possible CPU count equals the boot CPU count (preclu= ding >> hotplug). >> +=A0=A0=A0=A0=A0 // >> +=A0=A0=A0=A0=A0 DEBUG ((DEBUG_WARN, "%a: modern CPU hotplug interface >> unavailable\n", >> +=A0=A0=A0=A0=A0=A0=A0 __FUNCTION__)); >> +=A0=A0=A0=A0=A0 mMaxCpuCount =3D BootCpuCount; >> +=A0=A0=A0 } else { >> +=A0=A0=A0=A0=A0 // >> +=A0=A0=A0=A0=A0 // Grab the possible CPU count from the modern CPU hotp= lug >> interface. >> +=A0=A0=A0=A0=A0 // >> +=A0=A0=A0=A0=A0 UINT32 Present, Possible, Selected; >> + >> +=A0=A0=A0=A0=A0 Present =3D 0; >> +=A0=A0=A0=A0=A0 Possible =3D 0; >> + >> +=A0=A0=A0=A0=A0 // >> +=A0=A0=A0=A0=A0 // We've sent QEMU_CPUHP_CMD_GET_PENDING last; this ens= ures >> +=A0=A0=A0=A0=A0 // QEMU_CPUHP_RW_CMD_DATA can now be read usefully. How= ever, >> +=A0=A0=A0=A0=A0 // QEMU_CPUHP_CMD_GET_PENDING may have selected a CPU w= ith >> actual pending >> +=A0=A0=A0=A0=A0 // hotplug events; therefore, select CPU#0 forcibly. >> +=A0=A0=A0=A0=A0 // >> +=A0=A0=A0=A0=A0 IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Possible); >=20 > Nitpicking, I find this easier to read (matches the comment): >=20 > =A0=A0=A0=A0=A0=A0=A0=A0 IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, 0); I considered that, but I chose this form consciously, in the end. Namely, if you search this patch for IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Possible); you will find that there are two instances of it: one before the loop, and one inside the loop. I wanted to make those operations *look* identical as well, not just *be* semantically identical. The same goal prevented me from changing something else too: namely, in the loop body, the following: ++Possible; IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Possible); could be condensed as: IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, ++Possible); But, that would again break the syntactical identity. And, again, for some reason I found it helpful to keep those two function calls syntactically identical. If you think the above effort doesn't actually improve readability, then I'm happy to respin with *both* changes at the same time (i.e., pass constant 0 in the first location, and pass "++Possible" in the second location). I don't think that implementing either change *in isolation* would be good. Thanks! Laszlo >=20 > Rest very well documented, thanks. >=20 > Reviewed-by: Philippe Mathieu-Daude >=20 >> + >> +=A0=A0=A0=A0=A0 do { >> +=A0=A0=A0=A0=A0=A0=A0 UINT8 CpuStatus; >> + >> +=A0=A0=A0=A0=A0=A0=A0 // >> +=A0=A0=A0=A0=A0=A0=A0 // Read the status of the currently selected CPU.= This will >> help with a >> +=A0=A0=A0=A0=A0=A0=A0 // sanity check against "BootCpuCount". >> +=A0=A0=A0=A0=A0=A0=A0 // >> +=A0=A0=A0=A0=A0=A0=A0 CpuStatus =3D IoRead8 (CpuHpBase + QEMU_CPUHP_R_C= PU_STAT); >> +=A0=A0=A0=A0=A0=A0=A0 if ((CpuStatus & QEMU_CPUHP_STAT_ENABLED) !=3D 0)= { >> +=A0=A0=A0=A0=A0=A0=A0=A0=A0 ++Present; >> +=A0=A0=A0=A0=A0=A0=A0 } >> +=A0=A0=A0=A0=A0=A0=A0 // >> +=A0=A0=A0=A0=A0=A0=A0 // Attempt to select the next CPU. >> +=A0=A0=A0=A0=A0=A0=A0 // >> +=A0=A0=A0=A0=A0=A0=A0 ++Possible; >> +=A0=A0=A0=A0=A0=A0=A0 IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Poss= ible); >> +=A0=A0=A0=A0=A0=A0=A0 // >> +=A0=A0=A0=A0=A0=A0=A0 // If the selection is successful, then the follo= wing read >> will return >> +=A0=A0=A0=A0=A0=A0=A0 // the selector (which we know is positive at thi= s point). >> Otherwise, >> +=A0=A0=A0=A0=A0=A0=A0 // the read will return 0. >> +=A0=A0=A0=A0=A0=A0=A0 // >> +=A0=A0=A0=A0=A0=A0=A0 Selected =3D IoRead32 (CpuHpBase + QEMU_CPUHP_RW_= CMD_DATA); >> +=A0=A0=A0=A0=A0=A0=A0 ASSERT (Selected =3D=3D Possible || Selected =3D= =3D 0); >> +=A0=A0=A0=A0=A0 } while (Selected > 0); >> + >> +=A0=A0=A0=A0=A0 // >> +=A0=A0=A0=A0=A0 // Sanity check: fw_cfg and the modern CPU hotplug inte= rface >> should >> +=A0=A0=A0=A0=A0 // return the same boot CPU count. >> +=A0=A0=A0=A0=A0 // >> +=A0=A0=A0=A0=A0 if (BootCpuCount !=3D Present) { >> +=A0=A0=A0=A0=A0=A0=A0 DEBUG ((DEBUG_WARN, "%a: QEMU v2.7 reset bug: Boo= tCpuCount=3D%d " >> +=A0=A0=A0=A0=A0=A0=A0=A0=A0 "Present=3D%u\n", __FUNCTION__, BootCpuCoun= t, Present)); >> +=A0=A0=A0=A0=A0=A0=A0 // >> +=A0=A0=A0=A0=A0=A0=A0 // The handling of QemuFwCfgItemSmpCpuCount, acro= ss CPU >> hotplug plus >> +=A0=A0=A0=A0=A0=A0=A0 // platform reset (including S3), was corrected i= n QEMU commit >> +=A0=A0=A0=A0=A0=A0=A0 // e3cadac073a9 ("pc: fix FW_CFG_NB_CPUS to accou= nt for >> -device added >> +=A0=A0=A0=A0=A0=A0=A0 // CPUs", 2016-11-16), part of release v2.8.0. >> +=A0=A0=A0=A0=A0=A0=A0 // >> +=A0=A0=A0=A0=A0=A0=A0 BootCpuCount =3D (UINT16)Present; >> +=A0=A0=A0=A0=A0 } >> + >> +=A0=A0=A0=A0=A0 mMaxCpuCount =3D Possible; >> +=A0=A0=A0 } >> =A0=A0=A0 } >> -=A0 // >> -=A0 // Otherwise, set mMaxCpuCount to the value reported by QEMU. >> -=A0 // >> -=A0 mMaxCpuCount =3D ProcessorCount; >> -=A0 // >> -=A0 // Additionally, tell UefiCpuPkg modules (a) the exact number of >> VCPUs, (b) >> -=A0 // to wait, in the initial AP bringup, exactly as long as it takes >> for all of >> -=A0 // the APs to report in. For this, we set the longest representable >> timeout >> -=A0 // (approx. 71 minutes). >> -=A0 // >> -=A0 PcdStatus =3D PcdSet32S (PcdCpuMaxLogicalProcessorNumber, >> ProcessorCount); >> + >> +=A0 DEBUG ((DEBUG_INFO, "%a: BootCpuCount=3D%d mMaxCpuCount=3D%u\n", >> __FUNCTION__, >> +=A0=A0=A0 BootCpuCount, mMaxCpuCount)); >> +=A0 ASSERT (BootCpuCount <=3D mMaxCpuCount); >> + >> +=A0 PcdStatus =3D PcdSet32S (PcdCpuBootLogicalProcessorNumber, >> BootCpuCount); >> =A0=A0=A0 ASSERT_RETURN_ERROR (PcdStatus); >> -=A0 PcdStatus =3D PcdSet32S (PcdCpuApInitTimeOutInMicroSeconds, MAX_UIN= T32); >> +=A0 PcdStatus =3D PcdSet32S (PcdCpuMaxLogicalProcessorNumber, mMaxCpuCo= unt); >> =A0=A0=A0 ASSERT_RETURN_ERROR (PcdStatus); >> -=A0 DEBUG ((DEBUG_INFO, "%a: QEMU reports %d processor(s)\n", >> __FUNCTION__, >> -=A0=A0=A0 ProcessorCount)); >> =A0 } >> =A0 =A0 =A0 /** >> =A0=A0=A0 Perform Platform PEI initialization. >> @@ -636,17 +757,18 @@ InitializePlatform ( >> =A0=A0=A0 } >> =A0 =A0=A0=A0 S3Verification (); >> =A0=A0=A0 BootModeInitialization (); >> =A0=A0=A0 AddressWidthInitialization (); >> -=A0 MaxCpuCountInitialization (); >> =A0 =A0=A0=A0 // >> =A0=A0=A0 // Query Host Bridge DID >> =A0=A0=A0 // >> =A0=A0=A0 mHostBridgeDevId =3D PciRead16 (OVMF_HOSTBRIDGE_DID); >> =A0 +=A0 MaxCpuCountInitialization (); >> + >> =A0=A0=A0 if (FeaturePcdGet (PcdSmmSmramRequire)) { >> =A0=A0=A0=A0=A0 Q35TsegMbytesInitialization (); >> =A0=A0=A0 } >> =A0 =A0=A0=A0 PublishPeiMemory (); >>