On 11/24/16 02:18, Fan, Jeff wrote: > Laszlo, > > Please use existing PcdCpuMaxLogicalProcessorNumber firstly.
Okay, I will submit a new version of the series with this change. > So, for the platforms do not support CPU hot-plug. It could benefit the boot > performance. > For the platforms supporting CPU hot-plug. It will have no any impact. > > Introducing new PCD is good idea to benefit boot performance on S3 path, but > it is some complex. > For example, if DXE phase disables some CPU and do reboot. Real platform may > need another policy to set this PCD used by PEI phase. > > Could you file one bugzilla for this new PCD requirement? I filed <https://bugzilla.tianocore.org/show_bug.cgi?id=251> and assigned it to you; I hope that's alright. Feel free to edit the BZ title; I might not have captured the use case exactly. Thanks! Laszlo > Thanks! > Jeff > > -----Original Message----- > From: Laszlo Ersek [mailto:[email protected]] > Sent: Thursday, November 24, 2016 12:05 AM > To: Fan, Jeff; edk2-devel-01 > Cc: Igor Mammedov > Subject: Re: [PATCH 3/4] UefiCpuPkg/MpInitLib: allow platforms to provide a > known CPU count upfront > > On 11/23/16 06:36, Fan, Jeff wrote: >> Laszlo, >> >> I ever submitted one bugzilla for such request at >> https://bugzilla.tianocore.org/show_bug.cgi?id=116 > > Huh, I didn't know about that. :) > >> My point is not to add new PCD and just to use existing PCD >> PcdCpuMaxLogicalProcessorNumber that supports dynamic type. >> >> Platform Peim could update it is platform pei module before memory ready. >> >> MpInitLib just exits from wait loop once the discovered processors >> number reaches the PcdCpuMaxLogicalProcessorNumber. >> >> Does it meet your requirement? > > This was the first idea I had as well, yes. However, I considered the > following case: > > - QEMU is started (and the guest is booted) with N online processors and M > > N *possible* processors. > > - During OS runtime, the user hotplugs one or more additional processors, so > that the number of currently online processors is now larger than N. > > - The user suspends and resumes (S3) the virtual machine. > > - During S3 resume, QEMU will report the increased number of boot processors. > (At S3 resume, QEMU specifically refreshes the fw_cfg item that returns this > value to the firmware, from the increased number of > VCPUs.) > > - During S3 resume, we cannot modify PcdCpuMaxLogicalProcessorNumber any > longer, because the UefiCpuPkg modules that consume the PCD have already > allocated their data structures using the first (lower) value. Those > allocations cannot be resized / extended during S3 resume. > > So the idea is, set PcdCpuMaxLogicalProcessorNumber statically to the number > of the expected maximum VCPU count, which will ensure the proper allocations > during first boot. Then allow PcdCpuKnownLogicalProcessorNumber to change -- > hot-unplug is also possible -- from normal boot to S3 resume. > > (The concept of "expected maximum" is not specific to VCPU hotplug in QEMU, > it applies to memory (DIMM) hotplug and memory ballooning as well.) > > Of course, I don't know (or expect) that the UefiCpuPkg modules fully support > this use case right now; it's just that I didn't want to > *prevent* this use case. So I figured I'd preserve the original role of > PcdCpuMaxLogicalProcessorNumber, and introduce a more dynamic value for the > "current boot VCPU count". > > Do you agree that it makes sense to keep these quantities separate? > > If you think we should just set PcdCpuMaxLogicalProcessorNumber dynamically > at this point, and introduce no new PCD for now, I can do that too. I just > wanted to make sure that we discuss VCPU hot-plug / hot-unplug and S3 resume > first. > > Thanks! > Laszlo > >> >> Thanks! >> Jeff >> >> -----Original Message----- >> From: Laszlo Ersek [mailto:[email protected]] >> Sent: Wednesday, November 23, 2016 4:26 AM >> To: edk2-devel-01 >> Cc: Igor Mammedov; Fan, Jeff >> Subject: [PATCH 3/4] UefiCpuPkg/MpInitLib: allow platforms to provide >> a known CPU count upfront >> >> On QEMU/KVM, the VCPU topology for a guest is specified dynamically. It can >> be a low number or a high number. >> >> Waiting for PcdCpuApInitTimeOutInMicroSeconds during the initial AP >> collection is impractical, because in a VM, the time needed for an AP to >> wake up can vary widely: >> >> - if the APs report back quickly, then we could be wasting time, >> >> - if the APs report back late (due to scheduling artifacts or VCPU >> over-subscription on the virtualization host), then the timeout can >> elapse before all APs increment CpuMpData->CpuCount in >> ApWakeupFunction(). >> >> Trying to set PcdCpuApInitTimeOutInMicroSeconds dynamically is also >> impractical, as scheduling artifacts on the KVM host may delay AP threads >> arbitrarily. >> >> Instead, allow OVMF (and other platforms) to tell MpInitLib the number of >> boot-time CPUs in advance. >> >> Cc: Igor Mammedov <[email protected]> >> Cc: Jeff Fan <[email protected]> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <[email protected]> >> --- >> UefiCpuPkg/UefiCpuPkg.dec | 11 +++++++++++ >> UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 1 + >> UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf | 4 ++-- >> UefiCpuPkg/Library/MpInitLib/MpLib.c | 16 ++++++++++++---- >> 4 files changed, 26 insertions(+), 6 deletions(-) >> >> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec >> index ca560398bbef..35903c4386e4 100644 >> --- a/UefiCpuPkg/UefiCpuPkg.dec >> +++ b/UefiCpuPkg/UefiCpuPkg.dec >> @@ -225,5 +225,16 @@ [PcdsDynamic, PcdsDynamicEx] >> # @ValidList 0x80000001 | 0 >> >> gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress|0x0|UINT64|0x600000 >> 11 >> >> + ## On platforms where the number of boot-time CPUs can be >> + dynamically # retrieved from a platform-specific information >> + source, the BSP does not # have to wait for >> + PcdCpuApInitTimeOutInMicroSeconds in order to detect all # APs for >> + the first time. On such platforms, this PCD specifies the number # >> + of CPUs available at boot. If the platform doesn't support this >> + feature, # this PCD should be set to 0. The platform is >> + responsible for ensuring that # this PCD is never set to a value larger >> than # PcdCpuMaxLogicalProcessorNumber. >> + # @Prompt Known number of CPUs available at boot. >> + >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownLogicalProcessorNumber|0|UINT32 >> + |0 >> + x60000012 >> + >> [UserExtensions.TianoCore."ExtraFiles"] >> UefiCpuPkgExtra.uni >> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf >> b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf >> index 11b230174ec8..dc18eaf6152d 100644 >> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf >> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf >> @@ -61,6 +61,7 @@ [Guids] >> >> [Pcd] >> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## >> CONSUMES >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownLogicalProcessorNumber ## >> SOMETIMES_CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ## >> SOMETIMES_CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ## >> CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress ## >> CONSUMES >> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf >> b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf >> index 0c6873da79db..2bcfa70ae7e5 100644 >> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf >> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf >> @@ -61,10 +61,10 @@ [Ppis] >> >> [Pcd] >> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## >> CONSUMES >> - gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ## >> CONSUMES >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownLogicalProcessorNumber ## >> CONSUMES >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ## >> SOMETIMES_CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ## >> CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress ## >> CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize ## >> CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode ## >> CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate ## >> SOMETIMES_CONSUMES >> - >> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c >> b/UefiCpuPkg/Library/MpInitLib/MpLib.c >> index 15dbfa1e7d6c..f7dfbd5bad13 100644 >> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c >> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c >> @@ -707,6 +707,7 @@ WakeUpAP ( >> CPU_AP_DATA *CpuData; >> BOOLEAN ResetVectorRequired; >> CPU_INFO_IN_HOB *CpuInfoInHob; >> + UINT32 KnownProcessorCount; >> >> CpuMpData->FinishedCount = 0; >> ResetVectorRequired = FALSE; >> @@ -745,10 +746,17 @@ WakeUpAP ( >> SendInitSipiSipiAllExcludingSelf ((UINT32) ExchangeInfo->BufferStart); >> } >> if (CpuMpData->InitFlag == ApInitConfig) { >> - // >> - // Wait for all potential APs waken up in one specified period >> - // >> - MicroSecondDelay (PcdGet32(PcdCpuApInitTimeOutInMicroSeconds)); >> + KnownProcessorCount = PcdGet32 (PcdCpuKnownLogicalProcessorNumber); >> + if (KnownProcessorCount > 0) { >> + while (CpuMpData->FinishedCount < (KnownProcessorCount - 1)) { >> + CpuPause (); >> + } >> + } else { >> + // >> + // Wait for all potential APs waken up in one specified period >> + // >> + MicroSecondDelay (PcdGet32(PcdCpuApInitTimeOutInMicroSeconds)); >> + } >> } else { >> // >> // Wait all APs waken up if this is not the 1st broadcast of >> SIPI >> -- >> 2.9.2 >> >> > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
