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 699D52097FA92 for ; Thu, 12 Jul 2018 15:13:53 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B5D7D4075742; Thu, 12 Jul 2018 22:13:52 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-124-188.rdu2.redhat.com [10.10.124.188]) by smtp.corp.redhat.com (Postfix) with ESMTP id 633D5111AF3C; Thu, 12 Jul 2018 22:13:51 +0000 (UTC) From: Laszlo Ersek To: Eric Dong , edk2-devel@lists.01.org Cc: Ruiyu Ni References: <20180712104927.12220-1-eric.dong@intel.com> <20180712104927.12220-2-eric.dong@intel.com> Message-ID: Date: Fri, 13 Jul 2018 00:13:50 +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: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Thu, 12 Jul 2018 22:13:52 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.7]); Thu, 12 Jul 2018 22:13:52 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' 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: Thu, 12 Jul 2018 22:13:53 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 = GetCpuMpDataFromGuidedHob (); >> if (OldCpuMpData == NULL) { >> @@ -1587,8 +1588,39 @@ 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. >> + // >> + MicrocodePatchInRam = NULL; >> + if (MaxLogicalProcessorNumber > 1) { >> + MicrocodePatchInRam = AllocatePages ( >> + EFI_SIZE_TO_PAGES ( >> + (UINTN)CpuMpData->MicrocodePatchRegionSize >> + ) >> + ); >> + ASSERT (MicrocodePatchInRam != NULL); >> + } >> + 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; >> + } >> + >> InitializeSpinLock(&CpuMpData->MpLock); >> >> // >> > > Reviewed-by: Laszlo Ersek > Sorry, I have to take that back -- please do not commit this patch. For this version of the patch: Nacked-by: Laszlo Ersek The ASSERT() is wrong. Again, in the code above, AllocatePages() can return 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. 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 about it too. :) In particular, this patch would make it *impossible* to boot OVMF with multiple processors, because OVMF *never* provides a microcode update. So, please remove the ASSERT. Alternatively, you could modify the ASSERT() like this: // // if we attempt actual memory allocation, we expect it to succeed // ASSERT ( (CpuMpData->MicrocodePatchRegionSize == 0) || (MicrocodePatchInRam != NULL) ); Thanks, Laszlo