On Wed, 2 Aug 2023 at 20:59, Oliver Smith-Denny
<o...@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_llind...@quicinc.com>
> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>
> Cc: Sami Mujawar <sami.muja...@arm.com>
> Cc: Gerd Hoffmann <kra...@redhat.com>
>
> Signed-off-by: Oliver Smith-Denny <o...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
