From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: imammedo@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Tue, 08 Oct 2019 08:06:59 -0700 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 07EFE10C050B; Tue, 8 Oct 2019 15:06:59 +0000 (UTC) Received: from localhost (unknown [10.43.2.182]) by smtp.corp.redhat.com (Postfix) with ESMTP id 5526C60923; Tue, 8 Oct 2019 15:06:57 +0000 (UTC) Date: Tue, 8 Oct 2019 17:06:55 +0200 From: "Igor Mammedov" To: Laszlo Ersek Cc: edk2-devel-groups-io , Anthony Perard , Ard Biesheuvel , Eric Dong , Jordan Justen , Julien Grall , Ray Ni Subject: Re: [PATCH 4/4] OvmfPkg/PlatformPei: rewrite MaxCpuCountInitialization() for CPU hotplug Message-ID: <20191008170655.3c0362a1@redhat.com> In-Reply-To: <20191008112714.6215-5-lersek@redhat.com> References: <20191008112714.6215-1-lersek@redhat.com> <20191008112714.6215-5-lersek@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.65]); Tue, 08 Oct 2019 15:06:59 +0000 (UTC) Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Tue, 8 Oct 2019 13:27:14 +0200 Laszlo Ersek 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 > Cc: Ard Biesheuvel > Cc: Eric Dong > Cc: Igor Mammedov > Cc: Jordan Justen > Cc: Julien Grall > Cc: Ray Ni > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515 > Signed-off-by: Laszlo Ersek > --- > 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); > } > >