* [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit @ 2023-10-31 8:22 Ni, Ray 2023-11-03 19:25 ` Nate DeSimone 2023-11-03 20:39 ` Nate DeSimone 0 siblings, 2 replies; 6+ messages in thread From: Ni, Ray @ 2023-10-31 8:22 UTC (permalink / raw) To: devel; +Cc: Chasel Chiu When FSP runs in API mode, it saves the IDTR in its own stack then switches to bootloader's stack before it returns from FspMemoryInit. Next time when the bootloader calls TempRamExit, FSP switches to its own stack and restores IDTR from its stack saved earlier. However, due to a bug in BaseFspSwitchStackLib, the IDTR saved on FSP's stack might be corrupted that results the following TempRamExit call fails inside FSP due to PeiServices pointer cannot be retrieved from IDT.base - 8. The bug is the assembly code doesn't reserve 32 bytes before calling the C routine in 64bit. According to the x86-64 calling convention, caller is responsible for allocating 32 bytes of "shadow space" on the stack right before calling the function (regardless of the actual number of parameters used). When FSP is built in optimization-off mode, the C routine makes use of the 32-byte "shadow space" which is not reserved by the assembly caller. That causes the IDTR saved on the stack is corrupted by the C routine. The patch fixes so by reserving the 32 bytes before calling C routine. Signed-off-by: Ray Ni <ray.ni@intel.com> Cc: Chasel Chiu <chasel.chiu@intel.com> M: Nate DeSimone <nathaniel.l.desimone@intel.com> M: Duggapu Chinni B <chinni.b.duggapu@intel.com> M: Ray Han Lim Ng <ray.han.lim.ng@intel.com> R: Star Zeng <star.zeng@intel.com> R: Ted Kuo <ted.kuo@intel.com> R: Ashraf Ali S <ashraf.ali.s@intel.com> R: Susovan Mohapatra <susovan.mohapatra@intel.com> --- IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm index 1ea1220608..e3a7cf002f 100644 --- a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm +++ b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm @@ -1,6 +1,6 @@ ;------------------------------------------------------------------------------ ; -; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> +; Copyright (c) 2022 - 2023, Intel Corporation. All rights reserved.<BR> ; SPDX-License-Identifier: BSD-2-Clause-Patent ; ; Abstract: @@ -60,7 +60,9 @@ ASM_PFX(FspSwitchStack): ; Load new stack mov rcx, rsp + sub rsp, 0x20 call ASM_PFX(SwapStack) + add rsp, 0x20 mov rsp, rax ; Restore previous contexts -- 2.39.1.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110384): https://edk2.groups.io/g/devel/message/110384 Mute This Topic: https://groups.io/mt/102293342/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit 2023-10-31 8:22 [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit Ni, Ray @ 2023-11-03 19:25 ` Nate DeSimone 2023-11-03 20:39 ` Nate DeSimone 1 sibling, 0 replies; 6+ messages in thread From: Nate DeSimone @ 2023-11-03 19:25 UTC (permalink / raw) To: devel@edk2.groups.io, Ni, Ray; +Cc: Chiu, Chasel Good catch! Reviewed-by: Nate DeSimone <nathaniel.l.desimone@intel.com> -----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray Sent: Tuesday, October 31, 2023 1:22 AM To: devel@edk2.groups.io Cc: Chiu, Chasel <chasel.chiu@intel.com> Subject: [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit When FSP runs in API mode, it saves the IDTR in its own stack then switches to bootloader's stack before it returns from FspMemoryInit. Next time when the bootloader calls TempRamExit, FSP switches to its own stack and restores IDTR from its stack saved earlier. However, due to a bug in BaseFspSwitchStackLib, the IDTR saved on FSP's stack might be corrupted that results the following TempRamExit call fails inside FSP due to PeiServices pointer cannot be retrieved from IDT.base - 8. The bug is the assembly code doesn't reserve 32 bytes before calling the C routine in 64bit. According to the x86-64 calling convention, caller is responsible for allocating 32 bytes of "shadow space" on the stack right before calling the function (regardless of the actual number of parameters used). When FSP is built in optimization-off mode, the C routine makes use of the 32-byte "shadow space" which is not reserved by the assembly caller. That causes the IDTR saved on the stack is corrupted by the C routine. The patch fixes so by reserving the 32 bytes before calling C routine. Signed-off-by: Ray Ni <ray.ni@intel.com> Cc: Chasel Chiu <chasel.chiu@intel.com> M: Nate DeSimone <nathaniel.l.desimone@intel.com> M: Duggapu Chinni B <chinni.b.duggapu@intel.com> M: Ray Han Lim Ng <ray.han.lim.ng@intel.com> R: Star Zeng <star.zeng@intel.com> R: Ted Kuo <ted.kuo@intel.com> R: Ashraf Ali S <ashraf.ali.s@intel.com> R: Susovan Mohapatra <susovan.mohapatra@intel.com> --- IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm index 1ea1220608..e3a7cf002f 100644 --- a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm +++ b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm @@ -1,6 +1,6 @@ ;------------------------------------------------------------------------------ ;-; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>+; Copyright (c) 2022 - 2023, Intel Corporation. All rights reserved.<BR> ; SPDX-License-Identifier: BSD-2-Clause-Patent ; ; Abstract:@@ -60,7 +60,9 @@ ASM_PFX(FspSwitchStack): ; Load new stack mov rcx, rsp+ sub rsp, 0x20 call ASM_PFX(SwapStack)+ add rsp, 0x20 mov rsp, rax ; Restore previous contexts-- 2.39.1.windows.1 -=-=-=-=-=-= Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110384): https://edk2.groups.io/g/devel/message/110384 Mute This Topic: https://groups.io/mt/102293342/1767664 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/3861758/1767664/1187970101/xyzzy [nathaniel.l.desimone@intel.com] -=-=-=-=-=-= -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110667): https://edk2.groups.io/g/devel/message/110667 Mute This Topic: https://groups.io/mt/102293342/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit 2023-10-31 8:22 [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit Ni, Ray 2023-11-03 19:25 ` Nate DeSimone @ 2023-11-03 20:39 ` Nate DeSimone 1 sibling, 0 replies; 6+ messages in thread From: Nate DeSimone @ 2023-11-03 20:39 UTC (permalink / raw) To: devel@edk2.groups.io, Ni, Ray; +Cc: Chiu, Chasel Pushed as 0b4acb8. -----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray Sent: Tuesday, October 31, 2023 1:22 AM To: devel@edk2.groups.io Cc: Chiu, Chasel <chasel.chiu@intel.com> Subject: [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit When FSP runs in API mode, it saves the IDTR in its own stack then switches to bootloader's stack before it returns from FspMemoryInit. Next time when the bootloader calls TempRamExit, FSP switches to its own stack and restores IDTR from its stack saved earlier. However, due to a bug in BaseFspSwitchStackLib, the IDTR saved on FSP's stack might be corrupted that results the following TempRamExit call fails inside FSP due to PeiServices pointer cannot be retrieved from IDT.base - 8. The bug is the assembly code doesn't reserve 32 bytes before calling the C routine in 64bit. According to the x86-64 calling convention, caller is responsible for allocating 32 bytes of "shadow space" on the stack right before calling the function (regardless of the actual number of parameters used). When FSP is built in optimization-off mode, the C routine makes use of the 32-byte "shadow space" which is not reserved by the assembly caller. That causes the IDTR saved on the stack is corrupted by the C routine. The patch fixes so by reserving the 32 bytes before calling C routine. Signed-off-by: Ray Ni <ray.ni@intel.com> Cc: Chasel Chiu <chasel.chiu@intel.com> M: Nate DeSimone <nathaniel.l.desimone@intel.com> M: Duggapu Chinni B <chinni.b.duggapu@intel.com> M: Ray Han Lim Ng <ray.han.lim.ng@intel.com> R: Star Zeng <star.zeng@intel.com> R: Ted Kuo <ted.kuo@intel.com> R: Ashraf Ali S <ashraf.ali.s@intel.com> R: Susovan Mohapatra <susovan.mohapatra@intel.com> --- IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm index 1ea1220608..e3a7cf002f 100644 --- a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm +++ b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm @@ -1,6 +1,6 @@ ;------------------------------------------------------------------------------ ;-; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR>+; Copyright (c) 2022 - 2023, Intel Corporation. All rights reserved.<BR> ; SPDX-License-Identifier: BSD-2-Clause-Patent ; ; Abstract:@@ -60,7 +60,9 @@ ASM_PFX(FspSwitchStack): ; Load new stack mov rcx, rsp+ sub rsp, 0x20 call ASM_PFX(SwapStack)+ add rsp, 0x20 mov rsp, rax ; Restore previous contexts-- 2.39.1.windows.1 -=-=-=-=-=-= Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110384): https://edk2.groups.io/g/devel/message/110384 Mute This Topic: https://groups.io/mt/102293342/1767664 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/3861758/1767664/1187970101/xyzzy [nathaniel.l.desimone@intel.com] -=-=-=-=-=-= -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110668): https://edk2.groups.io/g/devel/message/110668 Mute This Topic: https://groups.io/mt/102293342/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 6+ messages in thread
[parent not found: <1793238D7061082B.20486@groups.io>]
* Re: [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit [not found] <1793238D7061082B.20486@groups.io> @ 2023-10-31 8:26 ` Ni, Ray 2023-10-31 8:35 ` Kuo, Ted 0 siblings, 1 reply; 6+ messages in thread From: Ni, Ray @ 2023-10-31 8:26 UTC (permalink / raw) To: devel@edk2.groups.io, Ni, Ray Cc: Chiu, Chasel, Desimone, Nathaniel L, Duggapu, Chinni B, Ng, Ray Han Lim, Zeng, Star, Kuo, Ted, S, Ashraf Ali, Mohapatra, Susovan [-- Attachment #1: Type: text/plain, Size: 3865 bytes --] Sorry, I copied the maintainers from Maintainers.txt but forgot to change all M/R to "Cc". That caused not all the maintainers/reviewers are CCed. I will fix the commit message before merging. Thanks, Ray ________________________________ From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Ni, Ray <ray.ni@intel.com> Sent: Tuesday, October 31, 2023 4:22 PM To: devel@edk2.groups.io <devel@edk2.groups.io> Cc: Chiu, Chasel <chasel.chiu@intel.com> Subject: [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit When FSP runs in API mode, it saves the IDTR in its own stack then switches to bootloader's stack before it returns from FspMemoryInit. Next time when the bootloader calls TempRamExit, FSP switches to its own stack and restores IDTR from its stack saved earlier. However, due to a bug in BaseFspSwitchStackLib, the IDTR saved on FSP's stack might be corrupted that results the following TempRamExit call fails inside FSP due to PeiServices pointer cannot be retrieved from IDT.base - 8. The bug is the assembly code doesn't reserve 32 bytes before calling the C routine in 64bit. According to the x86-64 calling convention, caller is responsible for allocating 32 bytes of "shadow space" on the stack right before calling the function (regardless of the actual number of parameters used). When FSP is built in optimization-off mode, the C routine makes use of the 32-byte "shadow space" which is not reserved by the assembly caller. That causes the IDTR saved on the stack is corrupted by the C routine. The patch fixes so by reserving the 32 bytes before calling C routine. Signed-off-by: Ray Ni <ray.ni@intel.com> Cc: Chasel Chiu <chasel.chiu@intel.com> M: Nate DeSimone <nathaniel.l.desimone@intel.com> M: Duggapu Chinni B <chinni.b.duggapu@intel.com> M: Ray Han Lim Ng <ray.han.lim.ng@intel.com> R: Star Zeng <star.zeng@intel.com> R: Ted Kuo <ted.kuo@intel.com> R: Ashraf Ali S <ashraf.ali.s@intel.com> R: Susovan Mohapatra <susovan.mohapatra@intel.com> --- IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm index 1ea1220608..e3a7cf002f 100644 --- a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm +++ b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm @@ -1,6 +1,6 @@ ;------------------------------------------------------------------------------ ; -; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> +; Copyright (c) 2022 - 2023, Intel Corporation. All rights reserved.<BR> ; SPDX-License-Identifier: BSD-2-Clause-Patent ; ; Abstract: @@ -60,7 +60,9 @@ ASM_PFX(FspSwitchStack): ; Load new stack mov rcx, rsp + sub rsp, 0x20 call ASM_PFX(SwapStack) + add rsp, 0x20 mov rsp, rax ; Restore previous contexts -- 2.39.1.windows.1 -=-=-=-=-=-= Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110384): https://edk2.groups.io/g/devel/message/110384 Mute This Topic: https://groups.io/mt/102293342/1712937 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/3759105/1712937/893644498/xyzzy [ray.ni@intel.com] -=-=-=-=-=-= -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110385): https://edk2.groups.io/g/devel/message/110385 Mute This Topic: https://groups.io/mt/102293342/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- [-- Attachment #2: Type: text/html, Size: 6707 bytes --] ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit 2023-10-31 8:26 ` Ni, Ray @ 2023-10-31 8:35 ` Kuo, Ted 2023-10-31 8:47 ` Ashraf Ali S 0 siblings, 1 reply; 6+ messages in thread From: Kuo, Ted @ 2023-10-31 8:35 UTC (permalink / raw) To: Ni, Ray, devel@edk2.groups.io Cc: Chiu, Chasel, Desimone, Nathaniel L, Duggapu, Chinni B, Ng, Ray Han Lim, Zeng, Star, S, Ashraf Ali, Mohapatra, Susovan [-- Attachment #1: Type: text/plain, Size: 4969 bytes --] Reviewed-by: Ted Kuo <ted.kuo@intel.com<mailto:ted.kuo@intel.com>> From: Ni, Ray <ray.ni@intel.com> Sent: Tuesday, October 31, 2023 4:26 PM To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Duggapu, Chinni B <chinni.b.duggapu@intel.com>; Ng, Ray Han Lim <ray.han.lim.ng@intel.com>; Zeng, Star <star.zeng@intel.com>; Kuo, Ted <ted.kuo@intel.com>; S, Ashraf Ali <ashraf.ali.s@intel.com>; Mohapatra, Susovan <susovan.mohapatra@intel.com> Subject: Re: [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit Sorry, I copied the maintainers from Maintainers.txt but forgot to change all M/R to "Cc". That caused not all the maintainers/reviewers are CCed. I will fix the commit message before merging. Thanks, Ray ________________________________ From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> on behalf of Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>> Sent: Tuesday, October 31, 2023 4:22 PM To: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> Cc: Chiu, Chasel <chasel.chiu@intel.com<mailto:chasel.chiu@intel.com>> Subject: [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit When FSP runs in API mode, it saves the IDTR in its own stack then switches to bootloader's stack before it returns from FspMemoryInit. Next time when the bootloader calls TempRamExit, FSP switches to its own stack and restores IDTR from its stack saved earlier. However, due to a bug in BaseFspSwitchStackLib, the IDTR saved on FSP's stack might be corrupted that results the following TempRamExit call fails inside FSP due to PeiServices pointer cannot be retrieved from IDT.base - 8. The bug is the assembly code doesn't reserve 32 bytes before calling the C routine in 64bit. According to the x86-64 calling convention, caller is responsible for allocating 32 bytes of "shadow space" on the stack right before calling the function (regardless of the actual number of parameters used). When FSP is built in optimization-off mode, the C routine makes use of the 32-byte "shadow space" which is not reserved by the assembly caller. That causes the IDTR saved on the stack is corrupted by the C routine. The patch fixes so by reserving the 32 bytes before calling C routine. Signed-off-by: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>> Cc: Chasel Chiu <chasel.chiu@intel.com<mailto:chasel.chiu@intel.com>> M: Nate DeSimone <nathaniel.l.desimone@intel.com<mailto:nathaniel.l.desimone@intel.com>> M: Duggapu Chinni B <chinni.b.duggapu@intel.com<mailto:chinni.b.duggapu@intel.com>> M: Ray Han Lim Ng <ray.han.lim.ng@intel.com<mailto:ray.han.lim.ng@intel.com>> R: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>> R: Ted Kuo <ted.kuo@intel.com<mailto:ted.kuo@intel.com>> R: Ashraf Ali S <ashraf.ali.s@intel.com<mailto:ashraf.ali.s@intel.com>> R: Susovan Mohapatra <susovan.mohapatra@intel.com<mailto:susovan.mohapatra@intel.com>> --- IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm index 1ea1220608..e3a7cf002f 100644 --- a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm +++ b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm @@ -1,6 +1,6 @@ ;------------------------------------------------------------------------------ ; -; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> +; Copyright (c) 2022 - 2023, Intel Corporation. All rights reserved.<BR> ; SPDX-License-Identifier: BSD-2-Clause-Patent ; ; Abstract: @@ -60,7 +60,9 @@ ASM_PFX(FspSwitchStack): ; Load new stack mov rcx, rsp + sub rsp, 0x20 call ASM_PFX(SwapStack) + add rsp, 0x20 mov rsp, rax ; Restore previous contexts -- 2.39.1.windows.1 -=-=-=-=-=-= Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110384): https://edk2.groups.io/g/devel/message/110384 Mute This Topic: https://groups.io/mt/102293342/1712937 Group Owner: devel+owner@edk2.groups.io<mailto:devel+owner@edk2.groups.io> Unsubscribe: https://edk2.groups.io/g/devel/leave/3759105/1712937/893644498/xyzzy [ray.ni@intel.com] -=-=-=-=-=-= -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110387): https://edk2.groups.io/g/devel/message/110387 Mute This Topic: https://groups.io/mt/102293342/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- [-- Attachment #2: Type: text/html, Size: 10318 bytes --] ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit 2023-10-31 8:35 ` Kuo, Ted @ 2023-10-31 8:47 ` Ashraf Ali S 0 siblings, 0 replies; 6+ messages in thread From: Ashraf Ali S @ 2023-10-31 8:47 UTC (permalink / raw) To: Kuo, Ted, Ni, Ray, devel@edk2.groups.io Cc: Chiu, Chasel, Desimone, Nathaniel L, Duggapu, Chinni B, Ng, Ray Han Lim, Zeng, Star, Mohapatra, Susovan [-- Attachment #1: Type: text/plain, Size: 5959 bytes --] Reviewed-by: <ashraf.ali.s@intel.com<mailto:ashraf.ali.s@intel.com>>; Thanks., S, Ashraf Ali From: Kuo, Ted <ted.kuo@intel.com> Sent: Tuesday, October 31, 2023 2:06 PM To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Duggapu, Chinni B <chinni.b.duggapu@intel.com>; Ng, Ray Han Lim <ray.han.lim.ng@intel.com>; Zeng, Star <star.zeng@intel.com>; S, Ashraf Ali <ashraf.ali.s@intel.com>; Mohapatra, Susovan <susovan.mohapatra@intel.com> Subject: RE: [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit Reviewed-by: Ted Kuo <ted.kuo@intel.com<mailto:ted.kuo@intel.com>> From: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>> Sent: Tuesday, October 31, 2023 4:26 PM To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>> Cc: Chiu, Chasel <chasel.chiu@intel.com<mailto:chasel.chiu@intel.com>>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com<mailto:nathaniel.l.desimone@intel.com>>; Duggapu, Chinni B <chinni.b.duggapu@intel.com<mailto:chinni.b.duggapu@intel.com>>; Ng, Ray Han Lim <ray.han.lim.ng@intel.com<mailto:ray.han.lim.ng@intel.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>; Kuo, Ted <ted.kuo@intel.com<mailto:ted.kuo@intel.com>>; S, Ashraf Ali <ashraf.ali.s@intel.com<mailto:ashraf.ali.s@intel.com>>; Mohapatra, Susovan <susovan.mohapatra@intel.com<mailto:susovan.mohapatra@intel.com>> Subject: Re: [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit Sorry, I copied the maintainers from Maintainers.txt but forgot to change all M/R to "Cc". That caused not all the maintainers/reviewers are CCed. I will fix the commit message before merging. Thanks, Ray ________________________________ From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> on behalf of Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>> Sent: Tuesday, October 31, 2023 4:22 PM To: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> Cc: Chiu, Chasel <chasel.chiu@intel.com<mailto:chasel.chiu@intel.com>> Subject: [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit When FSP runs in API mode, it saves the IDTR in its own stack then switches to bootloader's stack before it returns from FspMemoryInit. Next time when the bootloader calls TempRamExit, FSP switches to its own stack and restores IDTR from its stack saved earlier. However, due to a bug in BaseFspSwitchStackLib, the IDTR saved on FSP's stack might be corrupted that results the following TempRamExit call fails inside FSP due to PeiServices pointer cannot be retrieved from IDT.base - 8. The bug is the assembly code doesn't reserve 32 bytes before calling the C routine in 64bit. According to the x86-64 calling convention, caller is responsible for allocating 32 bytes of "shadow space" on the stack right before calling the function (regardless of the actual number of parameters used). When FSP is built in optimization-off mode, the C routine makes use of the 32-byte "shadow space" which is not reserved by the assembly caller. That causes the IDTR saved on the stack is corrupted by the C routine. The patch fixes so by reserving the 32 bytes before calling C routine. Signed-off-by: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>> Cc: Chasel Chiu <chasel.chiu@intel.com<mailto:chasel.chiu@intel.com>> M: Nate DeSimone <nathaniel.l.desimone@intel.com<mailto:nathaniel.l.desimone@intel.com>> M: Duggapu Chinni B <chinni.b.duggapu@intel.com<mailto:chinni.b.duggapu@intel.com>> M: Ray Han Lim Ng <ray.han.lim.ng@intel.com<mailto:ray.han.lim.ng@intel.com>> R: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com>> R: Ted Kuo <ted.kuo@intel.com<mailto:ted.kuo@intel.com>> R: Ashraf Ali S <ashraf.ali.s@intel.com<mailto:ashraf.ali.s@intel.com>> R: Susovan Mohapatra <susovan.mohapatra@intel.com<mailto:susovan.mohapatra@intel.com>> --- IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm index 1ea1220608..e3a7cf002f 100644 --- a/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm +++ b/IntelFsp2Pkg/Library/BaseFspSwitchStackLib/X64/Stack.nasm @@ -1,6 +1,6 @@ ;------------------------------------------------------------------------------ ; -; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> +; Copyright (c) 2022 - 2023, Intel Corporation. All rights reserved.<BR> ; SPDX-License-Identifier: BSD-2-Clause-Patent ; ; Abstract: @@ -60,7 +60,9 @@ ASM_PFX(FspSwitchStack): ; Load new stack mov rcx, rsp + sub rsp, 0x20 call ASM_PFX(SwapStack) + add rsp, 0x20 mov rsp, rax ; Restore previous contexts -- 2.39.1.windows.1 -=-=-=-=-=-= Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110384): https://edk2.groups.io/g/devel/message/110384 Mute This Topic: https://groups.io/mt/102293342/1712937 Group Owner: devel+owner@edk2.groups.io<mailto:devel+owner@edk2.groups.io> Unsubscribe: https://edk2.groups.io/g/devel/leave/3759105/1712937/893644498/xyzzy [ray.ni@intel.com] -=-=-=-=-=-= -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110389): https://edk2.groups.io/g/devel/message/110389 Mute This Topic: https://groups.io/mt/102293342/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- [-- Attachment #2: Type: text/html, Size: 12119 bytes --] ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-11-03 20:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-10-31 8:22 [edk2-devel] [PATCH] IntelFsp2Pkg/SwitchStack: Reserve 32B when calling C function in 64bit Ni, Ray
2023-11-03 19:25 ` Nate DeSimone
2023-11-03 20:39 ` Nate DeSimone
[not found] <1793238D7061082B.20486@groups.io>
2023-10-31 8:26 ` Ni, Ray
2023-10-31 8:35 ` Kuo, Ted
2023-10-31 8:47 ` Ashraf Ali S
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox