On 11/24/16 19:32, Igor Mammedov wrote: > On Thu, 24 Nov 2016 10:36:38 +0100 > Laszlo Ersek <[email protected]> wrote: > >> 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. > Using present count of CPUs might potentially break things if > there were hotplugged CPUs with follow up S3 and wakeup dute this code: > > MpInitLibInitialize() > OldCpuMpData = GetCpuMpDataFromGuidedHob (); > > if (OldCpuMpData == NULL) { > > MaxLogicalProcessorNumber = PcdGet32(PcdCpuMaxLogicalProcessorNumber); > > } else { > > MaxLogicalProcessorNumber = OldCpuMpData->CpuCount; > > }
Yes, this is a correct assessment. And, we're aware of it in general; I raised the same scenario in my previous email. It seems that Jeff prefers the simpler solution for now. The hotplug case is tracked by <https://bugzilla.tianocore.org/show_bug.cgi?id=251>. Thanks Laszlo > I've traced this part of code under QEMU with PcdCpuMaxLogicalProcessorNumber > set to present number of CPUs and currently OldCpuMpData is always NULL on > resume > so PcdCpuMaxLogicalProcessorNumber gets updated CPU count from platform and > it resumes seemingly fine but I won't bet that it's correct. > > here is a patch I've used for this experiment: > > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc > index 3f4d42d..ce80690 100644 > --- a/OvmfPkg/OvmfPkgIa32.dsc > +++ b/OvmfPkg/OvmfPkgIa32.dsc > @@ -455,6 +455,7 @@ > > ################################################################################ > > [PcdsDynamicDefault] > + gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|10 > gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0 > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0 > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0 > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > index 5688475..f7697b1 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -461,6 +461,7 @@ > > ################################################################################ > > [PcdsDynamicDefault] > + gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|10 > gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0 > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0 > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0 > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index dcf64b9..4034989 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -460,6 +460,7 @@ > > ################################################################################ > > [PcdsDynamicDefault] > + gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|10 > gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved|0 > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64|0 > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase|0 > diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c > index d00a570..e205fe7 100644 > --- a/OvmfPkg/PlatformPei/MemDetect.c > +++ b/OvmfPkg/PlatformPei/MemDetect.c > @@ -342,6 +342,7 @@ PublishPeiMemory ( > UINT64 MemorySize; > UINT32 LowerMemorySize; > UINT32 PeiMemoryCap; > + UINT16 CpuCount; > > LowerMemorySize = GetSystemMemorySizeBelow4gb (); > if (FeaturePcdGet (PcdSmmSmramRequire)) { > @@ -351,6 +352,12 @@ PublishPeiMemory ( > LowerMemorySize -= FixedPcdGet8 (PcdQ35TsegMbytes) * SIZE_1MB; > } > > + QemuFwCfgSelectItem (QemuFwCfgItemSmpCpuCount); > + //QemuFwCfgSelectItem (QemuFwCfgItemMaximumCpuCount); > + CpuCount = QemuFwCfgRead16 (); > + Status = PcdSet32S(PcdCpuMaxLogicalProcessorNumber, CpuCount); > + DEBUG ((EFI_D_INFO, "XXXXXXXXXXXXXX: QemuFwCfgItemSmpCpuCount: %u\n", > CpuCount)); > + > // > // If S3 is supported, then the S3 permanent PEI memory is placed next, > // downwards. Its size is primarily dictated by CpuMpPei. The formula below > >>> 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
