From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by mx.groups.io with SMTP id smtpd.web10.104.1577166528940084526 for ; Mon, 23 Dec 2019 21:48:49 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.20, mailfrom: hao.a.wu@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Dec 2019 21:48:48 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,350,1571727600"; d="scan'208";a="249717989" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga002.fm.intel.com with ESMTP; 23 Dec 2019 21:48:48 -0800 Received: from fmsmsx126.amr.corp.intel.com (10.18.125.43) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 23 Dec 2019 21:48:47 -0800 Received: from shsmsx154.ccr.corp.intel.com (10.239.6.54) by FMSMSX126.amr.corp.intel.com (10.18.125.43) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 23 Dec 2019 21:48:47 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.90]) by SHSMSX154.ccr.corp.intel.com ([169.254.7.71]) with mapi id 14.03.0439.000; Tue, 24 Dec 2019 13:48:45 +0800 From: "Wu, Hao A" To: "Ni, Ray" , "devel@edk2.groups.io" CC: "Dong, Eric" , Laszlo Ersek , "Zeng, Star" , "Fu, Siyuan" , "Kinney, Michael D" Subject: Re: [PATCH v1 2/4] UefiCpuPkg/MpInitLib: Reduce the size when loading microcode patches Thread-Topic: [PATCH v1 2/4] UefiCpuPkg/MpInitLib: Reduce the size when loading microcode patches Thread-Index: AQHVufqnukV33t/cJUe2ax1OS0R4YafIG2KAgACiZ8A= Date: Tue, 24 Dec 2019 05:48:45 +0000 Message-ID: References: <20191224013656.13404-1-hao.a.wu@intel.com> <20191224013656.13404-3-hao.a.wu@intel.com> <734D49CCEBEEF84792F5B80ED585239D5C3A9E32@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C3A9E32@SHSMSX104.ccr.corp.intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: hao.a.wu@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Ni, Ray > Sent: Tuesday, December 24, 2019 11:32 AM > To: Wu, Hao A; devel@edk2.groups.io > Cc: Dong, Eric; Laszlo Ersek; Zeng, Star; Fu, Siyuan; Kinney, Michael D > Subject: RE: [PATCH v1 2/4] UefiCpuPkg/MpInitLib: Reduce the size when > loading microcode patches >=20 > 6 minor comments. Hello Ray, Thanks for the feedbacks. For 1,2,4,5, I will update the patch to address your comments. For 3, my concern is that: ALIGN_VALUE (TotalSize , SIZE_1KB)=20 might have a chance to overflow, how about changing the logic to: if (TotalSize > ALIGN_VALUE (TotalSize, SIZE_1KB) || ALIGN_VALUE (TotalSize, SIZE_1KB) > MAX_UINTN - TotalLoadSize) { goto OnExit; } For 6, the 'ApInitReconfig' flag value is still being used in function ResetProcessorToIdleState(). Also, I found that there are several places in MpInitLib that use: CpuMpData->InitFlag !=3D ApInitDone CpuMpData->InitFlag !=3D ApInitConfig In some if statements. So I prefer the evaluation of the removal of the 'ApInitReconfig' flag valu= e to be handled in another separate patch series. Do you agree? Best Regards, Hao Wu >=20 > > -----Original Message----- > > From: Wu, Hao A > > Sent: Tuesday, December 24, 2019 9:37 AM > > To: devel@edk2.groups.io > > Cc: Wu, Hao A ; Dong, Eric ; N= i, > > Ray ; Laszlo Ersek ; Zeng, Star > > ; Fu, Siyuan ; Kinney, Michae= l > > D > > Subject: [PATCH v1 2/4] UefiCpuPkg/MpInitLib: Reduce the size when > > loading microcode patches > > > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D2429 > > > > This commit will attempt to reduce the copy size when loading the > > microcode patches data from flash into memory. > > > > Such optimization is done by a pre-process of the microcode patch heade= rs > > (on flash). A microcode patch will be loaded into memory only when the > > below 2 criteria are met: > > > > A. With a microcode patch header (which means the data is not padding d= ata > > between microcode patches); > > B. The 'ProcessorSignature' & 'ProcessorFlags' fields in the header mat= ch > > at least one processor within system. > > > > Criterion B will require all the processors to be woken up once to coll= ect > > their CPUID and Platform ID information. Hence, this commit will move t= he > > copy, detect and apply of microcode patch on BSP and APs after all the > > processors have been woken up. > > > > Cc: Eric Dong > > Cc: Ray Ni > > Cc: Laszlo Ersek > > Cc: Star Zeng > > Cc: Siyuan Fu > > Cc: Michael D Kinney > > Signed-off-by: Hao A Wu > > --- > > UefiCpuPkg/Library/MpInitLib/MpLib.h | 24 ++ > > UefiCpuPkg/Library/MpInitLib/Microcode.c | 235 ++++++++++++++++++++ > > UefiCpuPkg/Library/MpInitLib/MpLib.c | 82 ++----- > > 3 files changed, 283 insertions(+), 58 deletions(-) > > > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h > > b/UefiCpuPkg/Library/MpInitLib/MpLib.h > > index 4440dc2701..56b0df664a 100644 > > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h > > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h > > @@ -44,6 +44,20 @@ > > #define CPU_SWITCH_STATE_LOADED 2 > > > > // > > +// Default maximum number of entries to store the microcode patches > > information > > +// > > +#define DEFAULT_MAX_MICROCODE_PATCH_NUM 8 > > + > > +// > > +// Data structure for microcode patch information > > +// > > +typedef struct { > > + UINTN Address; > > + UINTN Size; > > + UINTN AlignedSize; > > +} MICROCODE_PATCH_INFO; > > + > > +// > > // CPU exchange information for switch BSP > > // > > typedef struct { > > @@ -576,6 +590,16 @@ MicrocodeDetect ( > > ); > > > > /** > > + Load the required microcode patches data into memory. > > + > > + @param[in, out] CpuMpData The pointer to CPU MP Data structure. > > +**/ > > +VOID > > +LoadMicrocodePatch ( > > + IN OUT CPU_MP_DATA *CpuMpData > > + ); > > + > > +/** > > Detect whether Mwait-monitor feature is supported. > > > > @retval TRUE Mwait-monitor feature is supported. > > diff --git a/UefiCpuPkg/Library/MpInitLib/Microcode.c > > b/UefiCpuPkg/Library/MpInitLib/Microcode.c > > index 199b1f23ce..68088b26a5 100644 > > --- a/UefiCpuPkg/Library/MpInitLib/Microcode.c > > +++ b/UefiCpuPkg/Library/MpInitLib/Microcode.c > > @@ -331,3 +331,238 @@ Done: > > MicroData [0x%08x], Revision [0x%08x]\n", Eax.Uint32, Processor= Flags, > > (UINTN) MicrocodeData, LatestRevision)); > > } > > } > > + > > +/** > > + Actual worker function that loads the required microcode patches int= o > > memory. > > + > > + @param[in, out] CpuMpData The pointer to CPU MP Data struc= ture. > > + @param[in] PatchInfoBuffer The pointer to an array of infor= mation > on > > + the microcode patches that will = be loaded > > + into memory. > > + @param[in] PatchNumber The number of microcode patches = that > > will > > + be loaded into memory. > > + @param[in] TotalLoadSize The total size of all the microc= ode > > + patches to be loaded. > > +**/ > > +VOID > > +LoadMicrocodePatchWorker ( > > + IN OUT CPU_MP_DATA *CpuMpData, > > + IN MICROCODE_PATCH_INFO *PatchInfoBuffer, > > + IN UINTN PatchNumber, >=20 > 1. "PatchInfoBuffer" -> "Patches"? > "PatchNumber" -> "PatchCount"? >=20 > > + // > > + // Load all the required microcode patches into memory > > + // > > + for (Walker =3D MicrocodePatchInRam, Index =3D 0; Index < PatchNumbe= r; > > Index++) { > > + CopyMem ( > > + Walker, > > + (VOID *) PatchInfoBuffer[Index].Address, > > + PatchInfoBuffer[Index].Size > > + ); > > + > > + if (PatchInfoBuffer[Index].AlignedSize > PatchInfoBuffer[Index].Si= ze) { >=20 > 2. This if-check is not needed because AlignedSize always >=3D Size. > Below ZeroMem() will directly return due to the 2nd parameter is 0. >=20 > > + // > > + // Zero-fill the padding area > > + // > > + ZeroMem ( > > + Walker + PatchInfoBuffer[Index].Size, > > + PatchInfoBuffer[Index].AlignedSize - PatchInfoBuffer[Index].Si= ze > > + ); > > + } > > + >=20 > > + if (TotalSize > MAX_UINTN - TotalLoadSize || > > + ALIGN_VALUE (TotalSize, SIZE_1KB) > MAX_UINTN - TotalLoadSiz= e) { > > + goto OnExit; > > + } > 3. ALIGN_VALUE (TotalSize , SIZE_1KB) always >=3D TotalSize. > So can we only check > ALIGN_VALUE (TotalSize, SIZE_1KB) > MAX_UINTN - TotalLoadSize > ? >=20 >=20 > > + PatchInfoBuffer[PatchNumber - 1].Address =3D (UINTN) > > MicrocodeEntryPoint; > > + PatchInfoBuffer[PatchNumber - 1].Size =3D TotalSize; > > + PatchInfoBuffer[PatchNumber - 1].AlignedSize =3D ALIGN_VALUE > > (TotalSize, SIZE_1KB); >=20 > 4. "PatchNumber" -> "PatchCount"? >=20 > > + TotalLoadSize +=3D PatchInfoBuffer[PatchNumber - 1].AlignedSize; > > + } > > + > > + // > > + // Process the next microcode patch > > + // > > + MicrocodeEntryPoint =3D (CPU_MICROCODE_HEADER *) (((UINTN) > > MicrocodeEntryPoint) + TotalSize); > > + } while (((UINTN) MicrocodeEntryPoint < MicrocodeEnd)); > > + > > + if (PatchNumber =3D=3D 0) { > > + // > > + // No patch needs to be loaded > > + // > > + goto OnExit; >=20 > 5. This "goto" is not necessary. You can call below two lines when PatchC= ount !=3D > 0. > Less "goto" is better. >=20 > > + } > > + > > + DEBUG (( > > + DEBUG_INFO, > > + "%a: 0x%x microcode patches will be loaded into memory, with size > > 0x%x.\n", > > + __FUNCTION__, PatchNumber, TotalLoadSize > > + )); > > + > > + LoadMicrocodePatchWorker (CpuMpData, PatchInfoBuffer, PatchNumber, > > TotalLoadSize); > > + > > +OnExit: > > + if (PatchInfoBuffer !=3D NULL) { > > + FreePool (PatchInfoBuffer); > > + } > > + return; > > +} >=20 > > @@ -1788,7 +1752,6 @@ MpInitLibInitialize ( > > // > > CpuMpData->CpuCount =3D OldCpuMpData->CpuCount; > > CpuMpData->BspNumber =3D OldCpuMpData->BspNumber; > > - CpuMpData->InitFlag =3D ApInitReconfig; >=20 > 6. Do you think that ApInitReconfig definition can be removed?