From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.151; helo=mga17.intel.com; envelope-from=jian.j.wang@intel.com; receiver=edk2-devel@lists.01.org Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) (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 516452257C2BE for ; Sat, 3 Mar 2018 00:55:53 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga005.jf.intel.com ([10.7.209.41]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Mar 2018 01:02:03 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.47,416,1515484800"; d="scan'208";a="205183666" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga005.jf.intel.com with ESMTP; 03 Mar 2018 01:02:02 -0800 Received: from fmsmsx101.amr.corp.intel.com (10.18.124.199) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sat, 3 Mar 2018 01:02:02 -0800 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx101.amr.corp.intel.com (10.18.124.199) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sat, 3 Mar 2018 01:02:01 -0800 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.116]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.130]) with mapi id 14.03.0319.002; Sat, 3 Mar 2018 17:02:00 +0800 From: "Wang, Jian J" To: "Wang, Jian J" , "Ni, Ruiyu" , Laszlo Ersek , "edk2-devel@lists.01.org" CC: "Dong, Eric" Thread-Topic: [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in executable memory Thread-Index: AQHTshv9QF4z2fQygU6a2v3GZ9aLp6O8UQCAgAFmUjD//9siAIAAlpuwgAAOq8A= Date: Sat, 3 Mar 2018 09:01:59 +0000 Message-ID: References: <20180302055839.18248-1-jian.j.wang@intel.com> <31960905-5140-ea20-aa02-38eff5be3cba@redhat.com> <04d43b92-5697-2561-e672-600caa518141@Intel.com> <734D49CCEBEEF84792F5B80ED585239D5BBC8636@SHSMSX104.ccr.corp.intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNjJkZTViNzAtYzYyZi00N2Y0LTgzZTYtZmQxYzk2MDI2YzJhIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJYSFdZNm11NkMxUFYwWVhKRmFWU0thQmhwU2ErTlhWV2UycnFoY0dvd1pFTDJcL2lDWVN4eFQ3QXZxVlVEa2ZBOSJ9 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: put mReservedApLoopFunc in executable memory 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: Sat, 03 Mar 2018 08:55:53 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Ray and Laszlo, I'll send out a v2 patch. Please give your comments based the new one. Regards, Jian > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wa= ng, > Jian J > Sent: Saturday, March 03, 2018 4:10 PM > To: Ni, Ruiyu ; Laszlo Ersek ; edk= 2- > devel@lists.01.org > Cc: Dong, Eric > Subject: Re: [edk2] [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc > in executable memory >=20 > > Will MEMORY_XP be recorded in GCD in future? > > Based on today's implementation, I prefer to not check. > > >=20 > Yes, it's in plan. Since it will impact the memory map layout, we have to= be very > careful to make those changes and do thorough OS boot tests. >=20 > Regards, > Jian >=20 > > -----Original Message----- > > From: Ni, Ruiyu > > Sent: Saturday, March 03, 2018 3:08 PM > > To: Wang, Jian J ; Laszlo Ersek ; > > edk2-devel@lists.01.org > > Cc: Dong, Eric > > Subject: RE: [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in > > executable memory > > > > > > > > Thanks/Ray > > > > > -----Original Message----- > > > From: Wang, Jian J > > > Sent: Saturday, March 3, 2018 9:32 AM > > > To: Ni, Ruiyu ; Laszlo Ersek ; > > > edk2-devel@lists.01.org > > > Cc: Dong, Eric > > > Subject: RE: [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc in > > > executable memory > > > > > > > > > > > > Regards, > > > Jian > > > > > > > > > > -----Original Message----- > > > > From: Ni, Ruiyu > > > > Sent: Friday, March 02, 2018 7:58 PM > > > > To: Laszlo Ersek ; Wang, Jian J > > > > ; edk2-devel@lists.01.org > > > > Cc: Dong, Eric > > > > Subject: Re: [PATCH] UefiCpuPkg/MpInitLib: put mReservedApLoopFunc = in > > > > executable memory > > > > > > > > On 3/2/2018 7:45 PM, Laszlo Ersek wrote: > > > > > On 03/02/18 06:58, Jian J Wang wrote: > > > > >> if PcdDxeNxMemoryProtectionPolicy is enabled for > > > > >> EfiReservedMemoryType of memory, #PF will be triggered for each = APs > > > > >> after ExitBootServices in SCRT test. The root cause is that AP > > > > >> wakeup code executed at that time is stored in memory of type > > > > >> EfiReservedMemoryType (referenced by global > > > mReservedApLoopFunc), which is marked as non-executable. > > > > >> > > > > >> This patch fixes this issue by setting memory of > > > > >> mReservedApLoopFunc to be executable immediately after allocatio= n. > > > > >> > > > > >> Cc: Ruiyu Ni > > > > >> Cc: Eric Dong > > > > >> Cc: Laszlo Ersek > > > > >> Contributed-under: TianoCore Contribution Agreement 1.1 > > > > >> Signed-off-by: Jian J Wang > > > > >> --- > > > > >> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 15 +++++++++++++++ > > > > >> 1 file changed, 15 insertions(+) > > > > >> > > > > >> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > > > > b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > > > > >> index fd2317924f..5fcb08677c 100644 > > > > >> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > > > > >> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > > > > >> @@ -399,6 +399,21 @@ InitMpGlobalData ( > > > > >> &Address > > > > >> ); > > > > >> ASSERT_EFI_ERROR (Status); > > > > >> + > > > > >> + // > > > > >> + // Make sure that the buffer memory is executable. > > > > >> + // > > > > >> + Status =3D gDS->GetMemorySpaceDescriptor (Address, &MemDesc);= if > > > > >> + (!EFI_ERROR (Status)) { > > > > >> + gDS->SetMemorySpaceAttributes ( > > > > >> + Address, > > > > >> + EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES ( > > > > >> + CpuMpData->AddressMap.RelocateApLoopFuncSize > > > > >> + )), > > > > >> + MemDesc.Attributes & (~EFI_MEMORY_XP) > > > > >> + ); > > > > >> + } > > > > >> + > > > > >> mReservedApLoopFunc =3D (VOID *) (UINTN) Address; > > > > >> ASSERT (mReservedApLoopFunc !=3D NULL); > > > > >> mReservedTopOfApStack =3D (UINTN) Address + EFI_PAGES_TO_SIZ= E > > > > (EFI_SIZE_TO_PAGES (ApSafeBufferSize)); > > > > >> > > > > > > > > > > Honestly, I see little point in the "Dxe Nx Memory Protection Pol= icy" > > > > > when we then override it *every time* it gets in our way. > > > > > "RelocateApLoopFuncSize" is likely significantly smaller than a f= ull > > > > > page, so we're making a good chunk of the "safe stack(s)" executa= ble too. > > > > > > > > > > Anyway, can you perhaps check BIT0 (standing for > > > > > EfiReservedMemoryType) in PcdDxeNxMemoryProtectionPolicy, to see > > > if the above hack is necessary? > > > > > > > > > > Thanks > > > > > Laszlo > > > > > > > > > > > > > Checking PCD is not very good I think. > > > > If checking is really needed, how about check MemDesc.Attributes > > > > EFI_MEMORY_XP bit? > > > > > > > > > > > > > > a. Page attributes update has to be in page unit. If we want to avoid= making > > > stack memory executable, reserving it in a separate memory page is th= e only > > > way I can think of. > > > > > > b. Checking MemDesc.Attributes against EFI_MEMORY_XP doesn't work > > > here. The reason is that EFI_MEMORY_XP is set to configured type of > > > memory via CPU Arch protocol in DxeCore code, which won't be recorded= in > > > GCD service data. Maybe checking PCD BIT0 is the only way according > current > > > situation. > > > > Will MEMORY_XP be recorded in GCD in future? > > Based on today's implementation, I prefer to not check. > > > > > > > > > -- > > > > Thanks, > > > > Ray > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel