| Message ID | 20161122202619.12594-4-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
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:lersek@redhat.com] > 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 <imammedo@redhat.com> > Cc: Jeff Fan <jeff.fan@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > 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|0x60000011 > > + ## 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 edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo, Please use existing PcdCpuMaxLogicalProcessorNumber firstly. 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? Thanks! Jeff -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] 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:lersek@redhat.com] > 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 <imammedo@redhat.com> > Cc: Jeff Fan <jeff.fan@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > 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 edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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:lersek@redhat.com] > 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:lersek@redhat.com] >> 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 <imammedo@redhat.com> >> Cc: Jeff Fan <jeff.fan@intel.com> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> 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 edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Laszlo, Thanks! Jeff -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Thursday, November 24, 2016 5:37 PM 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/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:lersek@redhat.com] > 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:lersek@redhat.com] >> 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 <imammedo@redhat.com> >> Cc: Jeff Fan <jeff.fan@intel.com> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> 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|0x60000 >> 0 >> 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|UINT3 >> + 2 >> + |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 edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 11/24/16 19:32, Igor Mammedov wrote: > On Thu, 24 Nov 2016 10:36:38 +0100 > Laszlo Ersek <lersek@redhat.com> 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:lersek@redhat.com] >>> 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:lersek@redhat.com] >>>> 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 <imammedo@redhat.com> >>>> Cc: Jeff Fan <jeff.fan@intel.com> >>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >>>> --- >>>> 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 edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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|0x60000011 + ## 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|0x60000012 + [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
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 <imammedo@redhat.com> Cc: Jeff Fan <jeff.fan@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- 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(-) -- 2.9.2 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel