* Re: [edk2-devel][PATCH] MdeModulePkg: Make RSP 16-byte boundary aligned for PEI 64bit [not found] <16DAF0B16C1B1922.28111@groups.io> @ 2022-03-17 1:04 ` Kuo, Ted 2022-03-17 11:08 ` Marvin Häuser 0 siblings, 1 reply; 4+ messages in thread From: Kuo, Ted @ 2022-03-17 1:04 UTC (permalink / raw) To: devel@edk2.groups.io, Kuo, Ted Cc: Bi, Dandan, Gao, Liming, De, Debkumar, Han, Harry, West, Catharine, Wang, Jian J, S, Ashraf Ali, Kinney, Michael D Hi Liming and Mike, Can you please review the change? Thanks, Ted -----Original Message----- From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kuo, Ted Sent: Thursday, March 10, 2022 2:21 PM To: devel@edk2.groups.io Cc: Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; De, Debkumar <debkumar.de@intel.com>; Han, Harry <harry.han@intel.com>; West, Catharine <catharine.west@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; S, Ashraf Ali <ashraf.ali.s@intel.com> Subject: [edk2-devel][PATCH] MdeModulePkg: Make RSP 16-byte boundary aligned for PEI 64bit REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3865 Use SwitchPeiCore instead of calling PeiCore directly when switching PeiCore from temporary memory to permanent memory. For PEI 32bit, SwitchPeiCore always calls PeiCore without any additional step. For PEI 64bit, SwitchPeiCore makes RSP 16-byte boundary aligned and then allocate 32 bytes as a shadow store on call stack before calling PeiCore. Cc: Dandan Bi <dandan.bi@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Debkumar De <debkumar.de@intel.com> Cc: Harry Han <harry.han@intel.com> Cc: Catharine West <catharine.west@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Ashraf Ali S <ashraf.ali.s@intel.com> Signed-off-by: Ted Kuo <ted.kuo@intel.com> --- MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 2 +- MdeModulePkg/Core/Pei/Ia32/SwitchPeiCore.nasm | 33 +++++++++++++++++++++++ MdeModulePkg/Core/Pei/PeiMain.h | 25 ++++++++++++++++++ MdeModulePkg/Core/Pei/PeiMain.inf | 6 +++++ MdeModulePkg/Core/Pei/X64/SwitchPeiCore.nasm | 38 +++++++++++++++++++++++++++ 5 files changed, 103 insertions(+), 1 deletion(-) create mode 100644 MdeModulePkg/Core/Pei/Ia32/SwitchPeiCore.nasm create mode 100644 MdeModulePkg/Core/Pei/X64/SwitchPeiCore.nasm diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c index 3552feda8f..5af6e6e86f 100644 --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c @@ -871,7 +871,7 @@ PeiCheckAndSwitchStack ( // // Entry PEI Phase 2 // - PeiCore (SecCoreData, NULL, Private); + SwitchPeiCore (SecCoreData, NULL, Private); } else { // // Migrate memory pages allocated in pre-memory phase. diff --git a/MdeModulePkg/Core/Pei/Ia32/SwitchPeiCore.nasm b/MdeModulePkg/Core/Pei/Ia32/SwitchPeiCore.nasm new file mode 100644 index 0000000000..23cfb5090b --- /dev/null +++ b/MdeModulePkg/Core/Pei/Ia32/SwitchPeiCore.nasm @@ -0,0 +1,33 @@ +;---------------------------------------------------------------------- +-------- +; +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> ; +SPDX-License-Identifier: BSD-2-Clause-Patent ; ; Abstract: +; +; Switch PeiCore from temporary memory to permanent memory. +; +;---------------------------------------------------------------------- +-------- + + SECTION .text + +extern ASM_PFX(PeiCore) + +;---------------------------------------------------------------------- +-------- +; VOID +; EFIAPI +; SwitchPeiCore ( +; EFI_SEC_PEI_HAND_OFF *SecCoreDataPtr, +; EFI_PEI_PPI_DESCRIPTOR *PpiList, +; VOID *Data +; ); +;---------------------------------------------------------------------- +-------- +global ASM_PFX(SwitchPeiCore) +ASM_PFX(SwitchPeiCore): + push DWORD [esp + 12] + push DWORD [esp + 12] + push DWORD [esp + 12] + call ASM_PFX(PeiCore) + jmp $ ; Should never reach here + ret + diff --git a/MdeModulePkg/Core/Pei/PeiMain.h b/MdeModulePkg/Core/Pei/PeiMain.h index 556beddad5..8e8ed3dadf 100644 --- a/MdeModulePkg/Core/Pei/PeiMain.h +++ b/MdeModulePkg/Core/Pei/PeiMain.h @@ -2038,4 +2038,29 @@ PeiReinitializeFv ( IN PEI_CORE_INSTANCE *PrivateData ); +/** + This routine is invoked by main entry of PeiMain module during +transition + from temporary memory to permanent memory. + + @param SecCoreDataPtr Points to a data structure containing information about the PEI core's operating + environment, such as the size and location of temporary RAM, the stack location and + the BFV location. + @param PpiList Points to a list of one or more PPI descriptors to be installed initially by the PEI core. + An empty PPI list consists of a single descriptor with the end-tag + EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST. As part of its initialization + phase, the PEI Foundation will add these SEC-hosted PPIs to its PPI database such + that both the PEI Foundation and any modules can leverage the associated service + calls and/or code in these early PPIs + @param Data Pointer to old core data that is used to initialize the + core's data areas. + If NULL, it is first PeiCore entering. + +**/ +VOID +EFIAPI +SwitchPeiCore ( + IN CONST EFI_SEC_PEI_HAND_OFF *SecCoreDataPtr, + IN CONST EFI_PEI_PPI_DESCRIPTOR *PpiList, + IN VOID *Data + ); #endif diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf b/MdeModulePkg/Core/Pei/PeiMain.inf index 0cf357371a..b597aed8f6 100644 --- a/MdeModulePkg/Core/Pei/PeiMain.inf +++ b/MdeModulePkg/Core/Pei/PeiMain.inf @@ -47,6 +47,12 @@ PciCfg2/PciCfg2.c PeiMain.h +[Sources.IA32] + Ia32/SwitchPeiCore.nasm + +[Sources.X64] + X64/SwitchPeiCore.nasm + [Packages] MdePkg/MdePkg.dec MdeModulePkg/MdeModulePkg.dec diff --git a/MdeModulePkg/Core/Pei/X64/SwitchPeiCore.nasm b/MdeModulePkg/Core/Pei/X64/SwitchPeiCore.nasm new file mode 100644 index 0000000000..94e09be757 --- /dev/null +++ b/MdeModulePkg/Core/Pei/X64/SwitchPeiCore.nasm @@ -0,0 +1,38 @@ +;---------------------------------------------------------------------- +-------- +; +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> ; +SPDX-License-Identifier: BSD-2-Clause-Patent ; ; Abstract: +; +; Switch PeiCore from temporary memory to permanent memory. +; +;---------------------------------------------------------------------- +-------- + + SECTION .text + +extern ASM_PFX(PeiCore) + +;---------------------------------------------------------------------- +-------- +; VOID +; EFIAPI +; SwitchPeiCore ( +; EFI_SEC_PEI_HAND_OFF *SecCoreDataPtr, +; EFI_PEI_PPI_DESCRIPTOR *PpiList, +; VOID *Data +; ); +;---------------------------------------------------------------------- +-------- +global ASM_PFX(SwitchPeiCore) +ASM_PFX(SwitchPeiCore): + ; + ; Per X64 calling convention, make sure RSP is 16-byte aligned. + ; + mov rax, rsp + and rax, 0fh + sub rsp, rax + + sub rsp, 20h ; Allocate 32 bytes as a shadow store on call stack + call ASM_PFX(PeiCore) + jmp $ ; Should never reach here + ret + -- 2.16.2.windows.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-devel][PATCH] MdeModulePkg: Make RSP 16-byte boundary aligned for PEI 64bit 2022-03-17 1:04 ` [edk2-devel][PATCH] MdeModulePkg: Make RSP 16-byte boundary aligned for PEI 64bit Kuo, Ted @ 2022-03-17 11:08 ` Marvin Häuser 2022-03-18 13:46 ` Kuo, Ted 0 siblings, 1 reply; 4+ messages in thread From: Marvin Häuser @ 2022-03-17 11:08 UTC (permalink / raw) To: devel, ted.kuo Cc: Bi, Dandan, Gao, Liming, De, Debkumar, Han, Harry, West, Catharine, Wang, Jian J, S, Ashraf Ali, Kinney, Michael D Good day, > On 17. Mar 2022, at 02:05, Kuo, Ted <ted.kuo@intel.com> wrote: > > Hi Liming and Mike, > > Can you please review the change? > > Thanks, > Ted > > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kuo, Ted > Sent: Thursday, March 10, 2022 2:21 PM > To: devel@edk2.groups.io > Cc: Bi, Dandan <dandan.bi@intel.com>; Gao, Liming > <gaoliming@byosoft.com.cn>; De, Debkumar <debkumar.de@intel.com>; Han, > Harry <harry.han@intel.com>; West, Catharine > <catharine.west@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; S, > Ashraf Ali <ashraf.ali.s@intel.com> > Subject: [edk2-devel][PATCH] MdeModulePkg: Make RSP 16-byte boundary > aligned for PEI 64bit > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3865 > Use SwitchPeiCore instead of calling PeiCore directly when switching > PeiCore from temporary memory to permanent memory. For PEI 32bit, > SwitchPeiCore always calls PeiCore without any additional step. For > PEI 64bit, SwitchPeiCore makes RSP 16-byte boundary aligned and then > allocate 32 bytes as a shadow store on call stack before calling PeiCore. > > Cc: Dandan Bi <dandan.bi@intel.com> > Cc: Liming Gao <gaoliming@byosoft.com.cn> > Cc: Debkumar De <debkumar.de@intel.com> > Cc: Harry Han <harry.han@intel.com> > Cc: Catharine West <catharine.west@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Ashraf Ali S <ashraf.ali.s@intel.com> > Signed-off-by: Ted Kuo <ted.kuo@intel.com> > --- > MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 2 +- > MdeModulePkg/Core/Pei/Ia32/SwitchPeiCore.nasm | 33 > +++++++++++++++++++++++ > MdeModulePkg/Core/Pei/PeiMain.h | 25 ++++++++++++++++++ > MdeModulePkg/Core/Pei/PeiMain.inf | 6 +++++ > MdeModulePkg/Core/Pei/X64/SwitchPeiCore.nasm | 38 > +++++++++++++++++++++++++++ > 5 files changed, 103 insertions(+), 1 deletion(-) create mode 100644 > MdeModulePkg/Core/Pei/Ia32/SwitchPeiCore.nasm > create mode 100644 MdeModulePkg/Core/Pei/X64/SwitchPeiCore.nasm > > diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c > b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c > index 3552feda8f..5af6e6e86f 100644 > --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c > +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c > @@ -871,7 +871,7 @@ PeiCheckAndSwitchStack ( > // > // Entry PEI Phase 2 > // > - PeiCore (SecCoreData, NULL, Private); > + SwitchPeiCore (SecCoreData, NULL, Private); > } else { > // > // Migrate memory pages allocated in pre-memory phase. > diff --git a/MdeModulePkg/Core/Pei/Ia32/SwitchPeiCore.nasm > b/MdeModulePkg/Core/Pei/Ia32/SwitchPeiCore.nasm > new file mode 100644 > index 0000000000..23cfb5090b > --- /dev/null > +++ b/MdeModulePkg/Core/Pei/Ia32/SwitchPeiCore.nasm > @@ -0,0 +1,33 @@ > +;---------------------------------------------------------------------- > +-------- > +; > +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> ; > +SPDX-License-Identifier: BSD-2-Clause-Patent ; ; Abstract: > +; > +; Switch PeiCore from temporary memory to permanent memory. > +; > +;---------------------------------------------------------------------- > +-------- > + > + SECTION .text > + > +extern ASM_PFX(PeiCore) > + > +;---------------------------------------------------------------------- > +-------- > +; VOID > +; EFIAPI > +; SwitchPeiCore ( > +; EFI_SEC_PEI_HAND_OFF *SecCoreDataPtr, > +; EFI_PEI_PPI_DESCRIPTOR *PpiList, > +; VOID *Data > +; ); > +;---------------------------------------------------------------------- > +-------- > +global ASM_PFX(SwitchPeiCore) > +ASM_PFX(SwitchPeiCore): > + push DWORD [esp + 12] > + push DWORD [esp + 12] > + push DWORD [esp + 12] > + call ASM_PFX(PeiCore) > + jmp $ ; Should never reach here > + ret > + I think there were efforts in the past to avoid ASM whenever possible. Can’t this just remain a C function (for IA32 only of course) and if not, wouldn't a simple jmp instruction be sufficient? > diff --git a/MdeModulePkg/Core/Pei/PeiMain.h > b/MdeModulePkg/Core/Pei/PeiMain.h index 556beddad5..8e8ed3dadf 100644 > --- a/MdeModulePkg/Core/Pei/PeiMain.h > +++ b/MdeModulePkg/Core/Pei/PeiMain.h > @@ -2038,4 +2038,29 @@ PeiReinitializeFv ( > IN PEI_CORE_INSTANCE *PrivateData > ); > > +/** > + This routine is invoked by main entry of PeiMain module during > +transition > + from temporary memory to permanent memory. > + > + @param SecCoreDataPtr Points to a data structure containing > information about the PEI core's operating > + environment, such as the size and location > of temporary RAM, the stack location and > + the BFV location. > + @param PpiList Points to a list of one or more PPI > descriptors to be installed initially by the PEI core. > + An empty PPI list consists of a single > descriptor with the end-tag > + EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST. As > part of its initialization > + phase, the PEI Foundation will add these > SEC-hosted PPIs to its PPI database such > + that both the PEI Foundation and any modules > can leverage the associated service > + calls and/or code in these early PPIs > + @param Data Pointer to old core data that is used to > initialize the > + core's data areas. > + If NULL, it is first PeiCore entering. > + > +**/ > +VOID > +EFIAPI > +SwitchPeiCore ( > + IN CONST EFI_SEC_PEI_HAND_OFF *SecCoreDataPtr, > + IN CONST EFI_PEI_PPI_DESCRIPTOR *PpiList, > + IN VOID *Data > + ); > #endif > diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf > b/MdeModulePkg/Core/Pei/PeiMain.inf > index 0cf357371a..b597aed8f6 100644 > --- a/MdeModulePkg/Core/Pei/PeiMain.inf > +++ b/MdeModulePkg/Core/Pei/PeiMain.inf > @@ -47,6 +47,12 @@ > PciCfg2/PciCfg2.c > PeiMain.h > > +[Sources.IA32] > + Ia32/SwitchPeiCore.nasm > + > +[Sources.X64] > + X64/SwitchPeiCore.nasm > + > [Packages] > MdePkg/MdePkg.dec > MdeModulePkg/MdeModulePkg.dec > diff --git a/MdeModulePkg/Core/Pei/X64/SwitchPeiCore.nasm > b/MdeModulePkg/Core/Pei/X64/SwitchPeiCore.nasm > new file mode 100644 > index 0000000000..94e09be757 > --- /dev/null > +++ b/MdeModulePkg/Core/Pei/X64/SwitchPeiCore.nasm > @@ -0,0 +1,38 @@ > +;---------------------------------------------------------------------- > +-------- > +; > +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> ; > +SPDX-License-Identifier: BSD-2-Clause-Patent ; ; Abstract: > +; > +; Switch PeiCore from temporary memory to permanent memory. > +; > +;---------------------------------------------------------------------- > +-------- > + > + SECTION .text > + > +extern ASM_PFX(PeiCore) > + > +;---------------------------------------------------------------------- > +-------- > +; VOID > +; EFIAPI > +; SwitchPeiCore ( > +; EFI_SEC_PEI_HAND_OFF *SecCoreDataPtr, > +; EFI_PEI_PPI_DESCRIPTOR *PpiList, > +; VOID *Data > +; ); > +;---------------------------------------------------------------------- > +-------- > +global ASM_PFX(SwitchPeiCore) > +ASM_PFX(SwitchPeiCore): > + ; > + ; Per X64 calling convention, make sure RSP is 16-byte aligned. > + ; > + mov rax, rsp > + and rax, 0fh > + sub rsp, rax > + > + sub rsp, 20h ; Allocate 32 bytes as a shadow store on > call stack > + call ASM_PFX(PeiCore) > + jmp $ ; Should never reach here > + ret This might be my own ignorance, but this approach really worries me. The stack is correctly aligned by this function, ok. But this code is called from C, and the stack is not fully switched (only aligned relative to the current location), and no architectural mode switch seems to occur, which means 64-bit code already ran with an unaligned stack (otherwise it would not need to be aligned here), agreed? The stack is last switched by SecCore's TemporaryRamMigration() to TopOfNewStack - TemporaryStackSize. By ABI constraints, TemporaryStackSize must be 16-Byte-aligned (is it?). Furthermore, we have TopOfNewStack = Private->PhysicalMemoryBegin + NewStackSize. NewStackSize is manually aligned to 4K (ok), but may be shortened to PcdGet32 (PcdPeiCoreMaxPeiStackSize), which also must be 16-Byte-aligned for ABI constraints (is it?). Private->PhysicalMemoryBegin is where my knowledge falls flat, which guarantees hold for it? If it's not guaranteed to be 16-Byte-aligned, this looks like a culprit and the stack should be aligned in TemporaryRamMigration(). Otherwise, may one of the constraints I mentioned not hold? Otherwise, does some ASM in-between bork the stack alignment? Or am I completely lost as to what is going on? :) Thanks! Best regards, Marvin > + > -- > 2.16.2.windows.1 > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel][PATCH] MdeModulePkg: Make RSP 16-byte boundary aligned for PEI 64bit 2022-03-17 11:08 ` Marvin Häuser @ 2022-03-18 13:46 ` Kuo, Ted 0 siblings, 0 replies; 4+ messages in thread From: Kuo, Ted @ 2022-03-18 13:46 UTC (permalink / raw) To: Marvin Häuser, devel@edk2.groups.io Cc: Bi, Dandan, Gao, Liming, De, Debkumar, Han, Harry, West, Catharine, Wang, Jian J, S, Ashraf Ali, Kinney, Michael D Thanks Marvin for your feedback. I think your direction is right. The original stack alignment is correct before switching to new stack but the current implementation in SecCore's SecTemporaryRamSupport() will break the stack alignment after switching to new stack. We need to ensure the new stack and stack offset have same alignment as well as the old stack. I'll resend a new patch to keep the stack alignment aligned before and after switching stack. Thanks, Ted -----Original Message----- From: Marvin Häuser <mhaeuser@posteo.de> Sent: Thursday, March 17, 2022 7:09 PM To: devel@edk2.groups.io; Kuo, Ted <ted.kuo@intel.com> Cc: Bi, Dandan <dandan.bi@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; De, Debkumar <debkumar.de@intel.com>; Han, Harry <harry.han@intel.com>; West, Catharine <catharine.west@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; S, Ashraf Ali <ashraf.ali.s@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com> Subject: Re: [edk2-devel][PATCH] MdeModulePkg: Make RSP 16-byte boundary aligned for PEI 64bit Good day, > On 17. Mar 2022, at 02:05, Kuo, Ted <ted.kuo@intel.com> wrote: > > Hi Liming and Mike, > > Can you please review the change? > > Thanks, > Ted > > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Kuo, > Ted > Sent: Thursday, March 10, 2022 2:21 PM > To: devel@edk2.groups.io > Cc: Bi, Dandan <dandan.bi@intel.com>; Gao, Liming > <gaoliming@byosoft.com.cn>; De, Debkumar <debkumar.de@intel.com>; Han, > Harry <harry.han@intel.com>; West, Catharine > <catharine.west@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; S, > Ashraf Ali <ashraf.ali.s@intel.com> > Subject: [edk2-devel][PATCH] MdeModulePkg: Make RSP 16-byte boundary > aligned for PEI 64bit > > REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3865 > Use SwitchPeiCore instead of calling PeiCore directly when switching > PeiCore from temporary memory to permanent memory. For PEI 32bit, > SwitchPeiCore always calls PeiCore without any additional step. For > PEI 64bit, SwitchPeiCore makes RSP 16-byte boundary aligned and then > allocate 32 bytes as a shadow store on call stack before calling PeiCore. > > Cc: Dandan Bi <dandan.bi@intel.com> > Cc: Liming Gao <gaoliming@byosoft.com.cn> > Cc: Debkumar De <debkumar.de@intel.com> > Cc: Harry Han <harry.han@intel.com> > Cc: Catharine West <catharine.west@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Ashraf Ali S <ashraf.ali.s@intel.com> > Signed-off-by: Ted Kuo <ted.kuo@intel.com> > --- > MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 2 +- > MdeModulePkg/Core/Pei/Ia32/SwitchPeiCore.nasm | 33 > +++++++++++++++++++++++ > MdeModulePkg/Core/Pei/PeiMain.h | 25 ++++++++++++++++++ > MdeModulePkg/Core/Pei/PeiMain.inf | 6 +++++ > MdeModulePkg/Core/Pei/X64/SwitchPeiCore.nasm | 38 > +++++++++++++++++++++++++++ > 5 files changed, 103 insertions(+), 1 deletion(-) create mode 100644 > MdeModulePkg/Core/Pei/Ia32/SwitchPeiCore.nasm > create mode 100644 MdeModulePkg/Core/Pei/X64/SwitchPeiCore.nasm > > diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c > b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c > index 3552feda8f..5af6e6e86f 100644 > --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c > +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c > @@ -871,7 +871,7 @@ PeiCheckAndSwitchStack ( > // > // Entry PEI Phase 2 > // > - PeiCore (SecCoreData, NULL, Private); > + SwitchPeiCore (SecCoreData, NULL, Private); > } else { > // > // Migrate memory pages allocated in pre-memory phase. > diff --git a/MdeModulePkg/Core/Pei/Ia32/SwitchPeiCore.nasm > b/MdeModulePkg/Core/Pei/Ia32/SwitchPeiCore.nasm > new file mode 100644 > index 0000000000..23cfb5090b > --- /dev/null > +++ b/MdeModulePkg/Core/Pei/Ia32/SwitchPeiCore.nasm > @@ -0,0 +1,33 @@ > +;-------------------------------------------------------------------- > +-- > +-------- > +; > +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> ; > +SPDX-License-Identifier: BSD-2-Clause-Patent ; ; Abstract: > +; > +; Switch PeiCore from temporary memory to permanent memory. > +; > +;-------------------------------------------------------------------- > +-- > +-------- > + > + SECTION .text > + > +extern ASM_PFX(PeiCore) > + > +;-------------------------------------------------------------------- > +-- > +-------- > +; VOID > +; EFIAPI > +; SwitchPeiCore ( > +; EFI_SEC_PEI_HAND_OFF *SecCoreDataPtr, ; > +EFI_PEI_PPI_DESCRIPTOR *PpiList, ; VOID *Data ; > +); > +;-------------------------------------------------------------------- > +-- > +-------- > +global ASM_PFX(SwitchPeiCore) > +ASM_PFX(SwitchPeiCore): > + push DWORD [esp + 12] > + push DWORD [esp + 12] > + push DWORD [esp + 12] > + call ASM_PFX(PeiCore) > + jmp $ ; Should never reach here > + ret > + I think there were efforts in the past to avoid ASM whenever possible. Can’t this just remain a C function (for IA32 only of course) and if not, wouldn't a simple jmp instruction be sufficient? > diff --git a/MdeModulePkg/Core/Pei/PeiMain.h > b/MdeModulePkg/Core/Pei/PeiMain.h index 556beddad5..8e8ed3dadf 100644 > --- a/MdeModulePkg/Core/Pei/PeiMain.h > +++ b/MdeModulePkg/Core/Pei/PeiMain.h > @@ -2038,4 +2038,29 @@ PeiReinitializeFv ( > IN PEI_CORE_INSTANCE *PrivateData > ); > > +/** > + This routine is invoked by main entry of PeiMain module during > +transition > + from temporary memory to permanent memory. > + > + @param SecCoreDataPtr Points to a data structure containing > information about the PEI core's operating > + environment, such as the size and location > of temporary RAM, the stack location and > + the BFV location. > + @param PpiList Points to a list of one or more PPI > descriptors to be installed initially by the PEI core. > + An empty PPI list consists of a single > descriptor with the end-tag > + EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST. As > part of its initialization > + phase, the PEI Foundation will add these > SEC-hosted PPIs to its PPI database such > + that both the PEI Foundation and any modules > can leverage the associated service > + calls and/or code in these early PPIs > + @param Data Pointer to old core data that is used to > initialize the > + core's data areas. > + If NULL, it is first PeiCore entering. > + > +**/ > +VOID > +EFIAPI > +SwitchPeiCore ( > + IN CONST EFI_SEC_PEI_HAND_OFF *SecCoreDataPtr, > + IN CONST EFI_PEI_PPI_DESCRIPTOR *PpiList, > + IN VOID *Data > + ); > #endif > diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf > b/MdeModulePkg/Core/Pei/PeiMain.inf > index 0cf357371a..b597aed8f6 100644 > --- a/MdeModulePkg/Core/Pei/PeiMain.inf > +++ b/MdeModulePkg/Core/Pei/PeiMain.inf > @@ -47,6 +47,12 @@ > PciCfg2/PciCfg2.c > PeiMain.h > > +[Sources.IA32] > + Ia32/SwitchPeiCore.nasm > + > +[Sources.X64] > + X64/SwitchPeiCore.nasm > + > [Packages] > MdePkg/MdePkg.dec > MdeModulePkg/MdeModulePkg.dec > diff --git a/MdeModulePkg/Core/Pei/X64/SwitchPeiCore.nasm > b/MdeModulePkg/Core/Pei/X64/SwitchPeiCore.nasm > new file mode 100644 > index 0000000000..94e09be757 > --- /dev/null > +++ b/MdeModulePkg/Core/Pei/X64/SwitchPeiCore.nasm > @@ -0,0 +1,38 @@ > +;-------------------------------------------------------------------- > +-- > +-------- > +; > +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> ; > +SPDX-License-Identifier: BSD-2-Clause-Patent ; ; Abstract: > +; > +; Switch PeiCore from temporary memory to permanent memory. > +; > +;-------------------------------------------------------------------- > +-- > +-------- > + > + SECTION .text > + > +extern ASM_PFX(PeiCore) > + > +;-------------------------------------------------------------------- > +-- > +-------- > +; VOID > +; EFIAPI > +; SwitchPeiCore ( > +; EFI_SEC_PEI_HAND_OFF *SecCoreDataPtr, ; > +EFI_PEI_PPI_DESCRIPTOR *PpiList, ; VOID *Data ; > +); > +;-------------------------------------------------------------------- > +-- > +-------- > +global ASM_PFX(SwitchPeiCore) > +ASM_PFX(SwitchPeiCore): > + ; > + ; Per X64 calling convention, make sure RSP is 16-byte aligned. > + ; > + mov rax, rsp > + and rax, 0fh > + sub rsp, rax > + > + sub rsp, 20h ; Allocate 32 bytes as a shadow store on > call stack > + call ASM_PFX(PeiCore) > + jmp $ ; Should never reach here > + ret This might be my own ignorance, but this approach really worries me. The stack is correctly aligned by this function, ok. But this code is called from C, and the stack is not fully switched (only aligned relative to the current location), and no architectural mode switch seems to occur, which means 64-bit code already ran with an unaligned stack (otherwise it would not need to be aligned here), agreed? The stack is last switched by SecCore's TemporaryRamMigration() to TopOfNewStack - TemporaryStackSize. By ABI constraints, TemporaryStackSize must be 16-Byte-aligned (is it?). Furthermore, we have TopOfNewStack = Private->PhysicalMemoryBegin + NewStackSize. NewStackSize is manually aligned to 4K (ok), but may be shortened to PcdGet32 (PcdPeiCoreMaxPeiStackSize), which also must be 16-Byte-aligned for ABI constraints (is it?). Private->PhysicalMemoryBegin is where my knowledge falls flat, which guarantees hold for it? If it's not guaranteed to be 16-Byte-aligned, this looks like a culprit and the stack should be aligned in TemporaryRamMigration(). Otherwise, may one of the constraints I mentioned not hold? Otherwise, does some ASM in-between bork the stack alignment? Or am I completely lost as to what is going on? :) Thanks! Best regards, Marvin > + > -- > 2.16.2.windows.1 > > > > > > > > > > > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* [edk2-devel][PATCH] MdeModulePkg: Make RSP 16-byte boundary aligned for PEI 64bit @ 2022-03-10 6:20 Kuo, Ted 0 siblings, 0 replies; 4+ messages in thread From: Kuo, Ted @ 2022-03-10 6:20 UTC (permalink / raw) To: devel Cc: Dandan Bi, Liming Gao, Debkumar De, Harry Han, Catharine West, Jian J Wang, Ashraf Ali S REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3865 Use SwitchPeiCore instead of calling PeiCore directly when switching PeiCore from temporary memory to permanent memory. For PEI 32bit, SwitchPeiCore always calls PeiCore without any additional step. For PEI 64bit, SwitchPeiCore makes RSP 16-byte boundary aligned and then allocate 32 bytes as a shadow store on call stack before calling PeiCore. Cc: Dandan Bi <dandan.bi@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Debkumar De <debkumar.de@intel.com> Cc: Harry Han <harry.han@intel.com> Cc: Catharine West <catharine.west@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Ashraf Ali S <ashraf.ali.s@intel.com> Signed-off-by: Ted Kuo <ted.kuo@intel.com> --- MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 2 +- MdeModulePkg/Core/Pei/Ia32/SwitchPeiCore.nasm | 33 +++++++++++++++++++++++ MdeModulePkg/Core/Pei/PeiMain.h | 25 ++++++++++++++++++ MdeModulePkg/Core/Pei/PeiMain.inf | 6 +++++ MdeModulePkg/Core/Pei/X64/SwitchPeiCore.nasm | 38 +++++++++++++++++++++++++++ 5 files changed, 103 insertions(+), 1 deletion(-) create mode 100644 MdeModulePkg/Core/Pei/Ia32/SwitchPeiCore.nasm create mode 100644 MdeModulePkg/Core/Pei/X64/SwitchPeiCore.nasm diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c index 3552feda8f..5af6e6e86f 100644 --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c @@ -871,7 +871,7 @@ PeiCheckAndSwitchStack ( // // Entry PEI Phase 2 // - PeiCore (SecCoreData, NULL, Private); + SwitchPeiCore (SecCoreData, NULL, Private); } else { // // Migrate memory pages allocated in pre-memory phase. diff --git a/MdeModulePkg/Core/Pei/Ia32/SwitchPeiCore.nasm b/MdeModulePkg/Core/Pei/Ia32/SwitchPeiCore.nasm new file mode 100644 index 0000000000..23cfb5090b --- /dev/null +++ b/MdeModulePkg/Core/Pei/Ia32/SwitchPeiCore.nasm @@ -0,0 +1,33 @@ +;------------------------------------------------------------------------------ +; +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> +; SPDX-License-Identifier: BSD-2-Clause-Patent +; +; Abstract: +; +; Switch PeiCore from temporary memory to permanent memory. +; +;------------------------------------------------------------------------------ + + SECTION .text + +extern ASM_PFX(PeiCore) + +;------------------------------------------------------------------------------ +; VOID +; EFIAPI +; SwitchPeiCore ( +; EFI_SEC_PEI_HAND_OFF *SecCoreDataPtr, +; EFI_PEI_PPI_DESCRIPTOR *PpiList, +; VOID *Data +; ); +;------------------------------------------------------------------------------ +global ASM_PFX(SwitchPeiCore) +ASM_PFX(SwitchPeiCore): + push DWORD [esp + 12] + push DWORD [esp + 12] + push DWORD [esp + 12] + call ASM_PFX(PeiCore) + jmp $ ; Should never reach here + ret + diff --git a/MdeModulePkg/Core/Pei/PeiMain.h b/MdeModulePkg/Core/Pei/PeiMain.h index 556beddad5..8e8ed3dadf 100644 --- a/MdeModulePkg/Core/Pei/PeiMain.h +++ b/MdeModulePkg/Core/Pei/PeiMain.h @@ -2038,4 +2038,29 @@ PeiReinitializeFv ( IN PEI_CORE_INSTANCE *PrivateData ); +/** + This routine is invoked by main entry of PeiMain module during transition + from temporary memory to permanent memory. + + @param SecCoreDataPtr Points to a data structure containing information about the PEI core's operating + environment, such as the size and location of temporary RAM, the stack location and + the BFV location. + @param PpiList Points to a list of one or more PPI descriptors to be installed initially by the PEI core. + An empty PPI list consists of a single descriptor with the end-tag + EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST. As part of its initialization + phase, the PEI Foundation will add these SEC-hosted PPIs to its PPI database such + that both the PEI Foundation and any modules can leverage the associated service + calls and/or code in these early PPIs + @param Data Pointer to old core data that is used to initialize the + core's data areas. + If NULL, it is first PeiCore entering. + +**/ +VOID +EFIAPI +SwitchPeiCore ( + IN CONST EFI_SEC_PEI_HAND_OFF *SecCoreDataPtr, + IN CONST EFI_PEI_PPI_DESCRIPTOR *PpiList, + IN VOID *Data + ); #endif diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf b/MdeModulePkg/Core/Pei/PeiMain.inf index 0cf357371a..b597aed8f6 100644 --- a/MdeModulePkg/Core/Pei/PeiMain.inf +++ b/MdeModulePkg/Core/Pei/PeiMain.inf @@ -47,6 +47,12 @@ PciCfg2/PciCfg2.c PeiMain.h +[Sources.IA32] + Ia32/SwitchPeiCore.nasm + +[Sources.X64] + X64/SwitchPeiCore.nasm + [Packages] MdePkg/MdePkg.dec MdeModulePkg/MdeModulePkg.dec diff --git a/MdeModulePkg/Core/Pei/X64/SwitchPeiCore.nasm b/MdeModulePkg/Core/Pei/X64/SwitchPeiCore.nasm new file mode 100644 index 0000000000..94e09be757 --- /dev/null +++ b/MdeModulePkg/Core/Pei/X64/SwitchPeiCore.nasm @@ -0,0 +1,38 @@ +;------------------------------------------------------------------------------ +; +; Copyright (c) 2022, Intel Corporation. All rights reserved.<BR> +; SPDX-License-Identifier: BSD-2-Clause-Patent +; +; Abstract: +; +; Switch PeiCore from temporary memory to permanent memory. +; +;------------------------------------------------------------------------------ + + SECTION .text + +extern ASM_PFX(PeiCore) + +;------------------------------------------------------------------------------ +; VOID +; EFIAPI +; SwitchPeiCore ( +; EFI_SEC_PEI_HAND_OFF *SecCoreDataPtr, +; EFI_PEI_PPI_DESCRIPTOR *PpiList, +; VOID *Data +; ); +;------------------------------------------------------------------------------ +global ASM_PFX(SwitchPeiCore) +ASM_PFX(SwitchPeiCore): + ; + ; Per X64 calling convention, make sure RSP is 16-byte aligned. + ; + mov rax, rsp + and rax, 0fh + sub rsp, rax + + sub rsp, 20h ; Allocate 32 bytes as a shadow store on call stack + call ASM_PFX(PeiCore) + jmp $ ; Should never reach here + ret + -- 2.16.2.windows.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-03-18 13:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <16DAF0B16C1B1922.28111@groups.io>
2022-03-17 1:04 ` [edk2-devel][PATCH] MdeModulePkg: Make RSP 16-byte boundary aligned for PEI 64bit Kuo, Ted
2022-03-17 11:08 ` Marvin Häuser
2022-03-18 13:46 ` Kuo, Ted
2022-03-10 6:20 Kuo, Ted
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox