From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 7553321F833B5 for ; Thu, 4 Jan 2018 07:33:07 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 035392F7C31; Thu, 4 Jan 2018 15:38:12 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-44.rdu2.redhat.com [10.10.121.44]) by smtp.corp.redhat.com (Postfix) with ESMTP id A415760170; Thu, 4 Jan 2018 15:38:10 +0000 (UTC) To: Jian J Wang , edk2-devel@lists.01.org Cc: Jiewen Yao , Eric Dong References: <20180104030901.7072-1-jian.j.wang@intel.com> From: Laszlo Ersek Message-ID: <9abca7f5-e53b-6879-d0e5-7fcb0fbfe217@redhat.com> Date: Thu, 4 Jan 2018 16:38:09 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: <20180104030901.7072-1-jian.j.wang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Thu, 04 Jan 2018 15:38:12 +0000 (UTC) Subject: Re: [PATCH v2] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 04 Jan 2018 15:33:08 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 01/04/18 04:09, Jian J Wang wrote: >> v2 changes: >> a. Use each AP's ApTopOfStack to get the stack base address instead of >> cpu0's ApTopOfStack which is actually set incorrectly before. >> b. Fix cpu0's ApTopOfStack initialization. >> c. Fix wrong debug print format. The end result of this patch looks fine to me. However, please split update (b), which affects "MpLib.c", to a separate patch. The reason is that update (b) addresses a distinct bug. So in v3, - patch v3 1/2 should fix the top-of-stack initialization for the BSP, with its own commit message (referencing the BSP/AP switch service), - patch v3 2/2 should do everything else from v2. I'm ready to R-b such a v3. Thank you! Laszlo > The reason is that DXE part initialization will reuse the stack allocated > at PEI phase, if MP was initialized before. Some code added to check this > situation and use stack base address saved in HOB passed from PEI. > > Cc: Jiewen Yao > Cc: Eric Dong > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > --- > UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 19 ++++++++++++++++++- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 2 +- > 2 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > index 40c1bf407a..e832c16eca 100644 > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > @@ -295,6 +295,7 @@ InitMpGlobalData ( > UINTN Index; > EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc; > UINTN StackBase; > + CPU_INFO_IN_HOB *CpuInfoInHob; > > SaveCpuMpData (CpuMpData); > > @@ -314,8 +315,21 @@ InitMpGlobalData ( > ASSERT (FALSE); > } > > + // > + // DXE will reuse stack allocated for APs at PEI phase if it's available. > + // Let's check it here. > + // > + // Note: BSP's stack guard is set at DxeIpl phase. But for the sake of > + // BSP/AP exchange, stack guard for ApTopOfStack of cpu 0 will still be > + // set here. > + // > + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData->CpuInfoInHob; > for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { > - StackBase = CpuMpData->Buffer + Index * CpuMpData->CpuApStackSize; > + if (CpuInfoInHob != NULL && CpuInfoInHob[Index].ApTopOfStack != 0) { > + StackBase = CpuInfoInHob[Index].ApTopOfStack - CpuMpData->CpuApStackSize; > + } else { > + StackBase = CpuMpData->Buffer + Index * CpuMpData->CpuApStackSize; > + } > > Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); > ASSERT_EFI_ERROR (Status); > @@ -326,6 +340,9 @@ InitMpGlobalData ( > MemDesc.Attributes | EFI_MEMORY_RP > ); > ASSERT_EFI_ERROR (Status); > + > + DEBUG ((DEBUG_INFO, "Stack Guard set at %lx [cpu%lu]!\n", > + (UINT64)StackBase, (UINT64)Index)); > } > } > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index 0c2058a7b0..1bfab8467b 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -1498,7 +1498,7 @@ MpInitLibInitialize ( > // > // Set BSP basic information > // > - InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer); > + InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + ApStackSize); > // > // Save assembly code information > // >