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 <jirisl...@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
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#90343): https://edk2.groups.io/g/devel/message/90343
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]
-=-=-=-=-=-=-=-=-=-=-=-
