From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 EB20B81C57 for ; Thu, 10 Nov 2016 01:30:40 -0800 (PST) Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 176E9335E88; Thu, 10 Nov 2016 09:30:44 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-106.phx2.redhat.com [10.3.116.106]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uAA9Ug12022638; Thu, 10 Nov 2016 04:30:42 -0500 To: "Fan, Jeff" , "edk2-devel@ml01.01.org" References: <20161110060708.13932-1-jeff.fan@intel.com> <20161110060708.13932-2-jeff.fan@intel.com> <714ae963-4f97-bfb5-4265-fcdc8b96ace6@redhat.com> <542CF652F8836A4AB8DBFAAD40ED192A4A2D98DE@shsmsx102.ccr.corp.intel.com> Cc: "Kinney, Michael D" , Paolo Bonzini , "Yao, Jiewen" From: Laszlo Ersek Message-ID: <6a80e8e7-e62b-b35b-b8a5-6196e2bd6a20@redhat.com> Date: Thu, 10 Nov 2016 10:30:41 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <542CF652F8836A4AB8DBFAAD40ED192A4A2D98DE@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Thu, 10 Nov 2016 09:30:44 +0000 (UTC) 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:30:41 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 11/10/16 10:00, Fan, Jeff wrote: > I agree with your proposal. Great suggestion. I will create v2 later. Thank you. Since you're going to send a v2: while applying this patch for testing, git-am warned me that there were 7 new lines with trailing whitespace. Can you please trim those? Thanks Laszlo > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo 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 >> PiSmmCpuDxeSmm driver. However, we place AP in hlt-loop under 1MB >> space borrowed after CPU restoring CPU contexts. >> In case, one NMI or SMI happens, APs may exit from hlt state and >> execute the instruction after HLT instruction. But the code under 1MB >> is no longer safe at that time. >> >> This fix is to allocate one ACPI NVS range to place the AP hlt-loop >> code. When CPU finished restoration CPU contexts, AP will execute in this ACPI NVS range. >> >> https://bugzilla.tianocore.org/show_bug.cgi?id=216 >> >> 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 >> ++++++++++++++++++++++ >> 4 files changed, 95 insertions(+) >> >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c >> 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 = NULL; >> >> BOOLEAN mAcpiS3Enable = TRUE; >> >> +UINT8 *mApHltLoopCode = NULL; >> +UINT8 mApHltLoopCodeTemplate[] = { >> + 0xFA, // cli >> + 0xF4, // hlt >> + 0xEB, 0xFC // jmp $-2 >> + }; >> + >> /** >> Get MSR spin lock by MSR index. >> >> @@ -376,6 +383,8 @@ MPRendezvousProcedure ( >> CPU_REGISTER_TABLE *RegisterTableList; >> UINT32 InitApicId; >> UINTN Index; >> + UINT32 TopOfStack; >> + UINT8 Stack[128]; >> >> ProgramVirtualWireMode (); >> DisableLvtInterrupts (); >> @@ -393,6 +402,13 @@ MPRendezvousProcedure ( >> // Count down the number with lock mechanism. >> // >> InterlockedDecrement (&mNumberToFinish); >> + >> + // >> + // Place AP into the safe code >> + // >> + TopOfStack = (UINT32) (UINTN) Stack + sizeof (Stack); > > Here I suggest to append the following assignment: > > TopOfStack &= ~(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 will 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, >> + sizeof (mApHltLoopCodeTemplate)); TransferApToSafeState ((UINT32) >> + (UINTN) mApHltLoopCode, TopOfStack); >> } >> >> /** >> @@ -731,6 +747,8 @@ InitSmmS3ResumeState ( >> VOID *GuidHob; >> EFI_SMRAM_DESCRIPTOR *SmramDescriptor; >> SMM_S3_RESUME_STATE *SmmS3ResumeState; >> + EFI_PHYSICAL_ADDRESS Address; >> + EFI_STATUS Status; >> >> 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 >> + S3 path // Address = BASE_4GB - 1; Status = gBS->AllocatePages ( >> + AllocateMaxAddress, >> + EfiACPIMemoryNVS, >> + EFI_SIZE_TO_PAGES (sizeof (mApHltLoopCodeTemplate)), >> + &Address >> + ); >> + ASSERT_EFI_ERROR (Status); >> + mApHltLoopCode = (UINT8 *) (UINTN) Address; >> } >> >> /** >> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c >> 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 = GdtTableStepSize; >> return GdtTssTables; >> } >> + >> +/** >> + Transfer AP to safe hlt-loop after it finished restore CPU features on S3 patch. >> + >> + @param[in] ApHltLoopCode The 32-bit address of the safe hlt-loop function >> + @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 >> 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 >> ); >> >> +/** >> + Transfer AP to safe hlt-loop after it finished restore CPU features on S3 patch. >> + >> + @param[in] ApHltLoopCode The 32-bit address of the safe hlt-loop function >> + @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 >> 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 = GdtTableStepSize; >> return GdtTssTables; >> } >> +/** >> + Transfer AP to safe hlt-loop after it finished restore CPU features on S3 patch. >> + >> + @param[in] ApHltLoopCode The 32-bit address of the safe hlt-loop function >> + @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); >> +} >> + >> + >> > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >