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 6CB6A21F833AF for ; Thu, 4 Jan 2018 04:16:30 -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 77D284E8BF; Thu, 4 Jan 2018 12:21:34 +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 56CA460176; Thu, 4 Jan 2018 12:21:33 +0000 (UTC) To: "Wang, Jian J" , "edk2-devel@lists.01.org" Cc: "Yao, Jiewen" , "Dong, Eric" References: <20171229083631.16652-1-jian.j.wang@intel.com> From: Laszlo Ersek Message-ID: Date: Thu, 4 Jan 2018 13:21:32 +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: 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.38]); Thu, 04 Jan 2018 12:21:34 +0000 (UTC) Subject: Re: [PATCH] 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 12:16:30 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 01/04/18 02:45, Wang, Jian J wrote: > A correction: although BSP doesn't need it, we still need to initialize its ApTopOfStack > correctly because the MP service supports BSP/AP exchange. So I think the line 1501 > should be changed to > > InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + CpuMpData->CpuApStackSize); > > instead of > > InitializeApData (CpuMpData, 0, 0, NULL); Hmm... Although I don't immediately see all possible consequences of such a change, it looks like a correct fix. Unfortunately, I don't know of any code that actually exercises the BSP/AP exchange service. I think Intel must have access to some client code like this, because I vaguely recall some patches over time that improved BSP/AP exchange. If you modify the InitializeApData() call in question like suggested above, can you perhaps locate that client code, and test the change with it? Thanks! Laszlo >> -----Original Message----- >> From: Wang, Jian J >> Sent: Thursday, January 04, 2018 9:09 AM >> To: Wang, Jian J ; Laszlo Ersek ; >> edk2-devel@lists.01.org >> Cc: Yao, Jiewen ; Dong, Eric >> Subject: RE: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >> as Stack Guard >> >> Laszlo, >> >> More explanations: >> >> [UefiCpuPkg\Library\MpInitLib\MpLib.c] >> According to the code, the BSP's (CpuInfoInHob[0].ApTopOfStack) is initialized >> to >> the bottom of the stack (line 1501) but AP's ApTopOfStack is correctly initialized >> (line 598). Although my calculation is correct, I think it'd be better to use AP's >> ApTopOfStack directly. From this perspective, you're right. >> >> Maybe it'd be better to pass a NULL pointer at line 1501 because BSP doesn't >> need >> it anyway. >> >> Regards, >> Jian >> >> >>> -----Original Message----- >>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >> Wang, >>> Jian J >>> Sent: Thursday, January 04, 2018 8:42 AM >>> To: Laszlo Ersek ; edk2-devel@lists.01.org >>> Cc: Yao, Jiewen ; Dong, Eric >>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set >>> as Stack Guard >>> >>> Laszlo, >>> >>> I revisited code of MpInitLib. I found that CPU_INFO_IN_HOB.ApTopOfStack >>> was assigned to CpuMpData->Buffer in MpInitLibInitialize() >>> >>> (line1501) InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer); >>> >>> but in >>> >>> (line598) ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * >>> CpuMpData->CpuApStackSize; >>> (line608) InitializeApData (CpuMpData, ProcessorNumber, BistData, >>> ApTopOfStack); >>> >>> Since InitMpGlobalData() is called just after first situation, my patch is correct. >>> >>> I think the problem here is that ApTopOfStack initialized at line 1501 is not >>> correct. >>> >>> >>> Regards, >>> Jian >>> >>> >>>> -----Original Message----- >>>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>>> Sent: Thursday, January 04, 2018 1:33 AM >>>> To: Wang, Jian J ; edk2-devel@lists.01.org >>>> Cc: Yao, Jiewen ; Dong, Eric ; >>>> Jeff Fan >>>> Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address >> set >>>> as Stack Guard >>>> >>>> (CC Jeff) >>>> >>>> Sorry about the delay. >>>> >>>> I have some light comments below; I expect at least a few of them to be >>>> incorrect :) >>>> >>>> On 12/29/17 09:36, Jian J Wang wrote: >>>>> 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 | 17 +++++++++++++++-- >>>>> 1 file changed, 15 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>> b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >>>>> index 40c1bf407a..05484c9ff3 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,9 +315,18 @@ InitMpGlobalData ( >>>>> ASSERT (FALSE); >>>>> } >>>>> >>>>> - for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> - StackBase = CpuMpData->Buffer + Index * CpuMpData- >>> CpuApStackSize; >>>>> + // >>>>> + // DXE will reuse stack allocated for APs at PEI phase if it's available. >>>>> + // Let's check it here. >>>>> + // >>>>> + CpuInfoInHob = (CPU_INFO_IN_HOB *)(UINTN)CpuMpData- >>>>> CpuInfoInHob; >>>>> + if (CpuInfoInHob != NULL && CpuInfoInHob->ApTopOfStack != 0) { >>>>> + StackBase = CpuInfoInHob->ApTopOfStack; >>>>> + } else { >>>>> + StackBase = CpuMpData->Buffer; >>>>> + } >>>> >>>> So, if the HOB is not found, then StackBase is set okay. >>>> >>>> However, I'm unsure about the other case. The >>>> CPU_INFO_IN_HOB.ApTopOfStack field identifies the *top* of the stack >>>> (highest address, and the stack grows down); however the loop below >>>> *increments* StackBase. Given the incrementing nature of the loop, >>>> shouldn't we first calculate the actual base (= lowest address) from the >>>> CPU_INFO_IN_HOB.ApTopOfStack field? >>>> >>>> Actually... I'm even more confused. The CpuMpData->CpuInfoInHob field >>>> points to an *array* of CPU_INFO_IN_HOB structures. Therefore, for any >>>> given processor #N, we should not calculate the stack base as >>>> >>>> CpuMpData->CpuInfoInHob->ApTopOfStack + N * CpuMpData- >>>>> CpuApStackSize >>>> >>>> instead we should calculate the stack base as something like: >>>> >>>> CpuMpData->CpuInfoInHob[N].ApTopOfStack - CpuMpData- >>> CpuApStackSize >>>> >>>> See >>>> - the InitializeApData() function, >>>> - and its call site in the ApWakeupFunction() function. >>>> >>>> (To my surprise, I personally modified InitializeApData() earlier, in >>>> commit dd3fa0cd72de ("UefiCpuPkg/MpInitLib: support 64-bit AP stack >>>> addresses", 2016-11-17) -- I've totally forgotten about that by now!) >>>> >>>> What do you think? >>>> >>>>> >>>>> + for (Index = 0; Index < CpuMpData->CpuCount; ++Index) { >>>>> Status = gDS->GetMemorySpaceDescriptor (StackBase, &MemDesc); >>>>> ASSERT_EFI_ERROR (Status); >>>>> >>>>> @@ -326,6 +336,9 @@ InitMpGlobalData ( >>>>> MemDesc.Attributes | EFI_MEMORY_RP >>>>> ); >>>>> ASSERT_EFI_ERROR (Status); >>>>> + >>>>> + DEBUG ((DEBUG_VERBOSE, "Stack Guard set at %x [cpu%d]!\n", >>>> StackBase, Index)); >>>> >>>> StackBase has type UINTN, and so it should not be printed with %x. It >>>> should be cast to (UINT64), and then printed with %Lx. >>>> >>>> Similarly, Index has type UINTN. It should not be printed with %d. It >>>> should be cast to (UINT64) and printed with %Lu. >>>> >>>> >>>>> + StackBase += CpuMpData->CpuApStackSize; >>>> >>>> Again, I don't think the simple increment applies when the >>>> CpuMpData->CpuInfoInHob array exists. >>>> >>>>> } >>>>> } >>>>> >>>>> >>>> >>>> Thanks, >>>> Laszlo >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel