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 1529B81C91 for ; Thu, 10 Nov 2016 00:50:58 -0800 (PST) Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (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 EE5AE7EAB9; Thu, 10 Nov 2016 08:51:00 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-106.phx2.redhat.com [10.3.116.106]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uAA8owMT027419; Thu, 10 Nov 2016 03:50:59 -0500 To: Jeff Fan , edk2-devel@ml01.01.org References: <20161110060708.13932-1-jeff.fan@intel.com> <20161110060708.13932-2-jeff.fan@intel.com> Cc: Paolo Bonzini , Jiewen Yao , Michael D Kinney From: Laszlo Ersek Message-ID: <714ae963-4f97-bfb5-4265-fcdc8b96ace6@redhat.com> Date: Thu, 10 Nov 2016 09:50:58 +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: <20161110060708.13932-2-jeff.fan@intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Thu, 10 Nov 2016 08:51:01 +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 08:50:58 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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); > +} > + > + >