* [edk2-devel][PATCH v2 1/1] ArmPkg: DefaultExceptionHandlerLib: Do Not Allocate Memory
@ 2023-08-02 18:59 Oliver Smith-Denny
2023-08-03 14:43 ` [edk2-devel] [PATCH " Ard Biesheuvel
0 siblings, 1 reply; 2+ messages in thread
From: Oliver Smith-Denny @ 2023-08-02 18:59 UTC (permalink / raw)
To: devel; +Cc: Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Gerd Hoffmann
Changes since v1:
- Move OutputString write to after serial logging
- Drop sending "Recursive Exception" to ConOut
If gST->ConOut is available when Arm's DefaultExceptionHandler
is running, AsciiPrint will get called to attempt to print to
ConOut, in addition to the serial output.
AsciiPrint calls AsciiInternalPrint in UefiLibPrint.c which
in turn calls AllocatePool to allocate a buffer to convert
the Ascii input string to a Unicode string to pass to
ConOut->OutputString.
Per the comment on DefaultExceptionHandler, we should not be
allocating memory in the exception handler, as this can cause
the exception handler to fail if we had a memory exception or
the system state is such that we cannot allocate memory.
It has been observed on ArmVirtQemu that exceptions generated
in the memory handling code will fail to output the stack dump
and CPU state that is critical to debugging because the
AllocatePool will fail.
This patch fixes the Arm and AARCH64 DefaultExceptionHandlers to
not allocate memory when ConOut is available and instead use
stack memory to convert the Ascii string needed for SerialPortWrite
to the Unicode string needed for ConOut->OutputString. Correspondingly,
ArmVirtQemu can now output the stack dump and CPU state when hitting
an exception in memory code.
GitHub PR: https://github.com/tianocore/edk2/pull/4713
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Oliver Smith-Denny <osde@linux.microsoft.com>
---
ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c | 30 +++++++++++++-------
ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c | 22 +++++++++++---
2 files changed, 37 insertions(+), 15 deletions(-)
diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
index f2bca5d74005..f4967dabed14 100644
--- a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
+++ b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
@@ -22,6 +22,11 @@
#include <Protocol/DebugSupport.h>
#include <Protocol/LoadedImage.h>
+//
+// Maximum number of characters to print to serial (UINT8s) and to console if available (as UINT16s)
+//
+#define MAX_PRINT_CHARS 100
+
STATIC CHAR8 *gExceptionTypeString[] = {
"Synchronous",
"IRQ",
@@ -188,18 +193,14 @@ DefaultExceptionHandler (
IN OUT EFI_SYSTEM_CONTEXT SystemContext
)
{
- CHAR8 Buffer[100];
- UINTN CharCount;
- INT32 Offset;
+ CHAR8 Buffer[MAX_PRINT_CHARS];
+ CHAR16 UnicodeBuffer[MAX_PRINT_CHARS];
+ UINTN CharCount;
+ INT32 Offset;
if (mRecursiveException) {
STATIC CHAR8 CONST Message[] = "\nRecursive exception occurred while dumping the CPU state\n";
-
SerialPortWrite ((UINT8 *)Message, sizeof Message - 1);
- if (gST->ConOut != NULL) {
- AsciiPrint (Message);
- }
-
CpuDeadLoop ();
}
@@ -207,9 +208,9 @@ DefaultExceptionHandler (
CharCount = AsciiSPrint (Buffer, sizeof (Buffer), "\n\n%a Exception at 0x%016lx\n", gExceptionTypeString[ExceptionType], SystemContext.SystemContextAArch64->ELR);
SerialPortWrite ((UINT8 *)Buffer, CharCount);
- if (gST->ConOut != NULL) {
- AsciiPrint (Buffer);
- }
+
+ // prepare a unicode buffer for ConOut, if applicable, in case the buffer gets reused
+ UnicodeSPrintAsciiFormat (UnicodeBuffer, MAX_PRINT_CHARS, Buffer);
DEBUG_CODE_BEGIN ();
CHAR8 *Pdb, *PrevPdb;
@@ -330,6 +331,13 @@ DefaultExceptionHandler (
));
}
+ // attempt to print that we had a synchronous exception to ConOut
+ // we do this after the serial logging as ConOut's logging is more
+ // complex and we aren't guaranteed to succeed
+ if (gST->ConOut != NULL) {
+ gST->ConOut->OutputString (gST->ConOut, UnicodeBuffer);
+ }
+
ASSERT (FALSE);
CpuDeadLoop ();
}
diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
index 13b321e45615..f5fce4c4b516 100644
--- a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
+++ b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
@@ -23,6 +23,11 @@
#include <Protocol/DebugSupport.h>
#include <Library/DefaultExceptionHandlerLib.h>
+//
+// Maximum number of characters to print to serial (UINT8s) and to console if available (as UINT16s)
+//
+#define MAX_PRINT_CHARS 100
+
//
// The number of elements in a CHAR8 array, including the terminating NUL, that
// is meant to hold the string rendering of the CPSR.
@@ -198,7 +203,8 @@ DefaultExceptionHandler (
IN OUT EFI_SYSTEM_CONTEXT SystemContext
)
{
- CHAR8 Buffer[100];
+ CHAR8 Buffer[MAX_PRINT_CHARS];
+ CHAR16 UnicodeBuffer[MAX_PRINT_CHARS];
UINTN CharCount;
UINT32 DfsrStatus;
UINT32 IfsrStatus;
@@ -216,9 +222,9 @@ DefaultExceptionHandler (
SystemContext.SystemContextArm->CPSR
);
SerialPortWrite ((UINT8 *)Buffer, CharCount);
- if (gST->ConOut != NULL) {
- AsciiPrint (Buffer);
- }
+
+ // prepare a unicode buffer for ConOut, if applicable, as Buffer is used below
+ UnicodeSPrintAsciiFormat (UnicodeBuffer, MAX_PRINT_CHARS, Buffer);
DEBUG_CODE_BEGIN ();
CHAR8 *Pdb;
@@ -289,6 +295,14 @@ DefaultExceptionHandler (
}
DEBUG ((DEBUG_ERROR, "\n"));
+
+ // attempt to print that we had a synchronous exception to ConOut
+ // we do this after the serial logging as ConOut's logging is more
+ // complex and we aren't guaranteed to succeed
+ if (gST->ConOut != NULL) {
+ gST->ConOut->OutputString (gST->ConOut, UnicodeBuffer);
+ }
+
ASSERT (FALSE);
CpuDeadLoop (); // may return if executing under a debugger
--
2.40.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107469): https://edk2.groups.io/g/devel/message/107469
Mute This Topic: https://groups.io/mt/100512229/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/1] ArmPkg: DefaultExceptionHandlerLib: Do Not Allocate Memory
2023-08-02 18:59 [edk2-devel][PATCH v2 1/1] ArmPkg: DefaultExceptionHandlerLib: Do Not Allocate Memory Oliver Smith-Denny
@ 2023-08-03 14:43 ` Ard Biesheuvel
0 siblings, 0 replies; 2+ messages in thread
From: Ard Biesheuvel @ 2023-08-03 14:43 UTC (permalink / raw)
To: Oliver Smith-Denny
Cc: devel, Leif Lindholm, Ard Biesheuvel, Sami Mujawar, Gerd Hoffmann
On Wed, 2 Aug 2023 at 20:59, Oliver Smith-Denny
<osde@linux.microsoft.com> wrote:
>
> Changes since v1:
> - Move OutputString write to after serial logging
> - Drop sending "Recursive Exception" to ConOut
>
> If gST->ConOut is available when Arm's DefaultExceptionHandler
> is running, AsciiPrint will get called to attempt to print to
> ConOut, in addition to the serial output.
>
> AsciiPrint calls AsciiInternalPrint in UefiLibPrint.c which
> in turn calls AllocatePool to allocate a buffer to convert
> the Ascii input string to a Unicode string to pass to
> ConOut->OutputString.
>
> Per the comment on DefaultExceptionHandler, we should not be
> allocating memory in the exception handler, as this can cause
> the exception handler to fail if we had a memory exception or
> the system state is such that we cannot allocate memory.
>
> It has been observed on ArmVirtQemu that exceptions generated
> in the memory handling code will fail to output the stack dump
> and CPU state that is critical to debugging because the
> AllocatePool will fail.
>
> This patch fixes the Arm and AARCH64 DefaultExceptionHandlers to
> not allocate memory when ConOut is available and instead use
> stack memory to convert the Ascii string needed for SerialPortWrite
> to the Unicode string needed for ConOut->OutputString. Correspondingly,
> ArmVirtQemu can now output the stack dump and CPU state when hitting
> an exception in memory code.
>
> GitHub PR: https://github.com/tianocore/edk2/pull/4713
>
> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Sami Mujawar <sami.mujawar@arm.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
>
> Signed-off-by: Oliver Smith-Denny <osde@linux.microsoft.com>
Merged as #4717
Thanks!
> ---
> ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c | 30 +++++++++++++-------
> ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c | 22 +++++++++++---
> 2 files changed, 37 insertions(+), 15 deletions(-)
>
> diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> index f2bca5d74005..f4967dabed14 100644
> --- a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> @@ -22,6 +22,11 @@
> #include <Protocol/DebugSupport.h>
> #include <Protocol/LoadedImage.h>
>
> +//
> +// Maximum number of characters to print to serial (UINT8s) and to console if available (as UINT16s)
> +//
> +#define MAX_PRINT_CHARS 100
> +
> STATIC CHAR8 *gExceptionTypeString[] = {
> "Synchronous",
> "IRQ",
> @@ -188,18 +193,14 @@ DefaultExceptionHandler (
> IN OUT EFI_SYSTEM_CONTEXT SystemContext
> )
> {
> - CHAR8 Buffer[100];
> - UINTN CharCount;
> - INT32 Offset;
> + CHAR8 Buffer[MAX_PRINT_CHARS];
> + CHAR16 UnicodeBuffer[MAX_PRINT_CHARS];
> + UINTN CharCount;
> + INT32 Offset;
>
> if (mRecursiveException) {
> STATIC CHAR8 CONST Message[] = "\nRecursive exception occurred while dumping the CPU state\n";
> -
> SerialPortWrite ((UINT8 *)Message, sizeof Message - 1);
> - if (gST->ConOut != NULL) {
> - AsciiPrint (Message);
> - }
> -
> CpuDeadLoop ();
> }
>
> @@ -207,9 +208,9 @@ DefaultExceptionHandler (
>
> CharCount = AsciiSPrint (Buffer, sizeof (Buffer), "\n\n%a Exception at 0x%016lx\n", gExceptionTypeString[ExceptionType], SystemContext.SystemContextAArch64->ELR);
> SerialPortWrite ((UINT8 *)Buffer, CharCount);
> - if (gST->ConOut != NULL) {
> - AsciiPrint (Buffer);
> - }
> +
> + // prepare a unicode buffer for ConOut, if applicable, in case the buffer gets reused
> + UnicodeSPrintAsciiFormat (UnicodeBuffer, MAX_PRINT_CHARS, Buffer);
>
> DEBUG_CODE_BEGIN ();
> CHAR8 *Pdb, *PrevPdb;
> @@ -330,6 +331,13 @@ DefaultExceptionHandler (
> ));
> }
>
> + // attempt to print that we had a synchronous exception to ConOut
> + // we do this after the serial logging as ConOut's logging is more
> + // complex and we aren't guaranteed to succeed
> + if (gST->ConOut != NULL) {
> + gST->ConOut->OutputString (gST->ConOut, UnicodeBuffer);
> + }
> +
> ASSERT (FALSE);
> CpuDeadLoop ();
> }
> diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> index 13b321e45615..f5fce4c4b516 100644
> --- a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> @@ -23,6 +23,11 @@
> #include <Protocol/DebugSupport.h>
> #include <Library/DefaultExceptionHandlerLib.h>
>
> +//
> +// Maximum number of characters to print to serial (UINT8s) and to console if available (as UINT16s)
> +//
> +#define MAX_PRINT_CHARS 100
> +
> //
> // The number of elements in a CHAR8 array, including the terminating NUL, that
> // is meant to hold the string rendering of the CPSR.
> @@ -198,7 +203,8 @@ DefaultExceptionHandler (
> IN OUT EFI_SYSTEM_CONTEXT SystemContext
> )
> {
> - CHAR8 Buffer[100];
> + CHAR8 Buffer[MAX_PRINT_CHARS];
> + CHAR16 UnicodeBuffer[MAX_PRINT_CHARS];
> UINTN CharCount;
> UINT32 DfsrStatus;
> UINT32 IfsrStatus;
> @@ -216,9 +222,9 @@ DefaultExceptionHandler (
> SystemContext.SystemContextArm->CPSR
> );
> SerialPortWrite ((UINT8 *)Buffer, CharCount);
> - if (gST->ConOut != NULL) {
> - AsciiPrint (Buffer);
> - }
> +
> + // prepare a unicode buffer for ConOut, if applicable, as Buffer is used below
> + UnicodeSPrintAsciiFormat (UnicodeBuffer, MAX_PRINT_CHARS, Buffer);
>
> DEBUG_CODE_BEGIN ();
> CHAR8 *Pdb;
> @@ -289,6 +295,14 @@ DefaultExceptionHandler (
> }
>
> DEBUG ((DEBUG_ERROR, "\n"));
> +
> + // attempt to print that we had a synchronous exception to ConOut
> + // we do this after the serial logging as ConOut's logging is more
> + // complex and we aren't guaranteed to succeed
> + if (gST->ConOut != NULL) {
> + gST->ConOut->OutputString (gST->ConOut, UnicodeBuffer);
> + }
> +
> ASSERT (FALSE);
>
> CpuDeadLoop (); // may return if executing under a debugger
> --
> 2.40.1
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107548): https://edk2.groups.io/g/devel/message/107548
Mute This Topic: https://groups.io/mt/100512229/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-08-03 14:44 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-02 18:59 [edk2-devel][PATCH v2 1/1] ArmPkg: DefaultExceptionHandlerLib: Do Not Allocate Memory Oliver Smith-Denny
2023-08-03 14:43 ` [edk2-devel] [PATCH " Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox