[PATCH v6 0/3] Fix stack switching, this time for real.

[PATCH v6 0/3] Fix stack switching, this time for real.

[Gerd Hoffmann]

My testing was busted, ran the tests with outdated tools_def
so I didn't notice the patch had zero effect ...

So, revert the broken patch, drop two lines which are not used
anywhere to reduce confusion, then just disable omit-frame-pointers
for ia32 and x64 to get the source tree back into working state
with minimum fuss.

Not fully sure yet how to go forward with that longer-term.  Enabling
omit-frame-pointers unconditionally makes the NOOPT noticeable larger,
which is probably the reason why the gcc enables that by default only
for -O1 and higher.  So maybe we need different cflags for NOOPT vs.
DEBUG/RELEASE builds.

Or go for a completely different approach, like integrating
DebugAgentLib support into the Pei Dispatcher, so the need to
have a custom TemporaryRamMigration() for that goes away ...

Comments?

take care,
  Gerd

Gerd Hoffmann (3):
  Revert "OvmfPkg/Sec: fix stack switch"
  tools_def: remove GCC_IA32_CC_FLAGS/GCC_X64_CC_FLAGS
  tools_def: add -fno-omit-frame-pointer to GCC48_{IA32,X64}_CC_FLAGS

 OvmfPkg/Sec/SecMain.c             | 4 ----
 BaseTools/Conf/tools_def.template | 8 +++-----
 2 files changed, 3 insertions(+), 9 deletions(-)

-- 
2.36.1

[PATCH v6 1/3] Revert "OvmfPkg/Sec: fix stack switch"

[Gerd Hoffmann]

This reverts commit ff36b2550f94dc5fac838cf298ae5a23cfddf204.

Has no effect because GCC_IA32_CC_FLAGS and GCC_X64_CC_FLAGS are unused.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/Sec/SecMain.c             | 4 ----
 BaseTools/Conf/tools_def.template | 6 +++---
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
index 3ca0dcdfd3dd..1167d22a68cc 100644
--- a/OvmfPkg/Sec/SecMain.c
+++ b/OvmfPkg/Sec/SecMain.c
@@ -1052,15 +1052,11 @@ 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 adcd23f7273f..5ed19810b727 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -1849,9 +1849,9 @@ NOOPT_*_*_OBJCOPY_ADDDEBUGFLAG     = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_N
 *_*_*_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_IA32_CC_FLAGS           = DEF(GCC_ALL_CC_FLAGS) -m32 -malign-double -freorder-blocks -freorder-blocks-and-partition -O2 -mno-stack-arg-probe -fno-omit-frame-pointer
-DEFINE GCC_X64_CC_FLAGS            = DEF(GCC_ALL_CC_FLAGS) -mno-red-zone -Wno-address -mno-stack-arg-probe -fomit-frame-pointer -DOMIT_FRAME_POINTER=1
-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 -DOMIT_FRAME_POINTER=1 -Wno-address -mthumb -mfloat-abi=soft -fno-pic -fno-pie
+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_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

Re: [edk2-devel] [PATCH v6 1/3] Revert "OvmfPkg/Sec: fix stack switch"

[Yao, Jiewen]

OK. I will revert this at first.

Let's figure out a right solution.

Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> Hoffmann
> Sent: Friday, June 10, 2022 7:03 PM
> To: devel@edk2.groups.io
> Cc: Oliver Steffen <osteffen@redhat.com>; James Bottomley
> <jejb@linux.ibm.com>; Tom Lendacky <thomas.lendacky@amd.com>; Gerd
> Hoffmann <kraxel@redhat.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Chen, Christine <yuwei.chen@intel.com>; Brijesh Singh
> <brijesh.singh@amd.com>; Aktas, Erdem <erdemaktas@google.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
> Ard Biesheuvel <ardb+tianocore@kernel.org>; Pawel Polawski
> <ppolawsk@redhat.com>; Feng, Bob C <bob.c.feng@intel.com>; Xu, Min M
> <min.m.xu@intel.com>
> Subject: [edk2-devel] [PATCH v6 1/3] Revert "OvmfPkg/Sec: fix stack switch"
> 
> This reverts commit ff36b2550f94dc5fac838cf298ae5a23cfddf204.
> 
> Has no effect because GCC_IA32_CC_FLAGS and GCC_X64_CC_FLAGS are
> unused.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/Sec/SecMain.c             | 4 ----
>  BaseTools/Conf/tools_def.template | 6 +++---
>  2 files changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> index 3ca0dcdfd3dd..1167d22a68cc 100644
> --- a/OvmfPkg/Sec/SecMain.c
> +++ b/OvmfPkg/Sec/SecMain.c
> @@ -1052,15 +1052,11 @@ 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 adcd23f7273f..5ed19810b727 100755
> --- a/BaseTools/Conf/tools_def.template
> +++ b/BaseTools/Conf/tools_def.template
> @@ -1849,9 +1849,9 @@ NOOPT_*_*_OBJCOPY_ADDDEBUGFLAG     = --add-
> gnu-debuglink=$(DEBUG_DIR)/$(MODULE_N
>  *_*_*_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_IA32_CC_FLAGS           = DEF(GCC_ALL_CC_FLAGS) -m32 -malign-
> double -freorder-blocks -freorder-blocks-and-partition -O2 -mno-stack-arg-
> probe -fno-omit-frame-pointer
> -DEFINE GCC_X64_CC_FLAGS            = DEF(GCC_ALL_CC_FLAGS) -mno-red-zone -
> Wno-address -mno-stack-arg-probe -fomit-frame-pointer -
> DOMIT_FRAME_POINTER=1
> -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 -DOMIT_FRAME_POINTER=1 -Wno-address -
> mthumb -mfloat-abi=soft -fno-pic -fno-pie
> +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_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
> 
> 
> 
> 
> 

[PATCH v6 2/3] tools_def: remove GCC_IA32_CC_FLAGS/GCC_X64_CC_FLAGS

[Gerd Hoffmann]

They are not used anywhere.  Remove them.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 BaseTools/Conf/tools_def.template | 2 --
 1 file changed, 2 deletions(-)

diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index 5ed19810b727..a53199c9c76b 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -1849,8 +1849,6 @@ NOOPT_*_*_OBJCOPY_ADDDEBUGFLAG     = --add-gnu-debuglink=$(DEBUG_DIR)/$(MODULE_N
 *_*_*_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_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_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
-- 
2.36.1

[PATCH v6 3/3] tools_def: add -fno-omit-frame-pointer to GCC48_{IA32,X64}_CC_FLAGS

[Gerd Hoffmann]

Fixes problems due to code assuming it runs with frame pointers and thus
updates rbp / ebp registers when switching stacks.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 BaseTools/Conf/tools_def.template | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index a53199c9c76b..756f112b9395 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -1882,8 +1882,8 @@ DEFINE GCC_DEPS_FLAGS              = -MMD -MF $@.deps
 
 DEFINE GCC48_ALL_CC_FLAGS            = DEF(GCC_ALL_CC_FLAGS) -ffunction-sections -fdata-sections -DSTRING_ARRAY_NAME=$(BASE_NAME)Strings
 DEFINE GCC48_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-n,-q,--gc-sections -z common-page-size=0x20
-DEFINE GCC48_IA32_CC_FLAGS           = DEF(GCC48_ALL_CC_FLAGS) -m32 -march=i586 -malign-double -fno-stack-protector -D EFI32 -fno-asynchronous-unwind-tables -Wno-address
-DEFINE GCC48_X64_CC_FLAGS            = DEF(GCC48_ALL_CC_FLAGS) -m64 -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=small -fpie -fno-asynchronous-unwind-tables -Wno-address
+DEFINE GCC48_IA32_CC_FLAGS           = DEF(GCC48_ALL_CC_FLAGS) -m32 -march=i586 -malign-double -fno-stack-protector -D EFI32 -fno-asynchronous-unwind-tables -Wno-address -fno-omit-frame-pointer
+DEFINE GCC48_X64_CC_FLAGS            = DEF(GCC48_ALL_CC_FLAGS) -m64 -fno-stack-protector "-DEFIAPI=__attribute__((ms_abi))" -maccumulate-outgoing-args -mno-red-zone -Wno-address -mcmodel=small -fpie -fno-asynchronous-unwind-tables -Wno-address  -fno-omit-frame-pointer
 DEFINE GCC48_IA32_X64_ASLDLINK_FLAGS = DEF(GCC48_IA32_X64_DLINK_COMMON) -Wl,--entry,ReferenceAcpiTable -u ReferenceAcpiTable
 DEFINE GCC48_IA32_X64_DLINK_FLAGS    = DEF(GCC48_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map,--whole-archive
 DEFINE GCC48_IA32_DLINK2_FLAGS       = -Wl,--defsym=PECOFF_HEADER_SIZE=0x220 DEF(GCC_DLINK2_FLAGS_COMMON)
-- 
2.36.1