From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.31; helo=mga06.intel.com; envelope-from=jian.j.wang@intel.com; receiver=edk2-devel@lists.01.org Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (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 653422034B421 for ; Wed, 3 Jan 2018 17:04:24 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Jan 2018 17:09:27 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,504,1508828400"; d="scan'208";a="7212304" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga002.fm.intel.com with ESMTP; 03 Jan 2018 17:09:27 -0800 Received: from FMSMSX109.amr.corp.intel.com (10.18.116.9) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 3 Jan 2018 17:09:27 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx109.amr.corp.intel.com (10.18.116.9) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 3 Jan 2018 17:09:26 -0800 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.213]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.93]) with mapi id 14.03.0319.002; Thu, 4 Jan 2018 09:09:25 +0800 From: "Wang, Jian J" To: "Wang, Jian J" , Laszlo Ersek , "edk2-devel@lists.01.org" CC: "Yao, Jiewen" , "Dong, Eric" Thread-Topic: [edk2] [PATCH] UefiCpuPkg/MpInitLib: fix wrong base address set as Stack Guard Thread-Index: AQHTgIBBAhUXNh3QfEei4ASfd+cM3KNh6soAgAD5yACAAAkXYA== Date: Thu, 4 Jan 2018 01:09:24 +0000 Message-ID: References: <20171229083631.16652-1-jian.j.wang@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNDliY2U3NTYtYTE2ZC00YzY1LTg3OGItN2JkOGNkOGQxZmQyIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJqb2t6aDBCa1FKdERsdkh5Y29SRW54aHpQWlNQcEI5Nk92V2g5ZE5PY2hzSHV1d1lPYWpNc1dlXC83aGJHSDJnOCJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 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 01:04:24 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, More explanations: [UefiCpuPkg\Library\MpInitLib\MpLib.c] According to the code, the BSP's (CpuInfoInHob[0].ApTopOfStack) is initiali= zed to the bottom of the stack (line 1501) but AP's ApTopOfStack is correctly init= ialized (line 598). Although my calculation is correct, I think it'd be better to u= se AP's=20 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 Wa= ng, > 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 >=20 > Laszlo, >=20 > I revisited code of MpInitLib. I found that CPU_INFO_IN_HOB.ApTopOfStack > was assigned to CpuMpData->Buffer in MpInitLibInitialize() >=20 > (line1501) InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer); >=20 > but in >=20 > (line598) ApTopOfStack =3D CpuMpData->Buffer + (ProcessorNumber + 1) * > CpuMpData->CpuApStackSize; > (line608) InitializeApData (CpuMpData, ProcessorNumber, BistData, > ApTopOfStack); >=20 > Since InitMpGlobalData() is called just after first situation, my patch i= s correct. >=20 > I think the problem here is that ApTopOfStack initialized at line 1501 is= not > correct. >=20 >=20 > Regards, > Jian >=20 >=20 > > -----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 addres= s 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 alloc= ated > > > 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 =3D 0; Index < CpuMpData->CpuCount; ++Index) { > > > - StackBase =3D CpuMpData->Buffer + Index * CpuMpData->CpuApStac= kSize; > > > + // > > > + // DXE will reuse stack allocated for APs at PEI phase if it's a= vailable. > > > + // Let's check it here. > > > + // > > > + CpuInfoInHob =3D (CPU_INFO_IN_HOB *)(UINTN)CpuMpData- > > >CpuInfoInHob; > > > + if (CpuInfoInHob !=3D NULL && CpuInfoInHob->ApTopOfStack !=3D 0)= { > > > + StackBase =3D CpuInfoInHob->ApTopOfStack; > > > + } else { > > > + StackBase =3D 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 (=3D 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 =3D 0; Index < CpuMpData->CpuCount; ++Index) { > > > Status =3D 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 +=3D 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