From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (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 1056881ED1 for ; Wed, 23 Nov 2016 17:18:15 -0800 (PST) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga103.fm.intel.com with ESMTP; 23 Nov 2016 17:18:11 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,689,1473145200"; d="scan'208";a="35020375" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga005.fm.intel.com with ESMTP; 23 Nov 2016 17:18:11 -0800 Received: from fmsmsx113.amr.corp.intel.com (10.18.116.7) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 23 Nov 2016 17:18:11 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX113.amr.corp.intel.com (10.18.116.7) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 23 Nov 2016 17:18:10 -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 09:18:08 +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+fDrvPtep9EC3iX58XqQ9uaDmBhkAgAAwWACAARkqkA== Date: Thu, 24 Nov 2016 01:18:08 +0000 Message-ID: <542CF652F8836A4AB8DBFAAD40ED192A4A2E66C1@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> In-Reply-To: <17a2489c-467c-2dba-4dbc-2b1444340905@redhat.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMzljMzk1MWEtNmM3Ny00OWU3LTg5YWEtNzZlYTQ0OGZkMjQ4IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IlpWMmVoSVJ6OXZRRFVxVFJOaVwvT0VuQUdPNXpJMzErenBQVDI3dUc1UW5jPSJ9 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 01:18:15 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, Please use existing PcdCpuMaxLogicalProcessorNumber firstly. So, for the platforms do not support CPU hot-plug. It could benefit the boo= t 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, bu= t it is some complex.=20 For example, if DXE phase disables some CPU and do reboot. Real platform ma= y 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]=20 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, >=20 > I ever submitted one bugzilla for such request at > https://bugzilla.tianocore.org/show_bug.cgi?id=3D116 Huh, I didn't know about that. :) > My point is not to add new PCD and just to use existing PCD=20 > PcdCpuMaxLogicalProcessorNumber that supports dynamic type. >=20 > Platform Peim could update it is platform pei module before memory ready. >=20 > MpInitLib just exits from wait loop once the discovered processors=20 > number reaches the PcdCpuMaxLogicalProcessorNumber. >=20 > Does it meet your requirement? This was the first idea I had as well, yes. However, I considered the follo= wing 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, s= o 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 processor= s. (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 lo= nger, because the UefiCpuPkg modules that consume the PCD have already allo= cated their data structures using the first (lower) value. Those allocation= s cannot be resized / extended during S3 resume. So the idea is, set PcdCpuMaxLogicalProcessorNumber statically to the numbe= r of the expected maximum VCPU count, which will ensure the proper allocati= ons during first boot. Then allow PcdCpuKnownLogicalProcessorNumber to chan= ge -- 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 suppo= rt 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 Pcd= CpuMaxLogicalProcessorNumber, and introduce a more dynamic value for the "c= urrent 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 resu= me first. Thanks! Laszlo >=20 > Thanks! > Jeff >=20 > -----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 >=20 > On QEMU/KVM, the VCPU topology for a guest is specified dynamically. It c= an be a low number or a high number. >=20 > Waiting for PcdCpuApInitTimeOutInMicroSeconds during the initial AP colle= ction is impractical, because in a VM, the time needed for an AP to wake up= can vary widely: >=20 > - if the APs report back quickly, then we could be wasting time, >=20 > - 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(). >=20 > Trying to set PcdCpuApInitTimeOutInMicroSeconds dynamically is also impra= ctical, as scheduling artifacts on the KVM host may delay AP threads arbitr= arily. >=20 > Instead, allow OVMF (and other platforms) to tell MpInitLib the number of= boot-time CPUs in advance. >=20 > 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/Library= /MpInitLib/PeiMpInitLib.inf | 4 ++-- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 16 ++++++++++++---- > 4 files changed, 26 insertions(+), 6 deletions(-) >=20 > 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|0x600000 > 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 for=20 > + the first time. On such platforms, this PCD specifies the number # =20 > + of CPUs available at boot. If the platform doesn't support this=20 > + feature, # this PCD should be set to 0. The platform is=20 > + responsible for ensuring that # this PCD is never set to a value larg= er than # PcdCpuMaxLogicalProcessorNumber. > + # @Prompt Known number of CPUs available at boot. > + =20 > + gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownLogicalProcessorNumber|0|UINT32 > + |0 > + x60000012 > + > [UserExtensions.TianoCore."ExtraFiles"] > UefiCpuPkgExtra.uni > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf=20 > 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 ## CO= NSUMES > + gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownLogicalProcessorNumber ## SO= METIMES_CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ## SO= METIMES_CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ## CO= NSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress ## CO= NSUMES > diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf=20 > 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 ## CO= NSUMES > - gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ## CO= NSUMES > + gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownLogicalProcessorNumber ## CO= NSUMES > + gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ## SO= METIMES_CONSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ## CO= NSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress ## CO= NSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize ## CO= NSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode ## CO= NSUMES > gUefiCpuPkgTokenSpaceGuid.PcdCpuApTargetCstate ## SO= METIMES_CONSUMES > - > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c=20 > 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->BufferSta= rt); > } > if (CpuMpData->InitFlag =3D=3D ApInitConfig) { > - // > - // Wait for all potential APs waken up in one specified period > - // > - MicroSecondDelay (PcdGet32(PcdCpuApInitTimeOutInMicroSeconds)); > + KnownProcessorCount =3D PcdGet32 (PcdCpuKnownLogicalProcessorNumbe= r); > + 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 >=20