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 AFD2C21F833A6 for ; Thu, 4 Jan 2018 04:13:15 -0800 (PST) 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 mx1.redhat.com (Postfix) with ESMTPS id 8BC94C049D59; Thu, 4 Jan 2018 12:18:19 +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 4A41C5DD9C; Thu, 4 Jan 2018 12:18:18 +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: <2506f1e8-a5b7-80b3-8ca8-183373170a69@redhat.com> Date: Thu, 4 Jan 2018 13:18:17 +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.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Thu, 04 Jan 2018 12:18:19 +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:13:16 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 01/04/18 02:09, Wang, Jian J wrote: > 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. Right, after I sent my email, it occurred to me that your calculation could actually match the other calculation that originally populates the CpuInfoInHob[N].ApTopOfStack fields. In other words, the values assigned could be correct. However, I do think / agree that we shouldn't duplicate the calculation, instead we should reuse the pre-computed values. Thanks! Laszlo > 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