diff mbox series

[edk2,4/4] ArmPkg/DefaultExceptionHandlerLib: use console if available

Message ID 20181220173104.11481-5-ard.biesheuvel@linaro.org
State New
Headers show
Series ArmPkg: use console for minimal 'exception occurred' message | expand

Commit Message

Ard Biesheuvel Dec. 20, 2018, 5:31 p.m. UTC 
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 <ard.biesheuvel@linaro.org>

---
 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(-)

-- 
2.19.2

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek Dec. 26, 2018, 9:33 p.m. UTC | #1 
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 <ard.biesheuvel@linaro.org>

> ---

>  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 <Library/PrintLib.h>

>  #include <Library/ArmDisassemblerLib.h>

>  #include <Library/SerialPortLib.h>

> +#include <Library/UefiBootServicesTableLib.h>

>  

>  #include <Guid/DebugImageInfoTable.h>

>  #include <Protocol/DebugSupport.h>

> @@ -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 <Library/PrintLib.h>

>  #include <Library/ArmDisassemblerLib.h>

>  #include <Library/SerialPortLib.h>

> +#include <Library/UefiBootServicesTableLib.h>

>  

>  #include <Guid/DebugImageInfoTable.h>

>  

> @@ -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.

Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Dec. 27, 2018, 9:43 a.m. UTC | #2 
On Wed, 26 Dec 2018 at 22:34, Laszlo Ersek <lersek@redhat.com> 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 <ard.biesheuvel@linaro.org>

> > ---

> >  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 <Library/PrintLib.h>

> >  #include <Library/ArmDisassemblerLib.h>

> >  #include <Library/SerialPortLib.h>

> > +#include <Library/UefiBootServicesTableLib.h>

> >

> >  #include <Guid/DebugImageInfoTable.h>

> >  #include <Protocol/DebugSupport.h>

> > @@ -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 <Library/PrintLib.h>

> >  #include <Library/ArmDisassemblerLib.h>

> >  #include <Library/SerialPortLib.h>

> > +#include <Library/UefiBootServicesTableLib.h>

> >

> >  #include <Guid/DebugImageInfoTable.h>

> >

> > @@ -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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox series

Patch

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 <Library/PrintLib.h>
 #include <Library/ArmDisassemblerLib.h>
 #include <Library/SerialPortLib.h>
+#include <Library/UefiBootServicesTableLib.h>
 
 #include <Guid/DebugImageInfoTable.h>
 #include <Protocol/DebugSupport.h>
@@ -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 <Library/PrintLib.h>
 #include <Library/ArmDisassemblerLib.h>
 #include <Library/SerialPortLib.h>
+#include <Library/UefiBootServicesTableLib.h>
 
 #include <Guid/DebugImageInfoTable.h>
 
@@ -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