From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.100; helo=mga07.intel.com; envelope-from=eric.dong@intel.com; receiver=edk2-devel@lists.01.org Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (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 D2888209605A3 for ; Thu, 12 Jul 2018 17:46:01 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Jul 2018 17:46:00 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,345,1526367600"; d="scan'208";a="64331871" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by FMSMGA003.fm.intel.com with ESMTP; 12 Jul 2018 17:45:43 -0700 Received: from fmsmsx126.amr.corp.intel.com (10.18.125.43) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 12 Jul 2018 17:45:43 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX126.amr.corp.intel.com (10.18.125.43) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 12 Jul 2018 17:45:42 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.57]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.17]) with mapi id 14.03.0319.002; Fri, 13 Jul 2018 08:45:40 +0800 From: "Dong, Eric" To: Laszlo Ersek , "edk2-devel@lists.01.org" CC: "Ni, Ruiyu" Thread-Topic: [edk2] [Patch v2 1/3] UefiCpuPkg/MpInitLib: Relocate uCode to memory to save time. Thread-Index: AQHUGc4Q4YUP7ctK0UKDDhWwjdN7PaSLnXgAgAAECACAALBPIA== Date: Fri, 13 Jul 2018 00:45:40 +0000 Message-ID: References: <20180712104927.12220-1-eric.dong@intel.com> <20180712104927.12220-2-eric.dong@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [Patch v2 1/3] UefiCpuPkg/MpInitLib: Relocate uCode to memory to save time. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 13 Jul 2018 00:46:02 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Laszlo, > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Laszlo Ersek > Sent: Friday, July 13, 2018 6:14 AM > To: Dong, Eric ; edk2-devel@lists.01.org > Cc: Ni, Ruiyu > Subject: Re: [edk2] [Patch v2 1/3] UefiCpuPkg/MpInitLib: Relocate uCode t= o > memory to save time. >=20 > On 07/12/18 23:59, Laszlo Ersek wrote: > > On 07/12/18 12:49, Eric Dong wrote: > >> Read uCode from memory has better performance than from flash. > >> But it needs extra effort to let BSP copy uCode from flash to memory. > >> Also BSP already enable cache in SEC phase, so it use less time to > >> relocate uCode from flash to memory. After verification, if system > >> has more than one processor, it will reduce some time if load uCode > >> from memory. > >> > >> This change enable this optimization. > >> > >> Cc: Laszlo Ersek > >> Cc: Ruiyu Ni > >> Contributed-under: TianoCore Contribution Agreement 1.1 > >> Signed-off-by: Eric Dong > >> --- > >> UefiCpuPkg/Library/MpInitLib/MpLib.c | 34 > >> +++++++++++++++++++++++++++++++++- > >> 1 file changed, 33 insertions(+), 1 deletion(-) > >> > >> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c > >> b/UefiCpuPkg/Library/MpInitLib/MpLib.c > >> index 108eea0a6f..c3cd6d7d51 100644 > >> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > >> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > >> @@ -1520,6 +1520,7 @@ MpInitLibInitialize ( > >> UINTN ApResetVectorSize; > >> UINTN BackupBufferAddr; > >> UINTN ApIdtBase; > >> + VOID *MicrocodePatchInRam; > >> > >> OldCpuMpData =3D GetCpuMpDataFromGuidedHob (); > >> if (OldCpuMpData =3D=3D NULL) { > >> @@ -1587,8 +1588,39 @@ MpInitLibInitialize ( > >> CpuMpData->SwitchBspFlag =3D FALSE; > >> CpuMpData->CpuData =3D (CPU_AP_DATA *) (CpuMpData + 1); > >> CpuMpData->CpuInfoInHob =3D (UINT64) (UINTN) (CpuMpData- > >CpuData + MaxLogicalProcessorNumber); > >> - CpuMpData->MicrocodePatchAddress =3D PcdGet64 > (PcdCpuMicrocodePatchAddress); > >> CpuMpData->MicrocodePatchRegionSize =3D PcdGet64 > >> (PcdCpuMicrocodePatchRegionSize); > >> + // > >> + // If platform has more than one CPU, relocate microcode to memory > >> + to reduce // loading microcode time. > >> + // > >> + MicrocodePatchInRam =3D NULL; > >> + if (MaxLogicalProcessorNumber > 1) { > >> + MicrocodePatchInRam =3D AllocatePages ( > >> + EFI_SIZE_TO_PAGES ( > >> + (UINTN)CpuMpData->MicrocodePatchRegionS= ize > >> + ) > >> + ); > >> + ASSERT (MicrocodePatchInRam !=3D NULL); } if > >> + (MicrocodePatchInRam =3D=3D NULL) { > >> + // > >> + // there is only one processor, or no microcode patch is availabl= e, or > >> + // memory allocation failed > >> + // > >> + CpuMpData->MicrocodePatchAddress =3D PcdGet64 > >> + (PcdCpuMicrocodePatchAddress); } else { > >> + // > >> + // there are multiple processors, and a microcode patch is availa= ble, > and > >> + // memory allocation succeeded > >> + // > >> + CopyMem ( > >> + MicrocodePatchInRam, > >> + (VOID *)(UINTN)PcdGet64 (PcdCpuMicrocodePatchAddress), > >> + (UINTN)CpuMpData->MicrocodePatchRegionSize > >> + ); > >> + CpuMpData->MicrocodePatchAddress =3D (UINTN)MicrocodePatchInRam; > >> + } > >> + > >> InitializeSpinLock(&CpuMpData->MpLock); > >> > >> // > >> > > > > Reviewed-by: Laszlo Ersek > > >=20 > Sorry, I have to take that back -- please do not commit this patch. >=20 > For this version of the patch: >=20 > Nacked-by: Laszlo Ersek >=20 > The ASSERT() is wrong. Again, in the code above, AllocatePages() can retu= rn > NULL not only because of memory allocation failure, but also because the > number of pages to allocate can be zero! If the platform has no microcode > patch to apply. >=20 > I knew this full well when I suggested the code, but then I forgot about = it when > you mentioned the ASSERT(). I think you also knew about it, and forgot ab= out > it too. :) >=20 > In particular, this patch would make it *impossible* to boot OVMF with > multiple processors, because OVMF *never* provides a microcode update. >=20 > So, please remove the ASSERT. >=20 Agree, I removed ASSERT code and send V3 patches. > Alternatively, you could modify the ASSERT() like this: >=20 > // > // if we attempt actual memory allocation, we expect it to succeed > // > ASSERT ( > (CpuMpData->MicrocodePatchRegionSize =3D=3D 0) || > (MicrocodePatchInRam !=3D NULL) > ); >=20 > Thanks, > Laszlo > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel