From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 4285881EDC for ; Thu, 24 Nov 2016 05:49:04 -0800 (PST) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga104.fm.intel.com with ESMTP; 24 Nov 2016 05:49:03 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,543,1473145200"; d="scan'208";a="35200282" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga005.fm.intel.com with ESMTP; 24 Nov 2016 05:49:03 -0800 Received: from fmsmsx101.amr.corp.intel.com (10.18.124.199) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 24 Nov 2016 05:49:03 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx101.amr.corp.intel.com (10.18.124.199) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 24 Nov 2016 05:49:03 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.239]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.138]) with mapi id 14.03.0248.002; Thu, 24 Nov 2016 21:49:01 +0800 From: "Fan, Jeff" To: Laszlo Ersek , edk2-devel-01 Thread-Topic: [PATCH 3/4] UefiCpuPkg/MpInitLib: allow platforms to provide a known CPU count upfront Thread-Index: AQHSRP6+fDrvPtep9EC3iX58XqQ9uaDmBhkAgAAwWACAARkqkIAADM0AgADMfmA= Date: Thu, 24 Nov 2016 13:49:00 +0000 Message-ID: <542CF652F8836A4AB8DBFAAD40ED192A4A2E7286@shsmsx102.ccr.corp.intel.com> References: <20161122202619.12594-1-lersek@redhat.com> <20161122202619.12594-4-lersek@redhat.com> <542CF652F8836A4AB8DBFAAD40ED192A4A2E573F@shsmsx102.ccr.corp.intel.com> <17a2489c-467c-2dba-4dbc-2b1444340905@redhat.com> <542CF652F8836A4AB8DBFAAD40ED192A4A2E66C1@shsmsx102.ccr.corp.intel.com> <3d6def30-5606-941b-6d44-de99b675bfbf@redhat.com> In-Reply-To: <3d6def30-5606-941b-6d44-de99b675bfbf@redhat.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMWZjM2FjZjYtNDIxNS00Zjc0LThjNmYtNzk3MGY2MmYxNTBlIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6ImJEV05IK2tFajdcLzRRVWNqaHZYUWs0MjRyeklMclwvUlRaXC9VazhndE9Pcmc9In0= x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH 3/4] UefiCpuPkg/MpInitLib: allow platforms to provide a known CPU count upfront X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 Nov 2016 13:49:04 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, Thanks! Jeff -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com]=20 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, >=20 > 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 b= oot performance. > For the platforms supporting CPU hot-plug. It will have no any impact. >=20 > Introducing new PCD is good idea to benefit boot performance on S3 path, = but it is some complex.=20 > 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. >=20 > Could you file one bugzilla for this new PCD requirement? I filed 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 >=20 > -----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=20 > provide a known CPU count upfront >=20 > 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=3D116 >=20 > Huh, I didn't know about that. :) >=20 >> My point is not to add new PCD and just to use existing PCD=20 >> 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=20 >> number reaches the PcdCpuMaxLogicalProcessorNumber. >> >> Does it meet your requirement? >=20 > This was the first idea I had as well, yes. However, I considered the fol= lowing case: >=20 > - QEMU is started (and the guest is booted) with N online processors and = M > N *possible* processors. >=20 > - During OS runtime, the user hotplugs one or more additional processors,= so that the number of currently online processors is now larger than N. >=20 > - The user suspends and resumes (S3) the virtual machine. >=20 > - During S3 resume, QEMU will report the increased number of boot=20 > processors. (At S3 resume, QEMU specifically refreshes the fw_cfg item=20 > that returns this value to the firmware, from the increased number of > VCPUs.) >=20 > - During S3 resume, we cannot modify PcdCpuMaxLogicalProcessorNumber any = longer, because the UefiCpuPkg modules that consume the PCD have already al= located their data structures using the first (lower) value. Those allocati= ons cannot be resized / extended during S3 resume. >=20 > So the idea is, set PcdCpuMaxLogicalProcessorNumber statically to the num= ber of the expected maximum VCPU count, which will ensure the proper alloca= tions during first boot. Then allow PcdCpuKnownLogicalProcessorNumber to ch= ange -- hot-unplug is also possible -- from normal boot to S3 resume. >=20 > (The concept of "expected maximum" is not specific to VCPU hotplug in=20 > QEMU, it applies to memory (DIMM) hotplug and memory ballooning as=20 > well.) >=20 > Of course, I don't know (or expect) that the UefiCpuPkg modules fully=20 > 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 P= cdCpuMaxLogicalProcessorNumber, and introduce a more dynamic value for the = "current boot VCPU count". >=20 > Do you agree that it makes sense to keep these quantities separate? >=20 > If you think we should just set PcdCpuMaxLogicalProcessorNumber dynamical= ly at this point, and introduce no new PCD for now, I can do that too. I ju= st wanted to make sure that we discuss VCPU hot-plug / hot-unplug and S3 re= sume first. >=20 > Thanks! > Laszlo >=20 >> >> 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=20 >> 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 coll= ection is impractical, because in a VM, the time needed for an AP to wake u= p 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 impr= actical, as scheduling artifacts on the KVM host may delay AP threads arbit= rarily. >> >> Instead, allow OVMF (and other platforms) to tell MpInitLib the number o= f boot-time CPUs in advance. >> >> Cc: Igor Mammedov >> Cc: Jeff Fan >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek >> --- >> UefiCpuPkg/UefiCpuPkg.dec | 11 +++++++++++ >> UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 1 + UefiCpuPkg/Librar= y/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=20 >> index ca560398bbef..35903c4386e4 100644 >> --- a/UefiCpuPkg/UefiCpuPkg.dec >> +++ b/UefiCpuPkg/UefiCpuPkg.dec >> @@ -225,5 +225,16 @@ [PcdsDynamic, PcdsDynamicEx] >> # @ValidList 0x80000001 | 0 >> =20 >> gUefiCpuPkgTokenSpaceGuid.PcdCpuHotPlugDataAddress|0x0|UINT64|0x60000 >> 0 >> 11 >> =20 >> + ## On platforms where the number of boot-time CPUs can be=20 >> + dynamically # retrieved from a platform-specific information=20 >> + source, the BSP does not # have to wait for=20 >> + PcdCpuApInitTimeOutInMicroSeconds in order to detect all # APs=20 >> + for the first time. On such platforms, this PCD specifies the=20 >> + number # of CPUs available at boot. If the platform doesn't=20 >> + support this feature, # this PCD should be set to 0. The platform=20 >> + is responsible for ensuring that # this PCD is never set to a value = larger than # PcdCpuMaxLogicalProcessorNumber. >> + # @Prompt Known number of CPUs available at boot. >> + =20 >> + 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] >> =20 >> [Pcd] >> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## C= ONSUMES >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownLogicalProcessorNumber ## S= OMETIMES_CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ## S= OMETIMES_CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ## C= ONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress ## C= ONSUMES >> 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] >> =20 >> [Pcd] >> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## C= ONSUMES >> - gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ## C= ONSUMES >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownLogicalProcessorNumber ## C= ONSUMES >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ## S= OMETIMES_CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ## C= ONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress ## C= ONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize ## C= ONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode ## C= ONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate ## S= OMETIMES_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; >> =20 >> CpuMpData->FinishedCount =3D 0; >> ResetVectorRequired =3D FALSE; >> @@ -745,10 +746,17 @@ WakeUpAP ( >> SendInitSipiSipiAllExcludingSelf ((UINT32) ExchangeInfo->BufferSt= art); >> } >> if (CpuMpData->InitFlag =3D=3D ApInitConfig) { >> - // >> - // Wait for all potential APs waken up in one specified period >> - // >> - MicroSecondDelay (PcdGet32(PcdCpuApInitTimeOutInMicroSeconds)); >> + KnownProcessorCount =3D PcdGet32 (PcdCpuKnownLogicalProcessorNumb= er); >> + 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=20 >> SIPI >> -- >> 2.9.2 >> >> >=20