On Tue, 8 Oct 2019 13:27:14 +0200 Laszlo Ersek <ler...@redhat.com> 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 <anthony.per...@citrix.com> > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> > Cc: Eric Dong <eric.d...@intel.com> > Cc: Igor Mammedov <imamm...@redhat.com> > Cc: Jordan Justen <jordan.l.jus...@intel.com> > Cc: Julien Grall <julien.gr...@arm.com> > Cc: Ray Ni <ray...@intel.com> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1515 > Signed-off-by: Laszlo Ersek <ler...@redhat.com> > --- > 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); > } > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48584): https://edk2.groups.io/g/devel/message/48584 Mute This Topic: https://groups.io/mt/34441232/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
