EmulatorPkg Unix Host Segmentation fault.


Liu Yu <pedroa.liu@...>
 

OS: Ubuntu

Toolchain:GCC48

Issue Description :

Program received signal SIGSEGV, Segmentation fault.
at /home/pedroa/workspace/orign/edkcrb/MdeModulePkg/Core/Pei/Memory/MemoryServices.c:129
129 Private->MemoryPages.Size = (UINTN) (Private->HobList.HandoffInformationTable->EfiMemoryTop -


if the GCC optimization option is used not -O0 so the "rbp" register will be used as "general register"

in the SecTemporaryRamSupport function as below, this function will modify the rbp (as general register not stack base address pointer)value that result in program crash.

ASM_PFX(SecTemporaryRamSupport):
// Adjust callers %rbp to account for stack move
subq %rdx, %rbp // Calc offset of %rbp in Temp Memory
addq %r8, %rbp // add in permanent base to offset


Jordan Justen
 

On 2018-11-17 20:51:11, Liu Yu wrote:
OS: Ubuntu

Toolchain:GCC48
I don't have gcc-4.8, so I couldn't reproduce the issue, but I wonder
if this branch can fix the issue for you?

https://github.com/jljusten/edk2/tree/emulator-temp-ram

You can fetch this branch locally to a branch named `test` with a
command like this:

$ git fetch --no-tags https://github.com/jljusten/edk2.git emulator-temp-ram:test

Then checkout the `test` branch to try it.

First, there is some patches to cleanup Sec, but then I added a patch:

53a432e149 "EmulatorPkg/Sec: Disable optimizations for TemporaryRamMigration function"

Which I hope might help in your case.

-Jordan


Issue Description :

Program received signal SIGSEGV, Segmentation fault.
at /home/pedroa/workspace/orign/edkcrb/MdeModulePkg/Core/Pei/Memory/MemoryServices.c:129
129 Private->MemoryPages.Size = (UINTN) (Private->HobList.HandoffInformationTable->EfiMemoryTop -


if the GCC optimization option is used not -O0 so the "rbp" register will be used as "general register"

in the SecTemporaryRamSupport function as below, this function will modify the rbp (as general register not stack base address pointer)value that result in program crash.

ASM_PFX(SecTemporaryRamSupport):
// Adjust callers %rbp to account for stack move
subq %rdx, %rbp // Calc offset of %rbp in Temp Memory
addq %r8, %rbp // add in permanent base to offset

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Liu Yu <pedroa.liu@...>
 

sorry your  path can't fix this issue.   if this path just turn off
optimization option within sec.c not global project.

I have tested different version GCC such as (GCC4,8, GCC5.x, GCC7.x) 
and all of them can duplicate this issue  (Ubuntu 16.04, 16.10,18.04 )

I have traced this issue on my hand.

you can see Dispatcher.c (MdeModulePkg/Pei/DIspatcher/) Line 792:


790      if (StackOffsetPositive) {
791       SecCoreData = (CONST EFI_SEC_PEI_HAND_OFF *)((UINTN)(VOID
*)SecCoreData + StackOffset);
792      Private = (PEI_CORE_INSTANCE *)((UINTN)(VOID *)Private +
StackOffset);
793     } else {
794      ..........
795      ..........
796    }

 790 --792 disassembly code

 0x10200f2ca <PeiCheckAndSwitchStack+1030>:    test %r14b,%r14b
 0x10200f2cd <PeiCheckAndSwitchStack+1033>:    je 0x10200f2df
<PeiCheckAndSwitchStack+1051>
 0x10200f2cf <PeiCheckAndSwitchStack+1035>:    mov 0x38(%rsp),%rax
 0x10200f2d4 <PeiCheckAndSwitchStack+1040>:    lea 0x0(%rbp,%rax,1),%r14
 0x10200f2d9 <PeiCheckAndSwitchStack+1045>:    lea (%rbx,%rax,1),%rbp

 we can see Private value have been stored in %rbp  (rbp register be
used as general register )   so when call
TemporaryRamSupportPpi->TemporaryRamMigration()

this function would modify rbp value because it treat rbp as "stack base
address ".

816     MigrateMemoryPages (Private, TRUE);

// Private pointer point to other address, so this function would get a
NULL pointer that result in segment fault

I think we can turn off optimization options like this.

1. modify  EmulatorPkg.dsc

      MdeModulePkg/Core/Pei/PeiMain.inf {
         <BuildOptions>
          GCC:*_*_*_CC_FLAGS = -O0
  }

Reference GCC Manual description:

  -O also turns on -fomit-frame-pointer on machines where doing so does
not interfere with debugging.



在 2018/11/18 下午5:27, Jordan Justen 写道:

On 2018-11-17 20:51:11, Liu Yu wrote:
OS: Ubuntu

Toolchain:GCC48
I don't have gcc-4.8, so I couldn't reproduce the issue, but I wonder
if this branch can fix the issue for you?

https://github.com/jljusten/edk2/tree/emulator-temp-ram

You can fetch this branch locally to a branch named `test` with a
command like this:

$ git fetch --no-tags https://github.com/jljusten/edk2.git emulator-temp-ram:test

Then checkout the `test` branch to try it.

First, there is some patches to cleanup Sec, but then I added a patch:

53a432e149 "EmulatorPkg/Sec: Disable optimizations for TemporaryRamMigration function"

Which I hope might help in your case.

-Jordan

Issue Description :

Program received signal SIGSEGV, Segmentation fault.
at /home/pedroa/workspace/orign/edkcrb/MdeModulePkg/Core/Pei/Memory/MemoryServices.c:129
129 Private->MemoryPages.Size = (UINTN) (Private->HobList.HandoffInformationTable->EfiMemoryTop -


if the GCC optimization option is used not -O0 so the "rbp" register will be used as "general register"

in the SecTemporaryRamSupport function as below, this function will modify the rbp (as general register not stack base address pointer)value that result in program crash.

ASM_PFX(SecTemporaryRamSupport):
// Adjust callers %rbp to account for stack move
subq %rdx, %rbp // Calc offset of %rbp in Temp Memory
addq %r8, %rbp // add in permanent base to offset

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Andrew Fish
 

On Nov 18, 2018, at 4:07 AM, Liu Yu <pedroa.liu@outlook.com> wrote:

sorry your path can't fix this issue. if this path just turn off
optimization option within sec.c not global project.

I have tested different version GCC such as (GCC4,8, GCC5.x, GCC7.x)
and all of them can duplicate this issue (Ubuntu 16.04, 16.10,18.04 )

I have traced this issue on my hand.

you can see Dispatcher.c (MdeModulePkg/Pei/DIspatcher/) Line 792:


790 if (StackOffsetPositive) {
791 SecCoreData = (CONST EFI_SEC_PEI_HAND_OFF *)((UINTN)(VOID
*)SecCoreData + StackOffset);
792 Private = (PEI_CORE_INSTANCE *)((UINTN)(VOID *)Private +
StackOffset);
793 } else {
794 ..........
795 ..........
796 }

790 --792 disassembly code

0x10200f2ca <PeiCheckAndSwitchStack+1030>: test %r14b,%r14b
0x10200f2cd <PeiCheckAndSwitchStack+1033>: je 0x10200f2df
<PeiCheckAndSwitchStack+1051>
0x10200f2cf <PeiCheckAndSwitchStack+1035>: mov 0x38(%rsp),%rax
0x10200f2d4 <PeiCheckAndSwitchStack+1040>: lea 0x0(%rbp,%rax,1),%r14
0x10200f2d9 <PeiCheckAndSwitchStack+1045>: lea (%rbx,%rax,1),%rbp

we can see Private value have been stored in %rbp (rbp register be
used as general register ) so when call
TemporaryRamSupportPpi->TemporaryRamMigration()
The calling conventions define RBP as non-volatile must be preserved by callee. Using RBP as the frame pointer is optional.

Is it possible the assembly coder is assuming RBP is a frame pointer? That would imply for gcc/clang the correct answer would be to have compiler flags force frame pointer usage?

Assuming -O 0 does something seems like we are matching an implementation at a given point in time. I'd rather force the frame pointer usage (that is optional in the ABI) if that fixes the RBP usage assumption. I guess the other option would be to have different assembler if the compiler is using frame pointers or not. and I don't think we have that concept.

Given this is the common frame pointer pattern:

pushq %rbp
movq %rsp, %rbp
...
popq %rbp
retq

It follows the calling convention rules even if the frame pointer is not in general use. Thus it only seems like you would hit issues when you move the stack around.

Thanks,

Andrew Fish

PS Xcode clang always emits the frame pointer.

this function would modify rbp value because it treat rbp as "stack base
address ".

816 MigrateMemoryPages (Private, TRUE);

// Private pointer point to other address, so this function would get a
NULL pointer that result in segment fault

I think we can turn off optimization options like this.

1. modify EmulatorPkg.dsc

MdeModulePkg/Core/Pei/PeiMain.inf {
<BuildOptions>
GCC:*_*_*_CC_FLAGS = -O0
}

Reference GCC Manual description:

-O also turns on -fomit-frame-pointer on machines where doing so does
not interfere with debugging.



在 2018/11/18 下午5:27, Jordan Justen 写道:
On 2018-11-17 20:51:11, Liu Yu wrote:
OS: Ubuntu

Toolchain:GCC48
I don't have gcc-4.8, so I couldn't reproduce the issue, but I wonder
if this branch can fix the issue for you?

https://github.com/jljusten/edk2/tree/emulator-temp-ram

You can fetch this branch locally to a branch named `test` with a
command like this:

$ git fetch --no-tags https://github.com/jljusten/edk2.git emulator-temp-ram:test

Then checkout the `test` branch to try it.

First, there is some patches to cleanup Sec, but then I added a patch:

53a432e149 "EmulatorPkg/Sec: Disable optimizations for TemporaryRamMigration function"

Which I hope might help in your case.

-Jordan

Issue Description :

Program received signal SIGSEGV, Segmentation fault.
at /home/pedroa/workspace/orign/edkcrb/MdeModulePkg/Core/Pei/Memory/MemoryServices.c:129
129 Private->MemoryPages.Size = (UINTN) (Private->HobList.HandoffInformationTable->EfiMemoryTop -


if the GCC optimization option is used not -O0 so the "rbp" register will be used as "general register"

in the SecTemporaryRamSupport function as below, this function will modify the rbp (as general register not stack base address pointer)value that result in program crash.

ASM_PFX(SecTemporaryRamSupport):
// Adjust callers %rbp to account for stack move
subq %rdx, %rbp // Calc offset of %rbp in Temp Memory
addq %r8, %rbp // add in permanent base to offset

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Jordan Justen
 

On 2018-11-18 14:37:09, Andrew Fish wrote:


On Nov 18, 2018, at 4:07 AM, Liu Yu <pedroa.liu@outlook.com> wrote:

sorry your path can't fix this issue. if this path just turn off
optimization option within sec.c not global project.

I have tested different version GCC such as (GCC4,8, GCC5.x, GCC7.x)
and all of them can duplicate this issue (Ubuntu 16.04, 16.10,18.04 )

I have traced this issue on my hand.

you can see Dispatcher.c (MdeModulePkg/Pei/DIspatcher/) Line 792:


790 if (StackOffsetPositive) {
791 SecCoreData = (CONST EFI_SEC_PEI_HAND_OFF *)((UINTN)(VOID
*)SecCoreData + StackOffset);
792 Private = (PEI_CORE_INSTANCE *)((UINTN)(VOID *)Private +
StackOffset);
793 } else {
794 ..........
795 ..........
796 }

790 --792 disassembly code

0x10200f2ca <PeiCheckAndSwitchStack+1030>: test %r14b,%r14b
0x10200f2cd <PeiCheckAndSwitchStack+1033>: je 0x10200f2df
<PeiCheckAndSwitchStack+1051>
0x10200f2cf <PeiCheckAndSwitchStack+1035>: mov 0x38(%rsp),%rax
0x10200f2d4 <PeiCheckAndSwitchStack+1040>: lea 0x0(%rbp,%rax,1),%r14
0x10200f2d9 <PeiCheckAndSwitchStack+1045>: lea (%rbx,%rax,1),%rbp

we can see Private value have been stored in %rbp (rbp register be
used as general register ) so when call
TemporaryRamSupportPpi->TemporaryRamMigration()
The calling conventions define RBP as non-volatile must be preserved
by callee. Using RBP as the frame pointer is optional.
This is kind of a mess. I think the definition of
EFI_PEI_TEMPORARY_RAM_SUPPORT_PPI is really to blame. It probably
should not have been spec'd as 'change stack and return'. Instead, it
should have been given a new function pointer to switch-stack and call
into.

By returning with a switched stack, we don't really know what the
returning code could do with regard to the stack. For example, it
could have saved the stack value on the stack and then pop it into a
register and somehow switch the stack back to the old stack. It seems
unlikely, but I don't think anything prevents it.

Additionally, there is the issue of rbp/ebp being used as a frame
pointer. This can lead to some variables being used from the temp ram
location after we return from TemporaryRamMigration. (Assuming rbp/ebp
is not adjusted to point to the new stack.)

So, is it safe to adjust rbp? Unknown. It may not be if rbp is not
used as a frame pointer. Is it safe to *not* adjust rbp and
potentially allow the old temp ram stack to be used? Unknown.

Now, if TemporaryRamMigration received a new function to call into, we
can safely assume that the new stack transition would proceed as
expected without having to worry if a compiler is using rbp/ebp as a
framepointer or not.

Another advantage could have been that something like the BasePkg
SwitchStack function could have been used, hopefully preventing people
from trying to write error prone assembly code for
TemporaryRamMigration.

-Jordan

Is it possible the assembly coder is assuming RBP is a frame
pointer? That would imply for gcc/clang the correct answer would be
to have compiler flags force frame pointer usage?

Assuming -O 0 does something seems like we are matching an
implementation at a given point in time. I'd rather force the frame
pointer usage (that is optional in the ABI) if that fixes the RBP
usage assumption. I guess the other option would be to have
different assembler if the compiler is using frame pointers or not.
and I don't think we have that concept.

Given this is the common frame pointer pattern:

pushq %rbp
movq %rsp, %rbp
...
popq %rbp
retq

It follows the calling convention rules even if the frame pointer is
not in general use. Thus it only seems like you would hit issues
when you move the stack around.

Thanks,

Andrew Fish

PS Xcode clang always emits the frame pointer.

this function would modify rbp value because it treat rbp as "stack base
address ".

816 MigrateMemoryPages (Private, TRUE);

// Private pointer point to other address, so this function would get a
NULL pointer that result in segment fault

I think we can turn off optimization options like this.

1. modify EmulatorPkg.dsc

MdeModulePkg/Core/Pei/PeiMain.inf {
<BuildOptions>
GCC:*_*_*_CC_FLAGS = -O0
}

Reference GCC Manual description:

-O also turns on -fomit-frame-pointer on machines where doing so does
not interfere with debugging.



在 2018/11/18 下午5:27, Jordan Justen 写道:
On 2018-11-17 20:51:11, Liu Yu wrote:
OS: Ubuntu

Toolchain:GCC48
I don't have gcc-4.8, so I couldn't reproduce the issue, but I wonder
if this branch can fix the issue for you?

https://github.com/jljusten/edk2/tree/emulator-temp-ram

You can fetch this branch locally to a branch named `test` with a
command like this:

$ git fetch --no-tags https://github.com/jljusten/edk2.git emulator-temp-ram:test

Then checkout the `test` branch to try it.

First, there is some patches to cleanup Sec, but then I added a patch:

53a432e149 "EmulatorPkg/Sec: Disable optimizations for TemporaryRamMigration function"

Which I hope might help in your case.

-Jordan

Issue Description :

Program received signal SIGSEGV, Segmentation fault.
at /home/pedroa/workspace/orign/edkcrb/MdeModulePkg/Core/Pei/Memory/MemoryServices.c:129
129 Private->MemoryPages.Size = (UINTN) (Private->HobList.HandoffInformationTable->EfiMemoryTop -


if the GCC optimization option is used not -O0 so the "rbp" register will be used as "general register"

in the SecTemporaryRamSupport function as below, this function will modify the rbp (as general register not stack base address pointer)value that result in program crash.

ASM_PFX(SecTemporaryRamSupport):
// Adjust callers %rbp to account for stack move
subq %rdx, %rbp // Calc offset of %rbp in Temp Memory
addq %r8, %rbp // add in permanent base to offset

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


Jordan Justen
 

On 2018-11-18 17:13:21, Jordan Justen wrote:
On 2018-11-18 14:37:09, Andrew Fish wrote:


On Nov 18, 2018, at 4:07 AM, Liu Yu <pedroa.liu@outlook.com> wrote:

sorry your path can't fix this issue. if this path just turn off
optimization option within sec.c not global project.

I have tested different version GCC such as (GCC4,8, GCC5.x, GCC7.x)
and all of them can duplicate this issue (Ubuntu 16.04, 16.10,18.04 )

I have traced this issue on my hand.

you can see Dispatcher.c (MdeModulePkg/Pei/DIspatcher/) Line 792:


790 if (StackOffsetPositive) {
791 SecCoreData = (CONST EFI_SEC_PEI_HAND_OFF *)((UINTN)(VOID
*)SecCoreData + StackOffset);
792 Private = (PEI_CORE_INSTANCE *)((UINTN)(VOID *)Private +
StackOffset);
793 } else {
794 ..........
795 ..........
796 }

790 --792 disassembly code

0x10200f2ca <PeiCheckAndSwitchStack+1030>: test %r14b,%r14b
0x10200f2cd <PeiCheckAndSwitchStack+1033>: je 0x10200f2df
<PeiCheckAndSwitchStack+1051>
0x10200f2cf <PeiCheckAndSwitchStack+1035>: mov 0x38(%rsp),%rax
0x10200f2d4 <PeiCheckAndSwitchStack+1040>: lea 0x0(%rbp,%rax,1),%r14
0x10200f2d9 <PeiCheckAndSwitchStack+1045>: lea (%rbx,%rax,1),%rbp

we can see Private value have been stored in %rbp (rbp register be
used as general register ) so when call
TemporaryRamSupportPpi->TemporaryRamMigration()
The calling conventions define RBP as non-volatile must be preserved
by callee. Using RBP as the frame pointer is optional.
This is kind of a mess. I think the definition of
EFI_PEI_TEMPORARY_RAM_SUPPORT_PPI is really to blame. It probably
should not have been spec'd as 'change stack and return'. Instead, it
should have been given a new function pointer to switch-stack and call
into.
Andrew, Mike,

I developed a PEI Core fix for this issue, which Liu Yu verified.

Unfortunately, it involves add assembly code in a key area of the PEI
Core. See the top 2 commits of:

https://github.com/jljusten/edk2/commits/emulator-temp-ram

I only wrote assembly for X64 nasm. But, I notice that neither the PEI
or DXE Core modules include any assembly code. So, I want to make sure
I'm heading in the right direction before working on it further.

As I mentioned below, I think PIWG could consider an new
TemporarayRemSupport interface that might work better, but that also
may not be worth the effort.

Can something like this change be integrated into the PEI Core?

Thanks for your time,

-Jordan

By returning with a switched stack, we don't really know what the
returning code could do with regard to the stack. For example, it
could have saved the stack value on the stack and then pop it into a
register and somehow switch the stack back to the old stack. It seems
unlikely, but I don't think anything prevents it.

Additionally, there is the issue of rbp/ebp being used as a frame
pointer. This can lead to some variables being used from the temp ram
location after we return from TemporaryRamMigration. (Assuming rbp/ebp
is not adjusted to point to the new stack.)

So, is it safe to adjust rbp? Unknown. It may not be if rbp is not
used as a frame pointer. Is it safe to *not* adjust rbp and
potentially allow the old temp ram stack to be used? Unknown.

Now, if TemporaryRamMigration received a new function to call into, we
can safely assume that the new stack transition would proceed as
expected without having to worry if a compiler is using rbp/ebp as a
framepointer or not.

Another advantage could have been that something like the BasePkg
SwitchStack function could have been used, hopefully preventing people
from trying to write error prone assembly code for
TemporaryRamMigration.

-Jordan


Andrew Fish
 

On Nov 19, 2018, at 11:16 AM, Jordan Justen <jordan.l.justen@intel.com> wrote:

On 2018-11-18 17:13:21, Jordan Justen wrote:
On 2018-11-18 14:37:09, Andrew Fish wrote:


On Nov 18, 2018, at 4:07 AM, Liu Yu <pedroa.liu@outlook.com> wrote:

sorry your path can't fix this issue. if this path just turn off
optimization option within sec.c not global project.

I have tested different version GCC such as (GCC4,8, GCC5.x, GCC7.x)
and all of them can duplicate this issue (Ubuntu 16.04, 16.10,18.04 )

I have traced this issue on my hand.

you can see Dispatcher.c (MdeModulePkg/Pei/DIspatcher/) Line 792:


790 if (StackOffsetPositive) {
791 SecCoreData = (CONST EFI_SEC_PEI_HAND_OFF *)((UINTN)(VOID
*)SecCoreData + StackOffset);
792 Private = (PEI_CORE_INSTANCE *)((UINTN)(VOID *)Private +
StackOffset);
793 } else {
794 ..........
795 ..........
796 }

790 --792 disassembly code

0x10200f2ca <PeiCheckAndSwitchStack+1030>: test %r14b,%r14b
0x10200f2cd <PeiCheckAndSwitchStack+1033>: je 0x10200f2df
<PeiCheckAndSwitchStack+1051>
0x10200f2cf <PeiCheckAndSwitchStack+1035>: mov 0x38(%rsp),%rax
0x10200f2d4 <PeiCheckAndSwitchStack+1040>: lea 0x0(%rbp,%rax,1),%r14
0x10200f2d9 <PeiCheckAndSwitchStack+1045>: lea (%rbx,%rax,1),%rbp

we can see Private value have been stored in %rbp (rbp register be
used as general register ) so when call
TemporaryRamSupportPpi->TemporaryRamMigration()
The calling conventions define RBP as non-volatile must be preserved
by callee. Using RBP as the frame pointer is optional.
This is kind of a mess. I think the definition of
EFI_PEI_TEMPORARY_RAM_SUPPORT_PPI is really to blame. It probably
should not have been spec'd as 'change stack and return'. Instead, it
should have been given a new function pointer to switch-stack and call
into.
Andrew, Mike,

I developed a PEI Core fix for this issue, which Liu Yu verified.

Unfortunately, it involves add assembly code in a key area of the PEI
Core. See the top 2 commits of:

https://github.com/jljusten/edk2/commits/emulator-temp-ram <https://github.com/jljusten/edk2/commits/emulator-temp-ram>

I only wrote assembly for X64 nasm. But, I notice that neither the PEI
or DXE Core modules include any assembly code. So, I want to make sure
I'm heading in the right direction before working on it further.
Mike,

I seem to remember we hit an issue like this a long time ago? Do you remember the details? Maybe it was we needed to write the TempRamSupport code in assembly?

The issue we are hitting is the gEfiTemporaryRamSupportPpiGuid TemporaryRamMigration() function call does a stack switch, but it returns. This causes an issue with the C calling conventions as RBP is optionally a frame pointer. To quote the MSFT spec: "May be used as a frame pointer; must be preserved by callee"
1) It is used as a frame pointer. It looks like our existing code fixes up the frame pointer to match the new location the stack got moved to.
2) Used as a general purpose register, and the value must be preserved.

As I mentioned below, I think PIWG could consider an new
TemporarayRemSupport interface that might work better, but that also
may not be worth the effort.
If the current API is not really portable I don;t think it is a bad idea to update it.


Can something like this change be integrated into the PEI Core?
Jordan,

I'm not sure? For the RBP == frame pointer case the frame pointer is no longer valid (it is in temp RAM, not DRAM). It seems like the point of SecTemporaryRamSupport() fixing up the callers RBP is for the benefit of the caller. It looks to me like your fix is just negating that fixup. So that would imply that either we could just fix this in SecTemporaryRamSupport() or we have 2 code gen paths and we need to know how the compiler is using RBP to "do the right thing"

Do we have other code that supports X64 PEI? Is see OvmfPkg....
https://github.com/tianocore/edk2/blob/master/OvmfPkg/Sec/SecMain.c#L875

Looks like OvmfPkg uses SetJump()/LongJump() to change the stack.

//
// Use SetJump()/LongJump() to switch to a new stack.
//
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);
}

SaveAndSetDebugTimerInterrupt (OldStatus);

return EFI_SUCCESS;
}

But given the above code is C code RBP is going to be restored on return. This would seem to imply that the adjusting of the callers RBP is not required? So maybe try changing over Emulator to the OvmfPkg TemporaryRamMigration() algorithm?

Thanks,

Andrew Fish


Thanks for your time,

-Jordan

By returning with a switched stack, we don't really know what the
returning code could do with regard to the stack. For example, it
could have saved the stack value on the stack and then pop it into a
register and somehow switch the stack back to the old stack. It seems
unlikely, but I don't think anything prevents it.

Additionally, there is the issue of rbp/ebp being used as a frame
pointer. This can lead to some variables being used from the temp ram
location after we return from TemporaryRamMigration. (Assuming rbp/ebp
is not adjusted to point to the new stack.)

So, is it safe to adjust rbp? Unknown. It may not be if rbp is not
used as a frame pointer. Is it safe to *not* adjust rbp and
potentially allow the old temp ram stack to be used? Unknown.

Now, if TemporaryRamMigration received a new function to call into, we
can safely assume that the new stack transition would proceed as
expected without having to worry if a compiler is using rbp/ebp as a
framepointer or not.

Another advantage could have been that something like the BasePkg
SwitchStack function could have been used, hopefully preventing people
from trying to write error prone assembly code for
TemporaryRamMigration.

-Jordan


Laszlo Ersek
 

Jordan wrote:

So, is it safe to adjust rbp? Unknown. It may not be if rbp is not
used as a frame pointer. Is it safe to *not* adjust rbp and
potentially allow the old temp ram stack to be used? Unknown.
Andrew wrote:

Looks like OvmfPkg uses SetJump()/LongJump() to change the stack.

//
// Use SetJump()/LongJump() to switch to a new stack.
//
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);
}

SaveAndSetDebugTimerInterrupt (OldStatus);

return EFI_SUCCESS;
}

But given the above code is C code RBP is going to be restored on return. This would seem to imply that the adjusting of the callers RBP is not required? [...]
The Ebp/Rbp assignments were added in a separate bugfix, namely

https://github.com/tianocore/edk2/commit/89796c69d9fd

Laszlo


Jordan Justen
 

On 2018-11-19 13:22:27, Andrew Fish wrote:


On Nov 19, 2018, at 11:16 AM, Jordan Justen <jordan.l.justen@intel.com> wrote:

On 2018-11-18 17:13:21, Jordan Justen wrote:
On 2018-11-18 14:37:09, Andrew Fish wrote:


On Nov 18, 2018, at 4:07 AM, Liu Yu <pedroa.liu@outlook.com> wrote:

sorry your path can't fix this issue. if this path just turn off
optimization option within sec.c not global project.

I have tested different version GCC such as (GCC4,8, GCC5.x, GCC7.x)
and all of them can duplicate this issue (Ubuntu 16.04, 16.10,18.04 )

I have traced this issue on my hand.

you can see Dispatcher.c (MdeModulePkg/Pei/DIspatcher/) Line 792:


790 if (StackOffsetPositive) {
791 SecCoreData = (CONST EFI_SEC_PEI_HAND_OFF *)((UINTN)(VOID
*)SecCoreData + StackOffset);
792 Private = (PEI_CORE_INSTANCE *)((UINTN)(VOID *)Private +
StackOffset);
793 } else {
794 ..........
795 ..........
796 }

790 --792 disassembly code

0x10200f2ca <PeiCheckAndSwitchStack+1030>: test %r14b,%r14b
0x10200f2cd <PeiCheckAndSwitchStack+1033>: je 0x10200f2df
<PeiCheckAndSwitchStack+1051>
0x10200f2cf <PeiCheckAndSwitchStack+1035>: mov 0x38(%rsp),%rax
0x10200f2d4 <PeiCheckAndSwitchStack+1040>: lea 0x0(%rbp,%rax,1),%r14
0x10200f2d9 <PeiCheckAndSwitchStack+1045>: lea (%rbx,%rax,1),%rbp

we can see Private value have been stored in %rbp (rbp register be
used as general register ) so when call
TemporaryRamSupportPpi->TemporaryRamMigration()
The calling conventions define RBP as non-volatile must be preserved
by callee. Using RBP as the frame pointer is optional.
This is kind of a mess. I think the definition of
EFI_PEI_TEMPORARY_RAM_SUPPORT_PPI is really to blame. It probably
should not have been spec'd as 'change stack and return'. Instead, it
should have been given a new function pointer to switch-stack and call
into.
Andrew, Mike,

I developed a PEI Core fix for this issue, which Liu Yu verified.

Unfortunately, it involves add assembly code in a key area of the PEI
Core. See the top 2 commits of:

https://github.com/jljusten/edk2/commits/emulator-temp-ram <https://github.com/jljusten/edk2/commits/emulator-temp-ram>

I only wrote assembly for X64 nasm. But, I notice that neither the PEI
or DXE Core modules include any assembly code. So, I want to make sure
I'm heading in the right direction before working on it further.
Mike,

I seem to remember we hit an issue like this a long time ago? Do you
remember the details? Maybe it was we needed to write the
TempRamSupport code in assembly?
It does also creep up there, which is why we also adjust ebp/rbp in
the TemporaryRamMigration function in OVMF.

In that case, it helps prevent the stack from reverting to the old
stack when TemporaryRamMigration is returning to it's caller. (As
opposed to after it returns, which I think is what is happening now.)

The issue we are hitting is the gEfiTemporaryRamSupportPpiGuid
TemporaryRamMigration() function call does a stack switch, but it
returns. This causes an issue with the C calling conventions as RBP
is optionally a frame pointer. To quote the MSFT spec: "May be used
as a frame pointer; must be preserved by callee"
1) It is used as a frame pointer. It looks like our existing code
fixes up the frame pointer to match the new location the stack got
moved to.
2) Used as a general purpose register, and the value must be
preserved.
This is true, and I think it makes writing TemporaryRamMigration in C
unsafe. Nevertheless, we've been doing that in OVMF for a while now.


As I mentioned below, I think PIWG could consider an new
TemporarayRemSupport interface that might work better, but that also
may not be worth the effort.
If the current API is not really portable I don;t think it is a bad
idea to update it.
The trickier part is that the C ABI doesn't cover exactly how the
stack may be used after the call to TemporaryRamMigration returns. For
example, there's no reason that the compiler couldn't have saved the
stack pointer onto the stack (which was migrated) and somehow restore
it to an old value from the migrated stack.

I think the only safe way to implement calling the current
TemporaryRamMigration is by using assembly code, and then calling into
a new function from the assembly code. Once we call into a new C
function, there is no way C code-gen can accidentally restore the old
stack pointer.


Can something like this change be integrated into the PEI Core?
Jordan,

I'm not sure?
Part of my question was whether assembly code was allowed in the PEI
Core.

For the RBP == frame pointer case the frame pointer is
no longer valid (it is in temp RAM, not DRAM). It seems like the
point of SecTemporaryRamSupport() fixing up the callers RBP is for
the benefit of the caller. It looks to me like your fix is just
negating that fixup.
Actually, the rbp thing is a not important in terms of the
implementation. Sorry, that part confuses the situation and I should
have dropped it. (I was just trying to figure out how to get gdb to be
able backtrace passed the assembly function, but it didn't work.)

The key part of moving it to assembly is to prevent anything from the
C code gen from potentially being used to reset the stack back to the
old temp-ram.

Thanks,

-Jordan

So that would imply that either we could just
fix this in SecTemporaryRamSupport() or we have 2 code gen paths and
we need to know how the compiler is using RBP to "do the right
thing"

Do we have other code that supports X64 PEI? Is see OvmfPkg....
https://github.com/tianocore/edk2/blob/master/OvmfPkg/Sec/SecMain.c#L875

Looks like OvmfPkg uses SetJump()/LongJump() to change the stack.

//
// Use SetJump()/LongJump() to switch to a new stack.
//
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);
}

SaveAndSetDebugTimerInterrupt (OldStatus);

return EFI_SUCCESS;
}

But given the above code is C code RBP is going to be restored on return. This would seem to imply that the adjusting of the callers RBP is not required? So maybe try changing over Emulator to the OvmfPkg TemporaryRamMigration() algorithm?

Thanks,

Andrew Fish


Thanks for your time,

-Jordan

By returning with a switched stack, we don't really know what the
returning code could do with regard to the stack. For example, it
could have saved the stack value on the stack and then pop it into a
register and somehow switch the stack back to the old stack. It seems
unlikely, but I don't think anything prevents it.

Additionally, there is the issue of rbp/ebp being used as a frame
pointer. This can lead to some variables being used from the temp ram
location after we return from TemporaryRamMigration. (Assuming rbp/ebp
is not adjusted to point to the new stack.)

So, is it safe to adjust rbp? Unknown. It may not be if rbp is not
used as a frame pointer. Is it safe to *not* adjust rbp and
potentially allow the old temp ram stack to be used? Unknown.

Now, if TemporaryRamMigration received a new function to call into, we
can safely assume that the new stack transition would proceed as
expected without having to worry if a compiler is using rbp/ebp as a
framepointer or not.

Another advantage could have been that something like the BasePkg
SwitchStack function could have been used, hopefully preventing people
from trying to write error prone assembly code for
TemporaryRamMigration.

-Jordan


Andrew Fish
 

On Nov 19, 2018, at 2:12 PM, Laszlo Ersek <lersek@redhat.com> wrote:

Jordan wrote:

So, is it safe to adjust rbp? Unknown. It may not be if rbp is not
used as a frame pointer. Is it safe to *not* adjust rbp and
potentially allow the old temp ram stack to be used? Unknown.
Andrew wrote:

Looks like OvmfPkg uses SetJump()/LongJump() to change the stack.

//
// Use SetJump()/LongJump() to switch to a new stack.
//
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);
}

SaveAndSetDebugTimerInterrupt (OldStatus);

return EFI_SUCCESS;
}

But given the above code is C code RBP is going to be restored on return. This would seem to imply that the adjusting of the callers RBP is not required? [...]
The Ebp/Rbp assignments were added in a separate bugfix, namely

https://github.com/tianocore/edk2/commit/89796c69d9fd
Laszlo,

This makes sense since as the post-amble when using frame pointers is either:

addq $288, %rsp ## imm = 0x120
popq %rbp
retq

or

0x00000000fffcd42f <+403>: c9 leaveq
0x00000000fffcd430 <+404>: c3 retq
I've noticed that clang does not seem to be a big fan of the leave function and it adjusts the stack pointer using math rather than using %rbp.

The stack unwind algorithm implies that the frame point, %rbp in our case, is valid while the function is running. Even for the clang code gen not switching the frame point would break the stack unwind.

Thanks,

Andrew Fish



Laszlo


Andrew Fish
 

On Nov 19, 2018, at 2:29 PM, Jordan Justen <jordan.l.justen@intel.com> wrote:

On 2018-11-19 13:22:27, Andrew Fish wrote:


On Nov 19, 2018, at 11:16 AM, Jordan Justen <jordan.l.justen@intel.com> wrote:

On 2018-11-18 17:13:21, Jordan Justen wrote:
On 2018-11-18 14:37:09, Andrew Fish wrote:


On Nov 18, 2018, at 4:07 AM, Liu Yu <pedroa.liu@outlook.com> wrote:

sorry your path can't fix this issue. if this path just turn off
optimization option within sec.c not global project.

I have tested different version GCC such as (GCC4,8, GCC5.x, GCC7.x)
and all of them can duplicate this issue (Ubuntu 16.04, 16.10,18.04 )

I have traced this issue on my hand.

you can see Dispatcher.c (MdeModulePkg/Pei/DIspatcher/) Line 792:


790 if (StackOffsetPositive) {
791 SecCoreData = (CONST EFI_SEC_PEI_HAND_OFF *)((UINTN)(VOID
*)SecCoreData + StackOffset);
792 Private = (PEI_CORE_INSTANCE *)((UINTN)(VOID *)Private +
StackOffset);
793 } else {
794 ..........
795 ..........
796 }

790 --792 disassembly code

0x10200f2ca <PeiCheckAndSwitchStack+1030>: test %r14b,%r14b
0x10200f2cd <PeiCheckAndSwitchStack+1033>: je 0x10200f2df
<PeiCheckAndSwitchStack+1051>
0x10200f2cf <PeiCheckAndSwitchStack+1035>: mov 0x38(%rsp),%rax
0x10200f2d4 <PeiCheckAndSwitchStack+1040>: lea 0x0(%rbp,%rax,1),%r14
0x10200f2d9 <PeiCheckAndSwitchStack+1045>: lea (%rbx,%rax,1),%rbp

we can see Private value have been stored in %rbp (rbp register be
used as general register ) so when call
TemporaryRamSupportPpi->TemporaryRamMigration()
The calling conventions define RBP as non-volatile must be preserved
by callee. Using RBP as the frame pointer is optional.
This is kind of a mess. I think the definition of
EFI_PEI_TEMPORARY_RAM_SUPPORT_PPI is really to blame. It probably
should not have been spec'd as 'change stack and return'. Instead, it
should have been given a new function pointer to switch-stack and call
into.
Andrew, Mike,

I developed a PEI Core fix for this issue, which Liu Yu verified.

Unfortunately, it involves add assembly code in a key area of the PEI
Core. See the top 2 commits of:

https://github.com/jljusten/edk2/commits/emulator-temp-ram <https://github.com/jljusten/edk2/commits/emulator-temp-ram>

I only wrote assembly for X64 nasm. But, I notice that neither the PEI
or DXE Core modules include any assembly code. So, I want to make sure
I'm heading in the right direction before working on it further.
Mike,

I seem to remember we hit an issue like this a long time ago? Do you
remember the details? Maybe it was we needed to write the
TempRamSupport code in assembly?
It does also creep up there, which is why we also adjust ebp/rbp in
the TemporaryRamMigration function in OVMF.

In that case, it helps prevent the stack from reverting to the old
stack when TemporaryRamMigration is returning to it's caller. (As
opposed to after it returns, which I think is what is happening now.)
The stack is switched in both cases. The problem is the C code, for the gcc code gen, does a leave so it moves %rbp into %rsp. The %rbp in the C code was the temp stack frame on entry. So the leave instruction from the C code gen reverts to the old stack.


The issue we are hitting is the gEfiTemporaryRamSupportPpiGuid
TemporaryRamMigration() function call does a stack switch, but it
returns. This causes an issue with the C calling conventions as RBP
is optionally a frame pointer. To quote the MSFT spec: "May be used
as a frame pointer; must be preserved by callee"
1) It is used as a frame pointer. It looks like our existing code
fixes up the frame pointer to match the new location the stack got
moved to.
2) Used as a general purpose register, and the value must be
preserved.
This is true, and I think it makes writing TemporaryRamMigration in C
unsafe. Nevertheless, we've been doing that in OVMF for a while now.
The OVMF form is probably more portable as it is at least following the coding standards.

What I've observed is clang (at least the Xcode version) does not use the leave instruction as it just does math to the %rsp directly and thus does not move the %rbp into %rsp. VC++ and some flavors of gcc don't emit the frame pointer. So there is dependency on code construction.


As I mentioned below, I think PIWG could consider an new
TemporarayRemSupport interface that might work better, but that also
may not be worth the effort.
If the current API is not really portable I don;t think it is a bad
idea to update it.
The trickier part is that the C ABI doesn't cover exactly how the
stack may be used after the call to TemporaryRamMigration returns. For
example, there's no reason that the compiler couldn't have saved the
stack pointer onto the stack (which was migrated) and somehow restore
it to an old value from the migrated stack.

I think the only safe way to implement calling the current
TemporaryRamMigration is by using assembly code, and then calling into
a new function from the assembly code. Once we call into a new C
function, there is no way C code-gen can accidentally restore the old
stack pointer.
Not having a function to call back into is the bug in the current API that we should fix.

Ironically we have assembler code in BaseLib that does a proper stack switch.
https://github.com/tianocore/edk2/blob/master/MdePkg/Library/BaseLib/X64/SwitchStack.S#L38


#------------------------------------------------------------------------------
# Routine Description:
#
# Routine for switching stacks with 2 parameters
#
# Arguments:
#
# (rcx) EntryPoint - Entry point with new stack.
# (rdx) Context1 - Parameter1 for entry point.
# (r8) Context2 - Parameter2 for entry point.
# (r9) NewStack - The pointer to new stack.
#
# Returns:
#
# None
#
#------------------------------------------------------------------------------
ASM_GLOBAL ASM_PFX(InternalSwitchStack)
ASM_PFX(InternalSwitchStack):
pushq %rbp
movq %rsp, %rbp

mov %rcx, %rax // Shift registers for new call
mov %rdx, %rcx
mov %r8, %rdx
#
# Reserve space for register parameters (rcx, rdx, r8 & r9) on the stack,
# in case the callee wishes to spill them.
#
lea -0x20(%r9), %rsp
pushq $0 // stop gdb stack unwind
jmp *%rax // call EntryPoint ()





Can something like this change be integrated into the PEI Core?
Jordan,

I'm not sure?
Part of my question was whether assembly code was allowed in the PEI
Core.
It seems like if we need it we should use it?

For the RBP == frame pointer case the frame pointer is
no longer valid (it is in temp RAM, not DRAM). It seems like the
point of SecTemporaryRamSupport() fixing up the callers RBP is for
the benefit of the caller. It looks to me like your fix is just
negating that fixup.
Actually, the rbp thing is a not important in terms of the
implementation. Sorry, that part confuses the situation and I should
have dropped it. (I was just trying to figure out how to get gdb to be
able backtrace passed the assembly function, but it didn't work.)
I though the original bug was RBP getting trashed?

From a gdb point of view the assumption is %rbp is always the valid frame pointer (fp) for your current location. And you walk the frame via:
next_pc = contents of fp + 8
next_fp = contents of fp

if next_pc == 0 you stop unwinding.

The InternalSwitchStack is an example of a context less stack switch. The push $0 jmp *%rax stops the stack unwind. The saving of %rsp to %rbp means if you take a breakpoint in this function you can unwind the OLD stack. As soon as the EntryPoint preamble saves the new %rsp to %rbp then the debugger will only see the new stack.

In the temp RAM migration we actually copy the old stack over so you unwind as long as the temp memory is still available. If the temp RAM gets zero'ed out it will stop the stack unwind as when you point back into the temp ram stack the pc on that frame would always be zero.

The key part of moving it to assembly is to prevent anything from the
C code gen from potentially being used to reset the stack back to the
old temp-ram.
Given this is a generic issue with this API and the way C works don't we really need to fix it for all supported CPU architectures?

The only thing I can think of is the Emulator code is hacking on %rbp to work around the issue with C code that is using the leave instruction in the post-amble. It would be easy enough on OVFM to fill the TempRam with 0xAFAFAFAFAFAFAFAF, or any value that will GP fault then dereferenced. But it is likely sensitive to the flavor of compiler that is used.

Thanks,

Andrew Fish

Thanks,

-Jordan

So that would imply that either we could just
fix this in SecTemporaryRamSupport() or we have 2 code gen paths and
we need to know how the compiler is using RBP to "do the right
thing"

Do we have other code that supports X64 PEI? Is see OvmfPkg....
https://github.com/tianocore/edk2/blob/master/OvmfPkg/Sec/SecMain.c#L875

Looks like OvmfPkg uses SetJump()/LongJump() to change the stack.

//
// Use SetJump()/LongJump() to switch to a new stack.
//
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);
}

SaveAndSetDebugTimerInterrupt (OldStatus);

return EFI_SUCCESS;
}

But given the above code is C code RBP is going to be restored on return. This would seem to imply that the adjusting of the callers RBP is not required? So maybe try changing over Emulator to the OvmfPkg TemporaryRamMigration() algorithm?

Thanks,

Andrew Fish


Thanks for your time,

-Jordan

By returning with a switched stack, we don't really know what the
returning code could do with regard to the stack. For example, it
could have saved the stack value on the stack and then pop it into a
register and somehow switch the stack back to the old stack. It seems
unlikely, but I don't think anything prevents it.

Additionally, there is the issue of rbp/ebp being used as a frame
pointer. This can lead to some variables being used from the temp ram
location after we return from TemporaryRamMigration. (Assuming rbp/ebp
is not adjusted to point to the new stack.)

So, is it safe to adjust rbp? Unknown. It may not be if rbp is not
used as a frame pointer. Is it safe to *not* adjust rbp and
potentially allow the old temp ram stack to be used? Unknown.

Now, if TemporaryRamMigration received a new function to call into, we
can safely assume that the new stack transition would proceed as
expected without having to worry if a compiler is using rbp/ebp as a
framepointer or not.

Another advantage could have been that something like the BasePkg
SwitchStack function could have been used, hopefully preventing people
from trying to write error prone assembly code for
TemporaryRamMigration.

-Jordan


Laszlo Ersek
 

On 11/19/18 23:29, Jordan Justen wrote:
On 2018-11-19 13:22:27, Andrew Fish wrote:
I seem to remember we hit an issue like this a long time ago? Do you
remember the details? Maybe it was we needed to write the
TempRamSupport code in assembly?
It does also creep up there, which is why we also adjust ebp/rbp in
the TemporaryRamMigration function in OVMF.

In that case, it helps prevent the stack from reverting to the old
stack when TemporaryRamMigration is returning to it's caller. (As
opposed to after it returns, which I think is what is happening now.)
Right, in retrospect, I should have been clearer about this (= with
myself as well), when pointing out the commit that added the BP
massaging to OVMF's temp RAM migration. I just wanted to point out that
the BP massaging hadn't always been there in the OVMF code, so it wasn't
"part of the original design" or some such.

Thanks
Laszlo


Ni, Ray
 

I also met this issue.
I found three solutions:
1. Forcing PeiMain CC flag to "-O0" works.
2. Changing EmulatorPkg/Sec to not produce TemporaryRamSupportPpi also works.
3. Implement the temporary migration routine as below in EmulatorPkg/Sec module.

EFI_STATUS
EFIAPI
SecTemporaryRamSupport (
IN CONST EFI_PEI_SERVICES **PeiServices,
IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase,
IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase,
IN UINTN CopySize
)
{
VOID *OldHeap;
VOID *NewHeap;
VOID *OldStack;
VOID *NewStack;
UINTN StackMigrateOffset;
BASE_LIBRARY_JUMP_BUFFER JumpBuffer;

DEBUG ((EFI_D_INFO,
"TemporaryRamMigration(0x%Lx, 0x%Lx, 0x%Lx)\n",
TemporaryMemoryBase,
PermanentMemoryBase,
(UINT64)CopySize
));

//
// Assume Host prepare the stack and heap in the temprary ram that stack
// is below heap (stack is in smaller address).
// Stack/heap migration depends on the stack/heap location information
// in the temporary ram.
//
OldStack = (VOID*)(UINTN)TemporaryMemoryBase;
NewStack = (VOID*)((UINTN)PermanentMemoryBase);

OldHeap = (VOID*)((UINTN)TemporaryMemoryBase + (CopySize >> 1));
NewHeap = (VOID*)((UINTN)PermanentMemoryBase + (CopySize >> 1));

StackMigrateOffset = (UINTN)NewStack - (UINTN)OldStack;

//
// Migrate Heap and Stack
//
CopyMem (NewHeap, OldHeap, CopySize >> 1);
CopyMem (NewStack, OldStack, CopySize >> 1);

//
// Use SetJump()/LongJump() to switch to a new stack.
//
if (SetJump (&JumpBuffer) == 0) {
#if defined (MDE_CPU_IA32)
JumpBuffer.Esp = JumpBuffer.Esp + StackMigrateOffset;
JumpBuffer.Ebp = JumpBuffer.Ebp + StackMigrateOffset;
#endif
#if defined (MDE_CPU_X64)
JumpBuffer.Rsp = JumpBuffer.Rsp + StackMigrateOffset;
JumpBuffer.Rbp = JumpBuffer.Rbp + StackMigrateOffset;
#endif
LongJump (&JumpBuffer, (UINTN)-1);
}

ZeroMem ((VOID *)(UINTN) TemporaryMemoryBase, CopySize);

return EFI_SUCCESS;
}


Andrew,
I'd like to know why you chose to produce the migration PPI from
EmulatorPkg/Sec module.
Based on PI spec and current PeiCore implementation, PeiCore can do the migration when PPI is absent.



--
Thanks,
Ray


Ni, Ray
 

I also met this issue.
I found three solutions:
1. Forcing PeiMain CC flag to "-O0" works.
2. Changing EmulatorPkg/Sec to not produce TemporaryRamSupportPpi also works.
3. Implement the temporary migration routine as below in EmulatorPkg/Sec module.

EFI_STATUS
EFIAPI
SecTemporaryRamSupport (
IN CONST EFI_PEI_SERVICES **PeiServices,
IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase,
IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase,
IN UINTN CopySize
)
{
VOID *OldHeap;
VOID *NewHeap;
VOID *OldStack;
VOID *NewStack;
UINTN StackMigrateOffset;
BASE_LIBRARY_JUMP_BUFFER JumpBuffer;

DEBUG ((EFI_D_INFO,
"TemporaryRamMigration(0x%Lx, 0x%Lx, 0x%Lx)\n",
TemporaryMemoryBase,
PermanentMemoryBase,
(UINT64)CopySize
));

//
// Assume Host prepare the stack and heap in the temprary ram that stack
// is below heap (stack is in smaller address).
// Stack/heap migration depends on the stack/heap location information
// in the temporary ram.
//
OldStack = (VOID*)(UINTN)TemporaryMemoryBase;
NewStack = (VOID*)((UINTN)PermanentMemoryBase);

OldHeap = (VOID*)((UINTN)TemporaryMemoryBase + (CopySize >> 1));
NewHeap = (VOID*)((UINTN)PermanentMemoryBase + (CopySize >> 1));

StackMigrateOffset = (UINTN)NewStack - (UINTN)OldStack;

//
// Migrate Heap and Stack
//
CopyMem (NewHeap, OldHeap, CopySize >> 1);
CopyMem (NewStack, OldStack, CopySize >> 1);

//
// Use SetJump()/LongJump() to switch to a new stack.
//
if (SetJump (&JumpBuffer) == 0) {
#if defined (MDE_CPU_IA32)
JumpBuffer.Esp = JumpBuffer.Esp + StackMigrateOffset;
JumpBuffer.Ebp = JumpBuffer.Ebp + StackMigrateOffset;
#endif
#if defined (MDE_CPU_X64)
JumpBuffer.Rsp = JumpBuffer.Rsp + StackMigrateOffset;
JumpBuffer.Rbp = JumpBuffer.Rbp + StackMigrateOffset;
#endif
LongJump (&JumpBuffer, (UINTN)-1);
}

ZeroMem ((VOID *)(UINTN) TemporaryMemoryBase, CopySize);

return EFI_SUCCESS;
}


Andrew,
I'd like to know why you chose to produce the migration PPI from
EmulatorPkg/Sec module.
Based on PI spec and current PeiCore implementation, PeiCore can do the migration when PPI is absent.



--
Thanks,
Ray


Ni, Ray
 

(Sent third times to make sure Andrew and Laszlo are in the TO list.)

I also met this issue.
I found three solutions:
1. Forcing PeiMain CC flag to "-O0" works.
2. Changing EmulatorPkg/Sec to not produce TemporaryRamSupportPpi also works.
3. Implement the temporary migration routine as below in EmulatorPkg/Sec module.

EFI_STATUS
EFIAPI
SecTemporaryRamSupport (
IN CONST EFI_PEI_SERVICES **PeiServices,
IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase,
IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase,
IN UINTN CopySize
)
{
VOID *OldHeap;
VOID *NewHeap;
VOID *OldStack;
VOID *NewStack;
UINTN StackMigrateOffset;
BASE_LIBRARY_JUMP_BUFFER JumpBuffer;

DEBUG ((EFI_D_INFO,
"TemporaryRamMigration(0x%Lx, 0x%Lx, 0x%Lx)\n",
TemporaryMemoryBase,
PermanentMemoryBase,
(UINT64)CopySize
));

//
// Assume Host prepare the stack and heap in the temprary ram that stack
// is below heap (stack is in smaller address).
// Stack/heap migration depends on the stack/heap location information
// in the temporary ram.
//
OldStack = (VOID*)(UINTN)TemporaryMemoryBase;
NewStack = (VOID*)((UINTN)PermanentMemoryBase);

OldHeap = (VOID*)((UINTN)TemporaryMemoryBase + (CopySize >> 1));
NewHeap = (VOID*)((UINTN)PermanentMemoryBase + (CopySize >> 1));

StackMigrateOffset = (UINTN)NewStack - (UINTN)OldStack;

//
// Migrate Heap and Stack
//
CopyMem (NewHeap, OldHeap, CopySize >> 1);
CopyMem (NewStack, OldStack, CopySize >> 1);

//
// Use SetJump()/LongJump() to switch to a new stack.
//
if (SetJump (&JumpBuffer) == 0) {
#if defined (MDE_CPU_IA32)
JumpBuffer.Esp = JumpBuffer.Esp + StackMigrateOffset;
JumpBuffer.Ebp = JumpBuffer.Ebp + StackMigrateOffset;
#endif
#if defined (MDE_CPU_X64)
JumpBuffer.Rsp = JumpBuffer.Rsp + StackMigrateOffset;
JumpBuffer.Rbp = JumpBuffer.Rbp + StackMigrateOffset;
#endif
LongJump (&JumpBuffer, (UINTN)-1);
}

ZeroMem ((VOID *)(UINTN) TemporaryMemoryBase, CopySize);

return EFI_SUCCESS;
}


Andrew,
I'd like to know why you chose to produce the migration PPI from
EmulatorPkg/Sec module.
Based on PI spec and current PeiCore implementation, PeiCore can do the migration when PPI is absent.



--
Thanks,
Ray


Ni, Ray
 

On 2/16/2019 3:43 PM, Ni, Ray wrote:
(Sent third times to make sure Andrew and Laszlo are in the TO list.)
I also met this issue.
I found three solutions:
1. Forcing PeiMain CC flag to "-O0" works.
2. Changing EmulatorPkg/Sec to not produce TemporaryRamSupportPpi also works.
3. Implement the temporary migration routine as below in EmulatorPkg/Sec module.
EFI_STATUS
EFIAPI
SecTemporaryRamSupport (
  IN CONST EFI_PEI_SERVICES   **PeiServices,
  IN EFI_PHYSICAL_ADDRESS     TemporaryMemoryBase,
  IN EFI_PHYSICAL_ADDRESS     PermanentMemoryBase,
  IN UINTN                    CopySize
  )
{
  VOID                             *OldHeap;
  VOID                             *NewHeap;
  VOID                             *OldStack;
  VOID                             *NewStack;
  UINTN                            StackMigrateOffset;
  BASE_LIBRARY_JUMP_BUFFER         JumpBuffer;
  DEBUG ((EFI_D_INFO,
    "TemporaryRamMigration(0x%Lx, 0x%Lx, 0x%Lx)\n",
    TemporaryMemoryBase,
    PermanentMemoryBase,
    (UINT64)CopySize
    ));
  //
  // Assume Host prepare the stack and heap in the temprary ram that stack
  // is below heap (stack is in smaller address).
  // Stack/heap migration depends on the stack/heap location information
  // in the temporary ram.
  //
  OldStack = (VOID*)(UINTN)TemporaryMemoryBase;
  NewStack = (VOID*)((UINTN)PermanentMemoryBase);
  OldHeap = (VOID*)((UINTN)TemporaryMemoryBase + (CopySize >> 1));
  NewHeap = (VOID*)((UINTN)PermanentMemoryBase + (CopySize >> 1));
  StackMigrateOffset = (UINTN)NewStack - (UINTN)OldStack;
  //
  // Migrate Heap and Stack
  //
  CopyMem (NewHeap, OldHeap, CopySize >> 1);
  CopyMem (NewStack, OldStack, CopySize >> 1);
  //
  // Use SetJump()/LongJump() to switch to a new stack.
  //
  if (SetJump (&JumpBuffer) == 0) {
#if defined (MDE_CPU_IA32)
    JumpBuffer.Esp = JumpBuffer.Esp + StackMigrateOffset;
    JumpBuffer.Ebp = JumpBuffer.Ebp + StackMigrateOffset;
#endif
#if defined (MDE_CPU_X64)
    JumpBuffer.Rsp = JumpBuffer.Rsp + StackMigrateOffset;
    JumpBuffer.Rbp = JumpBuffer.Rbp + StackMigrateOffset;
#endif
    LongJump (&JumpBuffer, (UINTN)-1);
  }
  ZeroMem ((VOID *)(UINTN) TemporaryMemoryBase, CopySize);
  return EFI_SUCCESS;
}
Andrew,
I'd like to know why you chose to produce the migration PPI from
EmulatorPkg/Sec module.
Based on PI spec and current PeiCore implementation, PeiCore can do the migration when PPI is absent.
Study the PeiCore migration logic a bit more, I found since PeiCore knows the exact size of new stack in permanent memory, it migrates the old stack to the top of new stack.
But the migration logic in above C code (since it doesn't know the size of new stack, CopySize is the size of temporary memory) may copy the old stack to the middle in new stack.

--
Thanks,
Ray


Jordan Justen
 

On 2019-02-16 00:05:27, Ni, Ray wrote:
On 2/16/2019 3:43 PM, Ni, Ray wrote:
(Sent third times to make sure Andrew and Laszlo are in the TO list.)

I also met this issue.
I found three solutions:
1. Forcing PeiMain CC flag to "-O0" works.
2. Changing EmulatorPkg/Sec to not produce TemporaryRamSupportPpi also
works.
3. Implement the temporary migration routine as below in EmulatorPkg/Sec
module.

EFI_STATUS
EFIAPI
SecTemporaryRamSupport (
  IN CONST EFI_PEI_SERVICES   **PeiServices,
  IN EFI_PHYSICAL_ADDRESS     TemporaryMemoryBase,
  IN EFI_PHYSICAL_ADDRESS     PermanentMemoryBase,
  IN UINTN                    CopySize
  )
{
  VOID                             *OldHeap;
  VOID                             *NewHeap;
  VOID                             *OldStack;
  VOID                             *NewStack;
  UINTN                            StackMigrateOffset;
  BASE_LIBRARY_JUMP_BUFFER         JumpBuffer;

  DEBUG ((EFI_D_INFO,
    "TemporaryRamMigration(0x%Lx, 0x%Lx, 0x%Lx)\n",
    TemporaryMemoryBase,
    PermanentMemoryBase,
    (UINT64)CopySize
    ));

  //
  // Assume Host prepare the stack and heap in the temprary ram that stack
  // is below heap (stack is in smaller address).
  // Stack/heap migration depends on the stack/heap location information
  // in the temporary ram.
  //
  OldStack = (VOID*)(UINTN)TemporaryMemoryBase;
  NewStack = (VOID*)((UINTN)PermanentMemoryBase);

  OldHeap = (VOID*)((UINTN)TemporaryMemoryBase + (CopySize >> 1));
  NewHeap = (VOID*)((UINTN)PermanentMemoryBase + (CopySize >> 1));

  StackMigrateOffset = (UINTN)NewStack - (UINTN)OldStack;

  //
  // Migrate Heap and Stack
  //
  CopyMem (NewHeap, OldHeap, CopySize >> 1);
  CopyMem (NewStack, OldStack, CopySize >> 1);

  //
  // Use SetJump()/LongJump() to switch to a new stack.
  //
  if (SetJump (&JumpBuffer) == 0) {
#if defined (MDE_CPU_IA32)
    JumpBuffer.Esp = JumpBuffer.Esp + StackMigrateOffset;
    JumpBuffer.Ebp = JumpBuffer.Ebp + StackMigrateOffset;
#endif
#if defined (MDE_CPU_X64)
    JumpBuffer.Rsp = JumpBuffer.Rsp + StackMigrateOffset;
    JumpBuffer.Rbp = JumpBuffer.Rbp + StackMigrateOffset;
#endif
    LongJump (&JumpBuffer, (UINTN)-1);
  }

  ZeroMem ((VOID *)(UINTN) TemporaryMemoryBase, CopySize);

  return EFI_SUCCESS;
}

Andrew,
I'd like to know why you chose to produce the migration PPI from
EmulatorPkg/Sec module.
Based on PI spec and current PeiCore implementation, PeiCore can do the
migration when PPI is absent.
Study the PeiCore migration logic a bit more, I found since PeiCore
knows the exact size of new stack in permanent memory, it migrates the
old stack to the top of new stack.
But the migration logic in above C code (since it doesn't know the size
of new stack, CopySize is the size of temporary memory) may copy the old
stack to the middle in new stack.
We had a fair amount of discussion about this in November.

Search for "EmulatorPkg Unix" on
https://lists.01.org/pipermail/edk2-devel/2018-November/thread.html.

I have a branch related to that discussion:

https://github.com/jljusten/edk2/tree/temp-ram-support2

There are essentially 2 parts to the branch. First is a fix for the
PEI Core TemporaryRamMigration implementation.

After the "MdeModulePkg/Core/Pei: Use assembly for X64
TemporaryRamMigration" patch on that branch, I was working on a new
TemporaryRamMigration2 PPI that should fix the issues with
TemporaryRamMigration. Of course, that would have to become part of
the standard before EDK-II could add it.

-Jordan


Andrew Fish
 

On Feb 15, 2019, at 11:40 PM, Ni, Ray <ray.ni@Intel.com <mailto:ray.ni@Intel.com>> wrote:

I also met this issue.
I found three solutions:
1. Forcing PeiMain CC flag to "-O0" works.
2. Changing EmulatorPkg/Sec to not produce TemporaryRamSupportPpi also works.
3. Implement the temporary migration routine as below in EmulatorPkg/Sec module.

EFI_STATUS
EFIAPI
SecTemporaryRamSupport (
IN CONST EFI_PEI_SERVICES **PeiServices,
IN EFI_PHYSICAL_ADDRESS TemporaryMemoryBase,
IN EFI_PHYSICAL_ADDRESS PermanentMemoryBase,
IN UINTN CopySize
)
{
VOID *OldHeap;
VOID *NewHeap;
VOID *OldStack;
VOID *NewStack;
UINTN StackMigrateOffset;
BASE_LIBRARY_JUMP_BUFFER JumpBuffer;

DEBUG ((EFI_D_INFO,
"TemporaryRamMigration(0x%Lx, 0x%Lx, 0x%Lx)\n",
TemporaryMemoryBase,
PermanentMemoryBase,
(UINT64)CopySize
));

//
// Assume Host prepare the stack and heap in the temprary ram that stack
// is below heap (stack is in smaller address).
// Stack/heap migration depends on the stack/heap location information
// in the temporary ram.
//
OldStack = (VOID*)(UINTN)TemporaryMemoryBase;
NewStack = (VOID*)((UINTN)PermanentMemoryBase);

OldHeap = (VOID*)((UINTN)TemporaryMemoryBase + (CopySize >> 1));
NewHeap = (VOID*)((UINTN)PermanentMemoryBase + (CopySize >> 1));

StackMigrateOffset = (UINTN)NewStack - (UINTN)OldStack;

//
// Migrate Heap and Stack
//
CopyMem (NewHeap, OldHeap, CopySize >> 1);
CopyMem (NewStack, OldStack, CopySize >> 1);

//
// Use SetJump()/LongJump() to switch to a new stack.
//
if (SetJump (&JumpBuffer) == 0) {
#if defined (MDE_CPU_IA32)
JumpBuffer.Esp = JumpBuffer.Esp + StackMigrateOffset;
JumpBuffer.Ebp = JumpBuffer.Ebp + StackMigrateOffset;
#endif
#if defined (MDE_CPU_X64)
JumpBuffer.Rsp = JumpBuffer.Rsp + StackMigrateOffset;
JumpBuffer.Rbp = JumpBuffer.Rbp + StackMigrateOffset;
#endif
LongJump (&JumpBuffer, (UINTN)-1);
}

ZeroMem ((VOID *)(UINTN) TemporaryMemoryBase, CopySize);

return EFI_SUCCESS;
}


Andrew,
I'd like to know why you chose to produce the migration PPI from
EmulatorPkg/Sec module.
Based on PI spec and current PeiCore implementation, PeiCore can do the migration when PPI is absent.
Ray,

Sorry I don't remember if it was due to the code being so old, or if I was trying to better emulate a real platform.

Thanks,

Andrew Fish



--
Thanks,
Ray
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org <mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel <https://lists.01.org/mailman/listinfo/edk2-devel>


Ni, Ray
 

Jordan,
Study the PeiCore migration logic a bit more, I found since PeiCore
knows the exact size of new stack in permanent memory, it migrates
old stack to the top of new stack.
But the migration logic in above C code (since it doesn't know the
size of new stack, CopySize is the size of temporary memory) may copy the old stack to the middle in new stack.
Maybe your new RamMigration2 PPI needs to carry both the old and new stack/heap location and size.
It helps:
1. migrate the old stack to top of new stack (instead of middle of new stack).
2. potentially reduce the size of memory that needs to be copied.

TemporaryRamMigration (
IN CONST EFI_PEI_SERVICES **PeiServices,
IN EFI_PHYSICAL_ADDRESS TemporaryStackBase,
IN EFI_PHYSICAL_ADDRESS PermanentStackSize,
IN EFI_PHYSICAL_ADDRESS TemporaryHeapBase,
IN EFI_PHYSICAL_ADDRESS PermanentHeapSize,
IN TEMPORARY_RAM_MIGRATION_CALLBACK Callback,
IN VOID *Context
)


Before the finalize of PI spec change regarding the RamMigration2 change, I prefer to fix the EmulatorPkg boot crash ASAP using the
OVMF-like solution.
If you agree, I will send out the patch to let you review.

--
Thanks,
Ray


Jordan Justen
 

On 2019-02-17 18:25:01, Ni, Ray wrote:
Jordan,
> Study the PeiCore migration logic a bit more, I found since PeiCore
> knows the exact size of new stack in permanent memory, it migrates
> old stack to the top of new stack.
> But the migration logic in above C code (since it doesn't know the
> size of new stack, CopySize is the size of temporary memory) may copy
> the old stack to the middle in new stack.

Maybe your new RamMigration2 PPI needs to carry both the old and new
stack/heap location and size.
It helps:
1. migrate the old stack to top of new stack (instead of middle of new
stack).
2. potentially reduce the size of memory that needs to be copied.

TemporaryRamMigration (
IN CONST EFI_PEI_SERVICES **PeiServices,
IN EFI_PHYSICAL_ADDRESS TemporaryStackBase,
IN EFI_PHYSICAL_ADDRESS PermanentStackSize,
IN EFI_PHYSICAL_ADDRESS TemporaryHeapBase,
IN EFI_PHYSICAL_ADDRESS PermanentHeapSize,
IN TEMPORARY_RAM_MIGRATION_CALLBACK Callback,
IN VOID *Context
)
Hmm. I'll think about that for the new PPI.

Before the finalize of PI spec change regarding the RamMigration2
change, I prefer to fix the EmulatorPkg boot crash ASAP using the
OVMF-like solution.
If you agree, I will send out the patch to let you review.
You are right that it will take more time for a new PPI. But, I have
almost finished my patches to fix the PEI Core with the old PPI. I'll
try to send them out tonight.

-Jordan