* [PATCH v4 1/1] OvmfPkg/Sec: fix stack switch @ 2022-06-07 12:39 Gerd Hoffmann 2022-06-07 13:43 ` [edk2-devel] " Yao, Jiewen 0 siblings, 1 reply; 7+ messages in thread From: Gerd Hoffmann @ 2022-06-07 12:39 UTC (permalink / raw) To: devel Cc: Jiewen Yao, Gerd Hoffmann, Pawel Polawski, Brijesh Singh, Oliver Steffen, James Bottomley, Ard Biesheuvel, Erdem Aktas, Bob Feng, Yuwei Chen, Liming Gao, Min Xu, Tom Lendacky, Jordan Justen, Jiri Slaby 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v4 1/1] OvmfPkg/Sec: fix stack switch 2022-06-07 12:39 [PATCH v4 1/1] OvmfPkg/Sec: fix stack switch Gerd Hoffmann @ 2022-06-07 13:43 ` Yao, Jiewen 2022-06-07 14:45 ` Gerd Hoffmann 0 siblings, 1 reply; 7+ messages in thread From: Yao, Jiewen @ 2022-06-07 13:43 UTC (permalink / raw) To: devel@edk2.groups.io, kraxel@redhat.com Cc: Pawel Polawski, Brijesh Singh, Oliver Steffen, James Bottomley, Ard Biesheuvel, Aktas, Erdem, Feng, Bob C, Chen, Christine, Gao, Liming, Xu, Min M, Tom Lendacky, Justen, Jordan L, Jiri Slaby 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 > > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v4 1/1] OvmfPkg/Sec: fix stack switch 2022-06-07 13:43 ` [edk2-devel] " Yao, Jiewen @ 2022-06-07 14:45 ` Gerd Hoffmann 2022-06-07 14:58 ` Yao, Jiewen 0 siblings, 1 reply; 7+ messages in thread From: Gerd Hoffmann @ 2022-06-07 14:45 UTC (permalink / raw) To: Yao, Jiewen Cc: devel@edk2.groups.io, Pawel Polawski, Brijesh Singh, Oliver Steffen, James Bottomley, Ard Biesheuvel, Aktas, Erdem, Feng, Bob C, Chen, Christine, Gao, Liming, Xu, Min M, Tom Lendacky, Justen, Jordan L, Jiri Slaby 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v4 1/1] OvmfPkg/Sec: fix stack switch 2022-06-07 14:45 ` Gerd Hoffmann @ 2022-06-07 14:58 ` Yao, Jiewen 2022-06-08 6:54 ` Yao, Jiewen 0 siblings, 1 reply; 7+ messages in thread From: Yao, Jiewen @ 2022-06-07 14:58 UTC (permalink / raw) To: kraxel@redhat.com Cc: devel@edk2.groups.io, Pawel Polawski, Brijesh Singh, Oliver Steffen, James Bottomley, Ard Biesheuvel, Aktas, Erdem, Feng, Bob C, Chen, Christine, Gao, Liming, Xu, Min M, Tom Lendacky, Justen, Jordan L, Jiri Slaby 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v4 1/1] OvmfPkg/Sec: fix stack switch 2022-06-07 14:58 ` Yao, Jiewen @ 2022-06-08 6:54 ` Yao, Jiewen 2022-06-08 8:59 ` Gerd Hoffmann 0 siblings, 1 reply; 7+ messages in thread From: Yao, Jiewen @ 2022-06-08 6:54 UTC (permalink / raw) To: kraxel@redhat.com Cc: devel@edk2.groups.io, Pawel Polawski, Brijesh Singh, Oliver Steffen, James Bottomley, Ard Biesheuvel, Aktas, Erdem, Feng, Bob C, Chen, Christine, Gao, Liming, Xu, Min M, Tom Lendacky, Justen, Jordan L, Jiri Slaby 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v4 1/1] OvmfPkg/Sec: fix stack switch 2022-06-08 6:54 ` Yao, Jiewen @ 2022-06-08 8:59 ` Gerd Hoffmann 2022-06-08 10:09 ` Gerd Hoffmann 0 siblings, 1 reply; 7+ messages in thread From: Gerd Hoffmann @ 2022-06-08 8:59 UTC (permalink / raw) To: devel, jiewen.yao Cc: Pawel Polawski, Brijesh Singh, Oliver Steffen, James Bottomley, Ard Biesheuvel, Aktas, Erdem, Feng, Bob C, Chen, Christine, Gao, Liming, Xu, Min M, Tom Lendacky, Justen, Jordan L, Jiri Slaby 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH v4 1/1] OvmfPkg/Sec: fix stack switch 2022-06-08 8:59 ` Gerd Hoffmann @ 2022-06-08 10:09 ` Gerd Hoffmann 0 siblings, 0 replies; 7+ messages in thread From: Gerd Hoffmann @ 2022-06-08 10:09 UTC (permalink / raw) To: devel Cc: jiewen.yao, Pawel Polawski, Brijesh Singh, Oliver Steffen, James Bottomley, Ard Biesheuvel, Aktas, Erdem, Feng, Bob C, Chen, Christine, Gao, Liming, Xu, Min M, Tom Lendacky, Justen, Jordan L, Jiri Slaby > 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-06-08 10:09 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-06-07 12:39 [PATCH v4 1/1] OvmfPkg/Sec: fix stack switch Gerd Hoffmann 2022-06-07 13:43 ` [edk2-devel] " Yao, Jiewen 2022-06-07 14:45 ` Gerd Hoffmann 2022-06-07 14:58 ` Yao, Jiewen 2022-06-08 6:54 ` Yao, Jiewen 2022-06-08 8:59 ` Gerd Hoffmann 2022-06-08 10:09 ` Gerd Hoffmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox