Re: [edk2-devel] [PATCH v4 1/1] OvmfPkg/Sec: fix stack switch

["Yao, Jiewen" <jiewen.yao@intel.com>]

Hello
As far as I know the TemporaryRamMigration() is an optional PPI according to PI spec, I forget why we add it in the beginning.

To reduce the maintenance effort, can we try to remove it? As such the PEI core can perform the migration in https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c.

Thank you
Yao Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> Hoffmann
> Sent: Tuesday, June 7, 2022 8:39 PM
> To: devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Pawel Polawski <ppolawsk@redhat.com>; Brijesh Singh
> <brijesh.singh@amd.com>; Oliver Steffen <osteffen@redhat.com>; James
> Bottomley <jejb@linux.ibm.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>;
> Aktas, Erdem <erdemaktas@google.com>; Feng, Bob C
> <bob.c.feng@intel.com>; Chen, Christine <yuwei.chen@intel.com>; Gao, Liming
> <gaoliming@byosoft.com.cn>; Xu, Min M <min.m.xu@intel.com>; Tom
> Lendacky <thomas.lendacky@amd.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Jiri Slaby <jirislaby@kernel.org>
> Subject: [edk2-devel] [PATCH v4 1/1] OvmfPkg/Sec: fix stack switch
> 
> The ebp/rbp register can either be used for the frame pointer or
> as general purpose register.  With gcc (and clang) this depends
> on the -f(no-)omit-frame-pointer switch.
> 
> This patch updates tools_def.template to explicitly set the compiler
> option and also add a define to allow conditionally compile code.
> 
> The new define is used to fix stack switching in TemporaryRamMigration.
> The ebp/rbp must not be touched when the compiler can use it as general
> purpose register.  With version 12 gcc starts actually using the
> register, so changing it leads to firmware crashes in some
> configurations.
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3934
> Reported-by: Jiri Slaby <jirislaby@kernel.org>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/Sec/SecMain.c             | 4 ++++
>  BaseTools/Conf/tools_def.template | 4 ++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> index 1167d22a68cc..3ca0dcdfd3dd 100644
> --- a/OvmfPkg/Sec/SecMain.c
> +++ b/OvmfPkg/Sec/SecMain.c
> @@ -1052,11 +1052,15 @@ TemporaryRamMigration (
>    if (SetJump (&JumpBuffer) == 0) {
>   #if defined (MDE_CPU_IA32)
>      JumpBuffer.Esp = JumpBuffer.Esp + DebugAgentContext.StackMigrateOffset;
> + #ifndef OMIT_FRAME_POINTER
>      JumpBuffer.Ebp = JumpBuffer.Ebp + DebugAgentContext.StackMigrateOffset;
>   #endif
> + #endif
>   #if defined (MDE_CPU_X64)
>      JumpBuffer.Rsp = JumpBuffer.Rsp + DebugAgentContext.StackMigrateOffset;
> + #ifndef OMIT_FRAME_POINTER
>      JumpBuffer.Rbp = JumpBuffer.Rbp + DebugAgentContext.StackMigrateOffset;
> + #endif
>   #endif
>      LongJump (&JumpBuffer, (UINTN)-1);
>    }
> diff --git a/BaseTools/Conf/tools_def.template
> b/BaseTools/Conf/tools_def.template
> index 5ed19810b727..18e3d6c5e907 100755
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -1848,10 +1848,10 @@ NOOPT_*_*_OBJCOPY_ADDDEBUGFLAG     = --
> add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_N
>  *_*_*_DTCPP_PATH                   = DEF(DTCPP_BIN)
>  *_*_*_DTC_PATH                     = DEF(DTC_BIN)
> 
> -DEFINE GCC_ALL_CC_FLAGS            = -g -Os -fshort-wchar -fno-builtin -fno-
> strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-
> common
> +DEFINE GCC_ALL_CC_FLAGS            = -g -Os -fshort-wchar -fno-builtin -fno-
> strict-aliasing -Wall -Werror -Wno-array-bounds -include AutoGen.h -fno-
> common -fomit-frame-pointer -DOMIT_FRAME_POINTER=1
>  DEFINE GCC_IA32_CC_FLAGS           = DEF(GCC_ALL_CC_FLAGS) -m32 -malign-
> double -freorder-blocks -freorder-blocks-and-partition -O2 -mno-stack-arg-
> probe
>  DEFINE GCC_X64_CC_FLAGS            = DEF(GCC_ALL_CC_FLAGS) -mno-red-zone -
> Wno-address -mno-stack-arg-probe
> -DEFINE GCC_ARM_CC_FLAGS            = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian
> -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-
> sections -fomit-frame-pointer -Wno-address -mthumb -mfloat-abi=soft -fno-pic
> -fno-pie
> +DEFINE GCC_ARM_CC_FLAGS            = DEF(GCC_ALL_CC_FLAGS) -mlittle-endian
> -mabi=aapcs -fno-short-enums -funsigned-char -ffunction-sections -fdata-
> sections -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie
>  DEFINE GCC_ARM_CC_XIPFLAGS         = -mno-unaligned-access
>  DEFINE GCC_AARCH64_CC_FLAGS        = DEF(GCC_ALL_CC_FLAGS) -mlittle-
> endian -fno-short-enums -fverbose-asm -funsigned-char  -ffunction-sections -
> fdata-sections -Wno-address -fno-asynchronous-unwind-tables -fno-unwind-
> tables -fno-pic -fno-pie -ffixed-x18
>  DEFINE GCC_AARCH64_CC_XIPFLAGS     = -mstrict-align -mgeneral-regs-only
> --
> 2.36.1
> 
> 
> 
> 
>