* [PATCH 0/3] ArmPkg: increase robustness of the crash handler
@ 2017-03-20 20:52 Ard Biesheuvel
2017-03-20 20:52 ` [PATCH 1/3] ArmPkg/DefaultExceptionHandlerLib: add missing GUID to .inf Ard Biesheuvel
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2017-03-20 20:52 UTC (permalink / raw)
To: edk2-devel, leif.lindholm; +Cc: ryan.harkin, eugene, Ard Biesheuvel
Make the default exception handler deal with corrupted SP or PC registers,
by switching to the EL0 stack pointer for sync exceptions, and walking the
call stack even if PC is 0x0 or otherwise invalid.
Ard Biesheuvel (3):
ArmPkg/DefaultExceptionHandlerLib: add missing GUID to .inf
ArmPkg/DefaultExceptionHandlerLib: walk call stack unconditionally
ArmPkg/ArmExceptionLib: use EL0 stack for synchronous exceptions
ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c | 17 +++-
ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S | 86 ++++++++++++--------
ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf | 5 +-
ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c | 56 +++++++------
ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLib.inf | 3 +
ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf | 3 +
6 files changed, 107 insertions(+), 63 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] ArmPkg/DefaultExceptionHandlerLib: add missing GUID to .inf
2017-03-20 20:52 [PATCH 0/3] ArmPkg: increase robustness of the crash handler Ard Biesheuvel
@ 2017-03-20 20:52 ` Ard Biesheuvel
2017-03-22 13:01 ` Leif Lindholm
2017-03-20 20:53 ` [PATCH 2/3] ArmPkg/DefaultExceptionHandlerLib: walk call stack unconditionally Ard Biesheuvel
2017-03-20 20:53 ` [PATCH 3/3] ArmPkg/ArmExceptionLib: use EL0 stack for synchronous exceptions Ard Biesheuvel
2 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2017-03-20 20:52 UTC (permalink / raw)
To: edk2-devel, leif.lindholm; +Cc: ryan.harkin, eugene, Ard Biesheuvel
Add the gEfiDebugImageInfoTableGuid, which is referenced in the code,
to both .INF files describing this module.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLib.inf | 3 +++
ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf | 3 +++
2 files changed, 6 insertions(+)
diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLib.inf b/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLib.inf
index 5d3ce892ff3c..f5421b1240a1 100644
--- a/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLib.inf
+++ b/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLib.inf
@@ -42,3 +42,6 @@ [LibraryClasses]
PeCoffGetEntryPointLib
ArmDisassemblerLib
SerialPortLib
+
+[Guids]
+ gEfiDebugImageInfoTableGuid
diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf b/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf
index 8f5b3e1010c8..b53a5e89f507 100644
--- a/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf
+++ b/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf
@@ -40,3 +40,6 @@ [LibraryClasses]
PeCoffGetEntryPointLib
ArmDisassemblerLib
SerialPortLib
+
+[Guids]
+ gEfiDebugImageInfoTableGuid
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] ArmPkg/DefaultExceptionHandlerLib: walk call stack unconditionally
2017-03-20 20:52 [PATCH 0/3] ArmPkg: increase robustness of the crash handler Ard Biesheuvel
2017-03-20 20:52 ` [PATCH 1/3] ArmPkg/DefaultExceptionHandlerLib: add missing GUID to .inf Ard Biesheuvel
@ 2017-03-20 20:53 ` Ard Biesheuvel
2017-03-22 13:07 ` Leif Lindholm
2017-03-20 20:53 ` [PATCH 3/3] ArmPkg/ArmExceptionLib: use EL0 stack for synchronous exceptions Ard Biesheuvel
2 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2017-03-20 20:53 UTC (permalink / raw)
To: edk2-devel, leif.lindholm; +Cc: ryan.harkin, eugene, Ard Biesheuvel
Currently, we only attempt to walk the call stack and print a backtrace
if the program counter refers to a location covered by a PE/COFF image.
However, regardless of the value of PC, the frame pointer may still have
a meaningful value, and so we can still produce the remainder of the
backtrace.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c | 56 +++++++++++---------
1 file changed, 31 insertions(+), 25 deletions(-)
diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
index 2f9c2ede37c1..1024bf48c63d 100644
--- a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
+++ b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
@@ -181,37 +181,43 @@ DefaultExceptionHandler (
DEBUG ((EFI_D_ERROR, "PC 0x%012lx (0x%012lx+0x%08x) [ 0] %a\n",
SystemContext.SystemContextAArch64->ELR, ImageBase,
SystemContext.SystemContextAArch64->ELR - ImageBase, BaseName (Pdb)));
+ } else {
+ DEBUG ((EFI_D_ERROR, "PC 0x%012lx\n", SystemContext.SystemContextAArch64->ELR));
+ }
- if ((UINT64 *)SystemContext.SystemContextAArch64->FP != 0) {
- Idx = 0;
+ if ((UINT64 *)SystemContext.SystemContextAArch64->FP != 0) {
+ Idx = 0;
- RootFp[0] = ((UINT64 *)SystemContext.SystemContextAArch64->FP)[0];
- RootFp[1] = ((UINT64 *)SystemContext.SystemContextAArch64->FP)[1];
- if (RootFp[1] != SystemContext.SystemContextAArch64->LR) {
- RootFp[0] = SystemContext.SystemContextAArch64->FP;
- RootFp[1] = SystemContext.SystemContextAArch64->LR;
- }
- for (Fp = RootFp; Fp[0] != 0; Fp = (UINT64 *)Fp[0]) {
- Pdb = GetImageName (Fp[1], &ImageBase, &PeCoffSizeOfHeader);
- if (Pdb != NULL) {
- if (Pdb != PrevPdb) {
- Idx++;
- PrevPdb = Pdb;
- }
- DEBUG ((EFI_D_ERROR, "PC 0x%012lx (0x%012lx+0x%08x) [% 2d] %a\n",
- Fp[1], ImageBase, Fp[1] - ImageBase, Idx, BaseName (Pdb)));
+ RootFp[0] = ((UINT64 *)SystemContext.SystemContextAArch64->FP)[0];
+ RootFp[1] = ((UINT64 *)SystemContext.SystemContextAArch64->FP)[1];
+ if (RootFp[1] != SystemContext.SystemContextAArch64->LR) {
+ RootFp[0] = SystemContext.SystemContextAArch64->FP;
+ RootFp[1] = SystemContext.SystemContextAArch64->LR;
+ }
+ for (Fp = RootFp; Fp[0] != 0; Fp = (UINT64 *)Fp[0]) {
+ Pdb = GetImageName (Fp[1], &ImageBase, &PeCoffSizeOfHeader);
+ if (Pdb != NULL) {
+ if (Pdb != PrevPdb) {
+ Idx++;
+ PrevPdb = Pdb;
}
+ DEBUG ((EFI_D_ERROR, "PC 0x%012lx (0x%012lx+0x%08x) [% 2d] %a\n",
+ Fp[1], ImageBase, Fp[1] - ImageBase, Idx, BaseName (Pdb)));
+ } else {
+ DEBUG ((EFI_D_ERROR, "PC 0x%012lx\n", Fp[1]));
}
- PrevPdb = Pdb = GetImageName (SystemContext.SystemContextAArch64->ELR, &ImageBase, &PeCoffSizeOfHeader);
+ }
+ PrevPdb = Pdb = GetImageName (SystemContext.SystemContextAArch64->ELR, &ImageBase, &PeCoffSizeOfHeader);
+ if (Pdb != NULL) {
DEBUG ((EFI_D_ERROR, "\n[ 0] %a\n", Pdb));
+ }
- Idx = 0;
- for (Fp = RootFp; Fp[0] != 0; Fp = (UINT64 *)Fp[0]) {
- Pdb = GetImageName (Fp[1], &ImageBase, &PeCoffSizeOfHeader);
- if (Pdb != NULL && Pdb != PrevPdb) {
- DEBUG ((EFI_D_ERROR, "[% 2d] %a\n", ++Idx, Pdb));
- PrevPdb = Pdb;
- }
+ Idx = 0;
+ for (Fp = RootFp; Fp[0] != 0; Fp = (UINT64 *)Fp[0]) {
+ Pdb = GetImageName (Fp[1], &ImageBase, &PeCoffSizeOfHeader);
+ if (Pdb != NULL && Pdb != PrevPdb) {
+ DEBUG ((EFI_D_ERROR, "[% 2d] %a\n", ++Idx, Pdb));
+ PrevPdb = Pdb;
}
}
}
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3] ArmPkg/ArmExceptionLib: use EL0 stack for synchronous exceptions
2017-03-20 20:52 [PATCH 0/3] ArmPkg: increase robustness of the crash handler Ard Biesheuvel
2017-03-20 20:52 ` [PATCH 1/3] ArmPkg/DefaultExceptionHandlerLib: add missing GUID to .inf Ard Biesheuvel
2017-03-20 20:53 ` [PATCH 2/3] ArmPkg/DefaultExceptionHandlerLib: walk call stack unconditionally Ard Biesheuvel
@ 2017-03-20 20:53 ` Ard Biesheuvel
2017-03-22 13:18 ` Leif Lindholm
2 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2017-03-20 20:53 UTC (permalink / raw)
To: edk2-devel, leif.lindholm; +Cc: ryan.harkin, eugene, Ard Biesheuvel
In order to be able to produce meaningful diagnostic output when taking
synchronous exceptions that have been caused by corruption of the stack
pointer, prepare the EL0 stack pointer and switch to it when handling the
'Sync exception using SPx' exception class. Other exception classes (of
which we really only care about IrqSPx) are left functionally intact.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
Note that some code has been moved around so that the macro doesn't grow too
big to fit in a 128 byte slot, while keeping the code logically consistent.
ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c | 17 +++-
ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S | 86 ++++++++++++--------
ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf | 5 +-
3 files changed, 70 insertions(+), 38 deletions(-)
diff --git a/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c b/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
index 3d6eb4974d74..bd307628af87 100644
--- a/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
+++ b/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
@@ -16,7 +16,7 @@
#include <Uefi.h>
#include <Chipset/AArch64.h>
-
+#include <Library/MemoryAllocationLib.h>
#include <Protocol/DebugSupport.h> // for MAX_AARCH64_EXCEPTION
UINTN gMaxExceptionNumber = MAX_AARCH64_EXCEPTION;
@@ -25,11 +25,26 @@ EFI_EXCEPTION_CALLBACK gDebuggerExceptionHandlers[MAX_AARCH64_EXCEPTION + 1] =
PHYSICAL_ADDRESS gExceptionVectorAlignmentMask = ARM_VECTOR_TABLE_ALIGNMENT;
UINTN gDebuggerNoHandlerValue = 0; // todo: define for AArch64
+#define EL0_STACK_PAGES 2
+
+VOID
+RegisterEl0Stack (
+ IN VOID *Stack
+ );
+
RETURN_STATUS ArchVectorConfig(
IN UINTN VectorBaseAddress
)
{
UINTN HcrReg;
+ UINT8 *Stack;
+
+ Stack = AllocatePages (EL0_STACK_PAGES);
+ if (Stack == NULL) {
+ return RETURN_OUT_OF_RESOURCES;
+ }
+
+ RegisterEl0Stack ((UINT8 *)Stack + EFI_PAGES_TO_SIZE (EL0_STACK_PAGES));
if (ArmReadCurrentEL() == AARCH64_EL2) {
HcrReg = ArmReadHcr();
diff --git a/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S b/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
index ff1f5fc81316..ac426d72a150 100644
--- a/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
+++ b/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
@@ -100,6 +100,7 @@
GCC_ASM_EXPORT(ExceptionHandlersEnd)
GCC_ASM_EXPORT(CommonCExceptionHandler)
+GCC_ASM_EXPORT(RegisterEl0Stack)
.text
@@ -122,35 +123,41 @@ ASM_PFX(ExceptionHandlersStart):
VECTOR_BASE(ExceptionHandlersStart)
#endif
- .macro ExceptionEntry, val
+ .macro ExceptionEntry, val, sp=SPx
+ .ifnc \sp, SPx
+ msr SPsel, xzr
+ .endif
+
// Move the stackpointer so we can reach our structure with the str instruction.
sub sp, sp, #(FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE)
- // Push some GP registers so we can record the exception context
+ // Push the GP registers so we can record the exception context
stp x0, x1, [sp, #-GP_CONTEXT_SIZE]!
stp x2, x3, [sp, #0x10]
stp x4, x5, [sp, #0x20]
stp x6, x7, [sp, #0x30]
+ stp x8, x9, [sp, #0x40]
+ stp x10, x11, [sp, #0x50]
+ stp x12, x13, [sp, #0x60]
+ stp x14, x15, [sp, #0x70]
+ stp x16, x17, [sp, #0x80]
+ stp x18, x19, [sp, #0x90]
+ stp x20, x21, [sp, #0xa0]
+ stp x22, x23, [sp, #0xb0]
+ stp x24, x25, [sp, #0xc0]
+ stp x26, x27, [sp, #0xd0]
+ stp x28, x29, [sp, #0xe0]
+ add x28, sp, #(GP_CONTEXT_SIZE + FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE)
- EL1_OR_EL2_OR_EL3(x1)
-1:mrs x2, elr_el1 // Exception Link Register
- mrs x3, spsr_el1 // Saved Processor Status Register 32bit
- mrs x5, esr_el1 // EL1 Exception syndrome register 32bit
- mrs x6, far_el1 // EL1 Fault Address Register
- b 4f
-
-2:mrs x2, elr_el2 // Exception Link Register
- mrs x3, spsr_el2 // Saved Processor Status Register 32bit
- mrs x5, esr_el2 // EL2 Exception syndrome register 32bit
- mrs x6, far_el2 // EL2 Fault Address Register
- b 4f
-
-3:mrs x2, elr_el3 // Exception Link Register
- mrs x3, spsr_el3 // Saved Processor Status Register 32bit
- mrs x5, esr_el3 // EL3 Exception syndrome register 32bit
- mrs x6, far_el3 // EL3 Fault Address Register
+ .ifnc \sp, SPx
+ msr SPsel, #1 // Switch back to read the SP value upon entry
+ mov x7, sp
+ msr SPsel, xzr
+ .else
+ mov x7, x28 // x28 contains the SP value upon entry
+ .endif
-4:mrs x4, fpsr // Floating point Status Register 32bit
+ stp x30, x7, [sp, #0xf0]
// Record the type of exception that occurred.
mov x0, #\val
@@ -189,7 +196,7 @@ ASM_PFX(SErrorSP0):
//
VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_CUR_SPx_SYNC)
ASM_PFX(SynchronousExceptionSPx):
- ExceptionEntry EXCEPT_AARCH64_SYNCHRONOUS_EXCEPTIONS
+ ExceptionEntry EXCEPT_AARCH64_SYNCHRONOUS_EXCEPTIONS, SP0
VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_CUR_SPx_IRQ)
ASM_PFX(IrqSPx):
@@ -248,20 +255,25 @@ ASM_PFX(ExceptionHandlersEnd):
ASM_PFX(CommonExceptionEntry):
- // Stack the remaining GP registers
- stp x8, x9, [sp, #0x40]
- stp x10, x11, [sp, #0x50]
- stp x12, x13, [sp, #0x60]
- stp x14, x15, [sp, #0x70]
- stp x16, x17, [sp, #0x80]
- stp x18, x19, [sp, #0x90]
- stp x20, x21, [sp, #0xa0]
- stp x22, x23, [sp, #0xb0]
- stp x24, x25, [sp, #0xc0]
- stp x26, x27, [sp, #0xd0]
- stp x28, x29, [sp, #0xe0]
- add x28, sp, #GP_CONTEXT_SIZE + FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE
- stp x30, x28, [sp, #0xf0]
+ EL1_OR_EL2_OR_EL3(x1)
+1:mrs x2, elr_el1 // Exception Link Register
+ mrs x3, spsr_el1 // Saved Processor Status Register 32bit
+ mrs x5, esr_el1 // EL1 Exception syndrome register 32bit
+ mrs x6, far_el1 // EL1 Fault Address Register
+ b 4f
+
+2:mrs x2, elr_el2 // Exception Link Register
+ mrs x3, spsr_el2 // Saved Processor Status Register 32bit
+ mrs x5, esr_el2 // EL2 Exception syndrome register 32bit
+ mrs x6, far_el2 // EL2 Fault Address Register
+ b 4f
+
+3:mrs x2, elr_el3 // Exception Link Register
+ mrs x3, spsr_el3 // Saved Processor Status Register 32bit
+ mrs x5, esr_el3 // EL3 Exception syndrome register 32bit
+ mrs x6, far_el3 // EL3 Fault Address Register
+
+4:mrs x4, fpsr // Floating point Status Register 32bit
// Save the SYS regs
stp x2, x3, [x28, #-SYS_CONTEXT_SIZE]!
@@ -368,3 +380,7 @@ ASM_PFX(CommonExceptionEntry):
add sp, sp, #FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE
eret
+
+ASM_PFX(RegisterEl0Stack):
+ msr sp_el0, x0
+ ret
diff --git a/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf b/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
index 10d9ae0f4afc..cd9149cf76c6 100644
--- a/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
+++ b/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
@@ -53,10 +53,11 @@ [Packages]
[LibraryClasses]
ArmLib
- DebugLib
- DefaultExceptionHandlerLib
BaseMemoryLib
CacheMaintenanceLib
+ DebugLib
+ DefaultExceptionHandlerLib
+ MemoryAllocationLib
[Pcd]
gArmTokenSpaceGuid.PcdDebuggerExceptionSupport
--
2.7.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] ArmPkg/DefaultExceptionHandlerLib: add missing GUID to .inf
2017-03-20 20:52 ` [PATCH 1/3] ArmPkg/DefaultExceptionHandlerLib: add missing GUID to .inf Ard Biesheuvel
@ 2017-03-22 13:01 ` Leif Lindholm
0 siblings, 0 replies; 12+ messages in thread
From: Leif Lindholm @ 2017-03-22 13:01 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel, ryan.harkin, eugene
On Mon, Mar 20, 2017 at 08:52:59PM +0000, Ard Biesheuvel wrote:
> Add the gEfiDebugImageInfoTableGuid, which is referenced in the code,
> to both .INF files describing this module.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLib.inf | 3 +++
> ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf | 3 +++
> 2 files changed, 6 insertions(+)
>
> diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLib.inf b/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLib.inf
> index 5d3ce892ff3c..f5421b1240a1 100644
> --- a/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLib.inf
> +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLib.inf
> @@ -42,3 +42,6 @@ [LibraryClasses]
> PeCoffGetEntryPointLib
> ArmDisassemblerLib
> SerialPortLib
> +
> +[Guids]
> + gEfiDebugImageInfoTableGuid
> diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf b/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf
> index 8f5b3e1010c8..b53a5e89f507 100644
> --- a/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf
> +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLibBase.inf
> @@ -40,3 +40,6 @@ [LibraryClasses]
> PeCoffGetEntryPointLib
> ArmDisassemblerLib
> SerialPortLib
> +
> +[Guids]
> + gEfiDebugImageInfoTableGuid
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] ArmPkg/DefaultExceptionHandlerLib: walk call stack unconditionally
2017-03-20 20:53 ` [PATCH 2/3] ArmPkg/DefaultExceptionHandlerLib: walk call stack unconditionally Ard Biesheuvel
@ 2017-03-22 13:07 ` Leif Lindholm
2017-03-22 13:12 ` Ard Biesheuvel
0 siblings, 1 reply; 12+ messages in thread
From: Leif Lindholm @ 2017-03-22 13:07 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel, ryan.harkin, eugene
On Mon, Mar 20, 2017 at 08:53:00PM +0000, Ard Biesheuvel wrote:
> Currently, we only attempt to walk the call stack and print a backtrace
> if the program counter refers to a location covered by a PE/COFF image.
> However, regardless of the value of PC, the frame pointer may still have
> a meaningful value, and so we can still produce the remainder of the
> backtrace.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c | 56 +++++++++++---------
> 1 file changed, 31 insertions(+), 25 deletions(-)
>
> diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> index 2f9c2ede37c1..1024bf48c63d 100644
> --- a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> @@ -181,37 +181,43 @@ DefaultExceptionHandler (
> DEBUG ((EFI_D_ERROR, "PC 0x%012lx (0x%012lx+0x%08x) [ 0] %a\n",
> SystemContext.SystemContextAArch64->ELR, ImageBase,
> SystemContext.SystemContextAArch64->ELR - ImageBase, BaseName (Pdb)));
> + } else {
> + DEBUG ((EFI_D_ERROR, "PC 0x%012lx\n", SystemContext.SystemContextAArch64->ELR));
> + }
>
come_from:
> - if ((UINT64 *)SystemContext.SystemContextAArch64->FP != 0) {
> - Idx = 0;
> + if ((UINT64 *)SystemContext.SystemContextAArch64->FP != 0) {
> + Idx = 0;
>
> - RootFp[0] = ((UINT64 *)SystemContext.SystemContextAArch64->FP)[0];
> - RootFp[1] = ((UINT64 *)SystemContext.SystemContextAArch64->FP)[1];
> - if (RootFp[1] != SystemContext.SystemContextAArch64->LR) {
> - RootFp[0] = SystemContext.SystemContextAArch64->FP;
> - RootFp[1] = SystemContext.SystemContextAArch64->LR;
> - }
> - for (Fp = RootFp; Fp[0] != 0; Fp = (UINT64 *)Fp[0]) {
> - Pdb = GetImageName (Fp[1], &ImageBase, &PeCoffSizeOfHeader);
> - if (Pdb != NULL) {
> - if (Pdb != PrevPdb) {
> - Idx++;
> - PrevPdb = Pdb;
> - }
> - DEBUG ((EFI_D_ERROR, "PC 0x%012lx (0x%012lx+0x%08x) [% 2d] %a\n",
> - Fp[1], ImageBase, Fp[1] - ImageBase, Idx, BaseName (Pdb)));
> + RootFp[0] = ((UINT64 *)SystemContext.SystemContextAArch64->FP)[0];
> + RootFp[1] = ((UINT64 *)SystemContext.SystemContextAArch64->FP)[1];
> + if (RootFp[1] != SystemContext.SystemContextAArch64->LR) {
> + RootFp[0] = SystemContext.SystemContextAArch64->FP;
> + RootFp[1] = SystemContext.SystemContextAArch64->LR;
> + }
> + for (Fp = RootFp; Fp[0] != 0; Fp = (UINT64 *)Fp[0]) {
> + Pdb = GetImageName (Fp[1], &ImageBase, &PeCoffSizeOfHeader);
> + if (Pdb != NULL) {
> + if (Pdb != PrevPdb) {
> + Idx++;
> + PrevPdb = Pdb;
> }
> + DEBUG ((EFI_D_ERROR, "PC 0x%012lx (0x%012lx+0x%08x) [% 2d] %a\n",
> + Fp[1], ImageBase, Fp[1] - ImageBase, Idx, BaseName (Pdb)));
Diff's a bit iffy, but can you confirm there is no functional change
between come_from and here? Just the indentation shuffle?
> + } else {
> + DEBUG ((EFI_D_ERROR, "PC 0x%012lx\n", Fp[1]));
> }
> - PrevPdb = Pdb = GetImageName (SystemContext.SystemContextAArch64->ELR, &ImageBase, &PeCoffSizeOfHeader);
> + }
> + PrevPdb = Pdb = GetImageName (SystemContext.SystemContextAArch64->ELR, &ImageBase, &PeCoffSizeOfHeader);
> + if (Pdb != NULL) {
> DEBUG ((EFI_D_ERROR, "\n[ 0] %a\n", Pdb));
> + }
>
> - Idx = 0;
> - for (Fp = RootFp; Fp[0] != 0; Fp = (UINT64 *)Fp[0]) {
> - Pdb = GetImageName (Fp[1], &ImageBase, &PeCoffSizeOfHeader);
> - if (Pdb != NULL && Pdb != PrevPdb) {
> - DEBUG ((EFI_D_ERROR, "[% 2d] %a\n", ++Idx, Pdb));
> - PrevPdb = Pdb;
> - }
> + Idx = 0;
> + for (Fp = RootFp; Fp[0] != 0; Fp = (UINT64 *)Fp[0]) {
> + Pdb = GetImageName (Fp[1], &ImageBase, &PeCoffSizeOfHeader);
> + if (Pdb != NULL && Pdb != PrevPdb) {
> + DEBUG ((EFI_D_ERROR, "[% 2d] %a\n", ++Idx, Pdb));
> + PrevPdb = Pdb;
> }
> }
> }
> --
> 2.7.4
>
If so:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
/
Leif
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] ArmPkg/DefaultExceptionHandlerLib: walk call stack unconditionally
2017-03-22 13:07 ` Leif Lindholm
@ 2017-03-22 13:12 ` Ard Biesheuvel
2017-03-22 13:20 ` Leif Lindholm
0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2017-03-22 13:12 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel@lists.01.org, Ryan Harkin, Cohen, Eugene
On 22 March 2017 at 13:07, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Mon, Mar 20, 2017 at 08:53:00PM +0000, Ard Biesheuvel wrote:
>> Currently, we only attempt to walk the call stack and print a backtrace
>> if the program counter refers to a location covered by a PE/COFF image.
>> However, regardless of the value of PC, the frame pointer may still have
>> a meaningful value, and so we can still produce the remainder of the
>> backtrace.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c | 56 +++++++++++---------
>> 1 file changed, 31 insertions(+), 25 deletions(-)
>>
>> diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
>> index 2f9c2ede37c1..1024bf48c63d 100644
>> --- a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
>> +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
>> @@ -181,37 +181,43 @@ DefaultExceptionHandler (
>> DEBUG ((EFI_D_ERROR, "PC 0x%012lx (0x%012lx+0x%08x) [ 0] %a\n",
>> SystemContext.SystemContextAArch64->ELR, ImageBase,
>> SystemContext.SystemContextAArch64->ELR - ImageBase, BaseName (Pdb)));
>> + } else {
>> + DEBUG ((EFI_D_ERROR, "PC 0x%012lx\n", SystemContext.SystemContextAArch64->ELR));
>> + }
>>
>
> come_from:
>
>> - if ((UINT64 *)SystemContext.SystemContextAArch64->FP != 0) {
>> - Idx = 0;
>> + if ((UINT64 *)SystemContext.SystemContextAArch64->FP != 0) {
>> + Idx = 0;
>>
>> - RootFp[0] = ((UINT64 *)SystemContext.SystemContextAArch64->FP)[0];
>> - RootFp[1] = ((UINT64 *)SystemContext.SystemContextAArch64->FP)[1];
>> - if (RootFp[1] != SystemContext.SystemContextAArch64->LR) {
>> - RootFp[0] = SystemContext.SystemContextAArch64->FP;
>> - RootFp[1] = SystemContext.SystemContextAArch64->LR;
>> - }
>> - for (Fp = RootFp; Fp[0] != 0; Fp = (UINT64 *)Fp[0]) {
>> - Pdb = GetImageName (Fp[1], &ImageBase, &PeCoffSizeOfHeader);
>> - if (Pdb != NULL) {
>> - if (Pdb != PrevPdb) {
>> - Idx++;
>> - PrevPdb = Pdb;
>> - }
>> - DEBUG ((EFI_D_ERROR, "PC 0x%012lx (0x%012lx+0x%08x) [% 2d] %a\n",
>> - Fp[1], ImageBase, Fp[1] - ImageBase, Idx, BaseName (Pdb)));
>> + RootFp[0] = ((UINT64 *)SystemContext.SystemContextAArch64->FP)[0];
>> + RootFp[1] = ((UINT64 *)SystemContext.SystemContextAArch64->FP)[1];
>> + if (RootFp[1] != SystemContext.SystemContextAArch64->LR) {
>> + RootFp[0] = SystemContext.SystemContextAArch64->FP;
>> + RootFp[1] = SystemContext.SystemContextAArch64->LR;
>> + }
>> + for (Fp = RootFp; Fp[0] != 0; Fp = (UINT64 *)Fp[0]) {
>> + Pdb = GetImageName (Fp[1], &ImageBase, &PeCoffSizeOfHeader);
>> + if (Pdb != NULL) {
>> + if (Pdb != PrevPdb) {
>> + Idx++;
>> + PrevPdb = Pdb;
>> }
>> + DEBUG ((EFI_D_ERROR, "PC 0x%012lx (0x%012lx+0x%08x) [% 2d] %a\n",
>> + Fp[1], ImageBase, Fp[1] - ImageBase, Idx, BaseName (Pdb)));
>
> Diff's a bit iffy, but can you confirm there is no functional change
> between come_from and here? Just the indentation shuffle?
>
Yes. diff -w output is here http://pastebin.com/gRmeeipp
>> + } else {
>> + DEBUG ((EFI_D_ERROR, "PC 0x%012lx\n", Fp[1]));
>> }
>> - PrevPdb = Pdb = GetImageName (SystemContext.SystemContextAArch64->ELR, &ImageBase, &PeCoffSizeOfHeader);
>> + }
>> + PrevPdb = Pdb = GetImageName (SystemContext.SystemContextAArch64->ELR, &ImageBase, &PeCoffSizeOfHeader);
>> + if (Pdb != NULL) {
>> DEBUG ((EFI_D_ERROR, "\n[ 0] %a\n", Pdb));
>> + }
>>
>> - Idx = 0;
>> - for (Fp = RootFp; Fp[0] != 0; Fp = (UINT64 *)Fp[0]) {
>> - Pdb = GetImageName (Fp[1], &ImageBase, &PeCoffSizeOfHeader);
>> - if (Pdb != NULL && Pdb != PrevPdb) {
>> - DEBUG ((EFI_D_ERROR, "[% 2d] %a\n", ++Idx, Pdb));
>> - PrevPdb = Pdb;
>> - }
>> + Idx = 0;
>> + for (Fp = RootFp; Fp[0] != 0; Fp = (UINT64 *)Fp[0]) {
>> + Pdb = GetImageName (Fp[1], &ImageBase, &PeCoffSizeOfHeader);
>> + if (Pdb != NULL && Pdb != PrevPdb) {
>> + DEBUG ((EFI_D_ERROR, "[% 2d] %a\n", ++Idx, Pdb));
>> + PrevPdb = Pdb;
>> }
>> }
>> }
>> --
>> 2.7.4
>>
>
> If so:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
Thanks
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] ArmPkg/ArmExceptionLib: use EL0 stack for synchronous exceptions
2017-03-20 20:53 ` [PATCH 3/3] ArmPkg/ArmExceptionLib: use EL0 stack for synchronous exceptions Ard Biesheuvel
@ 2017-03-22 13:18 ` Leif Lindholm
2017-03-22 13:20 ` Ard Biesheuvel
2017-03-27 12:54 ` Ard Biesheuvel
0 siblings, 2 replies; 12+ messages in thread
From: Leif Lindholm @ 2017-03-22 13:18 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel, ryan.harkin, eugene
On Mon, Mar 20, 2017 at 08:53:01PM +0000, Ard Biesheuvel wrote:
> In order to be able to produce meaningful diagnostic output when taking
> synchronous exceptions that have been caused by corruption of the stack
> pointer, prepare the EL0 stack pointer and switch to it when handling the
> 'Sync exception using SPx' exception class. Other exception classes (of
> which we really only care about IrqSPx) are left functionally intact.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>
> Note that some code has been moved around so that the macro doesn't grow too
> big to fit in a 128 byte slot, while keeping the code logically consistent.
>
> ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c | 17 +++-
> ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S | 86 ++++++++++++--------
> ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf | 5 +-
> 3 files changed, 70 insertions(+), 38 deletions(-)
>
> diff --git a/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c b/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
> index 3d6eb4974d74..bd307628af87 100644
> --- a/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
> +++ b/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
> @@ -16,7 +16,7 @@
> #include <Uefi.h>
>
> #include <Chipset/AArch64.h>
> -
> +#include <Library/MemoryAllocationLib.h>
> #include <Protocol/DebugSupport.h> // for MAX_AARCH64_EXCEPTION
>
> UINTN gMaxExceptionNumber = MAX_AARCH64_EXCEPTION;
> @@ -25,11 +25,26 @@ EFI_EXCEPTION_CALLBACK gDebuggerExceptionHandlers[MAX_AARCH64_EXCEPTION + 1] =
> PHYSICAL_ADDRESS gExceptionVectorAlignmentMask = ARM_VECTOR_TABLE_ALIGNMENT;
> UINTN gDebuggerNoHandlerValue = 0; // todo: define for AArch64
>
> +#define EL0_STACK_PAGES 2
> +
> +VOID
> +RegisterEl0Stack (
> + IN VOID *Stack
> + );
> +
> RETURN_STATUS ArchVectorConfig(
> IN UINTN VectorBaseAddress
> )
> {
> UINTN HcrReg;
> + UINT8 *Stack;
> +
> + Stack = AllocatePages (EL0_STACK_PAGES);
> + if (Stack == NULL) {
> + return RETURN_OUT_OF_RESOURCES;
> + }
> +
> + RegisterEl0Stack ((UINT8 *)Stack + EFI_PAGES_TO_SIZE (EL0_STACK_PAGES));
>
> if (ArmReadCurrentEL() == AARCH64_EL2) {
> HcrReg = ArmReadHcr();
> diff --git a/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S b/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
> index ff1f5fc81316..ac426d72a150 100644
> --- a/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
> +++ b/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
> @@ -100,6 +100,7 @@
>
> GCC_ASM_EXPORT(ExceptionHandlersEnd)
> GCC_ASM_EXPORT(CommonCExceptionHandler)
> +GCC_ASM_EXPORT(RegisterEl0Stack)
>
> .text
>
> @@ -122,35 +123,41 @@ ASM_PFX(ExceptionHandlersStart):
> VECTOR_BASE(ExceptionHandlersStart)
> #endif
>
> - .macro ExceptionEntry, val
> + .macro ExceptionEntry, val, sp=SPx
> + .ifnc \sp, SPx
> + msr SPsel, xzr
> + .endif
Is this esoteric enough to motivate a comment?
> +
> // Move the stackpointer so we can reach our structure with the str instruction.
> sub sp, sp, #(FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE)
>
> - // Push some GP registers so we can record the exception context
> + // Push the GP registers so we can record the exception context
> stp x0, x1, [sp, #-GP_CONTEXT_SIZE]!
> stp x2, x3, [sp, #0x10]
> stp x4, x5, [sp, #0x20]
> stp x6, x7, [sp, #0x30]
> + stp x8, x9, [sp, #0x40]
> + stp x10, x11, [sp, #0x50]
> + stp x12, x13, [sp, #0x60]
> + stp x14, x15, [sp, #0x70]
> + stp x16, x17, [sp, #0x80]
> + stp x18, x19, [sp, #0x90]
> + stp x20, x21, [sp, #0xa0]
> + stp x22, x23, [sp, #0xb0]
> + stp x24, x25, [sp, #0xc0]
> + stp x26, x27, [sp, #0xd0]
> + stp x28, x29, [sp, #0xe0]
> + add x28, sp, #(GP_CONTEXT_SIZE + FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE)
>
> - EL1_OR_EL2_OR_EL3(x1)
> -1:mrs x2, elr_el1 // Exception Link Register
> - mrs x3, spsr_el1 // Saved Processor Status Register 32bit
> - mrs x5, esr_el1 // EL1 Exception syndrome register 32bit
> - mrs x6, far_el1 // EL1 Fault Address Register
> - b 4f
> -
> -2:mrs x2, elr_el2 // Exception Link Register
> - mrs x3, spsr_el2 // Saved Processor Status Register 32bit
> - mrs x5, esr_el2 // EL2 Exception syndrome register 32bit
> - mrs x6, far_el2 // EL2 Fault Address Register
> - b 4f
> -
> -3:mrs x2, elr_el3 // Exception Link Register
> - mrs x3, spsr_el3 // Saved Processor Status Register 32bit
> - mrs x5, esr_el3 // EL3 Exception syndrome register 32bit
> - mrs x6, far_el3 // EL3 Fault Address Register
> + .ifnc \sp, SPx
> + msr SPsel, #1 // Switch back to read the SP value upon entry
> + mov x7, sp
> + msr SPsel, xzr
> + .else
> + mov x7, x28 // x28 contains the SP value upon entry
> + .endif
>
> -4:mrs x4, fpsr // Floating point Status Register 32bit
> + stp x30, x7, [sp, #0xf0]
>
> // Record the type of exception that occurred.
> mov x0, #\val
> @@ -189,7 +196,7 @@ ASM_PFX(SErrorSP0):
> //
> VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_CUR_SPx_SYNC)
> ASM_PFX(SynchronousExceptionSPx):
> - ExceptionEntry EXCEPT_AARCH64_SYNCHRONOUS_EXCEPTIONS
> + ExceptionEntry EXCEPT_AARCH64_SYNCHRONOUS_EXCEPTIONS, SP0
>
> VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_CUR_SPx_IRQ)
> ASM_PFX(IrqSPx):
> @@ -248,20 +255,25 @@ ASM_PFX(ExceptionHandlersEnd):
>
> ASM_PFX(CommonExceptionEntry):
>
> - // Stack the remaining GP registers
> - stp x8, x9, [sp, #0x40]
> - stp x10, x11, [sp, #0x50]
> - stp x12, x13, [sp, #0x60]
> - stp x14, x15, [sp, #0x70]
> - stp x16, x17, [sp, #0x80]
> - stp x18, x19, [sp, #0x90]
> - stp x20, x21, [sp, #0xa0]
> - stp x22, x23, [sp, #0xb0]
> - stp x24, x25, [sp, #0xc0]
> - stp x26, x27, [sp, #0xd0]
> - stp x28, x29, [sp, #0xe0]
> - add x28, sp, #GP_CONTEXT_SIZE + FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE
> - stp x30, x28, [sp, #0xf0]
> + EL1_OR_EL2_OR_EL3(x1)
> +1:mrs x2, elr_el1 // Exception Link Register
> + mrs x3, spsr_el1 // Saved Processor Status Register 32bit
> + mrs x5, esr_el1 // EL1 Exception syndrome register 32bit
> + mrs x6, far_el1 // EL1 Fault Address Register
> + b 4f
> +
> +2:mrs x2, elr_el2 // Exception Link Register
> + mrs x3, spsr_el2 // Saved Processor Status Register 32bit
> + mrs x5, esr_el2 // EL2 Exception syndrome register 32bit
> + mrs x6, far_el2 // EL2 Fault Address Register
> + b 4f
> +
> +3:mrs x2, elr_el3 // Exception Link Register
> + mrs x3, spsr_el3 // Saved Processor Status Register 32bit
> + mrs x5, esr_el3 // EL3 Exception syndrome register 32bit
> + mrs x6, far_el3 // EL3 Fault Address Register
> +
> +4:mrs x4, fpsr // Floating point Status Register 32bit
>
> // Save the SYS regs
> stp x2, x3, [x28, #-SYS_CONTEXT_SIZE]!
> @@ -368,3 +380,7 @@ ASM_PFX(CommonExceptionEntry):
> add sp, sp, #FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE
>
> eret
> +
> +ASM_PFX(RegisterEl0Stack):
> + msr sp_el0, x0
> + ret
> diff --git a/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf b/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
> index 10d9ae0f4afc..cd9149cf76c6 100644
> --- a/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
> +++ b/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
> @@ -53,10 +53,11 @@ [Packages]
>
> [LibraryClasses]
> ArmLib
> - DebugLib
> - DefaultExceptionHandlerLib
> BaseMemoryLib
> CacheMaintenanceLib
> + DebugLib
> + DefaultExceptionHandlerLib
> + MemoryAllocationLib
>
> [Pcd]
> gArmTokenSpaceGuid.PcdDebuggerExceptionSupport
> --
> 2.7.4
>
Not tested, but it looks quite plausible, so if Eugene agrees:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] ArmPkg/DefaultExceptionHandlerLib: walk call stack unconditionally
2017-03-22 13:12 ` Ard Biesheuvel
@ 2017-03-22 13:20 ` Leif Lindholm
0 siblings, 0 replies; 12+ messages in thread
From: Leif Lindholm @ 2017-03-22 13:20 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.org, Ryan Harkin, Cohen, Eugene
On Wed, Mar 22, 2017 at 01:12:59PM +0000, Ard Biesheuvel wrote:
> On 22 March 2017 at 13:07, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Mon, Mar 20, 2017 at 08:53:00PM +0000, Ard Biesheuvel wrote:
> >> Currently, we only attempt to walk the call stack and print a backtrace
> >> if the program counter refers to a location covered by a PE/COFF image.
> >> However, regardless of the value of PC, the frame pointer may still have
> >> a meaningful value, and so we can still produce the remainder of the
> >> backtrace.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >> ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c | 56 +++++++++++---------
> >> 1 file changed, 31 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> >> index 2f9c2ede37c1..1024bf48c63d 100644
> >> --- a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> >> +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> >> @@ -181,37 +181,43 @@ DefaultExceptionHandler (
> >> DEBUG ((EFI_D_ERROR, "PC 0x%012lx (0x%012lx+0x%08x) [ 0] %a\n",
> >> SystemContext.SystemContextAArch64->ELR, ImageBase,
> >> SystemContext.SystemContextAArch64->ELR - ImageBase, BaseName (Pdb)));
> >> + } else {
> >> + DEBUG ((EFI_D_ERROR, "PC 0x%012lx\n", SystemContext.SystemContextAArch64->ELR));
> >> + }
> >>
> >
> > come_from:
> >
> >> - if ((UINT64 *)SystemContext.SystemContextAArch64->FP != 0) {
> >> - Idx = 0;
> >> + if ((UINT64 *)SystemContext.SystemContextAArch64->FP != 0) {
> >> + Idx = 0;
> >>
> >> - RootFp[0] = ((UINT64 *)SystemContext.SystemContextAArch64->FP)[0];
> >> - RootFp[1] = ((UINT64 *)SystemContext.SystemContextAArch64->FP)[1];
> >> - if (RootFp[1] != SystemContext.SystemContextAArch64->LR) {
> >> - RootFp[0] = SystemContext.SystemContextAArch64->FP;
> >> - RootFp[1] = SystemContext.SystemContextAArch64->LR;
> >> - }
> >> - for (Fp = RootFp; Fp[0] != 0; Fp = (UINT64 *)Fp[0]) {
> >> - Pdb = GetImageName (Fp[1], &ImageBase, &PeCoffSizeOfHeader);
> >> - if (Pdb != NULL) {
> >> - if (Pdb != PrevPdb) {
> >> - Idx++;
> >> - PrevPdb = Pdb;
> >> - }
> >> - DEBUG ((EFI_D_ERROR, "PC 0x%012lx (0x%012lx+0x%08x) [% 2d] %a\n",
> >> - Fp[1], ImageBase, Fp[1] - ImageBase, Idx, BaseName (Pdb)));
> >> + RootFp[0] = ((UINT64 *)SystemContext.SystemContextAArch64->FP)[0];
> >> + RootFp[1] = ((UINT64 *)SystemContext.SystemContextAArch64->FP)[1];
> >> + if (RootFp[1] != SystemContext.SystemContextAArch64->LR) {
> >> + RootFp[0] = SystemContext.SystemContextAArch64->FP;
> >> + RootFp[1] = SystemContext.SystemContextAArch64->LR;
> >> + }
> >> + for (Fp = RootFp; Fp[0] != 0; Fp = (UINT64 *)Fp[0]) {
> >> + Pdb = GetImageName (Fp[1], &ImageBase, &PeCoffSizeOfHeader);
> >> + if (Pdb != NULL) {
> >> + if (Pdb != PrevPdb) {
> >> + Idx++;
> >> + PrevPdb = Pdb;
> >> }
> >> + DEBUG ((EFI_D_ERROR, "PC 0x%012lx (0x%012lx+0x%08x) [% 2d] %a\n",
> >> + Fp[1], ImageBase, Fp[1] - ImageBase, Idx, BaseName (Pdb)));
> >
> > Diff's a bit iffy, but can you confirm there is no functional change
> > between come_from and here? Just the indentation shuffle?
> >
>
> Yes. diff -w output is here http://pastebin.com/gRmeeipp
Thanks - that's a tad more readable :)
> >> + } else {
> >> + DEBUG ((EFI_D_ERROR, "PC 0x%012lx\n", Fp[1]));
> >> }
> >> - PrevPdb = Pdb = GetImageName (SystemContext.SystemContextAArch64->ELR, &ImageBase, &PeCoffSizeOfHeader);
> >> + }
> >> + PrevPdb = Pdb = GetImageName (SystemContext.SystemContextAArch64->ELR, &ImageBase, &PeCoffSizeOfHeader);
> >> + if (Pdb != NULL) {
> >> DEBUG ((EFI_D_ERROR, "\n[ 0] %a\n", Pdb));
> >> + }
> >>
> >> - Idx = 0;
> >> - for (Fp = RootFp; Fp[0] != 0; Fp = (UINT64 *)Fp[0]) {
> >> - Pdb = GetImageName (Fp[1], &ImageBase, &PeCoffSizeOfHeader);
> >> - if (Pdb != NULL && Pdb != PrevPdb) {
> >> - DEBUG ((EFI_D_ERROR, "[% 2d] %a\n", ++Idx, Pdb));
> >> - PrevPdb = Pdb;
> >> - }
> >> + Idx = 0;
> >> + for (Fp = RootFp; Fp[0] != 0; Fp = (UINT64 *)Fp[0]) {
> >> + Pdb = GetImageName (Fp[1], &ImageBase, &PeCoffSizeOfHeader);
> >> + if (Pdb != NULL && Pdb != PrevPdb) {
> >> + DEBUG ((EFI_D_ERROR, "[% 2d] %a\n", ++Idx, Pdb));
> >> + PrevPdb = Pdb;
> >> }
> >> }
> >> }
> >> --
> >> 2.7.4
> >>
> >
> > If so:
> > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> >
>
> Thanks
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] ArmPkg/ArmExceptionLib: use EL0 stack for synchronous exceptions
2017-03-22 13:18 ` Leif Lindholm
@ 2017-03-22 13:20 ` Ard Biesheuvel
2017-03-27 12:54 ` Ard Biesheuvel
1 sibling, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2017-03-22 13:20 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel@lists.01.org, Ryan Harkin, Cohen, Eugene
On 22 March 2017 at 13:18, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Mon, Mar 20, 2017 at 08:53:01PM +0000, Ard Biesheuvel wrote:
>> In order to be able to produce meaningful diagnostic output when taking
>> synchronous exceptions that have been caused by corruption of the stack
>> pointer, prepare the EL0 stack pointer and switch to it when handling the
>> 'Sync exception using SPx' exception class. Other exception classes (of
>> which we really only care about IrqSPx) are left functionally intact.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>
>> Note that some code has been moved around so that the macro doesn't grow too
>> big to fit in a 128 byte slot, while keeping the code logically consistent.
>>
>> ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c | 17 +++-
>> ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S | 86 ++++++++++++--------
>> ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf | 5 +-
>> 3 files changed, 70 insertions(+), 38 deletions(-)
>>
>> diff --git a/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c b/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
>> index 3d6eb4974d74..bd307628af87 100644
>> --- a/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
>> +++ b/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
>> @@ -16,7 +16,7 @@
>> #include <Uefi.h>
>>
>> #include <Chipset/AArch64.h>
>> -
>> +#include <Library/MemoryAllocationLib.h>
>> #include <Protocol/DebugSupport.h> // for MAX_AARCH64_EXCEPTION
>>
>> UINTN gMaxExceptionNumber = MAX_AARCH64_EXCEPTION;
>> @@ -25,11 +25,26 @@ EFI_EXCEPTION_CALLBACK gDebuggerExceptionHandlers[MAX_AARCH64_EXCEPTION + 1] =
>> PHYSICAL_ADDRESS gExceptionVectorAlignmentMask = ARM_VECTOR_TABLE_ALIGNMENT;
>> UINTN gDebuggerNoHandlerValue = 0; // todo: define for AArch64
>>
>> +#define EL0_STACK_PAGES 2
>> +
>> +VOID
>> +RegisterEl0Stack (
>> + IN VOID *Stack
>> + );
>> +
>> RETURN_STATUS ArchVectorConfig(
>> IN UINTN VectorBaseAddress
>> )
>> {
>> UINTN HcrReg;
>> + UINT8 *Stack;
>> +
>> + Stack = AllocatePages (EL0_STACK_PAGES);
>> + if (Stack == NULL) {
>> + return RETURN_OUT_OF_RESOURCES;
>> + }
>> +
>> + RegisterEl0Stack ((UINT8 *)Stack + EFI_PAGES_TO_SIZE (EL0_STACK_PAGES));
>>
>> if (ArmReadCurrentEL() == AARCH64_EL2) {
>> HcrReg = ArmReadHcr();
>> diff --git a/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S b/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
>> index ff1f5fc81316..ac426d72a150 100644
>> --- a/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
>> +++ b/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
>> @@ -100,6 +100,7 @@
>>
>> GCC_ASM_EXPORT(ExceptionHandlersEnd)
>> GCC_ASM_EXPORT(CommonCExceptionHandler)
>> +GCC_ASM_EXPORT(RegisterEl0Stack)
>>
>> .text
>>
>> @@ -122,35 +123,41 @@ ASM_PFX(ExceptionHandlersStart):
>> VECTOR_BASE(ExceptionHandlersStart)
>> #endif
>>
>> - .macro ExceptionEntry, val
>> + .macro ExceptionEntry, val, sp=SPx
>> + .ifnc \sp, SPx
>> + msr SPsel, xzr
>> + .endif
>
> Is this esoteric enough to motivate a comment?
>
Yeah, good point. This looks like a suitable spot to dedicate some
space to explain what's going on
>> +
>> // Move the stackpointer so we can reach our structure with the str instruction.
>> sub sp, sp, #(FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE)
>>
>> - // Push some GP registers so we can record the exception context
>> + // Push the GP registers so we can record the exception context
>> stp x0, x1, [sp, #-GP_CONTEXT_SIZE]!
>> stp x2, x3, [sp, #0x10]
>> stp x4, x5, [sp, #0x20]
>> stp x6, x7, [sp, #0x30]
>> + stp x8, x9, [sp, #0x40]
>> + stp x10, x11, [sp, #0x50]
>> + stp x12, x13, [sp, #0x60]
>> + stp x14, x15, [sp, #0x70]
>> + stp x16, x17, [sp, #0x80]
>> + stp x18, x19, [sp, #0x90]
>> + stp x20, x21, [sp, #0xa0]
>> + stp x22, x23, [sp, #0xb0]
>> + stp x24, x25, [sp, #0xc0]
>> + stp x26, x27, [sp, #0xd0]
>> + stp x28, x29, [sp, #0xe0]
>> + add x28, sp, #(GP_CONTEXT_SIZE + FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE)
>>
>> - EL1_OR_EL2_OR_EL3(x1)
>> -1:mrs x2, elr_el1 // Exception Link Register
>> - mrs x3, spsr_el1 // Saved Processor Status Register 32bit
>> - mrs x5, esr_el1 // EL1 Exception syndrome register 32bit
>> - mrs x6, far_el1 // EL1 Fault Address Register
>> - b 4f
>> -
>> -2:mrs x2, elr_el2 // Exception Link Register
>> - mrs x3, spsr_el2 // Saved Processor Status Register 32bit
>> - mrs x5, esr_el2 // EL2 Exception syndrome register 32bit
>> - mrs x6, far_el2 // EL2 Fault Address Register
>> - b 4f
>> -
>> -3:mrs x2, elr_el3 // Exception Link Register
>> - mrs x3, spsr_el3 // Saved Processor Status Register 32bit
>> - mrs x5, esr_el3 // EL3 Exception syndrome register 32bit
>> - mrs x6, far_el3 // EL3 Fault Address Register
>> + .ifnc \sp, SPx
>> + msr SPsel, #1 // Switch back to read the SP value upon entry
>> + mov x7, sp
>> + msr SPsel, xzr
>> + .else
>> + mov x7, x28 // x28 contains the SP value upon entry
>> + .endif
>>
>> -4:mrs x4, fpsr // Floating point Status Register 32bit
>> + stp x30, x7, [sp, #0xf0]
>>
>> // Record the type of exception that occurred.
>> mov x0, #\val
>> @@ -189,7 +196,7 @@ ASM_PFX(SErrorSP0):
>> //
>> VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_CUR_SPx_SYNC)
>> ASM_PFX(SynchronousExceptionSPx):
>> - ExceptionEntry EXCEPT_AARCH64_SYNCHRONOUS_EXCEPTIONS
>> + ExceptionEntry EXCEPT_AARCH64_SYNCHRONOUS_EXCEPTIONS, SP0
>>
>> VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_CUR_SPx_IRQ)
>> ASM_PFX(IrqSPx):
>> @@ -248,20 +255,25 @@ ASM_PFX(ExceptionHandlersEnd):
>>
>> ASM_PFX(CommonExceptionEntry):
>>
>> - // Stack the remaining GP registers
>> - stp x8, x9, [sp, #0x40]
>> - stp x10, x11, [sp, #0x50]
>> - stp x12, x13, [sp, #0x60]
>> - stp x14, x15, [sp, #0x70]
>> - stp x16, x17, [sp, #0x80]
>> - stp x18, x19, [sp, #0x90]
>> - stp x20, x21, [sp, #0xa0]
>> - stp x22, x23, [sp, #0xb0]
>> - stp x24, x25, [sp, #0xc0]
>> - stp x26, x27, [sp, #0xd0]
>> - stp x28, x29, [sp, #0xe0]
>> - add x28, sp, #GP_CONTEXT_SIZE + FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE
>> - stp x30, x28, [sp, #0xf0]
>> + EL1_OR_EL2_OR_EL3(x1)
>> +1:mrs x2, elr_el1 // Exception Link Register
>> + mrs x3, spsr_el1 // Saved Processor Status Register 32bit
>> + mrs x5, esr_el1 // EL1 Exception syndrome register 32bit
>> + mrs x6, far_el1 // EL1 Fault Address Register
>> + b 4f
>> +
>> +2:mrs x2, elr_el2 // Exception Link Register
>> + mrs x3, spsr_el2 // Saved Processor Status Register 32bit
>> + mrs x5, esr_el2 // EL2 Exception syndrome register 32bit
>> + mrs x6, far_el2 // EL2 Fault Address Register
>> + b 4f
>> +
>> +3:mrs x2, elr_el3 // Exception Link Register
>> + mrs x3, spsr_el3 // Saved Processor Status Register 32bit
>> + mrs x5, esr_el3 // EL3 Exception syndrome register 32bit
>> + mrs x6, far_el3 // EL3 Fault Address Register
>> +
>> +4:mrs x4, fpsr // Floating point Status Register 32bit
>>
>> // Save the SYS regs
>> stp x2, x3, [x28, #-SYS_CONTEXT_SIZE]!
>> @@ -368,3 +380,7 @@ ASM_PFX(CommonExceptionEntry):
>> add sp, sp, #FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE
>>
>> eret
>> +
>> +ASM_PFX(RegisterEl0Stack):
>> + msr sp_el0, x0
>> + ret
>> diff --git a/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf b/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
>> index 10d9ae0f4afc..cd9149cf76c6 100644
>> --- a/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
>> +++ b/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
>> @@ -53,10 +53,11 @@ [Packages]
>>
>> [LibraryClasses]
>> ArmLib
>> - DebugLib
>> - DefaultExceptionHandlerLib
>> BaseMemoryLib
>> CacheMaintenanceLib
>> + DebugLib
>> + DefaultExceptionHandlerLib
>> + MemoryAllocationLib
>>
>> [Pcd]
>> gArmTokenSpaceGuid.PcdDebuggerExceptionSupport
>> --
>> 2.7.4
>>
>
> Not tested, but it looks quite plausible, so if Eugene agrees:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] ArmPkg/ArmExceptionLib: use EL0 stack for synchronous exceptions
2017-03-22 13:18 ` Leif Lindholm
2017-03-22 13:20 ` Ard Biesheuvel
@ 2017-03-27 12:54 ` Ard Biesheuvel
2017-03-27 12:55 ` Ard Biesheuvel
1 sibling, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2017-03-27 12:54 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel@lists.01.org, Ryan Harkin, Cohen, Eugene
On 22 March 2017 at 13:18, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Mon, Mar 20, 2017 at 08:53:01PM +0000, Ard Biesheuvel wrote:
>> In order to be able to produce meaningful diagnostic output when taking
>> synchronous exceptions that have been caused by corruption of the stack
>> pointer, prepare the EL0 stack pointer and switch to it when handling the
>> 'Sync exception using SPx' exception class. Other exception classes (of
>> which we really only care about IrqSPx) are left functionally intact.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>
>> Note that some code has been moved around so that the macro doesn't grow too
>> big to fit in a 128 byte slot, while keeping the code logically consistent.
>>
>> ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c | 17 +++-
>> ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S | 86 ++++++++++++--------
>> ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf | 5 +-
>> 3 files changed, 70 insertions(+), 38 deletions(-)
>>
>> diff --git a/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c b/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
>> index 3d6eb4974d74..bd307628af87 100644
>> --- a/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
>> +++ b/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
>> @@ -16,7 +16,7 @@
>> #include <Uefi.h>
>>
>> #include <Chipset/AArch64.h>
>> -
>> +#include <Library/MemoryAllocationLib.h>
>> #include <Protocol/DebugSupport.h> // for MAX_AARCH64_EXCEPTION
>>
>> UINTN gMaxExceptionNumber = MAX_AARCH64_EXCEPTION;
>> @@ -25,11 +25,26 @@ EFI_EXCEPTION_CALLBACK gDebuggerExceptionHandlers[MAX_AARCH64_EXCEPTION + 1] =
>> PHYSICAL_ADDRESS gExceptionVectorAlignmentMask = ARM_VECTOR_TABLE_ALIGNMENT;
>> UINTN gDebuggerNoHandlerValue = 0; // todo: define for AArch64
>>
>> +#define EL0_STACK_PAGES 2
>> +
>> +VOID
>> +RegisterEl0Stack (
>> + IN VOID *Stack
>> + );
>> +
>> RETURN_STATUS ArchVectorConfig(
>> IN UINTN VectorBaseAddress
>> )
>> {
>> UINTN HcrReg;
>> + UINT8 *Stack;
>> +
>> + Stack = AllocatePages (EL0_STACK_PAGES);
>> + if (Stack == NULL) {
>> + return RETURN_OUT_OF_RESOURCES;
>> + }
>> +
>> + RegisterEl0Stack ((UINT8 *)Stack + EFI_PAGES_TO_SIZE (EL0_STACK_PAGES));
>>
>> if (ArmReadCurrentEL() == AARCH64_EL2) {
>> HcrReg = ArmReadHcr();
>> diff --git a/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S b/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
>> index ff1f5fc81316..ac426d72a150 100644
>> --- a/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
>> +++ b/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
>> @@ -100,6 +100,7 @@
>>
>> GCC_ASM_EXPORT(ExceptionHandlersEnd)
>> GCC_ASM_EXPORT(CommonCExceptionHandler)
>> +GCC_ASM_EXPORT(RegisterEl0Stack)
>>
>> .text
>>
>> @@ -122,35 +123,41 @@ ASM_PFX(ExceptionHandlersStart):
>> VECTOR_BASE(ExceptionHandlersStart)
>> #endif
>>
>> - .macro ExceptionEntry, val
>> + .macro ExceptionEntry, val, sp=SPx
>> + .ifnc \sp, SPx
>> + msr SPsel, xzr
>> + .endif
>
> Is this esoteric enough to motivate a comment?
>
>> +
>> // Move the stackpointer so we can reach our structure with the str instruction.
>> sub sp, sp, #(FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE)
>>
>> - // Push some GP registers so we can record the exception context
>> + // Push the GP registers so we can record the exception context
>> stp x0, x1, [sp, #-GP_CONTEXT_SIZE]!
>> stp x2, x3, [sp, #0x10]
>> stp x4, x5, [sp, #0x20]
>> stp x6, x7, [sp, #0x30]
>> + stp x8, x9, [sp, #0x40]
>> + stp x10, x11, [sp, #0x50]
>> + stp x12, x13, [sp, #0x60]
>> + stp x14, x15, [sp, #0x70]
>> + stp x16, x17, [sp, #0x80]
>> + stp x18, x19, [sp, #0x90]
>> + stp x20, x21, [sp, #0xa0]
>> + stp x22, x23, [sp, #0xb0]
>> + stp x24, x25, [sp, #0xc0]
>> + stp x26, x27, [sp, #0xd0]
>> + stp x28, x29, [sp, #0xe0]
>> + add x28, sp, #(GP_CONTEXT_SIZE + FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE)
>>
>> - EL1_OR_EL2_OR_EL3(x1)
>> -1:mrs x2, elr_el1 // Exception Link Register
>> - mrs x3, spsr_el1 // Saved Processor Status Register 32bit
>> - mrs x5, esr_el1 // EL1 Exception syndrome register 32bit
>> - mrs x6, far_el1 // EL1 Fault Address Register
>> - b 4f
>> -
>> -2:mrs x2, elr_el2 // Exception Link Register
>> - mrs x3, spsr_el2 // Saved Processor Status Register 32bit
>> - mrs x5, esr_el2 // EL2 Exception syndrome register 32bit
>> - mrs x6, far_el2 // EL2 Fault Address Register
>> - b 4f
>> -
>> -3:mrs x2, elr_el3 // Exception Link Register
>> - mrs x3, spsr_el3 // Saved Processor Status Register 32bit
>> - mrs x5, esr_el3 // EL3 Exception syndrome register 32bit
>> - mrs x6, far_el3 // EL3 Fault Address Register
>> + .ifnc \sp, SPx
>> + msr SPsel, #1 // Switch back to read the SP value upon entry
>> + mov x7, sp
>> + msr SPsel, xzr
>> + .else
>> + mov x7, x28 // x28 contains the SP value upon entry
>> + .endif
>>
>> -4:mrs x4, fpsr // Floating point Status Register 32bit
>> + stp x30, x7, [sp, #0xf0]
>>
>> // Record the type of exception that occurred.
>> mov x0, #\val
>> @@ -189,7 +196,7 @@ ASM_PFX(SErrorSP0):
>> //
>> VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_CUR_SPx_SYNC)
>> ASM_PFX(SynchronousExceptionSPx):
>> - ExceptionEntry EXCEPT_AARCH64_SYNCHRONOUS_EXCEPTIONS
>> + ExceptionEntry EXCEPT_AARCH64_SYNCHRONOUS_EXCEPTIONS, SP0
>>
>> VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_CUR_SPx_IRQ)
>> ASM_PFX(IrqSPx):
>> @@ -248,20 +255,25 @@ ASM_PFX(ExceptionHandlersEnd):
>>
>> ASM_PFX(CommonExceptionEntry):
>>
>> - // Stack the remaining GP registers
>> - stp x8, x9, [sp, #0x40]
>> - stp x10, x11, [sp, #0x50]
>> - stp x12, x13, [sp, #0x60]
>> - stp x14, x15, [sp, #0x70]
>> - stp x16, x17, [sp, #0x80]
>> - stp x18, x19, [sp, #0x90]
>> - stp x20, x21, [sp, #0xa0]
>> - stp x22, x23, [sp, #0xb0]
>> - stp x24, x25, [sp, #0xc0]
>> - stp x26, x27, [sp, #0xd0]
>> - stp x28, x29, [sp, #0xe0]
>> - add x28, sp, #GP_CONTEXT_SIZE + FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE
>> - stp x30, x28, [sp, #0xf0]
>> + EL1_OR_EL2_OR_EL3(x1)
>> +1:mrs x2, elr_el1 // Exception Link Register
>> + mrs x3, spsr_el1 // Saved Processor Status Register 32bit
>> + mrs x5, esr_el1 // EL1 Exception syndrome register 32bit
>> + mrs x6, far_el1 // EL1 Fault Address Register
>> + b 4f
>> +
>> +2:mrs x2, elr_el2 // Exception Link Register
>> + mrs x3, spsr_el2 // Saved Processor Status Register 32bit
>> + mrs x5, esr_el2 // EL2 Exception syndrome register 32bit
>> + mrs x6, far_el2 // EL2 Fault Address Register
>> + b 4f
>> +
>> +3:mrs x2, elr_el3 // Exception Link Register
>> + mrs x3, spsr_el3 // Saved Processor Status Register 32bit
>> + mrs x5, esr_el3 // EL3 Exception syndrome register 32bit
>> + mrs x6, far_el3 // EL3 Fault Address Register
>> +
>> +4:mrs x4, fpsr // Floating point Status Register 32bit
>>
>> // Save the SYS regs
>> stp x2, x3, [x28, #-SYS_CONTEXT_SIZE]!
>> @@ -368,3 +380,7 @@ ASM_PFX(CommonExceptionEntry):
>> add sp, sp, #FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE
>>
>> eret
>> +
>> +ASM_PFX(RegisterEl0Stack):
>> + msr sp_el0, x0
>> + ret
>> diff --git a/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf b/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
>> index 10d9ae0f4afc..cd9149cf76c6 100644
>> --- a/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
>> +++ b/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
>> @@ -53,10 +53,11 @@ [Packages]
>>
>> [LibraryClasses]
>> ArmLib
>> - DebugLib
>> - DefaultExceptionHandlerLib
>> BaseMemoryLib
>> CacheMaintenanceLib
>> + DebugLib
>> + DefaultExceptionHandlerLib
>> + MemoryAllocationLib
>>
>> [Pcd]
>> gArmTokenSpaceGuid.PcdDebuggerExceptionSupport
>> --
>> 2.7.4
>>
>
> Not tested, but it looks quite plausible, so if Eugene agrees:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
No word back from Eugene, so I am going to assume that he is on board with this.
I have added the following comment
//
// Our backtrace and register dump code is written in C and so it requires
// a stack. This makes it difficult to produce meaningful diagnostics when
// the stack pointer has been corrupted. So in such cases (i.e., when taking
// synchronous exceptions), this macro is expanded with \sp set to SP0, in
// which case we switch to the SP_EL0 stack pointer, which has been
// initialized to point to a buffer that has been set aside for this purpose.
//
// Since 'sp' may no longer refer to the stack frame that was active when
// the exception was taken, we may have to switch back and forth between
// SP_EL0 and SP_ELx to record the correct value for SP in the context struct.
//
Pushed as 21dbcff5be3c
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] ArmPkg/ArmExceptionLib: use EL0 stack for synchronous exceptions
2017-03-27 12:54 ` Ard Biesheuvel
@ 2017-03-27 12:55 ` Ard Biesheuvel
0 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2017-03-27 12:55 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel@lists.01.org, Ryan Harkin, Cohen, Eugene
On 27 March 2017 at 13:54, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 22 March 2017 at 13:18, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> On Mon, Mar 20, 2017 at 08:53:01PM +0000, Ard Biesheuvel wrote:
>>> In order to be able to produce meaningful diagnostic output when taking
>>> synchronous exceptions that have been caused by corruption of the stack
>>> pointer, prepare the EL0 stack pointer and switch to it when handling the
>>> 'Sync exception using SPx' exception class. Other exception classes (of
>>> which we really only care about IrqSPx) are left functionally intact.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>
>>> Note that some code has been moved around so that the macro doesn't grow too
>>> big to fit in a 128 byte slot, while keeping the code logically consistent.
>>>
>>> ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c | 17 +++-
>>> ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S | 86 ++++++++++++--------
>>> ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf | 5 +-
>>> 3 files changed, 70 insertions(+), 38 deletions(-)
>>>
>>> diff --git a/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c b/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
>>> index 3d6eb4974d74..bd307628af87 100644
>>> --- a/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
>>> +++ b/ArmPkg/Library/ArmExceptionLib/AArch64/AArch64Exception.c
>>> @@ -16,7 +16,7 @@
>>> #include <Uefi.h>
>>>
>>> #include <Chipset/AArch64.h>
>>> -
>>> +#include <Library/MemoryAllocationLib.h>
>>> #include <Protocol/DebugSupport.h> // for MAX_AARCH64_EXCEPTION
>>>
>>> UINTN gMaxExceptionNumber = MAX_AARCH64_EXCEPTION;
>>> @@ -25,11 +25,26 @@ EFI_EXCEPTION_CALLBACK gDebuggerExceptionHandlers[MAX_AARCH64_EXCEPTION + 1] =
>>> PHYSICAL_ADDRESS gExceptionVectorAlignmentMask = ARM_VECTOR_TABLE_ALIGNMENT;
>>> UINTN gDebuggerNoHandlerValue = 0; // todo: define for AArch64
>>>
>>> +#define EL0_STACK_PAGES 2
>>> +
>>> +VOID
>>> +RegisterEl0Stack (
>>> + IN VOID *Stack
>>> + );
>>> +
>>> RETURN_STATUS ArchVectorConfig(
>>> IN UINTN VectorBaseAddress
>>> )
>>> {
>>> UINTN HcrReg;
>>> + UINT8 *Stack;
>>> +
>>> + Stack = AllocatePages (EL0_STACK_PAGES);
>>> + if (Stack == NULL) {
>>> + return RETURN_OUT_OF_RESOURCES;
>>> + }
>>> +
>>> + RegisterEl0Stack ((UINT8 *)Stack + EFI_PAGES_TO_SIZE (EL0_STACK_PAGES));
>>>
>>> if (ArmReadCurrentEL() == AARCH64_EL2) {
>>> HcrReg = ArmReadHcr();
>>> diff --git a/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S b/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
>>> index ff1f5fc81316..ac426d72a150 100644
>>> --- a/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
>>> +++ b/ArmPkg/Library/ArmExceptionLib/AArch64/ExceptionSupport.S
>>> @@ -100,6 +100,7 @@
>>>
>>> GCC_ASM_EXPORT(ExceptionHandlersEnd)
>>> GCC_ASM_EXPORT(CommonCExceptionHandler)
>>> +GCC_ASM_EXPORT(RegisterEl0Stack)
>>>
>>> .text
>>>
>>> @@ -122,35 +123,41 @@ ASM_PFX(ExceptionHandlersStart):
>>> VECTOR_BASE(ExceptionHandlersStart)
>>> #endif
>>>
>>> - .macro ExceptionEntry, val
>>> + .macro ExceptionEntry, val, sp=SPx
>>> + .ifnc \sp, SPx
>>> + msr SPsel, xzr
>>> + .endif
>>
>> Is this esoteric enough to motivate a comment?
>>
>>> +
>>> // Move the stackpointer so we can reach our structure with the str instruction.
>>> sub sp, sp, #(FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE)
>>>
>>> - // Push some GP registers so we can record the exception context
>>> + // Push the GP registers so we can record the exception context
>>> stp x0, x1, [sp, #-GP_CONTEXT_SIZE]!
>>> stp x2, x3, [sp, #0x10]
>>> stp x4, x5, [sp, #0x20]
>>> stp x6, x7, [sp, #0x30]
>>> + stp x8, x9, [sp, #0x40]
>>> + stp x10, x11, [sp, #0x50]
>>> + stp x12, x13, [sp, #0x60]
>>> + stp x14, x15, [sp, #0x70]
>>> + stp x16, x17, [sp, #0x80]
>>> + stp x18, x19, [sp, #0x90]
>>> + stp x20, x21, [sp, #0xa0]
>>> + stp x22, x23, [sp, #0xb0]
>>> + stp x24, x25, [sp, #0xc0]
>>> + stp x26, x27, [sp, #0xd0]
>>> + stp x28, x29, [sp, #0xe0]
>>> + add x28, sp, #(GP_CONTEXT_SIZE + FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE)
>>>
>>> - EL1_OR_EL2_OR_EL3(x1)
>>> -1:mrs x2, elr_el1 // Exception Link Register
>>> - mrs x3, spsr_el1 // Saved Processor Status Register 32bit
>>> - mrs x5, esr_el1 // EL1 Exception syndrome register 32bit
>>> - mrs x6, far_el1 // EL1 Fault Address Register
>>> - b 4f
>>> -
>>> -2:mrs x2, elr_el2 // Exception Link Register
>>> - mrs x3, spsr_el2 // Saved Processor Status Register 32bit
>>> - mrs x5, esr_el2 // EL2 Exception syndrome register 32bit
>>> - mrs x6, far_el2 // EL2 Fault Address Register
>>> - b 4f
>>> -
>>> -3:mrs x2, elr_el3 // Exception Link Register
>>> - mrs x3, spsr_el3 // Saved Processor Status Register 32bit
>>> - mrs x5, esr_el3 // EL3 Exception syndrome register 32bit
>>> - mrs x6, far_el3 // EL3 Fault Address Register
>>> + .ifnc \sp, SPx
>>> + msr SPsel, #1 // Switch back to read the SP value upon entry
>>> + mov x7, sp
>>> + msr SPsel, xzr
>>> + .else
>>> + mov x7, x28 // x28 contains the SP value upon entry
>>> + .endif
>>>
>>> -4:mrs x4, fpsr // Floating point Status Register 32bit
>>> + stp x30, x7, [sp, #0xf0]
>>>
>>> // Record the type of exception that occurred.
>>> mov x0, #\val
>>> @@ -189,7 +196,7 @@ ASM_PFX(SErrorSP0):
>>> //
>>> VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_CUR_SPx_SYNC)
>>> ASM_PFX(SynchronousExceptionSPx):
>>> - ExceptionEntry EXCEPT_AARCH64_SYNCHRONOUS_EXCEPTIONS
>>> + ExceptionEntry EXCEPT_AARCH64_SYNCHRONOUS_EXCEPTIONS, SP0
>>>
>>> VECTOR_ENTRY(ExceptionHandlersStart, ARM_VECTOR_CUR_SPx_IRQ)
>>> ASM_PFX(IrqSPx):
>>> @@ -248,20 +255,25 @@ ASM_PFX(ExceptionHandlersEnd):
>>>
>>> ASM_PFX(CommonExceptionEntry):
>>>
>>> - // Stack the remaining GP registers
>>> - stp x8, x9, [sp, #0x40]
>>> - stp x10, x11, [sp, #0x50]
>>> - stp x12, x13, [sp, #0x60]
>>> - stp x14, x15, [sp, #0x70]
>>> - stp x16, x17, [sp, #0x80]
>>> - stp x18, x19, [sp, #0x90]
>>> - stp x20, x21, [sp, #0xa0]
>>> - stp x22, x23, [sp, #0xb0]
>>> - stp x24, x25, [sp, #0xc0]
>>> - stp x26, x27, [sp, #0xd0]
>>> - stp x28, x29, [sp, #0xe0]
>>> - add x28, sp, #GP_CONTEXT_SIZE + FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE
>>> - stp x30, x28, [sp, #0xf0]
>>> + EL1_OR_EL2_OR_EL3(x1)
>>> +1:mrs x2, elr_el1 // Exception Link Register
>>> + mrs x3, spsr_el1 // Saved Processor Status Register 32bit
>>> + mrs x5, esr_el1 // EL1 Exception syndrome register 32bit
>>> + mrs x6, far_el1 // EL1 Fault Address Register
>>> + b 4f
>>> +
>>> +2:mrs x2, elr_el2 // Exception Link Register
>>> + mrs x3, spsr_el2 // Saved Processor Status Register 32bit
>>> + mrs x5, esr_el2 // EL2 Exception syndrome register 32bit
>>> + mrs x6, far_el2 // EL2 Fault Address Register
>>> + b 4f
>>> +
>>> +3:mrs x2, elr_el3 // Exception Link Register
>>> + mrs x3, spsr_el3 // Saved Processor Status Register 32bit
>>> + mrs x5, esr_el3 // EL3 Exception syndrome register 32bit
>>> + mrs x6, far_el3 // EL3 Fault Address Register
>>> +
>>> +4:mrs x4, fpsr // Floating point Status Register 32bit
>>>
>>> // Save the SYS regs
>>> stp x2, x3, [x28, #-SYS_CONTEXT_SIZE]!
>>> @@ -368,3 +380,7 @@ ASM_PFX(CommonExceptionEntry):
>>> add sp, sp, #FP_CONTEXT_SIZE + SYS_CONTEXT_SIZE
>>>
>>> eret
>>> +
>>> +ASM_PFX(RegisterEl0Stack):
>>> + msr sp_el0, x0
>>> + ret
>>> diff --git a/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf b/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
>>> index 10d9ae0f4afc..cd9149cf76c6 100644
>>> --- a/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
>>> +++ b/ArmPkg/Library/ArmExceptionLib/ArmExceptionLib.inf
>>> @@ -53,10 +53,11 @@ [Packages]
>>>
>>> [LibraryClasses]
>>> ArmLib
>>> - DebugLib
>>> - DefaultExceptionHandlerLib
>>> BaseMemoryLib
>>> CacheMaintenanceLib
>>> + DebugLib
>>> + DefaultExceptionHandlerLib
>>> + MemoryAllocationLib
>>>
>>> [Pcd]
>>> gArmTokenSpaceGuid.PcdDebuggerExceptionSupport
>>> --
>>> 2.7.4
>>>
>>
>> Not tested, but it looks quite plausible, so if Eugene agrees:
>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
> No word back from Eugene, so I am going to assume that he is on board with this.
>
> I have added the following comment
>
> //
> // Our backtrace and register dump code is written in C and so it requires
> // a stack. This makes it difficult to produce meaningful diagnostics when
> // the stack pointer has been corrupted. So in such cases (i.e., when taking
> // synchronous exceptions), this macro is expanded with \sp set to SP0, in
> // which case we switch to the SP_EL0 stack pointer, which has been
> // initialized to point to a buffer that has been set aside for this purpose.
> //
> // Since 'sp' may no longer refer to the stack frame that was active when
> // the exception was taken, we may have to switch back and forth between
> // SP_EL0 and SP_ELx to record the correct value for SP in the context struct.
> //
>
> Pushed as 21dbcff5be3c
.. or rather, 2d120489583a
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-03-27 12:55 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-20 20:52 [PATCH 0/3] ArmPkg: increase robustness of the crash handler Ard Biesheuvel
2017-03-20 20:52 ` [PATCH 1/3] ArmPkg/DefaultExceptionHandlerLib: add missing GUID to .inf Ard Biesheuvel
2017-03-22 13:01 ` Leif Lindholm
2017-03-20 20:53 ` [PATCH 2/3] ArmPkg/DefaultExceptionHandlerLib: walk call stack unconditionally Ard Biesheuvel
2017-03-22 13:07 ` Leif Lindholm
2017-03-22 13:12 ` Ard Biesheuvel
2017-03-22 13:20 ` Leif Lindholm
2017-03-20 20:53 ` [PATCH 3/3] ArmPkg/ArmExceptionLib: use EL0 stack for synchronous exceptions Ard Biesheuvel
2017-03-22 13:18 ` Leif Lindholm
2017-03-22 13:20 ` Ard Biesheuvel
2017-03-27 12:54 ` Ard Biesheuvel
2017-03-27 12:55 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox