Good enhancement. I think it resolved my compatibility concern for the old API.
Some comment:
1) Can we just use CPU_EXCEPTION_INIT_DATA instead of
CPU_EXCEPTION_INIT_DATA_EX? I am not sure why we add _EX here.
2) Is CPU_EXCEPTION_INIT_DATA_EX for IA32 only or both IA32/X64? I found "Ia32"
may bring confusing here. See EFI_DEBUG_SUPPORT_PROTOCOL, we have Ia32 for IA32
arch, X64 for X64 arch.
//
// Flag to indicate if default handlers should be initialized or not.
//
BOOLEAN InitDefaultHandlers;
} Ia32;
} CPU_EXCEPTION_INIT_DATA_EX;
3) Can we add IdtTableSize in CPU_EXCEPTION_INIT_DATA_EX?
4) Can we add Version field in CPU_EXCEPTION_INIT_DATA_EX? I am not sure if we
need add more entry later.
5) You mentioned "KnownGoodStackTop is for *ALL* exceptions".
Does ALL here mean StackSwitchExceptionNumber, or arch specific number such as
0x20 for X86 system?
I think StackSwitchExceptionNumber is enough.
6) There might be more than one TSS entry in GDT.
Does TssDesc/Tss in CPU_EXCEPTION_INIT_DATA_EX mean the exception Tss? (normal
TSS does not need be reported here)
If so, I suggest we use ExceptionTss as the keyword.
7) Below code may cause buffer overrun on IST.
for (Index = 0; Index < StackSwitchData->Ia32.StackSwitchExceptionNumber;
++Index) {
//
// Fixup IST
//
Tss->IST[Index] = StackTop;
I suggest we add some basic check for StackSwitchExceptionNumber.
8) Do you think we need mention the TssDesc/Tss size requirement for that?
TssDesc = StackSwitchData->Ia32.TssDesc;
Tss = StackSwitchData->Ia32.Tss;
for (Index = 0; Index < StackSwitchData->Ia32.StackSwitchExceptionNumber;
++Index) {
TssDesc += 1;
Tss += 1;
I suggest we add TssDescSize and TssSize in CPU_EXCEPTION_INIT_DATA_EX and
check the size in the code.
9) Last but not least important, have you evaluated if current
CPU_EXCEPTION_INIT_DATA_EX is enough for SMM version stack guard exception?
Thank you
Yao Jiewen
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Jian J
> Wang
> Sent: Friday, December 1, 2017 10:37 AM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH v3 00/11] Implement stack guard feature
>
> > v3:
> > a. Change new API InitializeCpuExceptionStackSwitchHandlers() to
> > InitializeCpuExceptionHandlersEx(). Related code are updated
> accordingly.
> > b. Move EXCEPTION_STACK_SWITCH_DATA to CpuExceptionHandlerLib.h
> > and change the name to CPU_EXCEPTION_INIT_DATA_EX for the sake
> > of the API name change.
> > c. Add more general macros in BaseLib.h.
> > d. Add dummy implementation of InitializeCpuExceptionHandlersEx for
> > SEC, PEI and SMM but implement a full version for DXE.
> > e. Add dummy InitializeCpuExceptionHandlersEx for ARM's
> CpuExceptionHandlerLib
> > and NULL version of CpuExceptionHandlerLib
> > f. Call InitializeCpuExceptionHandlersEx() in DxeMain instead of
> > InitializeCpuExceptionHandlers().
>
>
> > v2:
> > a. Introduce and implement new API
> InitializeCpuExceptionStackSwitchHandlers().
> > b. Add stack switch related general definitions of IA32 in BaseLib.h.
> > c. Add two new PCDs to configure exception vector list and stack size.
> > d. Add code to save/restore GDTR, IDTR and TR for AP.
> > e. Refactor exception handler code for stack switch.
> > f. Add code to setup stack switch for AP besides BSP.
>
> Stack guard feature makes use of paging mechanism to monitor if there's a
> stack overflow occurred during boot. A new PCD PcdCpuStackGuard is added to
> enable/disable this feature. PCD PcdCpuStackSwitchExceptionList and
> PcdCpuKnownGoodStackSize are introduced to configure the required
> exceptions
> and stack size.
>
> If this feature is enabled, DxeIpl will setup page tables and set page where
> the stack bottom is at to be NON-PRESENT. If stack overflow occurs, Page
> Fault exception will be triggered.
>
> In order to make sure exception handler works normally even when the stack
> is corrupted, stack switching is implemented in exception library.
>
> Due to the mechanism behind Stack Guard, this feature is only avaiable for
> UEFI drivers (memory avaiable). That also means it doesn't support NT32
> emulated platform (paging not supported).
>
> Jian J Wang (11):
> MdeModulePkg/metafile: Add PCD PcdCpuStackGuard
> UefiCpuPkg/UefiCpuPkg.dec: Add two new PCDs for stack switch
> MdeModulePkg/CpuExceptionHandlerLib.h: Add a new API
> InitializeCpuExceptionHandlersEx
> MdePkg/BaseLib: Add stack switch related definitions for IA32
> UefiCpuPkg/CpuExceptionHandlerLib: Add stack switch support
> MdeModulePkg/CpuExceptionHandlerLibNull: Add new API implementation
> ArmPkg/ArmExceptionLib: Add implementation of new API
> UefiCpuPkg/MpLib: Add GDTR, IDTR and TR in saved AP data
> UefiCpuPkg/CpuDxe: Initialize stack switch for MP
> MdeModulePkg/Core/Dxe: Call new API InitializeCpuExceptionHandlersEx
> instead
> MdeModulePkg/DxeIpl: Enable paging for Stack Guard
>
> ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.c | 33 ++
> MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c | 2 +-
> MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf | 5 +-
> MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c | 4 +
> MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c | 1 +
> MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 51 ++-
> .../Include/Library/CpuExceptionHandlerLib.h | 78 ++++
> .../CpuExceptionHandlerLibNull.c | 34 ++
> MdeModulePkg/MdeModulePkg.dec | 7 +
> MdeModulePkg/MdeModulePkg.uni | 7 +
> MdePkg/Include/Library/BaseLib.h | 117 ++++++
> MdePkg/Library/BaseLib/BaseLib.inf | 3 +
> MdePkg/Library/BaseLib/Ia32/WriteTr.nasm | 36 ++
> MdePkg/Library/BaseLib/X64/WriteTr.nasm | 37 ++
> UefiCpuPkg/CpuDxe/CpuDxe.inf | 3 +
> UefiCpuPkg/CpuDxe/CpuMp.c | 177 +++++++++
> .../CpuExceptionHandlerLib/CpuExceptionCommon.h | 39 ++
> .../DxeCpuExceptionHandlerLib.inf | 6 +
> .../Library/CpuExceptionHandlerLib/DxeException.c | 79 ++++
> .../Ia32/ArchExceptionHandler.c | 167 +++++++++
> .../Ia32/ArchInterruptDefs.h | 8 +
> .../Ia32/ExceptionTssEntryAsm.nasm | 398
> +++++++++++++++++++++
> .../CpuExceptionHandlerLib/PeiCpuException.c | 34 +-
> .../PeiCpuExceptionHandlerLib.inf | 1 +
> .../CpuExceptionHandlerLib/SecPeiCpuException.c | 34 +-
> .../SecPeiCpuExceptionHandlerLib.inf | 1 +
> .../SmmCpuExceptionHandlerLib.inf | 1 +
> .../Library/CpuExceptionHandlerLib/SmmException.c | 34 +-
> .../X64/ArchExceptionHandler.c | 134 +++++++
> .../CpuExceptionHandlerLib/X64/ArchInterruptDefs.h | 3 +
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 17 +
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 3 +
> UefiCpuPkg/UefiCpuPkg.dec | 12 +
> 33 files changed, 1547 insertions(+), 19 deletions(-)
> create mode 100644 MdePkg/Library/BaseLib/Ia32/WriteTr.nasm
> create mode 100644 MdePkg/Library/BaseLib/X64/WriteTr.nasm
> create mode 100644
> UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ExceptionTssEntryAsm.nasm
>
> --
> 2.14.1.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
