From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web12.2273.1599032574652663437 for ; Wed, 02 Sep 2020 00:42:55 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=NW6fpWf/; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1599032573; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=JgPINth77o/utV8MGc6eC4gJTkGRXLgSLXwnMYLc5Js=; b=NW6fpWf/SXOITYeb4pVbmcNO8a/zJQgmkrG7Sn+5ouJJKQ5oOwLEsr/FweZWmFUmgdswYE RTwDK2ujoxjxlx+IylqI0ju432ZstL8tcok+2alp+a8Vn2NQWKLHbHBN41jB7vHin5vBcr ee50GOw6bv8QBr+7wYwZe2P3ebQadzA= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-315-mpAnnEzXML-jH4DN40-GsA-1; Wed, 02 Sep 2020 03:42:52 -0400 X-MC-Unique: mpAnnEzXML-jH4DN40-GsA-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 06FA9100746A; Wed, 2 Sep 2020 07:42:51 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-14.ams2.redhat.com [10.36.113.14]) by smtp.corp.redhat.com (Postfix) with ESMTP id D93815D9CC; Wed, 2 Sep 2020 07:42:49 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Add check for CR3/GDT/IDT. To: devel@edk2.groups.io, eric.dong@intel.com Cc: Ray Ni References: <20200902004353.1515-1-eric.dong@intel.com> From: "Laszlo Ersek" Message-ID: <75a0a10e-f303-304e-0a36-628a79116284@redhat.com> Date: Wed, 2 Sep 2020 09:42:48 +0200 MIME-Version: 1.0 In-Reply-To: <20200902004353.1515-1-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0.002 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US Hi Eric, On 09/02/20 02:43, Dong, Eric wrote: > AP needs to run from real mode to 32bit mode to LONG mode. Page table > (pointed by CR3) and GDT are necessary to set up to correct value when > CPU execution mode is switched to LONG mode. > AP uses the same location page table (CR3) and GDT as what BSP uses. > But when the page table or GDT is above 4GB, it's impossible for CPU > to use because GDTR.base and CR3 are 32bits before switching to LONG > mode. > This patch adds check for the CR3, GDT.Base and IDT.Base to not above > 32 bits restriction. > > Signed-off-by: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > --- > UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 8 +- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 108 +++++++++++++++++++----- > UefiCpuPkg/Library/MpInitLib/MpLib.h | 6 +- > 3 files changed, 97 insertions(+), 25 deletions(-) (1) This is not for edk2-stable202008, correct? (2) If I understand correctly, this patch does not solve the problem when any one of the IDT, GDT, and CR3, are out of 32-bit address space. Instead, the patch makes the failure symptoms more graceful. Is that correct? If so, can you explain in the commit message what happens without the patch, versus with the patch, when the IDT / GDT / CR3 are out of 32-bit address space? Like, if we abandon MpInitChangeApLoopCallback() mid-way, then (AIUI) the AP "pen" (HLT loop) will not be relocated to reserved memory at ExitBootServces(). I don't think that simply giving up can end well for the OS! (3) Can you remind us in the commit message where the IDT / GDT / page tables are actually allocated? That is, can you please name the component that is usually responsible for keeping the allocations in the 32-bit address space? And once we know where the IDT / GDT / page tables come from -- shouldn't we rather modify those modules, to allocate IDT / GDT / page tables in the 32-bit address space? More below: > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > index 2c00d72dde..27f12a75a8 100644 > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > @@ -393,13 +393,19 @@ MpInitChangeApLoopCallback ( > ) > { > CPU_MP_DATA *CpuMpData; > + EFI_STATUS Status; > > CpuMpData = GetCpuMpData (); > CpuMpData->PmCodeSegment = GetProtectedModeCS (); > CpuMpData->Pm16CodeSegment = GetProtectedMode16CS (); > CpuMpData->ApLoopMode = PcdGet8 (PcdCpuApLoopMode); > mNumberToFinish = CpuMpData->CpuCount - 1; > - WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL, TRUE); > + Status = WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL, TRUE); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "ERROR :: %a() Change Ap Loop Mode failed!\n", __FUNCTION__)); > + return; > + } > + > while (mNumberToFinish > 0) { > CpuPause (); > } > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index 07426274f6..21b17a7b40 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -470,7 +470,7 @@ GetProcessorNumber ( > > @return CPU count detected > **/ > -UINTN > +EFI_STATUS > CollectProcessorCount ( > IN CPU_MP_DATA *CpuMpData > ) > @@ -478,12 +478,17 @@ CollectProcessorCount ( > UINTN Index; > CPU_INFO_IN_HOB *CpuInfoInHob; > BOOLEAN X2Apic; > + EFI_STATUS Status; > > // > // Send 1st broadcast IPI to APs to wakeup APs > // > CpuMpData->InitFlag = ApInitConfig; > - WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL, TRUE); > + Status = WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL, TRUE); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > CpuMpData->InitFlag = ApInitDone; > ASSERT (CpuMpData->CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); > // > @@ -520,7 +525,11 @@ CollectProcessorCount ( > // > // Wakeup all APs to enable x2APIC mode > // > - WakeUpAP (CpuMpData, TRUE, 0, ApFuncEnableX2Apic, NULL, TRUE); > + Status = WakeUpAP (CpuMpData, TRUE, 0, ApFuncEnableX2Apic, NULL, TRUE); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > // > // Wait for all known APs finished > // > @@ -546,7 +555,7 @@ CollectProcessorCount ( > > DEBUG ((DEBUG_INFO, "MpInitLib: Find %d processors in system.\n", CpuMpData->CpuCount)); > > - return CpuMpData->CpuCount; > + return EFI_SUCCESS; > } > > /** > @@ -990,7 +999,7 @@ WaitApWakeup ( > @param[in] CpuMpData Pointer to CPU MP Data > > **/ > -VOID > +EFI_STATUS > FillExchangeInfoData ( > IN CPU_MP_DATA *CpuMpData > ) > @@ -1001,6 +1010,35 @@ FillExchangeInfoData ( > IA32_CR4 Cr4; > > ExchangeInfo = CpuMpData->MpCpuExchangeInfo; > + ExchangeInfo->Cr3 = AsmReadCr3 (); > + if (ExchangeInfo->Cr3 > 0xFFFFFFFF) { > + // > + // AP needs to run from real mode to 32bit mode to LONG mode. Page table > + // (pointed by CR3) and GDT are necessary to set up to correct value when > + // CPU execution mode is switched to LONG mode. > + // AP uses the same location page table (CR3) and GDT as what BSP uses. > + // But when the page table or GDT is above 4GB, it's impossible for CPU > + // to use because GDTR.base and CR3 are 32bits before switching to LONG > + // mode. > + // Here add check for the CR3, GDT.Base and IDT.Base to not above 32 bits > + // limitation. > + // > + return EFI_UNSUPPORTED; > + } > + > + // > + // Get the BSP's data of GDT and IDT > + // > + AsmReadGdtr ((IA32_DESCRIPTOR *) &ExchangeInfo->GdtrProfile); > + if (ExchangeInfo->GdtrProfile.Base > 0xFFFFFFFF) { > + return EFI_UNSUPPORTED; > + } > + > + AsmReadIdtr ((IA32_DESCRIPTOR *) &ExchangeInfo->IdtrProfile); > + if (ExchangeInfo->IdtrProfile.Base > 0xFFFFFFFF) { > + return EFI_UNSUPPORTED; > + } > + > ExchangeInfo->Lock = 0; > ExchangeInfo->StackStart = CpuMpData->Buffer; > ExchangeInfo->StackSize = CpuMpData->CpuApStackSize; (4) Style: I think we should use >= BASE_4GB rather than > 0xFFFFFFFF (5) Correctness: the conditions above only check the base addresses of the tables. However I think we should check whether the *last byte* in each table fits in 32-bits. (6) How about a different approach: in case we realize the IDT / GDT / page tables are "out of reach", can we simply copy them under 4GB, and pass the *copy* to the APs? (I guess this would be difficult with the page table hierarchy.) --*-- All in all I'm really surprised (and concerned!) that we need to add an after-the-fact check for this situation. This situation should never occur, in my opinion. Basically the idea seems to be that FillExchangeInfoData() can now fail, and because of that, WakeUpAP() can now fail too. And we attempt to propagate that failure out to the callers of WakeUpAP(). But WakeUpAP() is called from *lots* of places, which currently assume that WakeUpAP() can not fail. I don't think it may even be possible to cleanly handle WakeUpAP() failure in some of those places. I mentioned MpInitChangeApLoopCallback() in point (2) above, but CollectProcessorCount() is a good example too. If CollectProcessorCount() is abandoned early, then we won't know how many CPUs are in the system, xAPIC / x2APIC mode will not be set, etc. So basically MP services in the system will be unusable. I don't think we should even attempt booting at all, in that case. I think we should look into point (3) more deeply; that is, figure out where the page tables / IDT / GDT are allocated in the first place, and make sure *in those modules* that the allocation is safe. Hmm wait -- if MpInitLibInitialize() fails (for example because CollectProcessorCount() fails), then we hit ASSERT_EFI_ERROR()s anyway! In the following locations: - InitializeMpSupport() function in "UefiCpuPkg/CpuDxe/CpuMp.c", - MemoryDiscoveredPpiNotifyCallback() function in "UefiCpuPkg/CpuMpPei/CpuPaging.c" -- via InitializeCpuMpWorker() So it seems like this condition is impossible to mitigate, and we're not going to boot even with this patch (in NOOPT/DEBUG builds) if the condition occurs (and RELEASE builds will just crash or misbehave badly). However, in that case, I don't think we should complicate the code with a bunch of error propagation. We identify the problem in FillExchangeInfoData(). *If* we cannot fix the agents that are responsible for the GDT / IDT / page table allocations in the first place, *then* we should ASSERT() in FillExchangeInfoData() immediately. It's a serious platform misconfiguration, and we shouldn't even attempt to continue. Consider it from the following angle too: if this condition causes MpInitLibInitialize() to fail, then we'll never reach functions such as SwitchBSPWorker() and MpInitChangeApLoopCallback() -- so why complicate them with new branches (that don't do the right thing anyway)? Is it perhaps the case that the open source code in edk2 *itself* is safe, but you have some proprietary firmware platforms that use MpInitLib *but* do not satisfy the 32-bit allocation requirements? How and where was the problem actually experienced? Thanks, Laszlo > @@ -1009,9 +1047,6 @@ FillExchangeInfoData ( > > ExchangeInfo->CodeSegment = AsmReadCs (); > ExchangeInfo->DataSegment = AsmReadDs (); > - > - ExchangeInfo->Cr3 = AsmReadCr3 (); > - > ExchangeInfo->CFunction = (UINTN) ApWakeupFunction; > ExchangeInfo->ApIndex = 0; > ExchangeInfo->NumApsExecuting = 0; > @@ -1037,13 +1072,6 @@ FillExchangeInfoData ( > > ExchangeInfo->SevEsIsEnabled = CpuMpData->SevEsIsEnabled; > ExchangeInfo->GhcbBase = (UINTN) CpuMpData->GhcbBase; > - > - // > - // Get the BSP's data of GDT and IDT > - // > - AsmReadGdtr ((IA32_DESCRIPTOR *) &ExchangeInfo->GdtrProfile); > - AsmReadIdtr ((IA32_DESCRIPTOR *) &ExchangeInfo->IdtrProfile); > - > // > // Find a 32-bit code segment > // > @@ -1084,6 +1112,8 @@ FillExchangeInfoData ( > (UINT32)ExchangeInfo->ModeOffset - > (UINT32)CpuMpData->AddressMap.ModeTransitionOffset; > ExchangeInfo->ModeHighSegment = (UINT16)ExchangeInfo->CodeSegment; > + > + return EFI_SUCCESS; > } > > /** > @@ -1308,8 +1338,12 @@ SetSevEsJumpTable ( > @param[in] Procedure The function to be invoked by AP > @param[in] ProcedureArgument The argument to be passed into AP function > @param[in] WakeUpDisabledAps Whether need to wake up disabled APs in broadcast mode. > + > + @retval EFI_SUCCESS Wake up the AP success. > + @retval EFI_UNSUPPORTED Invalid CR3, IDT, GDT value caused fail to wake up AP. > + > **/ > -VOID > +EFI_STATUS > WakeUpAP ( > IN CPU_MP_DATA *CpuMpData, > IN BOOLEAN Broadcast, > @@ -1324,6 +1358,7 @@ WakeUpAP ( > CPU_AP_DATA *CpuData; > BOOLEAN ResetVectorRequired; > CPU_INFO_IN_HOB *CpuInfoInHob; > + EFI_STATUS Status; > > CpuMpData->FinishedCount = 0; > ResetVectorRequired = FALSE; > @@ -1333,7 +1368,10 @@ WakeUpAP ( > ResetVectorRequired = TRUE; > AllocateResetVector (CpuMpData); > AllocateSevEsAPMemory (CpuMpData); > - FillExchangeInfoData (CpuMpData); > + Status = FillExchangeInfoData (CpuMpData); > + if (EFI_ERROR (Status)) { > + return Status; > + } > SaveLocalApicTimerSetting (CpuMpData); > } > > @@ -1500,6 +1538,8 @@ WakeUpAP ( > // S3SmmInitDone Ppi. > // > CpuMpData->WakeUpByInitSipiSipi = (CpuMpData->ApLoopMode == ApInHltLoop); > + > + return EFI_SUCCESS; > } > > /** > @@ -1945,6 +1985,7 @@ MpInitLibInitialize ( > UINTN ApResetVectorSize; > UINTN BackupBufferAddr; > UINTN ApIdtBase; > + EFI_STATUS Status; > > OldCpuMpData = GetCpuMpDataFromGuidedHob (); > if (OldCpuMpData == NULL) { > @@ -2067,7 +2108,10 @@ MpInitLibInitialize ( > // > // Wakeup all APs and calculate the processor count in system > // > - CollectProcessorCount (CpuMpData); > + Status = CollectProcessorCount (CpuMpData); > + if (EFI_ERROR (Status)) { > + return Status; > + } > } > } else { > // > @@ -2118,7 +2162,11 @@ MpInitLibInitialize ( > // > CpuMpData->InitFlag = ApInitReconfig; > } > - WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE); > + Status = WakeUpAP (CpuMpData, TRUE, 0, ApInitializeSync, CpuMpData, TRUE); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > // > // Wait for all APs finished initialization > // > @@ -2262,6 +2310,7 @@ SwitchBSPWorker ( > MSR_IA32_APIC_BASE_REGISTER ApicBaseMsr; > BOOLEAN OldInterruptState; > BOOLEAN OldTimerInterruptState; > + EFI_STATUS Status; > > // > // Save and Disable Local APIC timer interrupt > @@ -2333,7 +2382,10 @@ SwitchBSPWorker ( > // > // Need to wakeUp AP (future BSP). > // > - WakeUpAP (CpuMpData, FALSE, ProcessorNumber, FutureBSPProc, CpuMpData, TRUE); > + Status = WakeUpAP (CpuMpData, FALSE, ProcessorNumber, FutureBSPProc, CpuMpData, TRUE); > + if (EFI_ERROR (Status)) { > + return Status; > + } > > AsmExchangeRole (&CpuMpData->BSPInfo, &CpuMpData->APInfo); > > @@ -2669,14 +2721,21 @@ StartupAllCPUsWorker ( > CpuMpData->WaitEvent = WaitEvent; > > if (!SingleThread) { > - WakeUpAP (CpuMpData, TRUE, 0, Procedure, ProcedureArgument, FALSE); > + Status = WakeUpAP (CpuMpData, TRUE, 0, Procedure, ProcedureArgument, FALSE); > + if (EFI_ERROR (Status)) { > + return Status; > + } > } else { > for (ProcessorNumber = 0; ProcessorNumber < ProcessorCount; ProcessorNumber++) { > if (ProcessorNumber == CallerNumber) { > continue; > } > if (CpuMpData->CpuData[ProcessorNumber].Waiting) { > - WakeUpAP (CpuMpData, FALSE, ProcessorNumber, Procedure, ProcedureArgument, TRUE); > + Status = WakeUpAP (CpuMpData, FALSE, ProcessorNumber, Procedure, ProcedureArgument, TRUE); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + > break; > } > } > @@ -2795,7 +2854,10 @@ StartupThisAPWorker ( > CpuData->ExpectedTime = CalculateTimeout (TimeoutInMicroseconds, &CpuData->CurrentTime); > CpuData->TotalTime = 0; > > - WakeUpAP (CpuMpData, FALSE, ProcessorNumber, Procedure, ProcedureArgument, TRUE); > + Status = WakeUpAP (CpuMpData, FALSE, ProcessorNumber, Procedure, ProcedureArgument, TRUE); > + if (EFI_ERROR (Status)) { > + return Status; > + } > > // > // If WaitEvent is NULL, execute in blocking mode. > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h > index 02652eaae1..9cf5c0f9b4 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h > @@ -459,8 +459,12 @@ GetSevEsAPMemory ( > @param[in] Procedure The function to be invoked by AP > @param[in] ProcedureArgument The argument to be passed into AP function > @param[in] WakeUpDisabledAps Whether need to wake up disabled APs in broadcast mode. > + > + @retval EFI_SUCCESS Wake up the AP success. > + @retval EFI_UNSUPPORTED Invalid CR3, IDT, GDT value caused fail to wake up AP. > + > **/ > -VOID > +EFI_STATUS > WakeUpAP ( > IN CPU_MP_DATA *CpuMpData, > IN BOOLEAN Broadcast, >