From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4864:20::143; helo=mail-it1-x143.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it1-x143.google.com (mail-it1-x143.google.com [IPv6:2607:f8b0:4864:20::143]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 42B4B2119FF40 for ; Thu, 27 Dec 2018 01:44:01 -0800 (PST) Received: by mail-it1-x143.google.com with SMTP id g85so23875047ita.3 for ; Thu, 27 Dec 2018 01:44:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=A8I1+boKa8NN4qXANKRMMldhH+nldprjJY3ILqizA2Y=; b=HiutsMWusSV0HomCxmX1hpa5prO5gMQHUxqbQgQJQHDbCbO1lZDmIhCaBlgBpyAgOI 4EiDR5P19cUJvwcnyLvG8UjRXJfPD7PSjcwQRfhu2122hnlUygfoGpRASWgI+vyOafnp lZKwkeZrNn/mHOSjszFXyndiIaNC95IfcHOxo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=A8I1+boKa8NN4qXANKRMMldhH+nldprjJY3ILqizA2Y=; b=Q71zjD+CBsJBXcmH4KML4fJ687XYV3GZiuJjXzmCNQft+Ry3dM5+junPZngGhecItL MCAmaLzWVxy1Uzp+o8TwGyoOSySYOhu60rR2zyFi70U67Vpkyk59q5SaBNd024NSzobH fBJ29wLLQGssF7q6Qr1dVpqBi0gbfb8EUcrsCEP1dWV2StzWWp8ZuZM34tNM6koM+lQT y5VOweGySTHQINRzZHvvPDi+uilP5iAXn64xLMIfdDknB+n77KuA/TkcqA7CxNX68HSl byKwm0kiZAmNR0Isnw5P10zbvGhJ0Ucf9M74a8As49BP/2liOQ2c0RVHkSIuRAdkIetd Chww== X-Gm-Message-State: AA+aEWbtwNzu48z959T0gL6JyckRyLnPvH/ZLzrz3M+P0W2rmZPgUbV3 wl1urtMSKzwzysLR5PgPee1Y20SdZI+1B3wjb++568yFGXw= X-Google-Smtp-Source: AFSGD/WoczCy6pUpU3R+Fady8N2p5bdrVb6ZlxpROFKT33pwVKHnc+thPI4/yh5SBW2lL4IrMAyGPrwJcFb2zNSFt+c= X-Received: by 2002:a24:710:: with SMTP id f16mr12899324itf.121.1545903840377; Thu, 27 Dec 2018 01:44:00 -0800 (PST) MIME-Version: 1.0 References: <20181220173104.11481-1-ard.biesheuvel@linaro.org> <20181220173104.11481-5-ard.biesheuvel@linaro.org> In-Reply-To: From: Ard Biesheuvel Date: Thu, 27 Dec 2018 10:43:49 +0100 Message-ID: To: Laszlo Ersek Cc: "edk2-devel@lists.01.org" Subject: Re: [PATCH 4/4] ArmPkg/DefaultExceptionHandlerLib: use console if available X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 27 Dec 2018 09:44:01 -0000 Content-Type: text/plain; charset="UTF-8" On Wed, 26 Dec 2018 at 22:34, Laszlo Ersek wrote: > > On 12/20/18 18:31, Ard Biesheuvel wrote: > > Print the minimal 'exception occurred' message to the console instead > > of straight to the serial port if the console is available. This makes > > such messages visible on systems where the console is graphical and > > the serial is not connected. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Ard Biesheuvel > > --- > > ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c | 16 +++++++++++++--- > > ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c | 7 ++++++- > > ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLib.inf | 1 + > > 3 files changed, 20 insertions(+), 4 deletions(-) > > > > diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c > > index 1024bf48c63d..1aaf3c88f21e 100644 > > --- a/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c > > +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/AArch64/DefaultExceptionHandler.c > > @@ -22,6 +22,7 @@ > > #include > > #include > > #include > > +#include > > > > #include > > #include > > @@ -159,14 +160,23 @@ DefaultExceptionHandler ( > > INT32 Offset; > > > > if (mRecursiveException) { > > - CharCount = AsciiSPrint (Buffer, sizeof (Buffer),"\nRecursive exception occurred while dumping the CPU state\n"); > > - SerialPortWrite ((UINT8 *) Buffer, CharCount); > > + STATIC CHAR8 CONST Message[] = "\nRecursive exception occurred while dumping the CPU state\n"; > > + > > + if (gST->ConOut != NULL) { > > + AsciiPrint (Message); > > + } else { > > + SerialPortWrite ((UINT8 *)Message, AsciiStrLen (Message)); > > + } > > CpuDeadLoop (); > > } > > mRecursiveException = TRUE; > > > > 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); > > + } else { > > + SerialPortWrite ((UINT8 *)Buffer, CharCount); > > + } > > > > DEBUG_CODE_BEGIN (); > > CHAR8 *Pdb, *PrevPdb; > > diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c > > index 0b9da031b47d..9159b579da6f 100644 > > --- a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c > > +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c > > @@ -21,6 +21,7 @@ > > #include > > #include > > #include > > +#include > > > > #include > > > > @@ -194,7 +195,11 @@ DefaultExceptionHandler ( > > > > CharCount = AsciiSPrint (Buffer,sizeof (Buffer),"\n%a Exception PC at 0x%08x CPSR 0x%08x ", > > gExceptionTypeString[ExceptionType], SystemContext.SystemContextArm->PC, SystemContext.SystemContextArm->CPSR); > > - SerialPortWrite ((UINT8 *) Buffer, CharCount); > > + if (gST->ConOut != NULL) { > > + AsciiPrint (Buffer); > > + } else { > > + SerialPortWrite ((UINT8 *)Buffer, CharCount); > > + } > > > > DEBUG_CODE_BEGIN (); > > CHAR8 *Pdb; > > diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLib.inf b/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLib.inf > > index 7609f82d89a1..6bc48714c9dc 100644 > > --- a/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLib.inf > > +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/DefaultExceptionHandlerLib.inf > > @@ -42,6 +42,7 @@ > > PeCoffGetEntryPointLib > > ArmDisassemblerLib > > SerialPortLib > > + UefiBootServicesTableLib > > > > [Guids] > > gEfiDebugImageInfoTableGuid > > > > I feel that invoking such high level functionality from an exception > handler is risky, but I do understand this is a last resort / best > effort action, and if it *happens* to work on systems with no serial, > then it's already a win. > > However, I'd suggest improving the robustness by preserving the serial > write first, and then performing the (optional) console write in > addition, second. I guess it can lead to the message appearing twice on > serial (if the console is available, and it happens to be multiplexed to > the serial port as well), but I think that's a better compromise than > possibly losing the message altogether. > > I don't feel strongly about it either way, I just thought this worth > raising. > Excellent point. I'll change this in v2