Re: [edk2] [PATCH 1/3] MdePkg: introduce DxeRuntimeDebugLibSerialPort

[Ard Biesheuvel]

On 20 February 2018 at 19:22, Kinney, Michael D
 wrote:
> Ard,
>
> Both FixedAtBuild and PatchableInModule are
> safe at runtime.  Should not limit to only
> FixedAtBuild.
>

OK. So that means we'll need to cache the PCD values in the constructor instead.

> It is also possible to use Dynamic and DynamicEx
> from a runtime driver as long as the PCD values
> are retrieved before ExitBootServices().
>
> Mike
>
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-
>> boun...@lists.01.org] On Behalf Of Ard Biesheuvel
>> Sent: Tuesday, February 20, 2018 3:05 AM
>> To: edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu ; Ard Biesheuvel
>> ; af...@apple.com;
>> leif.lindh...@linaro.org; Gao, Liming
>> ; Kinney, Michael D
>> ; ler...@redhat.com; Zeng,
>> Star 
>> Subject: [edk2] [PATCH 1/3] MdePkg: introduce
>> DxeRuntimeDebugLibSerialPort
>>
>> Introduce a variant of BaseDebugLibSerialPort that
>> behaves correctly wrt
>> to use of the serial port after ExitBootServices().
>> Also, it uses fixed
>> PCDs for all the parameterized values so that no calls
>> into PcdLib are
>> made at runtime.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel
>> 
>> ---
>>  MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
>> | 342 
>>
>> MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDe
>> bugLibSerialPort.inf |  46 +++
>>
>> MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDe
>> bugLibSerialPort.uni |  21 ++
>>  3 files changed, 409 insertions(+)
>>
>> diff --git
>> a/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
>> b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
>> new file mode 100644
>> index ..d18267d91322
>> --- /dev/null
>> +++
>> b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
>> @@ -0,0 +1,342 @@
>> +/** @file
>> +  DXE runtime Debug library instance based on Serial
>> Port library.
>> +  It uses PrintLib to send debug messages to serial
>> port device.
>> +
>> +  NOTE: If the Serial Port library enables hardware
>> flow control, then a call
>> +  to DebugPrint() or DebugAssert() may hang if writes
>> to the serial port are
>> +  being blocked.  This may occur if a key(s) are
>> pressed in a terminal emulator
>> +  used to monitor the DEBUG() and ASSERT() messages.
>> +
>> +  Copyright (c) 2006 - 2011, Intel Corporation. All
>> rights reserved.
>> +  Copyright (c) 2018, Linaro, Ltd. All rights
>> reserved.
>> +
>> +  This program and the accompanying materials
>> +  are licensed and made available under the terms and
>> conditions of the BSD License
>> +  which accompanies this distribution.  The full text
>> of the license may be found at
>> +  http://opensource.org/licenses/bsd-license.php.
>> +
>> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON
>> AN "AS IS" BASIS,
>> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
>> EITHER EXPRESS OR IMPLIED.
>> +
>> +**/
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +STATIC EFI_EVENT  mEfiExitBootServicesEvent;
>> +STATIC BOOLEANmEfiAtRuntime;
>> +
>> +//
>> +// Define the maximum debug and assert message length
>> that this library supports
>> +//
>> +#define MAX_DEBUG_MESSAGE_LENGTH  0x100
>> +
>> +/**
>> +  Set AtRuntime flag as TRUE after ExitBootServices.
>> +
>> +  @param[in]  Event   The Event that is being
>> processed.
>> +  @param[in]  Context The Event Context.
>> +
>> +**/
>> +STATIC
>> +VOID
>> +EFIAPI
>> +RuntimeLibExitBootServicesEvent (
>> +  IN EFI_EVENTEvent,
>> +  IN VOID *Context
>> +  )
>> +{
>> +  mEfiAtRuntime = TRUE;
>> +}
>> +
>> +/**
>> +  The constructor function initialize the Serial Port
>> Library
>> +
>> +  @retval EFI_SUCCESS   The constructor always returns
>> RETURN_SUCCESS.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +DxeRuntimeDebugLibSerialPortConstructor (
>> +  IN EFI_HANDLEImageHandle,
>> +  IN EFI_SYSTEM_TABLE  *SystemTable
>> +  )
>> +{
>> +  EFI_STATUSStatus;
>> +
>> +  Status = SerialPortInitialize ();
>> +  if (EFI_ERROR (Status)) {
>> +return Status;
>> +  }
>> +
>> +  return SystemTable->BootServices->CreateEventEx (
>> +
>> EVT_NOTIFY_SIGNAL,
>> +  TPL_NOTIFY,
>> +
>> RuntimeLibExitBootServicesEvent,
>> +  NULL,
>> +
>> ,
>> +
>> );
>> +}
>> +
>> +/**
>> +  Prints a debug message to the debug output device if
>> the specified error level
>> +  is enabled.
>> +
>> +  If any bit in ErrorLevel is also set in
>> DebugPrintErrorLevelLib function
>> +  GetDebugPrintErrorLevel (), then print the message
>> specified by Format and the
>> +  associated variable argument list to the debug output
>> device.
>> +
>> +  If Format is NULL, then 

Re: [edk2] [PATCH 1/3] MdePkg: introduce DxeRuntimeDebugLibSerialPort

[Kinney, Michael D]

Ard,

Both FixedAtBuild and PatchableInModule are
safe at runtime.  Should not limit to only 
FixedAtBuild.

It is also possible to use Dynamic and DynamicEx
from a runtime driver as long as the PCD values
are retrieved before ExitBootServices().

Mike

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-
> boun...@lists.01.org] On Behalf Of Ard Biesheuvel
> Sent: Tuesday, February 20, 2018 3:05 AM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu ; Ard Biesheuvel
> ; af...@apple.com;
> leif.lindh...@linaro.org; Gao, Liming
> ; Kinney, Michael D
> ; ler...@redhat.com; Zeng,
> Star 
> Subject: [edk2] [PATCH 1/3] MdePkg: introduce
> DxeRuntimeDebugLibSerialPort
> 
> Introduce a variant of BaseDebugLibSerialPort that
> behaves correctly wrt
> to use of the serial port after ExitBootServices().
> Also, it uses fixed
> PCDs for all the parameterized values so that no calls
> into PcdLib are
> made at runtime.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel
> 
> ---
>  MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
> | 342 
> 
> MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDe
> bugLibSerialPort.inf |  46 +++
> 
> MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDe
> bugLibSerialPort.uni |  21 ++
>  3 files changed, 409 insertions(+)
> 
> diff --git
> a/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
> b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
> new file mode 100644
> index ..d18267d91322
> --- /dev/null
> +++
> b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
> @@ -0,0 +1,342 @@
> +/** @file
> +  DXE runtime Debug library instance based on Serial
> Port library.
> +  It uses PrintLib to send debug messages to serial
> port device.
> +
> +  NOTE: If the Serial Port library enables hardware
> flow control, then a call
> +  to DebugPrint() or DebugAssert() may hang if writes
> to the serial port are
> +  being blocked.  This may occur if a key(s) are
> pressed in a terminal emulator
> +  used to monitor the DEBUG() and ASSERT() messages.
> +
> +  Copyright (c) 2006 - 2011, Intel Corporation. All
> rights reserved.
> +  Copyright (c) 2018, Linaro, Ltd. All rights
> reserved.
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and
> conditions of the BSD License
> +  which accompanies this distribution.  The full text
> of the license may be found at
> +  http://opensource.org/licenses/bsd-license.php.
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON
> AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
> EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +STATIC EFI_EVENT  mEfiExitBootServicesEvent;
> +STATIC BOOLEANmEfiAtRuntime;
> +
> +//
> +// Define the maximum debug and assert message length
> that this library supports
> +//
> +#define MAX_DEBUG_MESSAGE_LENGTH  0x100
> +
> +/**
> +  Set AtRuntime flag as TRUE after ExitBootServices.
> +
> +  @param[in]  Event   The Event that is being
> processed.
> +  @param[in]  Context The Event Context.
> +
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +RuntimeLibExitBootServicesEvent (
> +  IN EFI_EVENTEvent,
> +  IN VOID *Context
> +  )
> +{
> +  mEfiAtRuntime = TRUE;
> +}
> +
> +/**
> +  The constructor function initialize the Serial Port
> Library
> +
> +  @retval EFI_SUCCESS   The constructor always returns
> RETURN_SUCCESS.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +DxeRuntimeDebugLibSerialPortConstructor (
> +  IN EFI_HANDLEImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUSStatus;
> +
> +  Status = SerialPortInitialize ();
> +  if (EFI_ERROR (Status)) {
> +return Status;
> +  }
> +
> +  return SystemTable->BootServices->CreateEventEx (
> +
> EVT_NOTIFY_SIGNAL,
> +  TPL_NOTIFY,
> +
> RuntimeLibExitBootServicesEvent,
> +  NULL,
> +
> ,
> +
> );
> +}
> +
> +/**
> +  Prints a debug message to the debug output device if
> the specified error level
> +  is enabled.
> +
> +  If any bit in ErrorLevel is also set in
> DebugPrintErrorLevelLib function
> +  GetDebugPrintErrorLevel (), then print the message
> specified by Format and the
> +  associated variable argument list to the debug output
> device.
> +
> +  If Format is NULL, then ASSERT().
> +
> +  @param  ErrorLevel  The error level of the debug
> message.
> +  @param  Format  Format string for the debug
> message to print.
> +  @param  ... Variable argument list whose
> contents are accessed
> +  based on the format string
> specified by Format.
> +
> +**/
> +VOID
> +EFIAPI
> +DebugPrint 

Re: [edk2] [PATCH 1/3] MdePkg: introduce DxeRuntimeDebugLibSerialPort

[Leif Lindholm]

On Tue, Feb 20, 2018 at 08:20:48AM -0800, Andrew Fish wrote:
> > On Feb 20, 2018, at 6:16 AM, Leif Lindholm  wrote:
> > 
> > On Tue, Feb 20, 2018 at 11:05:22AM +, Ard Biesheuvel wrote:
> >> +/**
> >> +  Prints an assert message containing a filename, line number, and 
> >> description.
> >> +  This may be followed by a breakpoint or a dead loop.
> >> +
> >> +  Print a message of the form "ASSERT (): 
> >> \n"
> >> +  to the debug output device.  If 
> >> DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED bit
> >> +  of PcdDebugProperyMask is set then CpuBreakpoint() is called. 
> >> Otherwise, if
> >> +  DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED bit of PcdDebugProperyMask is 
> >> set then
> >> +  CpuDeadLoop() is called.  If neither of these bits are set, then this 
> >> function
> >> +  returns immediately after the message is printed to the debug output 
> >> device.
> >> +  DebugAssert() must actively prevent recursion.  If DebugAssert() is 
> >> called
> >> +  while processing another DebugAssert(), then DebugAssert() must return
> >> +  immediately.
> >> +
> >> +  If FileName is NULL, then a  string of "(NULL) Filename" is 
> >> printed.
> >> +  If Description is NULL, then a  string of "(NULL) 
> >> Description" is
> >> +  printed.
> >> +
> >> +  @param  FileName The pointer to the name of the source file that 
> >> generated
> >> +   the assert condition.
> >> +  @param  LineNumber   The line number in the source file that generated 
> >> the
> >> +   assert condition
> >> +  @param  Description  The pointer to the description of the assert 
> >> condition.
> >> +
> >> +**/
> >> +VOID
> >> +EFIAPI
> >> +DebugAssert (
> >> +  IN CONST CHAR8  *FileName,
> >> +  IN UINTNLineNumber,
> >> +  IN CONST CHAR8  *Description
> >> +  )
> >> +{
> >> +  CHAR8  Buffer[MAX_DEBUG_MESSAGE_LENGTH];
> >> +
> >> +  if (!mEfiAtRuntime) {
> >> +//
> >> +// Generate the ASSERT() message in Ascii format
> >> +//
> >> +AsciiSPrint (Buffer, sizeof (Buffer), "ASSERT [%a] %a(%d): %a\n",
> >> +  gEfiCallerBaseName, FileName, LineNumber, Description);
> >> +
> >> +//
> >> +// Send the print string to the Console Output device
> >> +//
> >> +SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer));
> >> +  }
> >> +
> >> +  //
> >> +  // Generate a Breakpoint, DeadLoop, or NOP based on PCD settings
> >> +  //
> >> +  if ((FixedPcdGet8 (PcdDebugPropertyMask) &
> >> +   DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED) != 0) {
> >> +CpuBreakpoint ();
> >> +  } else if ((FixedPcdGet8 (PcdDebugPropertyMask) &
> >> +  DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED) != 0) {
> >> +CpuDeadLoop ();
> >> +  }
> > 
> > Hmm ... I know this does not fundamentally change the behaviour of the
> > existing implementation, but if we're looking to improve runtime
> > behaviour, we've just gone from generating a runtime fault to silently
> > freezing (if BREAKPOINT_ENABLED or DEADLOOP_ENABLED).
> > 
> > What do breakpoint/deadloop mean in a runtime context anyway - do we
> > not need to halt _all_ running cores?
> > 
> > I don't see an obvious "right way" solution here, and this only
> > affects DEBUG builds.
> 
> Leif,
> 
> It is not related to DEBUG builds, it is related to PCD configuration. 

Sorry, I was oversimplifying based on most RELEASE builds tending to
have DebugAssertEnabled disabled through PcdDebugPropertyMask.
Looking through edk2 platforms, that shows to be incorrect even among
most open platform ports, so thanks for pointing this out.

> > Possible ways of handling this that I can think
> > of include:
> > - Don't respect BREAKPOINT/DEADLOOP if at runtime.
> > - Respect BREAKPOINT/DEADLOOP and disable all cores.
> > - Take ownership back of the system and re-enable 1:1 mapping so
> >  messages can be printed.
> 
> There is not much risk of losing user data if you "panic" EFI, that
> is not true if you are going to "panic" the OS. I've seen more bugs
> at runtime from confusion about what is legal to do at runtime,
> vs. actual bugs. You can always just return device error on failure,
> and if the RT Services hangs you can core dump the OS. Given the OS
> provided the virtual mapping it is likely the stuck EFI could would
> be in the stack trace of the panic.

Oh, indeed. My concern is regarding the fact that this library (with
either of those options set) would halt the executing processor (and
no others) without any output whatsoever.

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

Re: [edk2] [PATCH 1/3] MdePkg: introduce DxeRuntimeDebugLibSerialPort

[Laszlo Ersek]

On 02/20/18 12:05, Ard Biesheuvel wrote:
> Introduce a variant of BaseDebugLibSerialPort that behaves correctly wrt
> to use of the serial port after ExitBootServices(). Also, it uses fixed
> PCDs for all the parameterized values so that no calls into PcdLib are
> made at runtime.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel 
> ---
>  MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c   
> | 342 
>  MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf 
> |  46 +++
>  MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.uni 
> |  21 ++
>  3 files changed, 409 insertions(+)
> 
> diff --git a/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c 
> b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
> new file mode 100644
> index ..d18267d91322
> --- /dev/null
> +++ b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
> @@ -0,0 +1,342 @@
> +/** @file
> +  DXE runtime Debug library instance based on Serial Port library.
> +  It uses PrintLib to send debug messages to serial port device.
> +
> +  NOTE: If the Serial Port library enables hardware flow control, then a call
> +  to DebugPrint() or DebugAssert() may hang if writes to the serial port are
> +  being blocked.  This may occur if a key(s) are pressed in a terminal 
> emulator
> +  used to monitor the DEBUG() and ASSERT() messages.
> +
> +  Copyright (c) 2006 - 2011, Intel Corporation. All rights reserved.
> +  Copyright (c) 2018, Linaro, Ltd. All rights reserved.
> +
> +  This program and the accompanying materials
> +  are licensed and made available under the terms and conditions of the BSD 
> License
> +  which accompanies this distribution.  The full text of the license may be 
> found at
> +  http://opensource.org/licenses/bsd-license.php.
> +
> +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
> +
> +**/
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +STATIC EFI_EVENT  mEfiExitBootServicesEvent;
> +STATIC BOOLEANmEfiAtRuntime;
> +
> +//
> +// Define the maximum debug and assert message length that this library 
> supports
> +//
> +#define MAX_DEBUG_MESSAGE_LENGTH  0x100
> +
> +/**
> +  Set AtRuntime flag as TRUE after ExitBootServices.
> +
> +  @param[in]  Event   The Event that is being processed.
> +  @param[in]  Context The Event Context.
> +
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +RuntimeLibExitBootServicesEvent (
> +  IN EFI_EVENTEvent,
> +  IN VOID *Context
> +  )
> +{
> +  mEfiAtRuntime = TRUE;
> +}
> +
> +/**
> +  The constructor function initialize the Serial Port Library
> +
> +  @retval EFI_SUCCESS   The constructor always returns RETURN_SUCCESS.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +DxeRuntimeDebugLibSerialPortConstructor (
> +  IN EFI_HANDLEImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUSStatus;
> +
> +  Status = SerialPortInitialize ();
> +  if (EFI_ERROR (Status)) {
> +return Status;
> +  }
> +
> +  return SystemTable->BootServices->CreateEventEx (
> +  EVT_NOTIFY_SIGNAL,
> +  TPL_NOTIFY,
> +  RuntimeLibExitBootServicesEvent,
> +  NULL,
> +  ,
> +  );
> +}
> +
> +/**
> +  Prints a debug message to the debug output device if the specified error 
> level
> +  is enabled.
> +
> +  If any bit in ErrorLevel is also set in DebugPrintErrorLevelLib function
> +  GetDebugPrintErrorLevel (), then print the message specified by Format and 
> the
> +  associated variable argument list to the debug output device.
> +
> +  If Format is NULL, then ASSERT().
> +
> +  @param  ErrorLevel  The error level of the debug message.
> +  @param  Format  Format string for the debug message to print.
> +  @param  ... Variable argument list whose contents are accessed
> +  based on the format string specified by Format.
> +
> +**/
> +VOID
> +EFIAPI
> +DebugPrint (
> +  IN  UINTNErrorLevel,
> +  IN  CONST CHAR8  *Format,
> +  ...
> +  )
> +{
> +  CHAR8Buffer[MAX_DEBUG_MESSAGE_LENGTH];
> +  VA_LIST  Marker;
> +
> +  if (mEfiAtRuntime) {
> +return;
> +  }
> +
> +  //
> +  // Check driver debug mask value and global mask
> +  //
> +  if ((ErrorLevel & GetDebugPrintErrorLevel ()) == 0) {
> +return;
> +  }
> +
> +  //
> +  // Convert the DEBUG() message to an ASCII String
> +  //
> +  VA_START (Marker, Format);
> +  AsciiVSPrint (Buffer, sizeof (Buffer), Format, Marker);
> +  VA_END (Marker);
> +
> +  //
> +  // Send the print string to a Serial Port
> +  //
> +  

Re: [edk2] [PATCH 1/3] MdePkg: introduce DxeRuntimeDebugLibSerialPort

[Andrew Fish]

> On Feb 20, 2018, at 6:16 AM, Leif Lindholm  wrote:
> 
> On Tue, Feb 20, 2018 at 11:05:22AM +, Ard Biesheuvel wrote:
>> +/**
>> +  Prints an assert message containing a filename, line number, and 
>> description.
>> +  This may be followed by a breakpoint or a dead loop.
>> +
>> +  Print a message of the form "ASSERT (): 
>> \n"
>> +  to the debug output device.  If DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED 
>> bit
>> +  of PcdDebugProperyMask is set then CpuBreakpoint() is called. Otherwise, 
>> if
>> +  DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED bit of PcdDebugProperyMask is set 
>> then
>> +  CpuDeadLoop() is called.  If neither of these bits are set, then this 
>> function
>> +  returns immediately after the message is printed to the debug output 
>> device.
>> +  DebugAssert() must actively prevent recursion.  If DebugAssert() is called
>> +  while processing another DebugAssert(), then DebugAssert() must return
>> +  immediately.
>> +
>> +  If FileName is NULL, then a  string of "(NULL) Filename" is 
>> printed.
>> +  If Description is NULL, then a  string of "(NULL) 
>> Description" is
>> +  printed.
>> +
>> +  @param  FileName The pointer to the name of the source file that 
>> generated
>> +   the assert condition.
>> +  @param  LineNumber   The line number in the source file that generated the
>> +   assert condition
>> +  @param  Description  The pointer to the description of the assert 
>> condition.
>> +
>> +**/
>> +VOID
>> +EFIAPI
>> +DebugAssert (
>> +  IN CONST CHAR8  *FileName,
>> +  IN UINTNLineNumber,
>> +  IN CONST CHAR8  *Description
>> +  )
>> +{
>> +  CHAR8  Buffer[MAX_DEBUG_MESSAGE_LENGTH];
>> +
>> +  if (!mEfiAtRuntime) {
>> +//
>> +// Generate the ASSERT() message in Ascii format
>> +//
>> +AsciiSPrint (Buffer, sizeof (Buffer), "ASSERT [%a] %a(%d): %a\n",
>> +  gEfiCallerBaseName, FileName, LineNumber, Description);
>> +
>> +//
>> +// Send the print string to the Console Output device
>> +//
>> +SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer));
>> +  }
>> +
>> +  //
>> +  // Generate a Breakpoint, DeadLoop, or NOP based on PCD settings
>> +  //
>> +  if ((FixedPcdGet8 (PcdDebugPropertyMask) &
>> +   DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED) != 0) {
>> +CpuBreakpoint ();
>> +  } else if ((FixedPcdGet8 (PcdDebugPropertyMask) &
>> +  DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED) != 0) {
>> +CpuDeadLoop ();
>> +  }
> 
> Hmm ... I know this does not fundamentally change the behaviour of the
> existing implementation, but if we're looking to improve runtime
> behaviour, we've just gone from generating a runtime fault to silently
> freezing (if BREAKPOINT_ENABLED or DEADLOOP_ENABLED).
> 
> What do breakpoint/deadloop mean in a runtime context anyway - do we
> not need to halt _all_ running cores?
> 
> I don't see an obvious "right way" solution here, and this only
> affects DEBUG builds.

Leif,

It is not related to DEBUG builds, it is related to PCD configuration. 

> Possible ways of handling this that I can think
> of include:
> - Don't respect BREAKPOINT/DEADLOOP if at runtime.
> - Respect BREAKPOINT/DEADLOOP and disable all cores.
> - Take ownership back of the system and re-enable 1:1 mapping so
>  messages can be printed.
> 

There is not much risk of losing user data if you "panic" EFI, that is not true 
if you are going to "panic" the OS. I've seen more bugs at runtime from 
confusion about what is legal to do at runtime, vs. actual bugs. You can always 
just return device error on failure, and if the RT Services hangs you can core 
dump the OS. Given the OS provided the virtual mapping it is likely the stuck 
EFI could would be in the stack trace of the panic. 

Thanks,

Andrew Fish

> /
>Leif
> ___
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

Re: [edk2] [PATCH 1/3] MdePkg: introduce DxeRuntimeDebugLibSerialPort

[Leif Lindholm]

On Tue, Feb 20, 2018 at 11:05:22AM +, Ard Biesheuvel wrote:
> +/**
> +  Prints an assert message containing a filename, line number, and 
> description.
> +  This may be followed by a breakpoint or a dead loop.
> +
> +  Print a message of the form "ASSERT (): 
> \n"
> +  to the debug output device.  If DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED 
> bit
> +  of PcdDebugProperyMask is set then CpuBreakpoint() is called. Otherwise, if
> +  DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED bit of PcdDebugProperyMask is set 
> then
> +  CpuDeadLoop() is called.  If neither of these bits are set, then this 
> function
> +  returns immediately after the message is printed to the debug output 
> device.
> +  DebugAssert() must actively prevent recursion.  If DebugAssert() is called
> +  while processing another DebugAssert(), then DebugAssert() must return
> +  immediately.
> +
> +  If FileName is NULL, then a  string of "(NULL) Filename" is 
> printed.
> +  If Description is NULL, then a  string of "(NULL) 
> Description" is
> +  printed.
> +
> +  @param  FileName The pointer to the name of the source file that 
> generated
> +   the assert condition.
> +  @param  LineNumber   The line number in the source file that generated the
> +   assert condition
> +  @param  Description  The pointer to the description of the assert 
> condition.
> +
> +**/
> +VOID
> +EFIAPI
> +DebugAssert (
> +  IN CONST CHAR8  *FileName,
> +  IN UINTNLineNumber,
> +  IN CONST CHAR8  *Description
> +  )
> +{
> +  CHAR8  Buffer[MAX_DEBUG_MESSAGE_LENGTH];
> +
> +  if (!mEfiAtRuntime) {
> +//
> +// Generate the ASSERT() message in Ascii format
> +//
> +AsciiSPrint (Buffer, sizeof (Buffer), "ASSERT [%a] %a(%d): %a\n",
> +  gEfiCallerBaseName, FileName, LineNumber, Description);
> +
> +//
> +// Send the print string to the Console Output device
> +//
> +SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer));
> +  }
> +
> +  //
> +  // Generate a Breakpoint, DeadLoop, or NOP based on PCD settings
> +  //
> +  if ((FixedPcdGet8 (PcdDebugPropertyMask) &
> +   DEBUG_PROPERTY_ASSERT_BREAKPOINT_ENABLED) != 0) {
> +CpuBreakpoint ();
> +  } else if ((FixedPcdGet8 (PcdDebugPropertyMask) &
> +  DEBUG_PROPERTY_ASSERT_DEADLOOP_ENABLED) != 0) {
> +CpuDeadLoop ();
> +  }

Hmm ... I know this does not fundamentally change the behaviour of the
existing implementation, but if we're looking to improve runtime
behaviour, we've just gone from generating a runtime fault to silently
freezing (if BREAKPOINT_ENABLED or DEADLOOP_ENABLED).

What do breakpoint/deadloop mean in a runtime context anyway - do we
not need to halt _all_ running cores?

I don't see an obvious "right way" solution here, and this only
affects DEBUG builds. Possible ways of handling this that I can think
of include:
- Don't respect BREAKPOINT/DEADLOOP if at runtime.
- Respect BREAKPOINT/DEADLOOP and disable all cores.
- Take ownership back of the system and re-enable 1:1 mapping so
  messages can be printed.

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