Re: [edk2-devel] ovmf miscompiles with gcc-12
Mike, This sounds like a conversation we had years ago? I think we concluded we needed to write stuff in assembler and not depend on implementation choices of the compiler with regard to registers usage? I want to say something broke with Xcode clang. I think it might have been the Emulator code TemporaryRamMigration, so that was probably similar to this case? I’m a little unclear why this code is not using SwitchStack()[1]? I thought one of the points of jumping to the new function was giving up of old RBP usage? Also calling assembler and the indirect procedure call (SWITCH_STACK_ENTRY_POINT) force the complier to serialize to the ABI and thus greatly reduce the exposure to compiler optimization choices. Not sure if there is something I’m missing and SwitchStack() can’t work? Doh just realized to use SwitchStack the TemporaryRamMigration API would need to pass in the function to return too like SwitchStack. So that is a big NO GO. OK so this is like the Emulator case when the OS uses SYS V ABI and EFI does not. OK I think the only pedantic way to fix this is to make the public API call an assembler shim that calls the C code. Then the C code becomes.. EFI_STATUS EFIAPI TemporaryRamMigration ( IN CONST EFI_PEI_SERVICES **PeiServices, IN EFI_PHYSICAL_ADDRESSTemporaryMemoryBase, IN EFI_PHYSICAL_ADDRESSPermanentMemoryBase, IN UINTN CopySize IN OUT UINTN.*Rsp, IN OUT UINTN.*Rbp, OUT BOOLEAN *DebugTimerStatus ) The assembler shim can capture the actual RSP/RBP on entry and just fix them them on exit. The stub may need to call SaveAndSetDebugTimerInterrupt (DebugTimerStatus); prior to exit. Then the existing SetJump/LongJump code becomes; *Rsp += DebugAgentContext.StackMigrateOffset; *Rbp += DebugAgentContext.StackMigrateOffset; return EFI_SUCCESS; The SaveAndSetDebugTimerInterrupt (OldStatus); call gets moved to the assembler code after the stack shift. The tricky bit would seem to be the storage used for *Rsp, Rbp, and *DebugTimerStauts. I guess worst case the C code could return DebugAgentContext.StackMigrateOffset and the assembly could “adjust”. Then the assembler code just hard codes a EFI_SUCCESS return; [1] https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseLib/X64/SwitchStack.nasm#L35 /** Transfers control to a function starting with a new stack. Transfers control to the function specified by EntryPoint using the new stack specified by NewStack and passing in the parameters specified by Context1 and Context2. Context1 and Context2 are optional and may be NULL. The function EntryPoint must never return. This function supports a variable number of arguments following the NewStack parameter. These additional arguments are ignored on IA-32, x64, and EBC. IPF CPUs expect one additional parameter of type VOID * that specifies the new backing store pointer. If EntryPoint is NULL, then ASSERT(). If NewStack is NULL, then ASSERT(). @param EntryPoint A pointer to function to call with the new stack. @param Context1A pointer to the context to pass into the EntryPoint function. @param Context2A pointer to the context to pass into the EntryPoint function. @param NewStackA pointer to the new stack to use for the EntryPoint function. @param ... This variable argument list is ignored for IA32, x64, and EBC. For IPF, this variable argument list is expected to contain a single parameter of type VOID * that specifies the new backing store pointer. **/ VOID EFIAPI SwitchStack ( IN SWITCH_STACK_ENTRY_POINT EntryPoint, IN VOID *Context1 OPTIONAL, IN VOID *Context2 OPTIONAL, IN VOID *NewStack, ... ) Thanks, Andrew Fish PS The Xcode clang always emits the frame pointer to enable stack walks. > On Jun 7, 2022, at 4:14 AM, Jiri Slaby wrote: > > On 07. 06. 22, 13:07, Gerd Hoffmann wrote: >> On Tue, Jun 07, 2022 at 12:38:46PM +0200, Jiri Slaby wrote: >>> Hi, >>> >>> On 07. 06. 22, 12:31, Gerd Hoffmann wrote: > The reason is TemporaryRamMigration() overwrites rbp unconditionally -- it > adds an offset to rbp even if rbp is NOT used as a frame pointer > Now, what is the right way to fix this? Do the SetJump/LongJump in > assembly > and wrap it into push rbp/pop rbp? push/pop rbp will break in case frame pointers are used, no? >>> >>> Yes, see the downstream bug at: >>> >>> https://bugzilla.suse.com/show_bug.cgi?id=1199597#c45 >>> >>> and read further. >>> I think essentially the code needs to know whenever frame pointers are used or not and then update (or not) rbp depending on that. Update compiler flags to explicitly set -f(no-)omit-frame-pointer, also add
Re: [edk2-devel] ovmf miscompiles with gcc-12
Hi, On 07. 06. 22, 12:31, Gerd Hoffmann wrote: The reason is TemporaryRamMigration() overwrites rbp unconditionally -- it adds an offset to rbp even if rbp is NOT used as a frame pointer Now, what is the right way to fix this? Do the SetJump/LongJump in assembly and wrap it into push rbp/pop rbp? push/pop rbp will break in case frame pointers are used, no? Yes, see the downstream bug at: https://bugzilla.suse.com/show_bug.cgi?id=1199597#c45 and read further. I think essentially the code needs to know whenever frame pointers are used or not and then update (or not) rbp depending on that. Update compiler flags to explicitly set -f(no-)omit-frame-pointer, also add -D OMIT_FRAME_POINTER=1, the compile conditionally on OMIT_FRAME_POINTER? Yes, the comment above mentions this too (cf. CONFIG_FRAME_POINTER in the kernel). So see the downstream bugzilla for discussion. The upstream bugzilla needs an account which I don't have and cannot create automatically. It needs manual intervention and I am too lazy to do so. So I didn't comment there: https://bugzilla.tianocore.org/show_bug.cgi?id=3934 thanks, -- js -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#90337): https://edk2.groups.io/g/devel/message/90337 Mute This Topic: https://groups.io/mt/91216215/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] ovmf miscompiles with gcc-12
On 07. 06. 22, 13:07, Gerd Hoffmann wrote:
On Tue, Jun 07, 2022 at 12:38:46PM +0200, Jiri Slaby wrote:
Hi,
On 07. 06. 22, 12:31, Gerd Hoffmann wrote:
The reason is TemporaryRamMigration() overwrites rbp unconditionally -- it
adds an offset to rbp even if rbp is NOT used as a frame pointer
Now, what is the right way to fix this? Do the SetJump/LongJump in assembly
and wrap it into push rbp/pop rbp?
push/pop rbp will break in case frame pointers are used, no?
Yes, see the downstream bug at:
https://bugzilla.suse.com/show_bug.cgi?id=1199597#c45
and read further.
I think essentially the code needs to know whenever frame pointers are
used or not and then update (or not) rbp depending on that. Update
compiler flags to explicitly set -f(no-)omit-frame-pointer, also add
-D OMIT_FRAME_POINTER=1, the compile conditionally on OMIT_FRAME_POINTER?
Yes, the comment above mentions this too (cf. CONFIG_FRAME_POINTER in the
kernel). So see the downstream bugzilla for discussion.
Ok. So what is the status here? Someone working on patches?
I don't know of anybody. I only tracked it down, reported and worked
around locally by:
--- a/OvmfPkg/Sec/SecMain.c
+++ b/OvmfPkg/Sec/SecMain.c
@@ -928,7 +928,7 @@ SecStartupPhase2 (
CpuDeadLoop ();
}
-EFI_STATUS
+EFI_STATUS __attribute__((optimize("-fno-omit-frame-pointer")))
EFIAPI
TemporaryRamMigration (
IN CONST EFI_PEI_SERVICES **PeiServices,
The upstream bugzilla needs an account which I don't have and cannot create
automatically. It needs manual intervention and I am too lazy to do so.
It's just an email though:
https://github.com/tianocore/tianocore.github.io/wiki/Reporting-Issues
As I wrote earlier, there is a bug created by the openSUSE ovmf
maintainer (Joey):
https://bugzilla.tianocore.org/show_bug.cgi?id=3934
If there is any input needed from me, I might reconsider... So far, it's
stuck.
regards,
--
js
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#90338): https://edk2.groups.io/g/devel/message/90338
Mute This Topic: https://groups.io/mt/91216215/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] ovmf miscompiles with gcc-12
On Tue, Jun 07, 2022 at 12:38:46PM +0200, Jiri Slaby wrote: > Hi, > > On 07. 06. 22, 12:31, Gerd Hoffmann wrote: > > > The reason is TemporaryRamMigration() overwrites rbp unconditionally -- it > > > adds an offset to rbp even if rbp is NOT used as a frame pointer > > > > > Now, what is the right way to fix this? Do the SetJump/LongJump in > > > assembly > > > and wrap it into push rbp/pop rbp? > > > > push/pop rbp will break in case frame pointers are used, no? > > Yes, see the downstream bug at: > > https://bugzilla.suse.com/show_bug.cgi?id=1199597#c45 > > and read further. > > > I think essentially the code needs to know whenever frame pointers are > > used or not and then update (or not) rbp depending on that. Update > > compiler flags to explicitly set -f(no-)omit-frame-pointer, also add > > -D OMIT_FRAME_POINTER=1, the compile conditionally on OMIT_FRAME_POINTER? > > Yes, the comment above mentions this too (cf. CONFIG_FRAME_POINTER in the > kernel). So see the downstream bugzilla for discussion. Ok. So what is the status here? Someone working on patches? > The upstream bugzilla needs an account which I don't have and cannot create > automatically. It needs manual intervention and I am too lazy to do so. It's just an email though: https://github.com/tianocore/tianocore.github.io/wiki/Reporting-Issues Used to be open in the past, as far I know it was closed to get the bugzilla spam bots under control, not to exclude people ... take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#90286): https://edk2.groups.io/g/devel/message/90286 Mute This Topic: https://groups.io/mt/91216215/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] ovmf miscompiles with gcc-12
Hi, > The reason is TemporaryRamMigration() overwrites rbp unconditionally -- it > adds an offset to rbp even if rbp is NOT used as a frame pointer > Now, what is the right way to fix this? Do the SetJump/LongJump in assembly > and wrap it into push rbp/pop rbp? push/pop rbp will break in case frame pointers are used, no? I think essentially the code needs to know whenever frame pointers are used or not and then update (or not) rbp depending on that. Update compiler flags to explicitly set -f(no-)omit-frame-pointer, also add -D OMIT_FRAME_POINTER=1, the compile conditionally on OMIT_FRAME_POINTER? take care, Gerd -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#90281): https://edk2.groups.io/g/devel/message/90281 Mute This Topic: https://groups.io/mt/91216215/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] ovmf miscompiles with gcc-12
Hi all,
I have filed this issue on tianocore bugzilla:
Bug 3934 - ovmf miscompiles with gcc-12
https://bugzilla.tianocore.org/show_bug.cgi?id=3934
Thanks
Joey Lee
On Thu, May 19, 2022 at 07:43:12AM +0200, Jiri Slaby via groups.io wrote:
> Hi,
>
> we discovered that qemu-ovmf-x86_64 doesn't start when compiled using
> gcc-12. Originally reported as:
> https://bugzilla.suse.com/show_bug.cgi?id=1199597
>
> I run qemu as:
> qemu-kvm -drive file=/dev/null,format=raw -drive
> if=pflash,format=raw,unit=0,readonly=on,file=OVMF.fd -m 3000
>
> The platform repeatedly resets after TemporaryRamMigration as can be seen in
> the debuglog:
> https://bugzilla.suse.com/attachment.cgi?id=858969
>
> The reason is TemporaryRamMigration() overwrites rbp unconditionally -- it
> adds an offset to rbp even if rbp is NOT used as a frame pointer
> (-fomit-frame-pointer was always used for compilation here). So commenting
> out:
> > //JumpBuffer.Rbp = JumpBuffer.Rbp + DebugAgentContext.StackMigrateOffset;
>
> makes it all work again. Also marking TemporaryRamMigration() as:
> __attribute__((optimize("-fno-omit-frame-pointer")))
> works around the problem too. (But that doesn't guarantee anything.)
>
> The code is:
> > if (SetJump () == 0) {
> > #if defined (MDE_CPU_IA32)
> > JumpBuffer.Esp = JumpBuffer.Esp + DebugAgentContext.StackMigrateOffset;
> > JumpBuffer.Ebp = JumpBuffer.Ebp + DebugAgentContext.StackMigrateOffset;
> > #endif
> > #if defined (MDE_CPU_X64)
> > JumpBuffer.Rsp = JumpBuffer.Rsp + DebugAgentContext.StackMigrateOffset;
> > JumpBuffer.Rbp = JumpBuffer.Rbp + DebugAgentContext.StackMigrateOffset;
> > #endif
> > LongJump (, (UINTN)-1);
> > }
>
> It was only coincidence this ever worked -- gcc-11 omits the frame pointer
> too, but apparently the caller (PeiCheckAndSwitchStack) does not use rbp.
>
> PeiCheckAndSwitchStack() (gcc-12):
>
> > 79a6: 4c 29 fdsub%r15,%rbp <-- used rbp
> > 79a9: 4d 29 fesub%r15,%r14
> > 79ac: 48 83 ec 20 sub$0x20,%rsp
> > 79b0: 4d 89 e0mov%r12,%r8
> > 79b3: 48 8d 4b 08 lea0x8(%rbx),%rcx
> > 79b7: 48 8b 44 24 50 mov0x50(%rsp),%rax
> > 79bc: 48 8b 54 24 20 mov0x20(%rsp),%rdx
> > 79c1: 4d 29 e8sub%r13,%r8
> > 79c4: 4c 8b 4c 24 30 mov0x30(%rsp),%r9
> > 79c9: ff 10 call *(%rax) <--- call
> > to TemporaryRamMigration
> > 79cb: 48 83 c4 20 add$0x20,%rsp
> > 79cf: be 01 00 00 00 mov$0x1,%esi
> > 79d4: 4c 89 f7mov%r14,%rdi
> > 79d7: e8 f4 a8 ff ff call 22d0
> > 79dc: 48 83 ec 20 sub$0x20,%rsp
> > 79e0: 4d 89 f0mov%r14,%r8
> > 79e3: 31 d2 xor%edx,%edx
> > 79e5: 48 89 e9mov%rbp,%rcx <-- rbp used
>
> gcc-11 seems to copy rbp to r8 first and operates on r8 there instead.
>
> Now, what is the right way to fix this? Do the SetJump/LongJump in assembly
> and wrap it into push rbp/pop rbp?
>
> thanks,
> --
> js
> suse labs
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#90078): https://edk2.groups.io/g/devel/message/90078
Mute This Topic: https://groups.io/mt/91216215/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
[edk2-devel] ovmf miscompiles with gcc-12
Hi,
we discovered that qemu-ovmf-x86_64 doesn't start when compiled using
gcc-12. Originally reported as:
https://bugzilla.suse.com/show_bug.cgi?id=1199597
I run qemu as:
qemu-kvm -drive file=/dev/null,format=raw -drive
if=pflash,format=raw,unit=0,readonly=on,file=OVMF.fd -m 3000
The platform repeatedly resets after TemporaryRamMigration as can be
seen in the debuglog:
https://bugzilla.suse.com/attachment.cgi?id=858969
The reason is TemporaryRamMigration() overwrites rbp unconditionally --
it adds an offset to rbp even if rbp is NOT used as a frame pointer
(-fomit-frame-pointer was always used for compilation here). So
commenting out:
//JumpBuffer.Rbp = JumpBuffer.Rbp + DebugAgentContext.StackMigrateOffset;
makes it all work again. Also marking TemporaryRamMigration() as:
__attribute__((optimize("-fno-omit-frame-pointer")))
works around the problem too. (But that doesn't guarantee anything.)
The code is:
if (SetJump () == 0) {
#if defined (MDE_CPU_IA32)
JumpBuffer.Esp = JumpBuffer.Esp + DebugAgentContext.StackMigrateOffset;
JumpBuffer.Ebp = JumpBuffer.Ebp + DebugAgentContext.StackMigrateOffset;
#endif
#if defined (MDE_CPU_X64)
JumpBuffer.Rsp = JumpBuffer.Rsp + DebugAgentContext.StackMigrateOffset;
JumpBuffer.Rbp = JumpBuffer.Rbp + DebugAgentContext.StackMigrateOffset;
#endif
LongJump (, (UINTN)-1);
}
It was only coincidence this ever worked -- gcc-11 omits the frame
pointer too, but apparently the caller (PeiCheckAndSwitchStack) does not
use rbp.
PeiCheckAndSwitchStack() (gcc-12):
79a6: 4c 29 fdsub%r15,%rbp <-- used rbp
79a9: 4d 29 fesub%r15,%r14
79ac: 48 83 ec 20 sub$0x20,%rsp
79b0: 4d 89 e0mov%r12,%r8
79b3: 48 8d 4b 08 lea0x8(%rbx),%rcx
79b7: 48 8b 44 24 50 mov0x50(%rsp),%rax
79bc: 48 8b 54 24 20 mov0x20(%rsp),%rdx
79c1: 4d 29 e8sub%r13,%r8
79c4: 4c 8b 4c 24 30 mov0x30(%rsp),%r9
79c9: ff 10 call *(%rax) <--- call to
TemporaryRamMigration
79cb: 48 83 c4 20 add$0x20,%rsp
79cf: be 01 00 00 00 mov$0x1,%esi
79d4: 4c 89 f7mov%r14,%rdi
79d7: e8 f4 a8 ff ff call 22d0
79dc: 48 83 ec 20 sub$0x20,%rsp
79e0: 4d 89 f0mov%r14,%r8
79e3: 31 d2 xor%edx,%edx
79e5: 48 89 e9mov%rbp,%rcx <-- rbp used
gcc-11 seems to copy rbp to r8 first and operates on r8 there instead.
Now, what is the right way to fix this? Do the SetJump/LongJump in
assembly and wrap it into push rbp/pop rbp?
thanks,
--
js
suse labs
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89906): https://edk2.groups.io/g/devel/message/89906
Mute This Topic: https://groups.io/mt/91216215/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
