From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by mx.groups.io with SMTP id smtpd.web09.2454.1571995027634964502 for ; Fri, 25 Oct 2019 02:17:07 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: philmd@redhat.com) Received: from mail-wr1-f72.google.com (mail-wr1-f72.google.com [209.85.221.72]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9FBA8C049E17 for ; Fri, 25 Oct 2019 09:17:06 +0000 (UTC) Received: by mail-wr1-f72.google.com with SMTP id e25so711285wra.9 for ; Fri, 25 Oct 2019 02:17:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=mjL+qNy0sp+9b24JeTqmrRSc6MOIHMHacCRt0/8+Qzc=; b=jXnnG6vi6Ej8jL28H7b1UPTU0Fhan2TYwmHG8PjAdLoO3DQEE+KbU2QHD5RrvJoi/1 rhePQHIRi2yOdulxnbI0QefRUkG8zkECyjBWLiAjNiGYjEgTCKzqAVvX0U5idhV2quPC 8c2EdiKH/pgy7gW/TmkSAzhwIfhLajTbRZkSAQ8Sc/a+zFYgyS5nbeHohscQvy1xSw+O w1pOjYczrGKmUfMp9YKrLexvRyAcSeFY8vbEBOCD1eRqf/UnPsbhpO661aVBFqa+CK89 rs9exWA5ZOzhYwmokz+uJht6lYyp5LkG4ZnoRKnnydm5vHOg2/1tEC1n09wM6aKaGlhC OpYA== X-Gm-Message-State: APjAAAWtz2xWiUBOK0qY0EWAz0Gf33i5akdmhDh/RdmuYvwaAaSArQYv u5GADBpOQ7eLJQgJyGJ/nNJs8lX5lyS1gzRVNalRlwEfbuZy21/pS4O4Gw8GzjOw2pkA+GM9IFY uxrlVPmdOGAbzqw== X-Received: by 2002:adf:fcd1:: with SMTP id f17mr2124256wrs.82.1571995025251; Fri, 25 Oct 2019 02:17:05 -0700 (PDT) X-Google-Smtp-Source: APXvYqwkLZQf3i2KtXgdkcDL2c1H/QB3CwZD7NHY7noF3r4qVmsUTSHbrg3J1UWNhaRzEML5kDRuSQ== X-Received: by 2002:adf:fcd1:: with SMTP id f17mr2124220wrs.82.1571995024894; Fri, 25 Oct 2019 02:17:04 -0700 (PDT) Received: from [192.168.1.115] (129.red-83-57-174.dynamicip.rima-tde.net. [83.57.174.129]) by smtp.gmail.com with ESMTPSA id a189sm1538412wma.2.2019.10.25.02.17.03 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 25 Oct 2019 02:17:04 -0700 (PDT) Subject: Re: [edk2-devel] [PATCH v2 3/3] OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug To: Laszlo Ersek , 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> <9145bf25-41fb-efb2-6a52-5bf44ce378e8@redhat.com> From: =?UTF-8?B?UGhpbGlwcGUgTWF0aGlldS1EYXVkw6k=?= Message-ID: <6e499967-467b-95d7-fe88-58556bc88f84@redhat.com> Date: Fri, 25 Oct 2019 11:17:02 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.1 MIME-Version: 1.0 In-Reply-To: <9145bf25-41fb-efb2-6a52-5bf44ce378e8@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 10/25/19 10:29 AM, Laszlo Ersek wrote: > 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) minu= s 1, or until >>> =A0=A0=A0=A0 the default PcdCpuApInitTimeOutInMicroSeconds (50,000) = elapses. >>> =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) be= low, 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.bo= ot_cpus"), >>> >>> =A0=A0=A0=A0 - and PcdCpuApInitTimeOutInMicroSeconds to practically = "infinity" >>> =A0=A0=A0=A0=A0=A0 (MAX_UINT32, ~71 minutes). >>> >>> =A0=A0=A0=A0 That causes MpInitLib to enumerate exactly the present = (boot) APs. >>> >>> =A0=A0=A0=A0 With CPU hotplug in mind, this method is not good enoug= h. Because, >>> =A0=A0=A0=A0 using QEMU terminology, UefiCpuPkg expects >>> =A0=A0=A0=A0 PcdCpuMaxLogicalProcessorNumber to provide the "possibl= e CPUs" count >>> =A0=A0=A0=A0 ("MachineState.smp.max_cpus"), which includes present a= nd not >>> present >>> =A0=A0=A0=A0 CPUs both (with not present CPUs being subject for hot-= plugging). >>> =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= set >>> =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 = however: >>> we now >>> =A0=A0=A0=A0 set the new PcdCpuBootLogicalProcessorNumber to the boo= t CPU count >>> =A0=A0=A0=A0 (while continuing to set PcdCpuMaxLogicalProcessorNumbe= r >>> identically). >>> =A0=A0=A0=A0 PcdCpuApInitTimeOutInMicroSeconds becomes irrelevant. >>> >>> (3) QEMU reports both the boot CPU count ("PCMachineState.boot_cpus",= via >>> =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 throug= h >>> =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_REQUI= RE. >>> 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 re= sume >>> causes PcdCpuMaxLogicalProcessorNumber to increase as well, which is = not >>> permitted. >>> >>> With the patch, PcdCpuMaxLogicalProcessorNumber stays the same, namel= y >>> "MachineState.smp.max_cpus". Therefore, the CPU structures allocated >>> during normal boot can accommodate the CPUs at S3 resume that have be= en >>> 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 messa= ge [Igor] >>> =A0=A0=A0=A0 =A0=A0=A0=A0 - use QEMU's existent CPU hotplug register= block, 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 h= otplug 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|FA= LSE >>> =A0 =A0=A0=A0 # UefiCpuPkg PCDs related to initial AP bringup and ge= neral AP >>> management. >>> =A0=A0=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|= 64 >>> -=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|5000= 0 >>> +=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0 >>> =A0 =A0=A0=A0 # Set memory encryption mask >>> =20 >>> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x= 0 >>> =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|FA= LSE >>> =A0 =A0=A0=A0 # UefiCpuPkg PCDs related to initial AP bringup and ge= neral AP >>> management. >>> =A0=A0=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|= 64 >>> -=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|5000= 0 >>> +=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0 >>> =A0 =A0=A0=A0 # Set memory encryption mask >>> =20 >>> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x= 0 >>> =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|FA= LSE >>> =A0 =A0=A0=A0 # UefiCpuPkg PCDs related to initial AP bringup and ge= neral AP >>> management. >>> =A0=A0=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|= 64 >>> -=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|5000= 0 >>> +=A0 gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0 >>> =A0 =A0=A0=A0 # Set memory encryption mask >>> =20 >>> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x= 0 >>> =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.PcdPteMemoryEncryptionAddre= ssOrMask >>> =A0=A0=A0 gEfiSecurityPkgTokenSpaceGuid.PcdOptionRomImageVerificatio= nPolicy >>> =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 UefiCpu= Pkg >>> modules. >>> -=A0 Set the mMaxCpuCount variable. >>> +=A0 Fetch the boot CPU count and the possible CPU count from QEMU, a= nd >>> 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 (PcdCpuMaxLogicalProcessorNum= ber - >>> 1), or >>> +=A0=A0=A0 // until PcdCpuApInitTimeOutInMicroSeconds elapses (whiche= ver 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 (PcdCpuMaxLogicalProcessor= Number); >>> -=A0=A0=A0 return; >>> +=A0 } else { >>> +=A0=A0=A0 // >>> +=A0=A0=A0 // We will expose BootCpuCount to MpInitLib. MpInitLib wil= l count >>> 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_DEVI= CE_ID) ? >>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 ICH9_CPU_HOTPLUG_BA= SE : PIIX4_CPU_HOTPLUG_BASE); >>> + >>> +=A0=A0=A0 // >>> +=A0=A0=A0 // If only legacy mode is available in the CPU hotplug reg= ister >>> block, or >>> +=A0=A0=A0 // the register block is completely missing, then the writ= es >>> 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 presen= t and >>> 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 th= e selector. (The >>> +=A0=A0=A0 //=A0=A0=A0 selector may change, but it must not become in= valid.) >>> +=A0=A0=A0 // >>> +=A0=A0=A0 //=A0=A0=A0 Send QEMU_CPUHP_CMD_GET_PENDING, as it will pr= ove useful >>> later. >>> +=A0=A0=A0 // >>> +=A0=A0=A0 IoWrite8 (CpuHpBase + QEMU_CPUHP_W_CMD, QEMU_CPUHP_CMD_GET= _PENDING); >>> +=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, the= n this 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 s= tands 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 st= ill reserved >>> (returning >>> +=A0=A0=A0 //=A0=A0=A0 all-bits-zero), or it is specified to read as = zero 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 inter= face. >>> Assume that the >>> +=A0=A0=A0=A0=A0 // possible CPU count equals the boot CPU count (pre= cluding >>> hotplug). >>> +=A0=A0=A0=A0=A0 // >>> +=A0=A0=A0=A0=A0 DEBUG ((DEBUG_WARN, "%a: modern CPU hotplug interfac= e >>> 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 h= otplug >>> 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 = ensures >>> +=A0=A0=A0=A0=A0 // QEMU_CPUHP_RW_CMD_DATA can now be read usefully. = However, >>> +=A0=A0=A0=A0=A0 // QEMU_CPUHP_CMD_GET_PENDING may have selected a CP= U with >>> 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, Possibl= e); >> >> Nitpicking, I find this easier to read (matches the comment): >> >> =A0=A0=A0=A0=A0=A0=A0=A0 IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL,= 0); >=20 > I considered that, but I chose this form consciously, in the end. > Namely, if you search this patch for >=20 > IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Possible); >=20 > 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. I see. > The same goal prevented me from changing something else too: namely, in > the loop body, the following: >=20 > ++Possible; > IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, Possible); >=20 > could be condensed as: >=20 > IoWrite32 (CpuHpBase + QEMU_CPUHP_W_CPU_SEL, ++Possible); >=20 > 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. Oh I really prefer the 2 different lines form, it is a no-brain effort to review, one logical operation at a time. > If you think the above effort doesn't actually improve readability, the= n > 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. No, since I prefer to not use the condensed form, let's keep this patch as it. Regards, Phil. >> Rest very well documented, thanks. >> >> Reviewed-by: Philippe Mathieu-Daude >> >>> + >>> +=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 C= PU. 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_CPU_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, P= ossible); >>> +=A0=A0=A0=A0=A0=A0=A0 // >>> +=A0=A0=A0=A0=A0=A0=A0 // If the selection is successful, then the fo= llowing read >>> will return >>> +=A0=A0=A0=A0=A0=A0=A0 // the selector (which we know is positive at = this 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 i= nterface >>> 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: = BootCpuCount=3D%d " >>> +=A0=A0=A0=A0=A0=A0=A0=A0=A0 "Present=3D%u\n", __FUNCTION__, BootCpuC= ount, Present)); >>> +=A0=A0=A0=A0=A0=A0=A0 // >>> +=A0=A0=A0=A0=A0=A0=A0 // The handling of QemuFwCfgItemSmpCpuCount, a= cross CPU >>> hotplug plus >>> +=A0=A0=A0=A0=A0=A0=A0 // platform reset (including S3), was correcte= d in QEMU commit >>> +=A0=A0=A0=A0=A0=A0=A0 // e3cadac073a9 ("pc: fix FW_CFG_NB_CPUS to ac= count 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 tak= es >>> for all of >>> -=A0 // the APs to report in. For this, we set the longest representa= ble >>> 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_= UINT32); >>> +=A0 PcdStatus =3D PcdSet32S (PcdCpuMaxLogicalProcessorNumber, mMaxCp= uCount); >>> =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 (); >>> >=20