ovmf miscompiles with gcc-12

ovmf miscompiles with gcc-12

[Jiri Slaby]

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 (&JumpBuffer) == 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 (&JumpBuffer, (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 fd                sub    %r15,%rbp <------ used rbp
>     79a9:       4d 29 fe                sub    %r15,%r14
>     79ac:       48 83 ec 20             sub    $0x20,%rsp
>     79b0:       4d 89 e0                mov    %r12,%r8
>     79b3:       48 8d 4b 08             lea    0x8(%rbx),%rcx
>     79b7:       48 8b 44 24 50          mov    0x50(%rsp),%rax
>     79bc:       48 8b 54 24 20          mov    0x20(%rsp),%rdx
>     79c1:       4d 29 e8                sub    %r13,%r8
>     79c4:       4c 8b 4c 24 30          mov    0x30(%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 f7                mov    %r14,%rdi
>     79d7:       e8 f4 a8 ff ff          call   22d0 <MigrateMemoryPages>
>     79dc:       48 83 ec 20             sub    $0x20,%rsp
>     79e0:       4d 89 f0                mov    %r14,%r8
>     79e3:       31 d2                   xor    %edx,%edx
>     79e5:       48 89 e9                mov    %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

Re: [edk2-devel] ovmf miscompiles with gcc-12

[joeyli]

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 (&JumpBuffer) == 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 (&JumpBuffer, (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 fd                sub    %r15,%rbp <------ used rbp
> >     79a9:       4d 29 fe                sub    %r15,%r14
> >     79ac:       48 83 ec 20             sub    $0x20,%rsp
> >     79b0:       4d 89 e0                mov    %r12,%r8
> >     79b3:       48 8d 4b 08             lea    0x8(%rbx),%rcx
> >     79b7:       48 8b 44 24 50          mov    0x50(%rsp),%rax
> >     79bc:       48 8b 54 24 20          mov    0x20(%rsp),%rdx
> >     79c1:       4d 29 e8                sub    %r13,%r8
> >     79c4:       4c 8b 4c 24 30          mov    0x30(%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 f7                mov    %r14,%rdi
> >     79d7:       e8 f4 a8 ff ff          call   22d0 <MigrateMemoryPages>
> >     79dc:       48 83 ec 20             sub    $0x20,%rsp
> >     79e0:       4d 89 f0                mov    %r14,%r8
> >     79e3:       31 d2                   xor    %edx,%edx
> >     79e5:       48 89 e9                mov    %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
> 
> 
> 
> 

Re: [edk2-devel] ovmf miscompiles with gcc-12

[Gerd Hoffmann]

  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

Re: [edk2-devel] ovmf miscompiles with gcc-12

[Jiri Slaby]

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

Re: [edk2-devel] ovmf miscompiles with gcc-12

[Gerd Hoffmann]

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

Re: [edk2-devel] ovmf miscompiles with gcc-12

[Jiri Slaby]

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

Re: [edk2-devel] ovmf miscompiles with gcc-12

[Andrew Fish]

[-- Attachment #1: Type: text/plain, Size: 6460 bytes --]

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_ADDRESS    TemporaryMemoryBase,
  IN EFI_PHYSICAL_ADDRESS    PermanentMemoryBase,
  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  Context1    A pointer to the context to pass into the EntryPoint
                      function.
  @param  Context2    A pointer to the context to pass into the EntryPoint
                      function.
  @param  NewStack    A 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 <jirislaby@kernel.org> 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
>>>> -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 <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 <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
> 
> 
> 


[-- Attachment #2: Type: text/html, Size: 27099 bytes --]