Re: [PATCH v1 1/1] OvmfPkg/SecMain: Fix stack switching to permanent memory fail

[Jordan Justen <jordan.l.justen@intel.com>]

Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

But, I would like to get a Tested-by or Acked-by from Laszlo, since I
expect he'd want to test S3 resume with this change.

-Jordan

On 2017-09-02 22:12:14, Ge Song wrote:
> In earlier PEI stage, temporary memory(Cache As Ram) is employed as stack
> and heap. We move them to the new room and do some relocation fixup when
> permanent memory becomes available. TemporaryRamMigration()
> is responsible for switching the stack.
> 
> In the begining of the TemporaryRamMigration(),
> Ebp/Rbp is populated with content of Esp/Rsp and used as frame pointer,
> after the execution of SetJump/LongJump, stack migrates to new position
> while the context keeps unchanged. But when TemporaryRamMigration() exits,
> Esp/Rsp is filled with the content of Ebp/Rbp to destroy this stack frame.
> The result is, stack switches back to previous temporary momery.
> 
> More detailed information:
> > TemporaryRamMigration (PeiServices=0x817790,
> > TemporaryMemoryBase=8454144, PermanentMemoryBase=469016576,
> > CopySize=32768)
> >     at /home/bird/src/edk2/OvmfPkg/Sec/SecMain.c:938
> > 938       LongJump (&JumpBuffer, (UINTN)-1);
> > (gdb) info registers
> > rax            0x1bf4d3b0     469029808
> > rbx            0x810248       8454728
> > rcx            0x8173e8       8483816
> > rdx            0x8173b0       8483760
> > rsi            0x1bf4a000     469016576
> > rdi            0x8000 32768
> > rbp            0x817520       0x817520
> > rsp            0x8173b0       0x8173b0
> > r8             0x0    0
> > ...
> > rip            0xfffcd409     0xfffcd409 <TemporaryRamMigration+365>
> > eflags         0x2    [ ]
> > cs             0x18   24
> > ss             0x8    8
> > ...
> 
> After execution of LongJump:
> > 943     return EFI_SUCCESS;
> > (gdb) info registers
> > rax            0x0    0
> > rbx            0x810248       8454728
> > rcx            0x0    0
> > rdx            0xffffffffffffffff     -1
> > rsi            0x1bf4a000     469016576
> > rdi            0x8000 32768
> > rbp            0x817520       0x817520
> > rsp            0x1bf4d3b0     0x1bf4d3b0
> > r8             0x0    0
> > ...
> > rip            0xfffcd42a     0xfffcd42a <TemporaryRamMigration+398>
> > eflags         0x86   [ PF SF ]
> > cs             0x18   24
> > ss             0x8    8
> > ...
> 
> We can find rsp has changed to new permanent memory
> 
> When leaving TemporaryRamMigration(), the stack swithes back to previous
> temporary memory:
> > (gdb) finish
> > Run till exit from #0  TemporaryRamMigration (PeiServices=0x817790,
> > TemporaryMemoryBase=8454144, PermanentMemoryBase=469016576,
> > CopySize=32768)
> >     at /home/bird/src/edk2/OvmfPkg/Sec/SecMain.c:943
> > PeiCheckAndSwitchStack (SecCoreData=0x1bf4df78, Private=0x1bf4d788) at
> > /home/bird/src/edk2/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c:806
> > 806         PeiCore (SecCoreData, NULL, Private);
> > Value returned is $1 = 0
> > (gdb) info registers
> > rax            0x0    0
> > rbx            0x810248       8454728
> > rcx            0x0    0
> > rdx            0xffffffffffffffff     -1
> > rsi            0x1bf4a000     469016576
> > rdi            0x8000 32768
> > rbp            0x817630       0x817630
> > rsp            0x817530       0x817530
> > r8             0x0    0
> > ...
> > rip            0x828135       0x828135 <PeiCheckAndSwitchStack+1573>
> > eflags         0x86   [ PF SF ]
> > cs             0x18   24
> > ss             0x8    8
> > ...
> 
> > (gdb) disassemble /r
> > Dump of assembler code for function TemporaryRamMigration:
> >   0x00000000fffcd29c <+0>:    55      push   %rbp
> >   0x00000000fffcd29d <+1>:    48 89 e5        mov    %rsp,%rbp
> >   0x00000000fffcd2a0 <+4>:    48 81 ec 70 01 00 00    sub
> > $0x170,%rsp
> >    ...
> >    ...
> >    0x00000000fffcd425 <+393>: e8 80 10 00 00  callq  0xfffce4aa
> > <SaveAndSetDebugTimerInterrupt>
> > => 0x00000000fffcd42a <+398>: b8 00 00 00 00  mov    $0x0,%eax
> >    0x00000000fffcd42f <+403>: c9      leaveq
> >    0x00000000fffcd430 <+404>: c3      retq
> > End of assembler dump.
> 
> See the description of leave(opcode: c9), from
> Intel® 64 and IA-32 Architectures Software Developer’s Manual, Volume 2A
> 
> "Releases the stack frame set up by an earlier ENTER instruction. The
> LEAVE instruction copies the frame pointer (in the EBP register) into
> the stack pointer register (ESP), which releases the stack space
> allocated to the stack frame. The old frame pointer (the frame pointer
> for the calling procedure that was saved by the ENTER instruction) is
> then popped from the stack into the EBP register, restoring the calling
> procedure’s stack frame."
> 
> To solve this, update Ebp/Rbp too when Esp/Rsp is updated
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ge Song <ge.song@hxt-semitech.com>
> ---
>  OvmfPkg/Sec/SecMain.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> index e1993ec347b5..f7fec3d8c03b 100644
> --- a/OvmfPkg/Sec/SecMain.c
> +++ b/OvmfPkg/Sec/SecMain.c
> @@ -931,9 +931,11 @@ TemporaryRamMigration (
>    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);
>    }
> -- 
> 2.11.0
>