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 32CBE211F8890 for ; Tue, 26 Jun 2018 11:57:27 -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 7C333814F0A4; Tue, 26 Jun 2018 18:57:26 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-137.rdu2.redhat.com [10.10.121.137]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9060F111763E; Tue, 26 Jun 2018 18:57:18 +0000 (UTC) To: Andrew Fish Cc: Ruiyu Ni , edk2-devel , Eric Dong , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= , Bandan Das , Jiewen Yao , Paolo Bonzini References: <20180625025402.201636-1-ruiyu.ni@intel.com> <4DFBB17A-3FCF-448D-B8F0-C4D66A33CF9F@apple.com> From: Laszlo Ersek Message-ID: Date: Tue, 26 Jun 2018 20:57:18 +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: <4DFBB17A-3FCF-448D-B8F0-C4D66A33CF9F@apple.com> 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.8]); Tue, 26 Jun 2018 18:57:26 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Tue, 26 Jun 2018 18:57:26 +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] UefiCpuPkg/MpInitLib: AP uses memory preceding IDT to store CpuMpData X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 26 Jun 2018 18:57:27 -0000 Content-Type: text/plain; charset=windows-1252 Content-Language: en-US Content-Transfer-Encoding: 7bit On 06/26/18 19:20, Andrew Fish wrote: >> On Jun 26, 2018, at 10:06 AM, Laszlo Ersek wrote: >> On 06/25/18 04:54, Ruiyu Ni wrote: >>> Today's MpInitLib PEI implementation directly calls >>> PeiServices->GetHobList() from AP which may cause racing issue. >>> >>> This patch fixes this issue by storing the CpuMpData to memory >>> preceding IDT. Pointer to PeiServices pointer is stored there, >>> so after AP procedure returns, the PeiServices pointer should be >>> restored. >>> >>> Contributed-under: TianoCore Contribution Agreement 1.1 >>> Signed-off-by: Ruiyu Ni >>> Cc: Jeff Fan >>> Cc: Eric Dong >>> Cc: Jiewen Yao >>> Cc: Fish Andrew >>> --- >>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 33 ++++++++++++++++++- >>> UefiCpuPkg/Library/MpInitLib/MpLib.c | 8 +++++ >>> UefiCpuPkg/Library/MpInitLib/MpLib.h | 27 +++++++++++++++- >>> UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 56 +++++++++++++++++++++++++++++++-- >>> 4 files changed, 119 insertions(+), 5 deletions(-) >>> >>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>> index e7ed21c6cd..26fead2c66 100644 >>> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>> @@ -1,7 +1,7 @@ >>> /** @file >>> MP initialize support functions for DXE phase. >>> >>> - Copyright (c) 2016, Intel Corporation. All rights reserved.
>>> + Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.
>>> This program and the accompanying materials >>> are licensed and made available under the terms and conditions of the BSD License >>> which accompanies this distribution. The full text of the license may be found at >>> @@ -75,6 +75,37 @@ SaveCpuMpData ( >>> mCpuMpData = CpuMpData; >>> } >>> >>> +/** >>> + Push the CpuMpData for AP to use. >>> + >>> + @param[in] The pointer to CPU MP Data structure will be pushed. >>> + @param[out] The pointer to the context which will be passed to PopCpuMpData(). >>> + >>> + @return The pointer value which was stored in where the CPU MP Data is pushed. >>> +**/ >>> +VOID * >>> +PushCpuMpData ( >>> + IN CPU_MP_DATA *CpuMpData, >>> + OUT VOID **Context >>> + ) >>> +{ >>> + return NULL; >>> +} >>> + >>> +/** >>> + Pop the CpuMpData. >>> + >>> + @param[in] Pointer The pointer value which was stored in where the CPU MP Data is pushed. >>> + @param[in] Context The context of push/pop operation. >>> +**/ >>> +VOID >>> +PopCpuMpData ( >>> + IN VOID *Pointer, >>> + IN VOID *Context >>> + ) >>> +{ >>> +} >>> + >>> /** >>> Get available system memory below 1MB by specified size. >>> >>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c >>> index f2ff40417a..786a7825d5 100644 >>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c >>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c >>> @@ -580,6 +580,8 @@ ApWakeupFunction ( >>> CPU_INFO_IN_HOB *CpuInfoInHob; >>> UINT64 ApTopOfStack; >>> UINTN CurrentApicMode; >>> + VOID *BackupPtr; >>> + VOID *Context; >>> >>> // >>> // AP finished assembly code and begin to execute C code >>> @@ -659,8 +661,14 @@ ApWakeupFunction ( >>> EnableDebugAgent (); >>> // >>> // Invoke AP function here >>> + // Use a BSP owned area (PeiServices Pointer storage) to store the CpuMpData. >>> + // It's required in PEI phase because CpuMpData cannot be cached in global variable as in DXE phase. >>> + // DXE version of Pushxxx andPopxxx is dummy implementation. >>> // >>> + BackupPtr = PushCpuMpData (CpuMpData, &Context); >>> Procedure (Parameter); >>> + PopCpuMpData (BackupPtr, Context); >>> + >>> CpuInfoInHob = (CPU_INFO_IN_HOB *) (UINTN) CpuMpData->CpuInfoInHob; >>> if (CpuMpData->SwitchBspFlag) { >>> // >>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h >>> index e7f9a4de0a..270d62ff20 100644 >>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h >>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h >>> @@ -1,7 +1,7 @@ >>> /** @file >>> Common header file for MP Initialize Library. >>> >>> - Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.
>>> + Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.
>>> This program and the accompanying materials >>> are licensed and made available under the terms and conditions of the BSD License >>> which accompanies this distribution. The full text of the license may be found at >>> @@ -321,6 +321,31 @@ SaveCpuMpData ( >>> IN CPU_MP_DATA *CpuMpData >>> ); >>> >>> +/** >>> + Push the CpuMpData for AP to use. >>> + >>> + @param[in] The pointer to CPU MP Data structure will be pushed. >>> + @param[out] The pointer to the context which will be passed to PopCpuMpData(). >>> + >>> + @return The pointer value which was stored in where the CPU MP Data is pushed. >>> +**/ >>> +VOID * >>> +PushCpuMpData ( >>> + IN CPU_MP_DATA *CpuMpData, >>> + OUT VOID **Context >>> + ); >>> + >>> +/** >>> + Pop the CpuMpData. >>> + >>> + @param[in] Pointer The pointer value which was stored in where the CPU MP Data is pushed. >>> + @param[in] Context The context of push/pop operation. >>> +**/ >>> +VOID >>> +PopCpuMpData ( >>> + IN VOID *Pointer, >>> + IN VOID *Context >>> + ); >>> >>> /** >>> Get available system memory below 1MB by specified size. >>> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c >>> index 791ae9db6e..5c9c4b3b1e 100644 >>> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c >>> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c >>> @@ -27,6 +27,9 @@ EnableDebugAgent ( >>> >>> /** >>> Get pointer to CPU MP Data structure. >>> + For BSP, the pointer is retrieved from HOB. >>> + For AP, the pointer is retrieved from the location which stores the PeiServices pointer. >>> + It's safe because BSP is blocking and has no chance to use PeiServices pointer when AP is executing. >>> >>> @return The pointer to CPU MP Data structure. >>> **/ >>> @@ -35,9 +38,17 @@ GetCpuMpData ( >>> VOID >>> ) >>> { >>> - CPU_MP_DATA *CpuMpData; >>> - >>> - CpuMpData = GetCpuMpDataFromGuidedHob (); >>> + CPU_MP_DATA *CpuMpData; >>> + MSR_IA32_APIC_BASE_REGISTER ApicBaseMsr; >>> + IA32_DESCRIPTOR Idtr; >>> + >>> + ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE); >>> + if (ApicBaseMsr.Bits.BSP == 1) { >>> + CpuMpData = GetCpuMpDataFromGuidedHob (); >>> + } else { >>> + AsmReadIdtr (&Idtr); >>> + CpuMpData = (CPU_MP_DATA *)(*(UINTN *) (Idtr.Base - sizeof (UINTN))); >>> + } >>> ASSERT (CpuMpData != NULL); >>> return CpuMpData; >>> } >>> @@ -64,6 +75,45 @@ SaveCpuMpData ( >>> ); >>> } >>> >>> +/** >>> + Push the CpuMpData for AP to use. >>> + >>> + @param[in] The pointer to CPU MP Data structure will be pushed. >>> + @param[out] The pointer to the context which will be passed to PopCpuMpData(). >>> + >>> + @return The pointer value which was stored in where the CPU MP Data is pushed. >>> +**/ >>> +VOID * >>> +PushCpuMpData ( >>> + IN CPU_MP_DATA *CpuMpData, >>> + OUT VOID **Context >>> + ) >>> +{ >>> + EFI_PEI_SERVICES **PeiServices; >>> + IA32_DESCRIPTOR Idtr; >>> + >>> + AsmReadIdtr (&Idtr); >>> + *Context = (VOID *) (Idtr.Base - sizeof (UINTN)); >>> + PeiServices = (EFI_PEI_SERVICES **)(*(UINTN *)(*Context)); >>> + *(UINTN *)(*Context) = (UINTN)CpuMpData; >>> + return PeiServices; >>> +} >>> + >>> +/** >>> + Pop the CpuMpData. >>> + >>> + @param[in] Pointer The pointer value which was stored in where the CPU MP Data is pushed. >>> + @param[in] Context The context of push/pop operation. >>> +**/ >>> +VOID >>> +PopCpuMpData ( >>> + IN VOID *Pointer, >>> + IN VOID *Context >>> + ) >>> +{ >>> + *(UINTN *)Context = (UINTN)Pointer; >>> +} >>> + >>> /** >>> Check if AP wakeup buffer is overlapped with existing allocated buffer. >>> >>> [snip] >> Ray, can you please explain how this patch is supposed to work? Are >> you re-purposing an otherwise unused (un-exercised) entry in the >> interrupt descriptor table, for storing a generic pointer? >> >> ... The commit message says, "memory preceding IDT", and the patch >> says "(Idtr.Base - sizeof (UINTN))". What memory is supposed to be >> there? >> > > Laszlo, > > Some context. A few weeks ago I reported that the APs in this code > were using PEI Services to get a HOB to store the CPU context > structure. PEI Services are not defined as MP Safe so this is not a > good long term direction. Given this is PEI Code and can run XIP > global variables may not be writable so this code needs some why > (other than a HOB) to have a pointer. Right, I remember: http://mid.mail-archive.com/31C6D3FB-91D3-491B-8059-4B6DEA11BDF9@apple.com Specifically, as I understood it, you reported about the following call stack: MpInitLibWhoAmI() [UefiCpuPkg/Library/MpInitLib/MpLib.c] GetCpuMpData() [UefiCpuPkg/Library/MpInitLib/PeiMpLib.c] GetCpuMpDataFromGuidedHob() [UefiCpuPkg/Library/MpInitLib/MpLib.c] GetFirstGuidHob() [MdePkg/Library/PeiHobLib/HobLib.c] GetHobList() [MdePkg/Library/PeiHobLib/HobLib.c] PeiServicesGetHobList() [MdePkg/Library/PeiServicesLib/PeiServicesLib.c] GetPeiServicesTablePointer() [MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointer.c] PeiGetHobList() [MdeModulePkg/Core/Pei/Hob/Hob.c] The MpInitLibWhoAmI() function itself is valid to call from AP procedures, e.g. through the EFI_PEI_MP_SERVICES_PPI.WhoAmI() member function: PeiWhoAmI() [UefiCpuPkg/CpuMpPei/CpuMpPei.c] MpInitLibWhoAmI() [UefiCpuPkg/Library/MpInitLib/MpLib.c] However, the bottom of the call stack (i.e. the implementation of the service) is invalid for APs to execute. So yes, I think I understand that. > Per the PI Spec Idtr.Base - sizeof(UINTN) is the address of the PEI > Services Table pointer. We add that so every PEI API did not need to > pass the PEI Services table pointer. It is what enables things like > ASSERT() and DEBUG() to work in PEI. Prior to this we had to have > PEI_ASSERT()/PEI_DEBUG() and pass the PEI Services Table pointer. Ah, OK, I did miss this information. I've checked the PI spec v1.6 now, and I do see this, in Volume 1, "5.4 PEI Services Table Retrieval", "5.4.2 x64". And that also explains the intent of this patch. Namely, (1) the patch replaces the PEI services pointer (located just below the IDT) with the CpuMpData address, on *each* AP, in the ApWakeupFunction() function, saving the previous PEI services pointer value to the AP stack, (2) the AP procedure is entered in this state, (3) whenever the AP procedure calls EFI_PEI_MP_SERVICES_PPI.WhoAmI(), the GetCpuMpData() function reaches back to the PEI services pointer location, and grabs the CpuMpData value from it, (4) once the AP procedure returns, the AP writes back the original PEI services pointer value, from the AP stack, to the PEI services pointer location. Here's the problem: the IDT is reused across all processors. In the MpInitLibInitialize() function, we have > // > // Save BSP's Control registers to APs > // > SaveVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters); And in the ApWakeupFunction(), we have, at startup: > if (CpuMpData->InitFlag == ApInitConfig) { > // > // Add CPU number > // > InterlockedIncrement ((UINT32 *) &CpuMpData->CpuCount); > ProcessorNumber = ApIndex; > // > // This is first time AP wakeup, get BIST information from AP stack > // > ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * CpuMpData->CpuApStackSize; > BistData = *(UINT32 *) ((UINTN) ApTopOfStack - sizeof (UINTN)); > // > // Do some AP initialize sync > // > ApInitializeSync (CpuMpData); > // > // Sync BSP's Control registers to APs > // > RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE); This means that, when multiple APs are fired up in parallel, they *race* for the PEI services pointer location. There is no isolation between APs when they execute PushCpuMpData() concurrently. First, this can corrupt the datum in the PEI services pointer location. Second, even assuming that PushCpuMpData() and PopCpuMpData() are atomic, the order in which APs complete the AP procedure is not deterministic, and it need not be the exact reverse of the entry order. We may have the following order, for example: - AP 1 writes CpuMpData, and saves the original PEI services pointer on its stack, - AP 2 writes CpuMpData, and saves the same CpuMpData value (originally written by AP 1) on its stack, - AP 1 completes the AP procedure and restores the original PEI services pointer from its stack, - AP 2 completes the AP procedure, and overwrites the PEI services pointer, with the CpuMpData value from its stack, that was originally written by AP 1. Assuming I (remotely) understand what's going on, I'd (vaguely) suggest three alternatives: - instead of the idea captured in this patch, we should use an APIC ID search similar to the one done initially in "MpFuncs.nasm", - or else, we should stick with the current idea, but use atomic compare-and-swap operations, so that the original PEI services pointer value be restored ultimately, at the least, - or else (possibly the simplest fix), allocate a separate IDT for each AP, including the UINTN that precedes the (now AP-specific) IDT. This means that the PEI services pointer *location* would be separate for each AP, and no contention would occur. Thanks, Laszlo