[PATCH v4 1/1] OvmfPkg/Sec: fix stack switch

[PATCH v4 1/1] OvmfPkg/Sec: fix stack switch

[Gerd Hoffmann]

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

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

[Yao, Jiewen]

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
> 
> 
> 
> 
> 

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

[Gerd Hoffmann]

On Tue, Jun 07, 2022 at 01:43:00PM +0000, Yao, Jiewen wrote:
> 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.

git log isn't very helpful here either, seems to date back to pre-git
times without descriptive commit messages.

just commenting it out doesn't work, so probably there is some reason
which is valid still ...

take care,
  Gerd

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

[Yao, Jiewen]

OK. Let's get it work at first.

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

> -----Original Message-----
> From: kraxel@redhat.com <kraxel@redhat.com>
> Sent: Tuesday, June 7, 2022 10:45 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: devel@edk2.groups.io; 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: Re: [edk2-devel] [PATCH v4 1/1] OvmfPkg/Sec: fix stack switch
> 
> On Tue, Jun 07, 2022 at 01:43:00PM +0000, Yao, Jiewen wrote:
> > 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.
> 
> git log isn't very helpful here either, seems to date back to pre-git
> times without descriptive commit messages.
> 
> just commenting it out doesn't work, so probably there is some reason
> which is valid still ...
> 
> take care,
>   Gerd

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

[Yao, Jiewen]

Hey Gerd
CI failed - https://github.com/tianocore/edk2/pull/2954

Have you run the CI before you submit the patch?

Thank you
Yao Jiewen


> -----Original Message-----
> From: Yao, Jiewen
> Sent: Tuesday, June 7, 2022 10:58 PM
> To: kraxel@redhat.com
> Cc: devel@edk2.groups.io; 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: RE: [edk2-devel] [PATCH v4 1/1] OvmfPkg/Sec: fix stack switch
> 
> OK. Let's get it work at first.
> 
> Acked-by: Jiewen Yao <Jiewen.yao@intel.com>
> 
> > -----Original Message-----
> > From: kraxel@redhat.com <kraxel@redhat.com>
> > Sent: Tuesday, June 7, 2022 10:45 PM
> > To: Yao, Jiewen <jiewen.yao@intel.com>
> > Cc: devel@edk2.groups.io; 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: Re: [edk2-devel] [PATCH v4 1/1] OvmfPkg/Sec: fix stack switch
> >
> > On Tue, Jun 07, 2022 at 01:43:00PM +0000, Yao, Jiewen wrote:
> > > 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.
> >
> > git log isn't very helpful here either, seems to date back to pre-git
> > times without descriptive commit messages.
> >
> > just commenting it out doesn't work, so probably there is some reason
> > which is valid still ...
> >
> > take care,
> >   Gerd

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

[Gerd Hoffmann]

On Wed, Jun 08, 2022 at 06:54:05AM +0000, Yao, Jiewen wrote:
> Hey Gerd
> CI failed - https://github.com/tianocore/edk2/pull/2954

Oh.

> Have you run the CI before you submit the patch?

Nope.

According to the gcc man page -fomit-frame-pointer is enabled by default
for -O1 (and higher levels), so I expected -fomit-frame-pointer being
used already implicitly.  Added it explicitly more for documentation and
clarity than for an actual change.

Apparently this not the case though.  Maybe older gcc versions have
different defaults.  I'll investigate.

take care,
  Gerd

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

[Gerd Hoffmann]

> Apparently this not the case though.  Maybe older gcc versions have
> different defaults.  I'll investigate.

Using -fomit-frame-pointer for x64 and
-fno-omit-frame-pointer for ia32 passes CI for me
(https://github.com/tianocore/edk2/pull/2955), I'll send the new patch
i a moment.

take care,
  Gerd