From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.43, mailfrom: ray.ni@intel.com) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by groups.io with SMTP; Thu, 01 Aug 2019 00:17:46 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 01 Aug 2019 00:17:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,333,1559545200"; d="scan'208";a="172824976" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga008.fm.intel.com with ESMTP; 01 Aug 2019 00:17:46 -0700 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 1 Aug 2019 00:17:46 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx156.amr.corp.intel.com (10.18.116.74) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 1 Aug 2019 00:17:45 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.112]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.62]) with mapi id 14.03.0439.000; Thu, 1 Aug 2019 15:17:44 +0800 From: "Ni, Ray" To: Laszlo Ersek , "Dong, Eric" , "devel@edk2.groups.io" Subject: Re: [Patch] UefiCpuPkg/MpInitLib: Avoid copy twice. Thread-Topic: [Patch] UefiCpuPkg/MpInitLib: Avoid copy twice. Thread-Index: AQHVR3YnM/iKlvTZe0SI8c8fZ6JveabkToyAgAGUgvA= Date: Thu, 1 Aug 2019 07:17:44 +0000 Deferred-Delivery: Thu, 1 Aug 2019 07:17:00 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C267528@SHSMSX104.ccr.corp.intel.com> References: <20190731080102.7292-1-eric.dong@intel.com> <7572eb62-0ba3-904c-8fc8-d3c074472c75@redhat.com> In-Reply-To: <7572eb62-0ba3-904c-8fc8-d3c074472c75@redhat.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Ray Ni > -----Original Message----- > From: Laszlo Ersek > Sent: Wednesday, July 31, 2019 11:08 PM > To: Dong, Eric ; devel@edk2.groups.io > Cc: Ni, Ray > Subject: Re: [Patch] UefiCpuPkg/MpInitLib: Avoid copy twice. >=20 > On 07/31/19 10:01, Eric Dong wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1982 > > > > MpInitLibInitialize in MpLib.c will be invoked on both PEI and DXE CPU > > code, MicrocodeDetect would be performed twice and copy Microcode > from > > flash to memory twice as well, which consider as duplicate work to > > lead longer boot time. > > This patch just use microcode memory copied in PEI phase if exist. > > > > Signed-off-by: Eric Dong > > Cc: Ray Ni > > Cc: Laszlo Ersek > > --- > > UefiCpuPkg/Library/MpInitLib/MpLib.c | 62 > > +++++++++++++++------------- > > 1 file changed, 33 insertions(+), 29 deletions(-) > > > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c > > b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > index 572495ec36..a1ad665564 100644 > > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > > @@ -1607,38 +1607,42 @@ MpInitLibInitialize ( > > CpuMpData->SwitchBspFlag =3D FALSE; > > CpuMpData->CpuData =3D (CPU_AP_DATA *) (CpuMpData + 1); > > CpuMpData->CpuInfoInHob =3D (UINT64) (UINTN) (CpuMpData- > >CpuData + MaxLogicalProcessorNumber); > > - 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->MicrocodePatchRegionSi= ze > > - ) > > - ); > > - } > > - if (MicrocodePatchInRam =3D=3D NULL) { > > - // > > - // there is only one processor, or no microcode patch is available= , or > > - // memory allocation failed > > - // > > - CpuMpData->MicrocodePatchAddress =3D PcdGet64 > (PcdCpuMicrocodePatchAddress); > > - } else { > > + if (OldCpuMpData =3D=3D NULL) { > > + CpuMpData->MicrocodePatchRegionSize =3D PcdGet64 > > + (PcdCpuMicrocodePatchRegionSize); > > // > > - // there are multiple processors, and a microcode patch is availab= le, and > > - // memory allocation succeeded > > + // If platform has more than one CPU, relocate microcode to memory= to > reduce > > + // loading microcode time. > > // > > - CopyMem ( > > - MicrocodePatchInRam, > > - (VOID *)(UINTN)PcdGet64 (PcdCpuMicrocodePatchAddress), > > - (UINTN)CpuMpData->MicrocodePatchRegionSize > > - ); > > - CpuMpData->MicrocodePatchAddress =3D (UINTN)MicrocodePatchInRam; > > + MicrocodePatchInRam =3D NULL; > > + if (MaxLogicalProcessorNumber > 1) { > > + MicrocodePatchInRam =3D AllocatePages ( > > + EFI_SIZE_TO_PAGES ( > > + (UINTN)CpuMpData->MicrocodePatchRegion= Size > > + ) > > + ); > > + } > > + if (MicrocodePatchInRam =3D=3D NULL) { > > + // > > + // there is only one processor, or no microcode patch is availab= le, or > > + // memory allocation failed > > + // > > + CpuMpData->MicrocodePatchAddress =3D PcdGet64 > (PcdCpuMicrocodePatchAddress); > > + } else { > > + // > > + // there are multiple processors, and a microcode patch is avail= able, > and > > + // memory allocation succeeded > > + // > > + CopyMem ( > > + MicrocodePatchInRam, > > + (VOID *)(UINTN)PcdGet64 (PcdCpuMicrocodePatchAddress), > > + (UINTN)CpuMpData->MicrocodePatchRegionSize > > + ); > > + CpuMpData->MicrocodePatchAddress =3D > (UINTN)MicrocodePatchInRam; > > + } > > + }else { > > + CpuMpData->MicrocodePatchRegionSize =3D OldCpuMpData- > >MicrocodePatchRegionSize; > > + CpuMpData->MicrocodePatchAddress =3D OldCpuMpData- > >MicrocodePatchAddress; > > } > > - > > InitializeSpinLock(&CpuMpData->MpLock); > > > > // > > >=20 > I applied this patch locally and reviewed it with: >=20 > git show -b -W >=20 > The change looks reasonable to me. >=20 > (1) Please clarify the subject line a bit. My suggestion is: >=20 > UefiCpuPkg/MpInitLib: don't shadow the microcode patch twice >=20 > (this is 58 characters). Feel free to tweak this further if you wish; the= point is > that "copy" is too generic in itself. >=20 >=20 > With (1) fixed: >=20 > Reviewed-by: Laszlo Ersek >=20 > Thanks > Laszlo