On Tue, 1 Aug 2023 at 00:04, Oliver Smith-Denny
wrote:
>
> 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/4703
>
> Cc: Leif Lindholm
> Cc: Ard Biesheuvel
> Cc: Sami Mujawar
> Cc: Gerd Hoffmann
>
> Signed-off-by: Oliver Smith-Denny
Thanks a lot for fixing this.
Is calling into gST->ConOut guaranteed to be safe in this regard? Or
is it still best effort?
> ---
> ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> | 18 +-
> ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> | 11 +--
> 2 files changed, 22 insertions(+), 7 deletions(-)
>
> diff --git
> a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> index f2bca5d74005..07c8aade1c5f 100644
> ---
> a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> +++
> b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c
> @@ -22,6 +22,11 @@
> #include
> #include
>
> +//
> +// 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,16 +193,18 @@ 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);
> + UnicodeSPrintAsciiFormat (UnicodeBuffer, MAX_PRINT_CHARS, Buffer);
> + gST->ConOut->OutputString (gST->ConOut, UnicodeBuffer);
> }
>
> CpuDeadLoop ();
> @@ -208,7 +215,8 @@ 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);
> +UnicodeSPrintAsciiFormat (UnicodeBuffer, MAX_PRINT_CHARS, Buffer);
> +gST->ConOut->OutputString (gST->ConOut, UnicodeBuffer);
>}
>
>DEBUG_CODE_BEGIN ();
> diff --git
> a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> index 13b321e45615..08d62c0dbfa2 100644
> --- a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> @@ -23,6 +23,11 @@
> #include
> #include
>
> +//
> +// 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
>)
> {
> - CHAR8Buffer[100];
> + CHAR8Buffer[MAX_PRINT_CHARS];
> + CHAR16 UnicodeBuffer[MAX_PRINT_CHARS];
>UINTNCharCount;
>UINT32 DfsrStatus;
>UINT32 IfsrStatus;
> @@ -217,7 +223,8 @@ DefaultExceptionHandler (
> );
>SerialPortWrite ((UINT8 *)Buffer, CharCount);
>if (gST->ConOut != NULL) {
> -AsciiPrint (Buffer);
> +
