Hi Laszlo, The patch has been checked in at R14977.
Thanks, Star -----Original Message----- From: Yao, Jiewen [mailto:jiewen....@intel.com] Sent: Tuesday, December 10, 2013 12:48 PM To: edk2-devel@lists.sourceforge.net Subject: Re: [edk2] [RFC v2 21/24] UefiCpuPkg: S3Resume2Pei: align return stacks explicitly Thanks for the patch. Reviewed by: Jiewen Yao <jiewen....@intel.com> -----Original Message----- From: Laszlo Ersek [mailto:ler...@redhat.com] Sent: Tuesday, December 10, 2013 11:56 AM To: edk2-devel@lists.sourceforge.net Subject: [edk2] [RFC v2 21/24] UefiCpuPkg: S3Resume2Pei: align return stacks explicitly S3RestoreConfig2() can optionally stack-switch to the SMM S3 Resume Entry Point and ask it to transfer to S3ResumeExecuteBootScript(). Similarly, S3ResumeExecuteBootScript() stack-switches explicitly to the boot script executor, and asks it to transfer to S3ResumeBootOs(). Currently the stack pointers specified for the SMM S3 Resume Entry Point and the boot script executor to use for returning are derived from addresses of the first local variables in S3RestoreConfig2() and S3ResumeExecuteBootScript(), respectively. Since (theoretically) the stack grows down as local variables are defined and functions are called, the idea is presumably to allow the respective callee to overwrite the caller's local variables. (The callees in question can never return normally, only by explicit stack switching.) Taking the address of "Status" is less portable than optimal however. Compilers are free to juggle local variables at build time as they please, including order and alignment on the stack. For example, when the code is built for 64-bit PEI with gcc-4.8.2, the address of "Status" trips up the alignment assertion in SwitchStack(). Let's align the address of "Status" down to CPU_STACK_ALIGNMENT explicitly. If a compiler ensures such alignment and places "Status" at the highest address automatically, then this change has no effect. Otherwise, we'll prepare ReturnStackPointer values that (a) are correctly aligned, (b) preserve the same amount or more (but never less) from the caller's local variables than before, which should be safe. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <ler...@redhat.com> --- UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c index e57398b..426eef7 100644 --- a/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c +++ b/UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume.c @@ -733,6 +733,16 @@ RestoreS3PageTables ( } /** + This macro aligns the address of a variable with auto storage + duration down to CPU_STACK_ALIGNMENT. + + Since the stack grows downward, the result preserves more of the +stack than + the original address (or the same amount), not less. +**/ +#define STACK_ALIGN_DOWN(Ptr) \ + ((UINTN)(Ptr) & ~(UINTN)(CPU_STACK_ALIGNMENT - 1)) + +/** Jump to boot script executor driver. The function will close and lock SMRAM and then jump to boot script execute driver to executing S3 boot script table. @@ -846,7 +856,7 @@ S3ResumeExecuteBootScript ( DEBUG (( EFI_D_ERROR, "PeiS3ResumeState - %x\r\n", PeiS3ResumeState)); PeiS3ResumeState->ReturnCs = 0x10; PeiS3ResumeState->ReturnEntryPoint = (EFI_PHYSICAL_ADDRESS)(UINTN)S3ResumeBootOs; - PeiS3ResumeState->ReturnStackPointer = (EFI_PHYSICAL_ADDRESS)(UINTN)&Status; + PeiS3ResumeState->ReturnStackPointer = + (EFI_PHYSICAL_ADDRESS)STACK_ALIGN_DOWN (&Status); // // Save IDT // @@ -1038,7 +1048,7 @@ S3RestoreConfig2 ( SmmS3ResumeState->ReturnEntryPoint = (EFI_PHYSICAL_ADDRESS)(UINTN)S3ResumeExecuteBootScript; SmmS3ResumeState->ReturnContext1 = (EFI_PHYSICAL_ADDRESS)(UINTN)AcpiS3Context; SmmS3ResumeState->ReturnContext2 = (EFI_PHYSICAL_ADDRESS)(UINTN)EfiBootScriptExecutorVariable; - SmmS3ResumeState->ReturnStackPointer = (EFI_PHYSICAL_ADDRESS)(UINTN)&Status; + SmmS3ResumeState->ReturnStackPointer = + (EFI_PHYSICAL_ADDRESS)STACK_ALIGN_DOWN (&Status); DEBUG (( EFI_D_ERROR, "SMM S3 Signature = %x\n", SmmS3ResumeState->Signature)); DEBUG (( EFI_D_ERROR, "SMM S3 Stack Base = %x\n", SmmS3ResumeState->SmmS3StackBase)); -- 1.8.3.1 ------------------------------------------------------------------------------ Sponsored by Intel(R) XDK Develop, test and display web and hybrid apps with a single code base. Download it for free now! http://pubads.g.doubleclick.net/gampad/clk?id=111408631&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Sponsored by Intel(R) XDK Develop, test and display web and hybrid apps with a single code base. Download it for free now! http://pubads.g.doubleclick.net/gampad/clk?id=111408631&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Rapidly troubleshoot problems before they affect your business. Most IT organizations don't have a clear picture of how application performance affects their revenue. With AppDynamics, you get 100% visibility into your Java,.NET, & PHP application. Start your 15-day FREE TRIAL of AppDynamics Pro! http://pubads.g.doubleclick.net/gampad/clk?id=84349831&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
