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

["Andrew Fish" <afish@apple.com>]

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