From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 E07BC207DF299 for ; Thu, 12 Jul 2018 02:26:08 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 77DD7407565C; Thu, 12 Jul 2018 09:26:07 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-118.rdu2.redhat.com [10.10.120.118]) by smtp.corp.redhat.com (Postfix) with ESMTP id A5CB12026D6B; Thu, 12 Jul 2018 09:26:06 +0000 (UTC) To: Eric Dong , edk2-devel@lists.01.org Cc: Ruiyu Ni References: <20180711110729.12604-1-eric.dong@intel.com> <20180711110729.12604-2-eric.dong@intel.com> From: Laszlo Ersek Message-ID: <9972d977-f3a0-d2e3-8b49-0aab616c01c8@redhat.com> Date: Thu, 12 Jul 2018 11:26:05 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180711110729.12604-2-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Thu, 12 Jul 2018 09:26:07 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Thu, 12 Jul 2018 09:26:07 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [Patch 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: Thu, 12 Jul 2018 09:26:09 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Eric, On 07/11/18 13:07, 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 | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index eec178b419..8b458a4a3a 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -1560,8 +1560,19 @@ MpInitLibInitialize ( > CpuMpData->SwitchBspFlag = FALSE; > CpuMpData->CpuData = (CPU_AP_DATA *) (CpuMpData + 1); > CpuMpData->CpuInfoInHob = (UINT64) (UINTN) (CpuMpData->CpuData + MaxLogicalProcessorNumber); > - CpuMpData->MicrocodePatchAddress = PcdGet64 (PcdCpuMicrocodePatchAddress); > CpuMpData->MicrocodePatchRegionSize = PcdGet64 (PcdCpuMicrocodePatchRegionSize); > + // > + // if platform has more than one CPU, relocate microcode to memory to reduce loading microcode time. > + // > + if (MaxLogicalProcessorNumber > 1) { > + CpuMpData->MicrocodePatchAddress = (UINT64) (UINTN) AllocatePages (EFI_SIZE_TO_PAGES ((UINTN)CpuMpData->MicrocodePatchRegionSize)); > + if (CpuMpData->MicrocodePatchAddress != 0) { > + CopyMem ((VOID *) (UINTN)CpuMpData->MicrocodePatchAddress, (VOID *)(UINTN)(PcdGet64 (PcdCpuMicrocodePatchAddress)), (UINTN)CpuMpData->MicrocodePatchRegionSize); > + } > + } else { > + CpuMpData->MicrocodePatchAddress = PcdGet64 (PcdCpuMicrocodePatchAddress); > + } > + > InitializeSpinLock(&CpuMpData->MpLock); > // > // Save BSP's Control registers to APs > (1) Can you please break up the added lines to multiple lines? They are extremely long, and difficult for me to handle. It should be possible to break up both AllocatePages() and CopyMem(), for example. (2) The code appears to handle the case well when PcdCpuMicrocodePatchRegionSize is zero. In that case, EFI_SIZE_TO_PAGES(...) evaluates to zero, and -- I checked -- AllocatePages() returns NULL. In this case, allocation and copying will not take place, and that's fine -- there is nothing to copy and no microcode to install. So, OK. (3) However, if there is a microcode update available, but we can't allocate memory, things will go wrong. The region size is nonzero, thus MicrocodeDetect() will not exit early. But MicrocodePatchAddress will be set to 0. So, I suggest the following instead: --------- VOID *MicrocodePatchInRam; // // If platform has more than one CPU, relocate microcode to memory to reduce // loading microcode time. // MicrocodePatchInRam = NULL; if (MaxLogicalProcessorNumber > 1) { MicrocodePatchInRam = AllocatePages ( EFI_SIZE_TO_PAGES ( (UINTN)CpuMpData->MicrocodePatchRegionSize ) ); } if (MicrocodePatchInRam == NULL) { // // there is only one processor, or no microcode patch is available, or // memory allocation failed // CpuMpData->MicrocodePatchAddress = PcdGet64 (PcdCpuMicrocodePatchAddress); } else { // // there are multiple processors, and a microcode patch is available, and // memory allocation succeeded // CopyMem ( MicrocodePatchInRam, (VOID *)(UINTN)PcdGet64 (PcdCpuMicrocodePatchAddress), (UINTN)CpuMpData->MicrocodePatchRegionSize ); CpuMpData->MicrocodePatchAddress = (UINTN)MicrocodePatchInRam; } --------- Thanks Laszlo