From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) (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 BFF8781DA1 for ; Thu, 10 Nov 2016 01:00:42 -0800 (PST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga104.fm.intel.com with ESMTP; 10 Nov 2016 01:00:46 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,618,1473145200"; d="scan'208";a="784766567" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by FMSMGA003.fm.intel.com with ESMTP; 10 Nov 2016 01:00:30 -0800 Received: from fmsmsx157.amr.corp.intel.com (10.18.116.73) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 10 Nov 2016 01:00:29 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX157.amr.corp.intel.com (10.18.116.73) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 10 Nov 2016 01:00:29 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.239]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.96]) with mapi id 14.03.0248.002; Thu, 10 Nov 2016 17:00:27 +0800 From: "Fan, Jeff" To: Laszlo Ersek , "edk2-devel@ml01.01.org" CC: "Kinney, Michael D" , Paolo Bonzini , "Yao, Jiewen" Thread-Topic: [edk2] [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Put AP into safe hlt-loop code on S3 path Thread-Index: AQHSOxjCzW/jeJg1pE+2CQbQn/hvAKDRYs4AgACIh8A= Date: Thu, 10 Nov 2016 09:00:27 +0000 Message-ID: <542CF652F8836A4AB8DBFAAD40ED192A4A2D98DE@shsmsx102.ccr.corp.intel.com> References: <20161110060708.13932-1-jeff.fan@intel.com> <20161110060708.13932-2-jeff.fan@intel.com> <714ae963-4f97-bfb5-4265-fcdc8b96ace6@redhat.com> In-Reply-To: <714ae963-4f97-bfb5-4265-fcdc8b96ace6@redhat.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMDFlOWY5YWUtYjM3Yy00YzVmLWFmZGUtODM1NmYzOTEzMTZkIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IlJoZUpFYVpiQ3ZXb0FtWFF4aFQ3bUJTblZnd1wveExXYnNMVWlYRVFpYzR3PSJ9 x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Put AP into safe hlt-loop code on S3 path X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 Nov 2016 09:00:42 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable I agree with your proposal. Great suggestion. I will create v2 later. -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Lasz= lo Ersek Sent: Thursday, November 10, 2016 4:51 PM To: Fan, Jeff; edk2-devel@ml01.01.org Cc: Kinney, Michael D; Paolo Bonzini; Yao, Jiewen Subject: Re: [edk2] [PATCH 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Put AP into safe= hlt-loop code on S3 path Before I test this, I have one question / suggestion: On 11/10/16 07:07, Jeff Fan wrote: > On S3 path, we will wake up APs to restore CPU context in=20 > PiSmmCpuDxeSmm driver. However, we place AP in hlt-loop under 1MB=20 > space borrowed after CPU restoring CPU contexts. > In case, one NMI or SMI happens, APs may exit from hlt state and=20 > execute the instruction after HLT instruction. But the code under 1MB=20 > is no longer safe at that time. >=20 > This fix is to allocate one ACPI NVS range to place the AP hlt-loop=20 > code. When CPU finished restoration CPU contexts, AP will execute in this= ACPI NVS range. >=20 > https://bugzilla.tianocore.org/show_bug.cgi?id=3D216 >=20 > Reported-by: Laszlo Ersek > Cc: Laszlo Ersek > Cc: Paolo Bonzini > Cc: Jiewen Yao > Cc: Michael D Kinney > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jeff Fan > --- > UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 31 +++++++++++++++++++++= ++++++ > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 25 +++++++++++++++++++++ > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 13 +++++++++++ > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 26=20 > ++++++++++++++++++++++ > 4 files changed, 95 insertions(+) >=20 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c=20 > b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > index 6a798ef..6f290b8 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c > @@ -77,6 +77,13 @@ SMM_S3_RESUME_STATE *mSmmS3ResumeState =3D NU= LL; > =20 > BOOLEAN mAcpiS3Enable =3D TRUE; > =20 > +UINT8 *mApHltLoopCode =3D NULL; > +UINT8 mApHltLoopCodeTemplate[] =3D { > + 0xFA, // cli > + 0xF4, // hlt > + 0xEB, 0xFC // jmp $-2 > + }; > + > /** > Get MSR spin lock by MSR index. > =20 > @@ -376,6 +383,8 @@ MPRendezvousProcedure ( > CPU_REGISTER_TABLE *RegisterTableList; > UINT32 InitApicId; > UINTN Index; > + UINT32 TopOfStack; > + UINT8 Stack[128]; > =20 > ProgramVirtualWireMode (); > DisableLvtInterrupts (); > @@ -393,6 +402,13 @@ MPRendezvousProcedure ( > // Count down the number with lock mechanism. > // > InterlockedDecrement (&mNumberToFinish); > + > + // > + // Place AP into the safe code > + // > + TopOfStack =3D (UINT32) (UINTN) Stack + sizeof (Stack); Here I suggest to append the following assignment: TopOfStack &=3D ~(UINT32) (CPU_STACK_ALIGNMENT - 1); This will clear the least significant bits of TopOfStack, wasting at most (= CPU_STACK_ALIGNMENT - 1) bytes from the Stack array. However, the stack wil= l be aligned correctly. Not sure if it's necessary, but the patch reminds me of commit f98f5ec304ec= ("S3Resume2Pei: align return stacks explicitly"). What do you think? Thanks! Laszlo > + CopyMem ((VOID *) (UINTN) mApHltLoopCode, mApHltLoopCodeTemplate,=20 > + sizeof (mApHltLoopCodeTemplate)); TransferApToSafeState ((UINT32)=20 > + (UINTN) mApHltLoopCode, TopOfStack); > } > =20 > /** > @@ -731,6 +747,8 @@ InitSmmS3ResumeState ( > VOID *GuidHob; > EFI_SMRAM_DESCRIPTOR *SmramDescriptor; > SMM_S3_RESUME_STATE *SmmS3ResumeState; > + EFI_PHYSICAL_ADDRESS Address; > + EFI_STATUS Status; > =20 > if (!mAcpiS3Enable) { > return; > @@ -773,6 +791,19 @@ InitSmmS3ResumeState ( > // Patch SmmS3ResumeState->SmmS3Cr3 > // > InitSmmS3Cr3 (); > + > + // > + // Allocate safe memory in ACPI NVS for AP to execute hlt loop on=20 > + S3 path // Address =3D BASE_4GB - 1; Status =3D gBS->AllocatePages = ( > + AllocateMaxAddress, > + EfiACPIMemoryNVS, > + EFI_SIZE_TO_PAGES (sizeof (mApHltLoopCodeTemplate)), > + &Address > + ); > + ASSERT_EFI_ERROR (Status); > + mApHltLoopCode =3D (UINT8 *) (UINTN) Address; > } > =20 > /** > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c=20 > b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c > index 545b534..3e99aab 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c > @@ -94,3 +94,28 @@ InitGdt ( > *GdtStepSize =3D GdtTableStepSize; > return GdtTssTables; > } > + > +/** > + Transfer AP to safe hlt-loop after it finished restore CPU features on= S3 patch. > + =20 > + @param[in] ApHltLoopCode The 32-bit address of the safe hlt-loop fu= nction=20 > + @param[in] TopOfStack A pointer to the new stack to use for the = ApHltLoopCode. > + > +**/ > +VOID > +TransferApToSafeState ( > + IN UINT32 ApHltLoopCode, > + IN UINT32 TopOfStack > + ) > +{ > + SwitchStack ( > + (SWITCH_STACK_ENTRY_POINT) (UINTN) ApHltLoopCode, > + NULL, > + NULL, > + (VOID *) (UINTN) TopOfStack > + ); > + // > + // It should never reach here > + // > + ASSERT (FALSE); > +} > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h=20 > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index 9b119c8..82fa5a6 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -825,4 +825,17 @@ GetAcpiS3EnableFlag ( > VOID > ); > =20 > +/** > + Transfer AP to safe hlt-loop after it finished restore CPU features on= S3 patch. > + =20 > + @param[in] ApHltLoopCode The 32-bit address of the safe hlt-loop fu= nction=20 > + @param[in] TopOfStack A pointer to the new stack to use for the = ApHltLoopCode. > + > +**/ > +VOID > +TransferApToSafeState ( > + IN UINT32 ApHltLoopCode, > + IN UINT32 TopOfStack > + ); > + > #endif > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c=20 > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > index b53aa45..c05dec7 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > @@ -68,3 +68,29 @@ InitGdt ( > *GdtStepSize =3D GdtTableStepSize; > return GdtTssTables; > } > +/** > + Transfer AP to safe hlt-loop after it finished restore CPU features on= S3 patch. > + =20 > + @param[in] ApHltLoopCode The 32-bit address of the safe hlt-loop fu= nction=20 > + @param[in] TopOfStack A pointer to the new stack to use for the = ApHltLoopCode. > + > +**/ > +VOID > +TransferApToSafeState ( > + IN UINT32 ApHltLoopCode, > + IN UINT32 TopOfStack > + ) > +{ > + SwitchStack ( > + (SWITCH_STACK_ENTRY_POINT) (UINTN) ApHltLoopCode, > + NULL, > + NULL, > + (VOID *) (UINTN) TopOfStack > + ); > + // > + // It should never reach here > + // > + ASSERT (FALSE); > +} > + > + >=20 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel