* [PATCH v1 0/1] Fix stack switching to permanent memory fail
@ 2017-09-03 2:12 Ge Song
2017-09-03 2:12 ` [PATCH v1 1/1] OvmfPkg/SecMain: " Ge Song
0 siblings, 1 reply; 15+ messages in thread
From: Ge Song @ 2017-09-03 2:12 UTC (permalink / raw)
To: edk2-devel; +Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel
Repo: https://github.com/sgbird/edk2.git
Branch: stack_switch
This patch is to fix that it failed to switch the stack from temporary
memory to permanent memory when the latter becomes available
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Ge Song (1):
OvmfPkg/SecMain: Fix stack switching to permanent memory fail
OvmfPkg/Sec/SecMain.c | 2 ++
1 file changed, 2 insertions(+)
--
2.11.0
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v1 1/1] OvmfPkg/SecMain: Fix stack switching to permanent memory fail
2017-09-03 2:12 [PATCH v1 0/1] Fix stack switching to permanent memory fail Ge Song
@ 2017-09-03 2:12 ` Ge Song
2017-09-03 4:37 ` Jordan Justen
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Ge Song @ 2017-09-03 2:12 UTC (permalink / raw)
To: edk2-devel; +Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/1] OvmfPkg/SecMain: Fix stack switching to permanent memory fail
2017-09-03 2:12 ` [PATCH v1 1/1] OvmfPkg/SecMain: " Ge Song
@ 2017-09-03 4:37 ` Jordan Justen
2017-09-03 20:50 ` Laszlo Ersek
2017-09-03 20:49 ` Laszlo Ersek
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Jordan Justen @ 2017-09-03 4:37 UTC (permalink / raw)
To: Ge Song, edk2-devel, Laszlo Ersek; +Cc: Ard Biesheuvel
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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/1] OvmfPkg/SecMain: Fix stack switching to permanent memory fail
2017-09-03 2:12 ` [PATCH v1 1/1] OvmfPkg/SecMain: " Ge Song
2017-09-03 4:37 ` Jordan Justen
@ 2017-09-03 20:49 ` Laszlo Ersek
2017-09-04 8:54 ` Ge Song
2017-09-04 22:26 ` Laszlo Ersek
2017-09-03 20:52 ` Laszlo Ersek
2017-09-03 20:55 ` Laszlo Ersek
3 siblings, 2 replies; 15+ messages in thread
From: Laszlo Ersek @ 2017-09-03 20:49 UTC (permalink / raw)
To: Ge Song, edk2-devel; +Cc: Jordan Justen, Ard Biesheuvel
On 09/03/17 04:12, Ge Song wrote:
> In earlier PEI stage, temporary memory(Cache As Ram) is employed as stack
Please remove the "Cache As Ram" reference; it doesn't apply to OVMF.
I suggest "temporary memory at PcdOvmfSecPeiTempRamBase"
> 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(),
I suggest rewording this as "before entering TemporaryRamMigration()" --
when I read the above, I went looking for Ebp/Rbp manipulation within
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
Please start a new sentence with "after the execution of ...".
> 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.
s/momery/memory/
But, more importantly, what are the practical consequences of this bug?
Does it mean that the temporary SEC/PEI stack is used until the end of
PEI, even after permanent RAM is installed, during both normal boot and
S3 resume?
If that's the case, I wonder how it was caught.
If it was caught during S3 resume, then the only symptom could have been
stack overflow. Because during normal boot, both temporary and permanent
PEI RAM are reserved, so even if we stay on the temporary stack during
S3, if we don't overflow it, we cannot corrupt data:
> /**
> Publish system RAM and reserve memory regions
>
> **/
> VOID
> InitializeRamRegions (
> VOID
> )
> {
> if (!mXen) {
> QemuInitializeRam ();
> } else {
> XenPublishRamRegions ();
> }
>
> if (mS3Supported && mBootMode != BOOT_ON_S3_RESUME) {
> //
> // This is the memory range that will be used for PEI on S3 resume
> //
> BuildMemoryAllocationHob (
> mS3AcpiReservedMemoryBase,
> mS3AcpiReservedMemorySize,
> EfiACPIMemoryNVS
> );
>
> //
> // Cover the initial RAM area used as stack and temporary PEI heap.
> //
> // This is reserved as ACPI NVS so it can be used on S3 resume.
> //
> BuildMemoryAllocationHob (
> PcdGet32 (PcdOvmfSecPeiTempRamBase),
> PcdGet32 (PcdOvmfSecPeiTempRamSize),
> EfiACPIMemoryNVS
> );
With stack overflow during S3 resume, we may have corrupted OS data
residing very low.
However, if the bug was caught during normal boot, then I don't know
what the symptom may have been. The permanent PEI RAM for normal boot is
pretty close to the top of low RAM (and the default RAM size for QEMU is
128MB). The temporary stack is very low however ("Temp Stack :
BaseAddress=0x814000 Length=0x4000"). So even if we overflow the
temporary stack (i.e. go below the address 0x814000) during normal boot,
I don't think anything allocated from the permanent PEI RAM could be
corrupted by that.
I'll do some testing in the next few days.
The analysis and the patch look great (and impressive!) to me. I'd like
the commit message to be cleaned up a bit -- please see my notes above,
and please describe:
- practical consequences of the bug,
- how the bug was caught.
I have one more (superficial) comment below:
> 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\x19s Manual, Volume 2A
I prefer keeping commit messages ASCII-clean.
Thanks
Laszlo
> "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\x19s 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);
> }
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/1] OvmfPkg/SecMain: Fix stack switching to permanent memory fail
2017-09-03 4:37 ` Jordan Justen
@ 2017-09-03 20:50 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2017-09-03 20:50 UTC (permalink / raw)
To: Jordan Justen, Ge Song, edk2-devel; +Cc: Ard Biesheuvel
On 09/03/17 06:37, Jordan Justen wrote:
> 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.
Much appreciated!
Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/1] OvmfPkg/SecMain: Fix stack switching to permanent memory fail
2017-09-03 2:12 ` [PATCH v1 1/1] OvmfPkg/SecMain: " Ge Song
2017-09-03 4:37 ` Jordan Justen
2017-09-03 20:49 ` Laszlo Ersek
@ 2017-09-03 20:52 ` Laszlo Ersek
2017-09-03 20:55 ` Laszlo Ersek
3 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2017-09-03 20:52 UTC (permalink / raw)
To: Ge Song, edk2-devel; +Cc: Jordan Justen, Ard Biesheuvel
On 09/03/17 04:12, Ge Song wrote:
> [...]
Furthermore, I suggest the following subject line (just drop the word
"fail"):
OvmfPkg/SecMain: Fix stack switching to permanent memory
Thank you!
Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/1] OvmfPkg/SecMain: Fix stack switching to permanent memory fail
2017-09-03 2:12 ` [PATCH v1 1/1] OvmfPkg/SecMain: " Ge Song
` (2 preceding siblings ...)
2017-09-03 20:52 ` Laszlo Ersek
@ 2017-09-03 20:55 ` Laszlo Ersek
2017-09-04 13:34 ` Jordan Justen
3 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2017-09-03 20:55 UTC (permalink / raw)
To: Ge Song, edk2-devel; +Cc: Jordan Justen, Ard Biesheuvel
On 09/03/17 04:12, 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.
[...]
> 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(+)
(No more separate emails, I promise.)
Before you send v2, can you please amend the patch like this:
git commit --amend --author='Ge Song <ge.song@hxt-semitech.com>'
It's OK if you mail it out from another email address, but IMO the git
authorship should match the first Signed-off-by.
The commit message body currently does not start with "From: Ge Song
<ge.song@hxt-semitech.com>", which means that the git authorship and the
first S-o-b disagree at the moment.
... If I'm wrong about this, I can be convinced, of course.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/1] OvmfPkg/SecMain: Fix stack switching to permanent memory fail
2017-09-03 20:49 ` Laszlo Ersek
@ 2017-09-04 8:54 ` Ge Song
2017-09-04 22:26 ` Laszlo Ersek
1 sibling, 0 replies; 15+ messages in thread
From: Ge Song @ 2017-09-04 8:54 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel; +Cc: Jordan Justen, Ard Biesheuvel
On 09/04/2017 04:49 AM, Laszlo Ersek wrote:
> On 09/03/17 04:12, Ge Song wrote:
>> In earlier PEI stage, temporary memory(Cache As Ram) is employed as stack
> Please remove the "Cache As Ram" reference; it doesn't apply to OVMF.
>
> I suggest "temporary memory at PcdOvmfSecPeiTempRamBase"
>
>> 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(),
> I suggest rewording this as "before entering TemporaryRamMigration()" --
> when I read the above, I went looking for Ebp/Rbp manipulation within
> 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
> Please start a new sentence with "after the execution of ...".
>
>> 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.
> s/momery/memory/
Thanks for your detailed guidance, I'll revise the things you mentioned in
V2 patch.
And I must express my thanks for your article
"Laszlo's unkempt git guide for edk2 contributors and maintainers".
It helps me a lot!
> But, more importantly, what are the practical consequences of this bug?
>
> Does it mean that the temporary SEC/PEI stack is used until the end of
> PEI, even after permanent RAM is installed, during both normal boot and
> S3 resume?
Yes, I think that's exactly true.
> If that's the case, I wonder how it was caught.
Once I was tracing a variable and found the abnormal value in SP.
> If it was caught during S3 resume, then the only symptom could have been
> stack overflow. Because during normal boot, both temporary and permanent
> PEI RAM are reserved, so even if we stay on the temporary stack during
> S3, if we don't overflow it, we cannot corrupt data:
>
>> /**
>> Publish system RAM and reserve memory regions
>>
>> **/
>> VOID
>> InitializeRamRegions (
>> VOID
>> )
>> {
>> if (!mXen) {
>> QemuInitializeRam ();
>> } else {
>> XenPublishRamRegions ();
>> }
>>
>> if (mS3Supported && mBootMode != BOOT_ON_S3_RESUME) {
>> //
>> // This is the memory range that will be used for PEI on S3 resume
>> //
>> BuildMemoryAllocationHob (
>> mS3AcpiReservedMemoryBase,
>> mS3AcpiReservedMemorySize,
>> EfiACPIMemoryNVS
>> );
>>
>> //
>> // Cover the initial RAM area used as stack and temporary PEI heap.
>> //
>> // This is reserved as ACPI NVS so it can be used on S3 resume.
>> //
>> BuildMemoryAllocationHob (
>> PcdGet32 (PcdOvmfSecPeiTempRamBase),
>> PcdGet32 (PcdOvmfSecPeiTempRamSize),
>> EfiACPIMemoryNVS
>> );
> With stack overflow during S3 resume, we may have corrupted OS data
> residing very low.
Yes, I think this is the real problem.
In current implementation, stack located in temporary memory will not be
exhausted during the whole PEI phase so there won't be an system crash
during S3 resume caused by this.
The size of stack in temporary memory is 32K, when in permanent memory it
is 128K. This approximately simulate the scene in real machine.
Logically the
stack should be switched to permanent memory when the latter becomes
available.
Since hardware protection mechanism for stack is disabled, currently using
larger stack can reduced the possibility of stack overflow, thus, the
possibility
of crash during S3 resume
Check the GDT set up by reset vector module:
GDT_BASE:
; null descriptor
NULL_SEL equ $-GDT_BASE
DW 0 ; limit 15:0
DW 0 ; base 15:0
DB 0 ; base 23:16
DB 0 ; sys flag, dpl, type
DB 0 ; limit 19:16, flags
DB 0 ; base 31:24
; linear data segment descriptor
LINEAR_SEL equ $-GDT_BASE
DW 0xffff ; limit 15:0
DW 0 ; base 15:0
DB 0 ; base 23:16
DB PRESENT_FLAG(1)|DPL(0)|SYSTEM_FLAG(1)|DESC_TYPE(DATA32_TYPE)
DB
GRANULARITY_FLAG(1)|DEFAULT_SIZE32(1)|CODE64_FLAG(0)|UPPER_LIMIT(0xf)
DB 0 ; base 31:24
; linear code segment descriptor
LINEAR_CODE_SEL equ $-GDT_BASE
DW 0xffff ; limit 15:0
DW 0 ; base 15:0
DB 0 ; base 23:16
DB PRESENT_FLAG(1)|DPL(0)|SYSTEM_FLAG(1)|DESC_TYPE(CODE32_TYPE)
DB
GRANULARITY_FLAG(1)|DEFAULT_SIZE32(1)|CODE64_FLAG(0)|UPPER_LIMIT(0xf)
DB 0 ; base 31:24
%ifdef ARCH_X64
; linear code (64-bit) segment descriptor
LINEAR_CODE64_SEL equ $-GDT_BASE
DW 0xffff ; limit 15:0
DW 0 ; base 15:0
DB 0 ; base 23:16
DB PRESENT_FLAG(1)|DPL(0)|SYSTEM_FLAG(1)|DESC_TYPE(CODE64_TYPE)
DB
GRANULARITY_FLAG(1)|DEFAULT_SIZE32(0)|CODE64_FLAG(1)|UPPER_LIMIT(0xf)
DB 0 ; base 31:24
%endif
GDT_END:
SS is set to use LINEAR_SEL descriptor, actually stack protection
provided by hardware is disabled
BTW, is it better to utilize the hardware mechanism to set up stack
overflow protection in both
stage(pre-memory and post-memory)?
>
> However, if the bug was caught during normal boot, then I don't know
> what the symptom may have been. The permanent PEI RAM for normal boot is
> pretty close to the top of low RAM (and the default RAM size for QEMU is
> 128MB). The temporary stack is very low however ("Temp Stack :
> BaseAddress=0x814000 Length=0x4000"). So even if we overflow the
> temporary stack (i.e. go below the address 0x814000) during normal boot,
> I don't think anything allocated from the permanent PEI RAM could be
> corrupted by that.
I strongly agree with you. Normal boot shouldn't be affected even stack
overflows.
> I'll do some testing in the next few days.
>
> The analysis and the patch look great (and impressive!) to me. I'd like
> the commit message to be cleaned up a bit -- please see my notes above,
> and please describe:
>
> - practical consequences of the bug,
>
> - how the bug was caught.
>
> I have one more (superficial) comment below:
>
>> 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\x19s Manual, Volume 2A
> I prefer keeping commit messages ASCII-clean.
>
> Thanks
> Laszlo
>
>> "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\x19s 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);
>> }
>>
On 09/04/2017 04:49 AM, Laszlo Ersek wrote:
> On 09/03/17 04:12, Ge Song wrote:
>> In earlier PEI stage, temporary memory(Cache As Ram) is employed as stack
> Please remove the "Cache As Ram" reference; it doesn't apply to OVMF.
>
> I suggest "temporary memory at PcdOvmfSecPeiTempRamBase"
>
>> 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(),
> I suggest rewording this as "before entering TemporaryRamMigration()" --
> when I read the above, I went looking for Ebp/Rbp manipulation within
> 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
> Please start a new sentence with "after the execution of ...".
>
>> 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.
> s/momery/memory/
>
>
> But, more importantly, what are the practical consequences of this bug?
>
> Does it mean that the temporary SEC/PEI stack is used until the end of
> PEI, even after permanent RAM is installed, during both normal boot and
> S3 resume?
>
> If that's the case, I wonder how it was caught.
>
> If it was caught during S3 resume, then the only symptom could have been
> stack overflow. Because during normal boot, both temporary and permanent
> PEI RAM are reserved, so even if we stay on the temporary stack during
> S3, if we don't overflow it, we cannot corrupt data:
>
>> /**
>> Publish system RAM and reserve memory regions
>>
>> **/
>> VOID
>> InitializeRamRegions (
>> VOID
>> )
>> {
>> if (!mXen) {
>> QemuInitializeRam ();
>> } else {
>> XenPublishRamRegions ();
>> }
>>
>> if (mS3Supported && mBootMode != BOOT_ON_S3_RESUME) {
>> //
>> // This is the memory range that will be used for PEI on S3 resume
>> //
>> BuildMemoryAllocationHob (
>> mS3AcpiReservedMemoryBase,
>> mS3AcpiReservedMemorySize,
>> EfiACPIMemoryNVS
>> );
>>
>> //
>> // Cover the initial RAM area used as stack and temporary PEI heap.
>> //
>> // This is reserved as ACPI NVS so it can be used on S3 resume.
>> //
>> BuildMemoryAllocationHob (
>> PcdGet32 (PcdOvmfSecPeiTempRamBase),
>> PcdGet32 (PcdOvmfSecPeiTempRamSize),
>> EfiACPIMemoryNVS
>> );
> With stack overflow during S3 resume, we may have corrupted OS data
> residing very low.
>
> However, if the bug was caught during normal boot, then I don't know
> what the symptom may have been. The permanent PEI RAM for normal boot is
> pretty close to the top of low RAM (and the default RAM size for QEMU is
> 128MB). The temporary stack is very low however ("Temp Stack :
> BaseAddress=0x814000 Length=0x4000"). So even if we overflow the
> temporary stack (i.e. go below the address 0x814000) during normal boot,
> I don't think anything allocated from the permanent PEI RAM could be
> corrupted by that.
>
> I'll do some testing in the next few days.
>
> The analysis and the patch look great (and impressive!) to me. I'd like
> the commit message to be cleaned up a bit -- please see my notes above,
> and please describe:
>
> - practical consequences of the bug,
>
> - how the bug was caught.
>
> I have one more (superficial) comment below:
>
>> 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\x19s Manual, Volume 2A
> I prefer keeping commit messages ASCII-clean.
>
> Thanks
> Laszlo
>
>> "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\x19s 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);
>> }
>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/1] OvmfPkg/SecMain: Fix stack switching to permanent memory fail
2017-09-03 20:55 ` Laszlo Ersek
@ 2017-09-04 13:34 ` Jordan Justen
2017-09-04 15:11 ` Ge Song
0 siblings, 1 reply; 15+ messages in thread
From: Jordan Justen @ 2017-09-04 13:34 UTC (permalink / raw)
To: Ge Song, Laszlo Ersek, edk2-devel; +Cc: Ard Biesheuvel
On 2017-09-03 16:55:36, Laszlo Ersek wrote:
> On 09/03/17 04:12, 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.
>
> [...]
>
> > 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(+)
>
> (No more separate emails, I promise.)
>
> Before you send v2, can you please amend the patch like this:
>
> git commit --amend --author='Ge Song <ge.song@hxt-semitech.com>'
>
> It's OK if you mail it out from another email address, but IMO the git
> authorship should match the first Signed-off-by.
I disagree with this. Unless the individual is well known to the EDK
II community as the owner of both email addresses, I think the author
needs to send it from their author email.
I suppose they can send it from another email, and then reply from
their other email saying they contributed it under the Tianocore
Contribution Agreement, but that sounds like a stretch...
-Jordan
> The commit message body currently does not start with "From: Ge Song
> <ge.song@hxt-semitech.com>", which means that the git authorship and the
> first S-o-b disagree at the moment.
>
> ... If I'm wrong about this, I can be convinced, of course.
>
> Thanks
> Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/1] OvmfPkg/SecMain: Fix stack switching to permanent memory fail
2017-09-04 13:34 ` Jordan Justen
@ 2017-09-04 15:11 ` Ge Song
2017-09-04 21:25 ` Laszlo Ersek
0 siblings, 1 reply; 15+ messages in thread
From: Ge Song @ 2017-09-04 15:11 UTC (permalink / raw)
To: Jordan Justen, Laszlo Ersek, edk2-devel; +Cc: Ard Biesheuvel
On 09/04/2017 09:34 PM, Jordan Justen wrote:
> On 2017-09-03 16:55:36, Laszlo Ersek wrote:
>> On 09/03/17 04:12, 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.
>> [...]
>>
>>> 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(+)
>> (No more separate emails, I promise.)
>>
>> Before you send v2, can you please amend the patch like this:
>>
>> git commit --amend --author='Ge Song <ge.song@hxt-semitech.com>'
>>
>> It's OK if you mail it out from another email address, but IMO the git
>> authorship should match the first Signed-off-by.
> I disagree with this. Unless the individual is well known to the EDK
> II community as the owner of both email addresses, I think the author
> needs to send it from their author email.
>
> I suppose they can send it from another email, and then reply from
> their other email saying they contributed it under the Tianocore
> Contribution Agreement, but that sounds like a stretch...
>
> -Jordan
OK, I'll try to send it from the same email address next time, the reason
that using two different mail addresses is because the author email is an
exchange account and it's hard to find the mail server preferences for
git-send-email...
>
>> The commit message body currently does not start with "From: Ge Song
>> <ge.song@hxt-semitech.com>", which means that the git authorship and the
>> first S-o-b disagree at the moment.
>>
>> ... If I'm wrong about this, I can be convinced, of course.
>>
>> Thanks
>> Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/1] OvmfPkg/SecMain: Fix stack switching to permanent memory fail
2017-09-04 15:11 ` Ge Song
@ 2017-09-04 21:25 ` Laszlo Ersek
2017-09-05 17:45 ` Jordan Justen
0 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2017-09-04 21:25 UTC (permalink / raw)
To: Ge Song, Jordan Justen, edk2-devel; +Cc: Ard Biesheuvel
On 09/04/17 17:11, Ge Song wrote:
> On 09/04/2017 09:34 PM, Jordan Justen wrote:
>
>> On 2017-09-03 16:55:36, Laszlo Ersek wrote:
>>> On 09/03/17 04:12, 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.
>>> [...]
>>>
>>>> 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(+)
>>> (No more separate emails, I promise.)
>>>
>>> Before you send v2, can you please amend the patch like this:
>>>
>>> git commit --amend --author='Ge Song <ge.song@hxt-semitech.com>'
>>>
>>> It's OK if you mail it out from another email address, but IMO the git
>>> authorship should match the first Signed-off-by.
>> I disagree with this. Unless the individual is well known to the EDK
>> II community as the owner of both email addresses, I think the author
>> needs to send it from their author email.
>>
>> I suppose they can send it from another email, and then reply from
>> their other email saying they contributed it under the Tianocore
>> Contribution Agreement, but that sounds like a stretch...
I'm fine either way, as long as we can establish some kind of agreement
between (two of?) the three email addresses (sender, git author, S-o-b).
Thanks,
Laszlo
> OK, I'll try to send it from the same email address next time, the reason
> that using two different mail addresses is because the author email is an
> exchange account and it's hard to find the mail server preferences for
> git-send-email...
>
>>
>>> The commit message body currently does not start with "From: Ge Song
>>> <ge.song@hxt-semitech.com>", which means that the git authorship and the
>>> first S-o-b disagree at the moment.
>>>
>>> ... If I'm wrong about this, I can be convinced, of course.
>>>
>>> Thanks
>>> Laszlo
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/1] OvmfPkg/SecMain: Fix stack switching to permanent memory fail
2017-09-03 20:49 ` Laszlo Ersek
2017-09-04 8:54 ` Ge Song
@ 2017-09-04 22:26 ` Laszlo Ersek
1 sibling, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2017-09-04 22:26 UTC (permalink / raw)
To: Ge Song, edk2-devel; +Cc: Jordan Justen, Ard Biesheuvel
On 09/03/17 22:49, Laszlo Ersek wrote:
> I'll do some testing in the next few days.
Attempted to do it now, but the patch doesn't apply. I tried both
git am --keep-cr ...
and
git am --no-keep-cr ...
However, I managed to fetch the patch from the branch noted in the blurb.
I tested the patch with normal boot and S3 resume:
- IA32 build, Q35, SMM
- IA32X64 build, Q35, SMM
- X64 build, i440fx, no SMM
Experienced no regressions. Please add the following to your v2:
Tested-by: Laszlo Ersek <lersek@redhat.com>
Also, contingent upon the requested updates to the commit message:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Jordan, could you please push Ge Song's upcoming v2, if you are
satisfied with it?
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/1] OvmfPkg/SecMain: Fix stack switching to permanent memory fail
2017-09-04 21:25 ` Laszlo Ersek
@ 2017-09-05 17:45 ` Jordan Justen
2017-09-05 18:03 ` Leif Lindholm
0 siblings, 1 reply; 15+ messages in thread
From: Jordan Justen @ 2017-09-05 17:45 UTC (permalink / raw)
To: Ge Song, Laszlo Ersek, edk2-devel
Cc: Ard Biesheuvel, Kinney, Michael D, Andrew Fish, Leif Lindholm
On 2017-09-04 14:25:48, Laszlo Ersek wrote:
> On 09/04/17 17:11, Ge Song wrote:
> > On 09/04/2017 09:34 PM, Jordan Justen wrote:
> >
> >> On 2017-09-03 16:55:36, Laszlo Ersek wrote:
> >>> On 09/03/17 04:12, 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.
> >>> [...]
> >>>
> >>>> 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(+)
> >>> (No more separate emails, I promise.)
> >>>
> >>> Before you send v2, can you please amend the patch like this:
> >>>
> >>> git commit --amend --author='Ge Song <ge.song@hxt-semitech.com>'
> >>>
> >>> It's OK if you mail it out from another email address, but IMO the git
> >>> authorship should match the first Signed-off-by.
> >> I disagree with this. Unless the individual is well known to the EDK
> >> II community as the owner of both email addresses, I think the author
> >> needs to send it from their author email.
> >>
> >> I suppose they can send it from another email, and then reply from
> >> their other email saying they contributed it under the Tianocore
> >> Contribution Agreement, but that sounds like a stretch...
>
> I'm fine either way, as long as we can establish some kind of agreement
> between (two of?) the three email addresses (sender, git author, S-o-b).
We've generally said they need to match. If it is a one-time
contribution, then I think it is acceptable for the contributor to
reply to the patch from the author's email saying: "This is me. I
contributed it under the Tiancore Contribution Agreement"
I think any change from this policy should be decided by the stewards.
Regarding Ge Song's contribution, I think things are now even more
complicated. It sounds like the contribution is being made by
ge.song@hxt-semitech.com, but due to "IT issues" v2 is now being
authored/sent from songgebird@gmail.com.
Does hxt-semitech.com agree to this contribution? I think given that
we know ge.song@hxt-semitech.com is the intended author, the patch
should have ge.song@hxt-semitech.com as the author with
ge.song@hxt-semitech.com in the Signed-off-by.
Ideally it should be sent from ge.song@hxt-semitech.com, but if that
is not possible, then ge.song@hxt-semitech.com should reply to the
patch sent by songgebird@gmail.com re-iterating the
Signed-off-by/Contributed-under is from ge.song@hxt-semitech.com.
Ge Song, Can you send a v3 with the author reset to
ge.song@hxt-semitech.com (see lersek's git commit command above) and
the Signed-off-by using ge.song@hxt-semitech.com, and then reply to
the patch posting from ge.song@hxt-semitech.com to re-iterate the
Signed-off-by and Contributed-under?
> > OK, I'll try to send it from the same email address next time, the reason
> > that using two different mail addresses is because the author email is an
> > exchange account and it's hard to find the mail server preferences for
> > git-send-email...
Sorry for the hassle, but maybe if you are able to work with your IT
department to setup git send-email then you'll be able to contribute
to other open source project's easier from your
ge.song@hxt-semitech.com email address.
-Jordan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/1] OvmfPkg/SecMain: Fix stack switching to permanent memory fail
2017-09-05 17:45 ` Jordan Justen
@ 2017-09-05 18:03 ` Leif Lindholm
2017-09-07 1:42 ` Songge
0 siblings, 1 reply; 15+ messages in thread
From: Leif Lindholm @ 2017-09-05 18:03 UTC (permalink / raw)
To: Jordan Justen
Cc: Ge Song, Laszlo Ersek, edk2-devel, Ard Biesheuvel,
Kinney, Michael D, Andrew Fish
On Tue, Sep 05, 2017 at 10:45:57AM -0700, Jordan Justen wrote:
> On 2017-09-04 14:25:48, Laszlo Ersek wrote:
> > On 09/04/17 17:11, Ge Song wrote:
> > > On 09/04/2017 09:34 PM, Jordan Justen wrote:
> > >
> > >> On 2017-09-03 16:55:36, Laszlo Ersek wrote:
> > >>> On 09/03/17 04:12, 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.
> > >>> [...]
> > >>>
> > >>>> 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(+)
> > >>> (No more separate emails, I promise.)
> > >>>
> > >>> Before you send v2, can you please amend the patch like this:
> > >>>
> > >>> git commit --amend --author='Ge Song <ge.song@hxt-semitech.com>'
> > >>>
> > >>> It's OK if you mail it out from another email address, but IMO the git
> > >>> authorship should match the first Signed-off-by.
> > >> I disagree with this. Unless the individual is well known to the EDK
> > >> II community as the owner of both email addresses, I think the author
> > >> needs to send it from their author email.
> > >>
> > >> I suppose they can send it from another email, and then reply from
> > >> their other email saying they contributed it under the Tianocore
> > >> Contribution Agreement, but that sounds like a stretch...
> >
> > I'm fine either way, as long as we can establish some kind of agreement
> > between (two of?) the three email addresses (sender, git author, S-o-b).
>
> We've generally said they need to match. If it is a one-time
> contribution, then I think it is acceptable for the contributor to
> reply to the patch from the author's email saying: "This is me. I
> contributed it under the Tiancore Contribution Agreement"
>
> I think any change from this policy should be decided by the stewards.
>
> Regarding Ge Song's contribution, I think things are now even more
> complicated. It sounds like the contribution is being made by
> ge.song@hxt-semitech.com, but due to "IT issues" v2 is now being
> authored/sent from songgebird@gmail.com.
>
> Does hxt-semitech.com agree to this contribution? I think given that
> we know ge.song@hxt-semitech.com is the intended author, the patch
> should have ge.song@hxt-semitech.com as the author with
> ge.song@hxt-semitech.com in the Signed-off-by.
>
> Ideally it should be sent from ge.song@hxt-semitech.com, but if that
> is not possible, then ge.song@hxt-semitech.com should reply to the
> patch sent by songgebird@gmail.com re-iterating the
> Signed-off-by/Contributed-under is from ge.song@hxt-semitech.com.
>
> Ge Song, Can you send a v3 with the author reset to
> ge.song@hxt-semitech.com (see lersek's git commit command above) and
> the Signed-off-by using ge.song@hxt-semitech.com, and then reply to
> the patch posting from ge.song@hxt-semitech.com to re-iterate the
> Signed-off-by and Contributed-under?
While obviously not an ideal situation, it is not the first time I
have come across this sort of issue. (And I doubt it will be the
last.)
Your suggestion sounds like a good workaround.
For a series, a reply to the cover-letter should be sufficient.
> > > OK, I'll try to send it from the same email address next time, the reason
> > > that using two different mail addresses is because the author email is an
> > > exchange account and it's hard to find the mail server preferences for
> > > git-send-email...
>
> Sorry for the hassle, but maybe if you are able to work with your IT
> department to setup git send-email then you'll be able to contribute
> to other open source project's easier from your
> ge.song@hxt-semitech.com email address.
In my experience, getting IT departments on board with this sort of
thing can be very difficult. Clearly, it should be attempted.
Since hxt-semtech.com is a Linaro member, I will try to raise the
importance of getting this situation resolved internally.
/
Leif
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v1 1/1] OvmfPkg/SecMain: Fix stack switching to permanent memory fail
2017-09-05 18:03 ` Leif Lindholm
@ 2017-09-07 1:42 ` Songge
0 siblings, 0 replies; 15+ messages in thread
From: Songge @ 2017-09-07 1:42 UTC (permalink / raw)
To: Leif Lindholm
Cc: Jordan Justen, Laszlo Ersek, edk2-devel, Ard Biesheuvel,
Kinney, Michael D, Andrew Fish
2017-09-06 2:03 GMT+08:00 Leif Lindholm <leif.lindholm@linaro.org>:
> On Tue, Sep 05, 2017 at 10:45:57AM -0700, Jordan Justen wrote:
> > On 2017-09-04 14:25:48, Laszlo Ersek wrote:
> > > On 09/04/17 17:11, Ge Song wrote:
> > > > On 09/04/2017 09:34 PM, Jordan Justen wrote:
> > > >
> > > >> On 2017-09-03 16:55:36, Laszlo Ersek wrote:
> > > >>> On 09/03/17 04:12, 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.
> > > >>> [...]
> > > >>>
> > > >>>> 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(+)
> > > >>> (No more separate emails, I promise.)
> > > >>>
> > > >>> Before you send v2, can you please amend the patch like this:
> > > >>>
> > > >>> git commit --amend --author='Ge Song <ge.song@hxt-semitech.com
> >'
> > > >>>
> > > >>> It's OK if you mail it out from another email address, but IMO the
> git
> > > >>> authorship should match the first Signed-off-by.
> > > >> I disagree with this. Unless the individual is well known to the EDK
> > > >> II community as the owner of both email addresses, I think the
> author
> > > >> needs to send it from their author email.
> > > >>
> > > >> I suppose they can send it from another email, and then reply from
> > > >> their other email saying they contributed it under the Tianocore
> > > >> Contribution Agreement, but that sounds like a stretch...
> > >
> > > I'm fine either way, as long as we can establish some kind of agreement
> > > between (two of?) the three email addresses (sender, git author,
> S-o-b).
> >
> > We've generally said they need to match. If it is a one-time
> > contribution, then I think it is acceptable for the contributor to
> > reply to the patch from the author's email saying: "This is me. I
> > contributed it under the Tiancore Contribution Agreement"
> >
> > I think any change from this policy should be decided by the stewards.
> >
> > Regarding Ge Song's contribution, I think things are now even more
> > complicated. It sounds like the contribution is being made by
> > ge.song@hxt-semitech.com, but due to "IT issues" v2 is now being
> > authored/sent from songgebird@gmail.com.
> >
> > Does hxt-semitech.com agree to this contribution? I think given that
> > we know ge.song@hxt-semitech.com is the intended author, the patch
> > should have ge.song@hxt-semitech.com as the author with
> > ge.song@hxt-semitech.com in the Signed-off-by.
> >
> > Ideally it should be sent from ge.song@hxt-semitech.com, but if that
> > is not possible, then ge.song@hxt-semitech.com should reply to the
> > patch sent by songgebird@gmail.com re-iterating the
> > Signed-off-by/Contributed-under is from ge.song@hxt-semitech.com.
> >
> > Ge Song, Can you send a v3 with the author reset to
> > ge.song@hxt-semitech.com (see lersek's git commit command above) and
> > the Signed-off-by using ge.song@hxt-semitech.com, and then reply to
> > the patch posting from ge.song@hxt-semitech.com to re-iterate the
> > Signed-off-by and Contributed-under?
>
> While obviously not an ideal situation, it is not the first time I
> have come across this sort of issue. (And I doubt it will be the
> last.)
>
> Your suggestion sounds like a good workaround.
> For a series, a reply to the cover-letter should be sufficient.
>
> > > > OK, I'll try to send it from the same email address next time, the
> reason
> > > > that using two different mail addresses is because the author email
> is an
> > > > exchange account and it's hard to find the mail server preferences
> for
> > > > git-send-email...
> >
> > Sorry for the hassle, but maybe if you are able to work with your IT
> > department to setup git send-email then you'll be able to contribute
> > to other open source project's easier from your
> > ge.song@hxt-semitech.com email address.
>
> In my experience, getting IT departments on board with this sort of
> thing can be very difficult. Clearly, it should be attempted.
>
> Since hxt-semtech.com is a Linaro member, I will try to raise the
> importance of getting this situation resolved internally.
>
> /
> Leif
>
I have raised the problem to our IT department, As Leif said, this supposed
to be a tough process, hope they can solve it finally
Thanks to Jordan for the great solution. And this discussion made me gain
a deep understanding of stewards, thank you everyone
--
Best Regards,
Songge
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-09-07 1:39 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-09-03 2:12 [PATCH v1 0/1] Fix stack switching to permanent memory fail Ge Song
2017-09-03 2:12 ` [PATCH v1 1/1] OvmfPkg/SecMain: " Ge Song
2017-09-03 4:37 ` Jordan Justen
2017-09-03 20:50 ` Laszlo Ersek
2017-09-03 20:49 ` Laszlo Ersek
2017-09-04 8:54 ` Ge Song
2017-09-04 22:26 ` Laszlo Ersek
2017-09-03 20:52 ` Laszlo Ersek
2017-09-03 20:55 ` Laszlo Ersek
2017-09-04 13:34 ` Jordan Justen
2017-09-04 15:11 ` Ge Song
2017-09-04 21:25 ` Laszlo Ersek
2017-09-05 17:45 ` Jordan Justen
2017-09-05 18:03 ` Leif Lindholm
2017-09-07 1:42 ` Songge
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox