On Tue, 1 Aug 2023 at 00:04, Oliver Smith-Denny
<o...@linux.microsoft.com> 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 <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>
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 <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,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 <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;
> @@ -217,7 +223,8 @@ DefaultExceptionHandler (
> );
> 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 ();
> --
> 2.40.1
>
>
>
> ------------
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#107412): https://edk2.groups.io/g/devel/message/107412
> Mute This Topic: https://groups.io/mt/100472023/5717338
> Group Owner: devel+ow...@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ardb+tianoc...@kernel.org]
> ------------
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#107459): https://edk2.groups.io/g/devel/message/107459
Mute This Topic: https://groups.io/mt/100472023/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
