[PATCH 0/3] Create UART DebugLib implementation for runtime drivers

[PATCH 0/3] Create UART DebugLib implementation for runtime drivers

[Ard Biesheuvel]

Commit 4bf95a9f361e ("MdeModulePkg/ResetSystemRuntimeDxe: Add more debug
message") broke the DEBUG build for systems using a MMIO mapped UART for
DEBUG output.

Instead of patching it up locally, let's fix this issue once and for all,
by creating a UART DebugLib implementation that does the right thing by
default, and blacklisting the BASE version for use by DXE_RUNTIME_DRIVER
modules.

Ard Biesheuvel (3):
  MdePkg: introduce DxeRuntimeDebugLibSerialPort
  ArmVirtPkg: switch to DXE runtime version of DebugLib where
    appropriate
  MdePkg/BaseDebugLibSerialPort: blacklist for use by DXE runtime
    drivers

 ArmVirtPkg/ArmVirt.dsc.inc                                                   |   3 +
 MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf             |   2 +-
 MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c                       | 342 ++++++++++++++++++++
 MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf |  46 +++
 MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.uni |  21 ++
 5 files changed, 413 insertions(+), 1 deletion(-)
 create mode 100644 MdePkg/Library/DxeRuntimeDebugLibSerialPort/DebugLib.c
 create mode 100644 MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
 create mode 100644 MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.uni

-- 
2.11.0

[PATCH 1/3] MdePkg: introduce DxeRuntimeDebugLibSerialPort

[Ard Biesheuvel]

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 <ard.biesheuvel@linaro.org>
---
 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 000000000000..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.<BR>
+  Copyright (c) 2018, Linaro, Ltd. All rights reserved.<BR>
+
+  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 <Base.h>
+#include <Library/DebugLib.h>
+#include <Library/DebugPrintErrorLevelLib.h>
+#include <Library/BaseLib.h>
+#include <Library/PrintLib.h>
+#include <Library/PcdLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/SerialPortLib.h>
+
+STATIC EFI_EVENT      mEfiExitBootServicesEvent;
+STATIC BOOLEAN        mEfiAtRuntime;
+
+//
+// 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_EVENT        Event,
+  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_HANDLE        ImageHandle,
+  IN EFI_SYSTEM_TABLE  *SystemTable
+  )
+{
+  EFI_STATUS    Status;
+
+  Status = SerialPortInitialize ();
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  return SystemTable->BootServices->CreateEventEx (
+                                      EVT_NOTIFY_SIGNAL,
+                                      TPL_NOTIFY,
+                                      RuntimeLibExitBootServicesEvent,
+                                      NULL,
+                                      &gEfiEventExitBootServicesGuid,
+                                      &mEfiExitBootServicesEvent);
+}
+
+/**
+  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  UINTN        ErrorLevel,
+  IN  CONST CHAR8  *Format,
+  ...
+  )
+{
+  CHAR8    Buffer[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
+  //
+  SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer));
+}
+
+
+/**
+  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 <FileName>(<LineNumber>): <Description>\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 <FileName> string of "(NULL) Filename" is printed.
+  If Description is NULL, then a <Description> 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 UINTN        LineNumber,
+  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 ();
+  }
+}
+
+
+/**
+  Fills a target buffer with PcdDebugClearMemoryValue, and returns the target
+  buffer.
+
+  This function fills Length bytes of Buffer with the value specified by
+  PcdDebugClearMemoryValue, and returns Buffer.
+
+  If Buffer is NULL, then ASSERT().
+  If Length is greater than (MAX_ADDRESS - Buffer + 1), then ASSERT().
+
+  @param   Buffer  The pointer to the target buffer to be filled with
+                   PcdDebugClearMemoryValue.
+  @param   Length  The number of bytes in Buffer to fill with
+                   PcdDebugClearMemoryValue.
+
+  @return  Buffer  The pointer to the target buffer filled with
+                   PcdDebugClearMemoryValue.
+
+**/
+VOID *
+EFIAPI
+DebugClearMemory (
+  OUT VOID  *Buffer,
+  IN UINTN  Length
+  )
+{
+  //
+  // SetMem() checks for the the ASSERT() condition on Length and returns Buffer
+  //
+  return SetMem (Buffer, Length, FixedPcdGet8 (PcdDebugClearMemoryValue));
+}
+
+
+/**
+  Returns TRUE if ASSERT() macros are enabled.
+
+  This function returns TRUE if the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED bit of
+  PcdDebugProperyMask is set.  Otherwise FALSE is returned.
+
+  @retval  TRUE    The DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED bit of
+                   PcdDebugProperyMask is set.
+  @retval  FALSE   The DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED bit of
+                   PcdDebugProperyMask is clear.
+
+**/
+BOOLEAN
+EFIAPI
+DebugAssertEnabled (
+  VOID
+  )
+{
+  return (FixedPcdGet8 (PcdDebugPropertyMask) &
+          DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED) != 0;
+}
+
+
+/**
+  Returns TRUE if DEBUG() macros are enabled.
+
+  This function returns TRUE if the DEBUG_PROPERTY_DEBUG_PRINT_ENABLED bit of
+  PcdDebugProperyMask is set.  Otherwise FALSE is returned.
+
+  @retval  TRUE    The DEBUG_PROPERTY_DEBUG_PRINT_ENABLED bit of
+                   PcdDebugProperyMask is set.
+  @retval  FALSE   The DEBUG_PROPERTY_DEBUG_PRINT_ENABLED bit of
+                   PcdDebugProperyMask is clear.
+
+**/
+BOOLEAN
+EFIAPI
+DebugPrintEnabled (
+  VOID
+  )
+{
+  return (FixedPcdGet8 (PcdDebugPropertyMask) &
+          DEBUG_PROPERTY_DEBUG_PRINT_ENABLED) != 0;
+}
+
+
+/**
+  Returns TRUE if DEBUG_CODE() macros are enabled.
+
+  This function returns TRUE if the DEBUG_PROPERTY_DEBUG_CODE_ENABLED bit of
+  PcdDebugProperyMask is set.  Otherwise FALSE is returned.
+
+  @retval  TRUE    The DEBUG_PROPERTY_DEBUG_CODE_ENABLED bit of
+                   PcdDebugProperyMask is set.
+  @retval  FALSE   The DEBUG_PROPERTY_DEBUG_CODE_ENABLED bit of
+                   PcdDebugProperyMask is clear.
+
+**/
+BOOLEAN
+EFIAPI
+DebugCodeEnabled (
+  VOID
+  )
+{
+  return (FixedPcdGet8 (PcdDebugPropertyMask) &
+          DEBUG_PROPERTY_DEBUG_CODE_ENABLED) != 0;
+}
+
+
+/**
+  Returns TRUE if DEBUG_CLEAR_MEMORY() macro is enabled.
+
+  This function returns TRUE if the DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED bit of
+  PcdDebugProperyMask is set.  Otherwise FALSE is returned.
+
+  @retval  TRUE    The DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED bit of
+                   PcdDebugProperyMask is set.
+  @retval  FALSE   The DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED bit of
+                   PcdDebugProperyMask is clear.
+
+**/
+BOOLEAN
+EFIAPI
+DebugClearMemoryEnabled (
+  VOID
+  )
+{
+  return (FixedPcdGet8 (PcdDebugPropertyMask) &
+          DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED) != 0;
+}
+
+/**
+  Returns TRUE if any one of the bit is set both in ErrorLevel and
+  PcdFixedDebugPrintErrorLevel.
+
+  This function compares the bit mask of ErrorLevel and
+  PcdFixedDebugPrintErrorLevel.
+
+  @retval  TRUE    Current ErrorLevel is supported.
+  @retval  FALSE   Current ErrorLevel is not supported.
+
+**/
+BOOLEAN
+EFIAPI
+DebugPrintLevelEnabled (
+  IN  CONST UINTN        ErrorLevel
+  )
+{
+  return (ErrorLevel & FixedPcdGet32 (PcdFixedDebugPrintErrorLevel)) != 0;
+}
diff --git a/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
new file mode 100644
index 000000000000..9f300f4f1b12
--- /dev/null
+++ b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
@@ -0,0 +1,46 @@
+## @file
+#
+#  Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2018, Linaro, Ltd. All rights reserved.<BR>
+#
+#  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.
+#
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = DxeRuntimeDebugLibSerialPort
+  MODULE_UNI_FILE                = DxeRuntimeDebugLibSerialPort.uni
+  FILE_GUID                      = 9D914E2F-7CCB-41DB-8E74-9AFF8F3BBFBF
+  MODULE_TYPE                    = DXE_RUNTIME_DRIVER
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = DebugLib
+  CONSTRUCTOR                    = DxeRuntimeDebugLibSerialPortConstructor
+
+[Sources]
+  DebugLib.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+
+[LibraryClasses]
+  BaseLib
+  BaseMemoryLib
+  DebugPrintErrorLevelLib
+  PcdLib
+  PrintLib
+  SerialPortLib
+
+[Guids]
+  gEfiEventExitBootServicesGuid
+
+[FixedPcd]
+  gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue     ## SOMETIMES_CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask         ## CONSUMES
+  gEfiMdePkgTokenSpaceGuid.PcdFixedDebugPrintErrorLevel ## CONSUMES
diff --git a/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.uni b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.uni
new file mode 100644
index 000000000000..cd65515c4177
--- /dev/null
+++ b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.uni
@@ -0,0 +1,21 @@
+// /** @file
+// Instance of Debug Library based on Serial Port Library.
+//
+// It uses Print Library to produce formatted output strings to seiral port device.
+//
+// Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.<BR>
+//
+// 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.
+//
+// **/
+
+
+#string STR_MODULE_ABSTRACT             #language en-US "Instance of Debug Library based on Serial Port Library"
+
+#string STR_MODULE_DESCRIPTION          #language en-US "It uses Print Library to produce formatted output strings to a serial port device."
+
-- 
2.11.0

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

[Leif Lindholm]

On Tue, Feb 20, 2018 at 11:05:22AM +0000, 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 <FileName>(<LineNumber>): <Description>\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 <FileName> string of "(NULL) Filename" is printed.
> +  If Description is NULL, then a <Description> 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 UINTN        LineNumber,
> +  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

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

[Andrew Fish]

> On Feb 20, 2018, at 6:16 AM, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> 
> On Tue, Feb 20, 2018 at 11:05:22AM +0000, 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 <FileName>(<LineNumber>): <Description>\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 <FileName> string of "(NULL) Filename" is printed.
>> +  If Description is NULL, then a <Description> 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 UINTN        LineNumber,
>> +  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

Re: [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 <leif.lindholm@linaro.org> wrote:
> > 
> > On Tue, Feb 20, 2018 at 11:05:22AM +0000, 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 <FileName>(<LineNumber>): <Description>\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 <FileName> string of "(NULL) Filename" is printed.
> >> +  If Description is NULL, then a <Description> 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 UINTN        LineNumber,
> >> +  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

Re: [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 <ard.biesheuvel@linaro.org>
> ---
>  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 000000000000..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.<BR>
> +  Copyright (c) 2018, Linaro, Ltd. All rights reserved.<BR>
> +
> +  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 <Base.h>
> +#include <Library/DebugLib.h>
> +#include <Library/DebugPrintErrorLevelLib.h>
> +#include <Library/BaseLib.h>
> +#include <Library/PrintLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/SerialPortLib.h>
> +
> +STATIC EFI_EVENT      mEfiExitBootServicesEvent;
> +STATIC BOOLEAN        mEfiAtRuntime;
> +
> +//
> +// 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_EVENT        Event,
> +  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_HANDLE        ImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUS    Status;
> +
> +  Status = SerialPortInitialize ();
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  return SystemTable->BootServices->CreateEventEx (
> +                                      EVT_NOTIFY_SIGNAL,
> +                                      TPL_NOTIFY,
> +                                      RuntimeLibExitBootServicesEvent,
> +                                      NULL,
> +                                      &gEfiEventExitBootServicesGuid,
> +                                      &mEfiExitBootServicesEvent);
> +}
> +
> +/**
> +  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  UINTN        ErrorLevel,
> +  IN  CONST CHAR8  *Format,
> +  ...
> +  )
> +{
> +  CHAR8    Buffer[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
> +  //
> +  SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer));
> +}
> +
> +
> +/**
> +  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 <FileName>(<LineNumber>): <Description>\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 <FileName> string of "(NULL) Filename" is printed.
> +  If Description is NULL, then a <Description> 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 UINTN        LineNumber,
> +  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 ();
> +  }
> +}
> +
> +
> +/**
> +  Fills a target buffer with PcdDebugClearMemoryValue, and returns the target
> +  buffer.
> +
> +  This function fills Length bytes of Buffer with the value specified by
> +  PcdDebugClearMemoryValue, and returns Buffer.
> +
> +  If Buffer is NULL, then ASSERT().
> +  If Length is greater than (MAX_ADDRESS - Buffer + 1), then ASSERT().
> +
> +  @param   Buffer  The pointer to the target buffer to be filled with
> +                   PcdDebugClearMemoryValue.
> +  @param   Length  The number of bytes in Buffer to fill with
> +                   PcdDebugClearMemoryValue.
> +
> +  @return  Buffer  The pointer to the target buffer filled with
> +                   PcdDebugClearMemoryValue.
> +
> +**/
> +VOID *
> +EFIAPI
> +DebugClearMemory (
> +  OUT VOID  *Buffer,
> +  IN UINTN  Length
> +  )
> +{
> +  //
> +  // SetMem() checks for the the ASSERT() condition on Length and returns Buffer
> +  //
> +  return SetMem (Buffer, Length, FixedPcdGet8 (PcdDebugClearMemoryValue));
> +}
> +
> +
> +/**
> +  Returns TRUE if ASSERT() macros are enabled.
> +
> +  This function returns TRUE if the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED bit of
> +  PcdDebugProperyMask is set.  Otherwise FALSE is returned.
> +
> +  @retval  TRUE    The DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED bit of
> +                   PcdDebugProperyMask is set.
> +  @retval  FALSE   The DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED bit of
> +                   PcdDebugProperyMask is clear.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +DebugAssertEnabled (
> +  VOID
> +  )
> +{
> +  return (FixedPcdGet8 (PcdDebugPropertyMask) &
> +          DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED) != 0;
> +}
> +
> +
> +/**
> +  Returns TRUE if DEBUG() macros are enabled.
> +
> +  This function returns TRUE if the DEBUG_PROPERTY_DEBUG_PRINT_ENABLED bit of
> +  PcdDebugProperyMask is set.  Otherwise FALSE is returned.
> +
> +  @retval  TRUE    The DEBUG_PROPERTY_DEBUG_PRINT_ENABLED bit of
> +                   PcdDebugProperyMask is set.
> +  @retval  FALSE   The DEBUG_PROPERTY_DEBUG_PRINT_ENABLED bit of
> +                   PcdDebugProperyMask is clear.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +DebugPrintEnabled (
> +  VOID
> +  )
> +{
> +  return (FixedPcdGet8 (PcdDebugPropertyMask) &
> +          DEBUG_PROPERTY_DEBUG_PRINT_ENABLED) != 0;
> +}
> +
> +
> +/**
> +  Returns TRUE if DEBUG_CODE() macros are enabled.
> +
> +  This function returns TRUE if the DEBUG_PROPERTY_DEBUG_CODE_ENABLED bit of
> +  PcdDebugProperyMask is set.  Otherwise FALSE is returned.
> +
> +  @retval  TRUE    The DEBUG_PROPERTY_DEBUG_CODE_ENABLED bit of
> +                   PcdDebugProperyMask is set.
> +  @retval  FALSE   The DEBUG_PROPERTY_DEBUG_CODE_ENABLED bit of
> +                   PcdDebugProperyMask is clear.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +DebugCodeEnabled (
> +  VOID
> +  )
> +{
> +  return (FixedPcdGet8 (PcdDebugPropertyMask) &
> +          DEBUG_PROPERTY_DEBUG_CODE_ENABLED) != 0;
> +}
> +
> +
> +/**
> +  Returns TRUE if DEBUG_CLEAR_MEMORY() macro is enabled.
> +
> +  This function returns TRUE if the DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED bit of
> +  PcdDebugProperyMask is set.  Otherwise FALSE is returned.
> +
> +  @retval  TRUE    The DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED bit of
> +                   PcdDebugProperyMask is set.
> +  @retval  FALSE   The DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED bit of
> +                   PcdDebugProperyMask is clear.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +DebugClearMemoryEnabled (
> +  VOID
> +  )
> +{
> +  return (FixedPcdGet8 (PcdDebugPropertyMask) &
> +          DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED) != 0;
> +}
> +
> +/**
> +  Returns TRUE if any one of the bit is set both in ErrorLevel and
> +  PcdFixedDebugPrintErrorLevel.
> +
> +  This function compares the bit mask of ErrorLevel and
> +  PcdFixedDebugPrintErrorLevel.
> +
> +  @retval  TRUE    Current ErrorLevel is supported.
> +  @retval  FALSE   Current ErrorLevel is not supported.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +DebugPrintLevelEnabled (
> +  IN  CONST UINTN        ErrorLevel
> +  )
> +{
> +  return (ErrorLevel & FixedPcdGet32 (PcdFixedDebugPrintErrorLevel)) != 0;
> +}
> diff --git a/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> new file mode 100644
> index 000000000000..9f300f4f1b12
> --- /dev/null
> +++ b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> @@ -0,0 +1,46 @@
> +## @file
> +#
> +#  Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR>
> +#  Copyright (c) 2018, Linaro, Ltd. All rights reserved.<BR>
> +#
> +#  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.
> +#
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = DxeRuntimeDebugLibSerialPort
> +  MODULE_UNI_FILE                = DxeRuntimeDebugLibSerialPort.uni
> +  FILE_GUID                      = 9D914E2F-7CCB-41DB-8E74-9AFF8F3BBFBF
> +  MODULE_TYPE                    = DXE_RUNTIME_DRIVER
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = DebugLib
> +  CONSTRUCTOR                    = DxeRuntimeDebugLibSerialPortConstructor
> +
> +[Sources]
> +  DebugLib.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  BaseMemoryLib
> +  DebugPrintErrorLevelLib
> +  PcdLib
> +  PrintLib
> +  SerialPortLib
> +
> +[Guids]
> +  gEfiEventExitBootServicesGuid
> +
> +[FixedPcd]
> +  gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue     ## SOMETIMES_CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask         ## CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdFixedDebugPrintErrorLevel ## CONSUMES
> diff --git a/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.uni b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.uni
> new file mode 100644
> index 000000000000..cd65515c4177
> --- /dev/null
> +++ b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.uni
> @@ -0,0 +1,21 @@
> +// /** @file
> +// Instance of Debug Library based on Serial Port Library.
> +//
> +// It uses Print Library to produce formatted output strings to seiral port device.
> +//
> +// Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.<BR>
> +//
> +// 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.
> +//
> +// **/
> +
> +
> +#string STR_MODULE_ABSTRACT             #language en-US "Instance of Debug Library based on Serial Port Library"
> +
> +#string STR_MODULE_DESCRIPTION          #language en-US "It uses Print Library to produce formatted output strings to a serial port device."
> +
> 

(1) The "raison d'etre" of this library instance is spelled out well in
the commit message, but it's not reflected in the files:
- The C file has a @file comment, but it's not (fully) up-to-date.
- The INF file has an empty @file comment.
- The UNI file has a @file comment, but both that comment and the STR_*
entries are out of date (IMO).

(2) I think it would be nice to keep VALID_ARCHITECTURES in the INF
file. (If you disagree, that's fine as well.)

(3) The LIBRARY_CLASS definition in the INF file should be restricted as
in: "DebugLib|DXE_RUNTIME_DRIVER". The MODULE_TYPE definition does not
effect this restriction, it only determines the constructor and
destructor signatures for the library instance.

(4) I think gEfiEventExitBootServicesGuid should be marked as "##
CONSUMES" in the INF file.

(5) Beyond having a constructor, this library instance needs a
destructor as well -- if a client runtime DXE driver exits its entry
point function with an error, then it is unloaded, so we have to close
the event in the lib instance's destructor function. See the oddly named
RuntimeDriverLibDeconstruct() function, for example, in
"MdePkg/Library/UefiRuntimeLib/RuntimeLib.c".

(6) the name "RuntimeLibExitBootServicesEvent" comes from
UefiRuntimeLib, I think we should find a better ("more local") name for
our function here. What about just "ExitBootServicesNotify"?

(7) The trailing (closing) paren after CreateEventEx() is not idiomatic.

(8) In DebugPrint(), I think it would make sense to preserve "ASSERT
(Format != NULL)"; at least if we continue to claim in the leading
comment, "If Format is NULL, then ASSERT()". (BTW this claim comes from
the lib class header, so we might have no choice here.)

(9) The same as (8) applies to DebugClearMemory().

(10) I suggest to keep the (BOOLEAN) cast in DebugPrintLevelEnabled(),
from the original. While it is terrible style, some VS compilers yell
that an "INT" (the result of the != operator) is converted to a UINT8
(i.e. BOOLEAN), causing possible loss of precision, or some such. :/

(11) I see there's a bunch of cleanup going on as well, such as
stripping trailing whitespace and rewrapping. I think that's great, but:
it should be first done to BaseDebugLibSerialPort, and the
DxeRuntimeDebugLibSerialPort instance should only contain the functional
changes on top. In my opinion anyway.

(12) In the commit message, "wrt to" should be replaced with "wrt the".


Before doing any of the above work, I suggest we await feedback from the
MdePkg maintainers. Also, please feel free to skip those comments from
my side that you disagree with.

Thank you for the patch!
Laszlo

Re: [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-
> bounces@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 <ruiyu.ni@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>; afish@apple.com;
> leif.lindholm@linaro.org; Gao, Liming
> <liming.gao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; lersek@redhat.com; Zeng,
> Star <star.zeng@intel.com>
> 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
> <ard.biesheuvel@linaro.org>
> ---
>  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 000000000000..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.<BR>
> +  Copyright (c) 2018, Linaro, Ltd. All rights
> reserved.<BR>
> +
> +  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 <Base.h>
> +#include <Library/DebugLib.h>
> +#include <Library/DebugPrintErrorLevelLib.h>
> +#include <Library/BaseLib.h>
> +#include <Library/PrintLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/SerialPortLib.h>
> +
> +STATIC EFI_EVENT      mEfiExitBootServicesEvent;
> +STATIC BOOLEAN        mEfiAtRuntime;
> +
> +//
> +// 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_EVENT        Event,
> +  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_HANDLE        ImageHandle,
> +  IN EFI_SYSTEM_TABLE  *SystemTable
> +  )
> +{
> +  EFI_STATUS    Status;
> +
> +  Status = SerialPortInitialize ();
> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  return SystemTable->BootServices->CreateEventEx (
> +
> EVT_NOTIFY_SIGNAL,
> +                                      TPL_NOTIFY,
> +
> RuntimeLibExitBootServicesEvent,
> +                                      NULL,
> +
> &gEfiEventExitBootServicesGuid,
> +
> &mEfiExitBootServicesEvent);
> +}
> +
> +/**
> +  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  UINTN        ErrorLevel,
> +  IN  CONST CHAR8  *Format,
> +  ...
> +  )
> +{
> +  CHAR8    Buffer[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
> +  //
> +  SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen
> (Buffer));
> +}
> +
> +
> +/**
> +  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
> <FileName>(<LineNumber>): <Description>\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 <FileName> string of
> "(NULL) Filename" is printed.
> +  If Description is NULL, then a <Description> 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 UINTN        LineNumber,
> +  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 ();
> +  }
> +}
> +
> +
> +/**
> +  Fills a target buffer with PcdDebugClearMemoryValue,
> and returns the target
> +  buffer.
> +
> +  This function fills Length bytes of Buffer with the
> value specified by
> +  PcdDebugClearMemoryValue, and returns Buffer.
> +
> +  If Buffer is NULL, then ASSERT().
> +  If Length is greater than (MAX_ADDRESS - Buffer + 1),
> then ASSERT().
> +
> +  @param   Buffer  The pointer to the target buffer to
> be filled with
> +                   PcdDebugClearMemoryValue.
> +  @param   Length  The number of bytes in Buffer to
> fill with
> +                   PcdDebugClearMemoryValue.
> +
> +  @return  Buffer  The pointer to the target buffer
> filled with
> +                   PcdDebugClearMemoryValue.
> +
> +**/
> +VOID *
> +EFIAPI
> +DebugClearMemory (
> +  OUT VOID  *Buffer,
> +  IN UINTN  Length
> +  )
> +{
> +  //
> +  // SetMem() checks for the the ASSERT() condition on
> Length and returns Buffer
> +  //
> +  return SetMem (Buffer, Length, FixedPcdGet8
> (PcdDebugClearMemoryValue));
> +}
> +
> +
> +/**
> +  Returns TRUE if ASSERT() macros are enabled.
> +
> +  This function returns TRUE if the
> DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED bit of
> +  PcdDebugProperyMask is set.  Otherwise FALSE is
> returned.
> +
> +  @retval  TRUE    The
> DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED bit of
> +                   PcdDebugProperyMask is set.
> +  @retval  FALSE   The
> DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED bit of
> +                   PcdDebugProperyMask is clear.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +DebugAssertEnabled (
> +  VOID
> +  )
> +{
> +  return (FixedPcdGet8 (PcdDebugPropertyMask) &
> +          DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED) != 0;
> +}
> +
> +
> +/**
> +  Returns TRUE if DEBUG() macros are enabled.
> +
> +  This function returns TRUE if the
> DEBUG_PROPERTY_DEBUG_PRINT_ENABLED bit of
> +  PcdDebugProperyMask is set.  Otherwise FALSE is
> returned.
> +
> +  @retval  TRUE    The
> DEBUG_PROPERTY_DEBUG_PRINT_ENABLED bit of
> +                   PcdDebugProperyMask is set.
> +  @retval  FALSE   The
> DEBUG_PROPERTY_DEBUG_PRINT_ENABLED bit of
> +                   PcdDebugProperyMask is clear.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +DebugPrintEnabled (
> +  VOID
> +  )
> +{
> +  return (FixedPcdGet8 (PcdDebugPropertyMask) &
> +          DEBUG_PROPERTY_DEBUG_PRINT_ENABLED) != 0;
> +}
> +
> +
> +/**
> +  Returns TRUE if DEBUG_CODE() macros are enabled.
> +
> +  This function returns TRUE if the
> DEBUG_PROPERTY_DEBUG_CODE_ENABLED bit of
> +  PcdDebugProperyMask is set.  Otherwise FALSE is
> returned.
> +
> +  @retval  TRUE    The
> DEBUG_PROPERTY_DEBUG_CODE_ENABLED bit of
> +                   PcdDebugProperyMask is set.
> +  @retval  FALSE   The
> DEBUG_PROPERTY_DEBUG_CODE_ENABLED bit of
> +                   PcdDebugProperyMask is clear.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +DebugCodeEnabled (
> +  VOID
> +  )
> +{
> +  return (FixedPcdGet8 (PcdDebugPropertyMask) &
> +          DEBUG_PROPERTY_DEBUG_CODE_ENABLED) != 0;
> +}
> +
> +
> +/**
> +  Returns TRUE if DEBUG_CLEAR_MEMORY() macro is
> enabled.
> +
> +  This function returns TRUE if the
> DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED bit of
> +  PcdDebugProperyMask is set.  Otherwise FALSE is
> returned.
> +
> +  @retval  TRUE    The
> DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED bit of
> +                   PcdDebugProperyMask is set.
> +  @retval  FALSE   The
> DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED bit of
> +                   PcdDebugProperyMask is clear.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +DebugClearMemoryEnabled (
> +  VOID
> +  )
> +{
> +  return (FixedPcdGet8 (PcdDebugPropertyMask) &
> +          DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED) != 0;
> +}
> +
> +/**
> +  Returns TRUE if any one of the bit is set both in
> ErrorLevel and
> +  PcdFixedDebugPrintErrorLevel.
> +
> +  This function compares the bit mask of ErrorLevel and
> +  PcdFixedDebugPrintErrorLevel.
> +
> +  @retval  TRUE    Current ErrorLevel is supported.
> +  @retval  FALSE   Current ErrorLevel is not supported.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +DebugPrintLevelEnabled (
> +  IN  CONST UINTN        ErrorLevel
> +  )
> +{
> +  return (ErrorLevel & FixedPcdGet32
> (PcdFixedDebugPrintErrorLevel)) != 0;
> +}
> diff --git
> a/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntime
> DebugLibSerialPort.inf
> b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntime
> DebugLibSerialPort.inf
> new file mode 100644
> index 000000000000..9f300f4f1b12
> --- /dev/null
> +++
> b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntime
> DebugLibSerialPort.inf
> @@ -0,0 +1,46 @@
> +## @file
> +#
> +#  Copyright (c) 2006 - 2015, Intel Corporation. All
> rights reserved.<BR>
> +#  Copyright (c) 2018, Linaro, Ltd. All rights
> reserved.<BR>
> +#
> +#  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.
> +#
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      =
> DxeRuntimeDebugLibSerialPort
> +  MODULE_UNI_FILE                =
> DxeRuntimeDebugLibSerialPort.uni
> +  FILE_GUID                      = 9D914E2F-7CCB-41DB-
> 8E74-9AFF8F3BBFBF
> +  MODULE_TYPE                    = DXE_RUNTIME_DRIVER
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = DebugLib
> +  CONSTRUCTOR                    =
> DxeRuntimeDebugLibSerialPortConstructor
> +
> +[Sources]
> +  DebugLib.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  BaseMemoryLib
> +  DebugPrintErrorLevelLib
> +  PcdLib
> +  PrintLib
> +  SerialPortLib
> +
> +[Guids]
> +  gEfiEventExitBootServicesGuid
> +
> +[FixedPcd]
> +  gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue
> ## SOMETIMES_CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask
> ## CONSUMES
> +  gEfiMdePkgTokenSpaceGuid.PcdFixedDebugPrintErrorLevel
> ## CONSUMES
> diff --git
> a/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntime
> DebugLibSerialPort.uni
> b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntime
> DebugLibSerialPort.uni
> new file mode 100644
> index 000000000000..cd65515c4177
> --- /dev/null
> +++
> b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntime
> DebugLibSerialPort.uni
> @@ -0,0 +1,21 @@
> +// /** @file
> +// Instance of Debug Library based on Serial Port
> Library.
> +//
> +// It uses Print Library to produce formatted output
> strings to seiral port device.
> +//
> +// Copyright (c) 2006 - 2014, Intel Corporation. All
> rights reserved.<BR>
> +//
> +// 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.
> +//
> +// **/
> +
> +
> +#string STR_MODULE_ABSTRACT             #language en-US
> "Instance of Debug Library based on Serial Port Library"
> +
> +#string STR_MODULE_DESCRIPTION          #language en-US
> "It uses Print Library to produce formatted output
> strings to a serial port device."
> +
> --
> 2.11.0
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

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

[Ard Biesheuvel]

On 20 February 2018 at 19:22, Kinney, Michael D
<michael.d.kinney@intel.com> 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-
>> bounces@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 <ruiyu.ni@intel.com>; Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>; afish@apple.com;
>> leif.lindholm@linaro.org; Gao, Liming
>> <liming.gao@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; lersek@redhat.com; Zeng,
>> Star <star.zeng@intel.com>
>> 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
>> <ard.biesheuvel@linaro.org>
>> ---
>>  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 000000000000..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.<BR>
>> +  Copyright (c) 2018, Linaro, Ltd. All rights
>> reserved.<BR>
>> +
>> +  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 <Base.h>
>> +#include <Library/DebugLib.h>
>> +#include <Library/DebugPrintErrorLevelLib.h>
>> +#include <Library/BaseLib.h>
>> +#include <Library/PrintLib.h>
>> +#include <Library/PcdLib.h>
>> +#include <Library/BaseMemoryLib.h>
>> +#include <Library/SerialPortLib.h>
>> +
>> +STATIC EFI_EVENT      mEfiExitBootServicesEvent;
>> +STATIC BOOLEAN        mEfiAtRuntime;
>> +
>> +//
>> +// 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_EVENT        Event,
>> +  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_HANDLE        ImageHandle,
>> +  IN EFI_SYSTEM_TABLE  *SystemTable
>> +  )
>> +{
>> +  EFI_STATUS    Status;
>> +
>> +  Status = SerialPortInitialize ();
>> +  if (EFI_ERROR (Status)) {
>> +    return Status;
>> +  }
>> +
>> +  return SystemTable->BootServices->CreateEventEx (
>> +
>> EVT_NOTIFY_SIGNAL,
>> +                                      TPL_NOTIFY,
>> +
>> RuntimeLibExitBootServicesEvent,
>> +                                      NULL,
>> +
>> &gEfiEventExitBootServicesGuid,
>> +
>> &mEfiExitBootServicesEvent);
>> +}
>> +
>> +/**
>> +  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  UINTN        ErrorLevel,
>> +  IN  CONST CHAR8  *Format,
>> +  ...
>> +  )
>> +{
>> +  CHAR8    Buffer[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
>> +  //
>> +  SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen
>> (Buffer));
>> +}
>> +
>> +
>> +/**
>> +  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
>> <FileName>(<LineNumber>): <Description>\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 <FileName> string of
>> "(NULL) Filename" is printed.
>> +  If Description is NULL, then a <Description> 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 UINTN        LineNumber,
>> +  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 ();
>> +  }
>> +}
>> +
>> +
>> +/**
>> +  Fills a target buffer with PcdDebugClearMemoryValue,
>> and returns the target
>> +  buffer.
>> +
>> +  This function fills Length bytes of Buffer with the
>> value specified by
>> +  PcdDebugClearMemoryValue, and returns Buffer.
>> +
>> +  If Buffer is NULL, then ASSERT().
>> +  If Length is greater than (MAX_ADDRESS - Buffer + 1),
>> then ASSERT().
>> +
>> +  @param   Buffer  The pointer to the target buffer to
>> be filled with
>> +                   PcdDebugClearMemoryValue.
>> +  @param   Length  The number of bytes in Buffer to
>> fill with
>> +                   PcdDebugClearMemoryValue.
>> +
>> +  @return  Buffer  The pointer to the target buffer
>> filled with
>> +                   PcdDebugClearMemoryValue.
>> +
>> +**/
>> +VOID *
>> +EFIAPI
>> +DebugClearMemory (
>> +  OUT VOID  *Buffer,
>> +  IN UINTN  Length
>> +  )
>> +{
>> +  //
>> +  // SetMem() checks for the the ASSERT() condition on
>> Length and returns Buffer
>> +  //
>> +  return SetMem (Buffer, Length, FixedPcdGet8
>> (PcdDebugClearMemoryValue));
>> +}
>> +
>> +
>> +/**
>> +  Returns TRUE if ASSERT() macros are enabled.
>> +
>> +  This function returns TRUE if the
>> DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED bit of
>> +  PcdDebugProperyMask is set.  Otherwise FALSE is
>> returned.
>> +
>> +  @retval  TRUE    The
>> DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED bit of
>> +                   PcdDebugProperyMask is set.
>> +  @retval  FALSE   The
>> DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED bit of
>> +                   PcdDebugProperyMask is clear.
>> +
>> +**/
>> +BOOLEAN
>> +EFIAPI
>> +DebugAssertEnabled (
>> +  VOID
>> +  )
>> +{
>> +  return (FixedPcdGet8 (PcdDebugPropertyMask) &
>> +          DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED) != 0;
>> +}
>> +
>> +
>> +/**
>> +  Returns TRUE if DEBUG() macros are enabled.
>> +
>> +  This function returns TRUE if the
>> DEBUG_PROPERTY_DEBUG_PRINT_ENABLED bit of
>> +  PcdDebugProperyMask is set.  Otherwise FALSE is
>> returned.
>> +
>> +  @retval  TRUE    The
>> DEBUG_PROPERTY_DEBUG_PRINT_ENABLED bit of
>> +                   PcdDebugProperyMask is set.
>> +  @retval  FALSE   The
>> DEBUG_PROPERTY_DEBUG_PRINT_ENABLED bit of
>> +                   PcdDebugProperyMask is clear.
>> +
>> +**/
>> +BOOLEAN
>> +EFIAPI
>> +DebugPrintEnabled (
>> +  VOID
>> +  )
>> +{
>> +  return (FixedPcdGet8 (PcdDebugPropertyMask) &
>> +          DEBUG_PROPERTY_DEBUG_PRINT_ENABLED) != 0;
>> +}
>> +
>> +
>> +/**
>> +  Returns TRUE if DEBUG_CODE() macros are enabled.
>> +
>> +  This function returns TRUE if the
>> DEBUG_PROPERTY_DEBUG_CODE_ENABLED bit of
>> +  PcdDebugProperyMask is set.  Otherwise FALSE is
>> returned.
>> +
>> +  @retval  TRUE    The
>> DEBUG_PROPERTY_DEBUG_CODE_ENABLED bit of
>> +                   PcdDebugProperyMask is set.
>> +  @retval  FALSE   The
>> DEBUG_PROPERTY_DEBUG_CODE_ENABLED bit of
>> +                   PcdDebugProperyMask is clear.
>> +
>> +**/
>> +BOOLEAN
>> +EFIAPI
>> +DebugCodeEnabled (
>> +  VOID
>> +  )
>> +{
>> +  return (FixedPcdGet8 (PcdDebugPropertyMask) &
>> +          DEBUG_PROPERTY_DEBUG_CODE_ENABLED) != 0;
>> +}
>> +
>> +
>> +/**
>> +  Returns TRUE if DEBUG_CLEAR_MEMORY() macro is
>> enabled.
>> +
>> +  This function returns TRUE if the
>> DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED bit of
>> +  PcdDebugProperyMask is set.  Otherwise FALSE is
>> returned.
>> +
>> +  @retval  TRUE    The
>> DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED bit of
>> +                   PcdDebugProperyMask is set.
>> +  @retval  FALSE   The
>> DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED bit of
>> +                   PcdDebugProperyMask is clear.
>> +
>> +**/
>> +BOOLEAN
>> +EFIAPI
>> +DebugClearMemoryEnabled (
>> +  VOID
>> +  )
>> +{
>> +  return (FixedPcdGet8 (PcdDebugPropertyMask) &
>> +          DEBUG_PROPERTY_CLEAR_MEMORY_ENABLED) != 0;
>> +}
>> +
>> +/**
>> +  Returns TRUE if any one of the bit is set both in
>> ErrorLevel and
>> +  PcdFixedDebugPrintErrorLevel.
>> +
>> +  This function compares the bit mask of ErrorLevel and
>> +  PcdFixedDebugPrintErrorLevel.
>> +
>> +  @retval  TRUE    Current ErrorLevel is supported.
>> +  @retval  FALSE   Current ErrorLevel is not supported.
>> +
>> +**/
>> +BOOLEAN
>> +EFIAPI
>> +DebugPrintLevelEnabled (
>> +  IN  CONST UINTN        ErrorLevel
>> +  )
>> +{
>> +  return (ErrorLevel & FixedPcdGet32
>> (PcdFixedDebugPrintErrorLevel)) != 0;
>> +}
>> diff --git
>> a/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntime
>> DebugLibSerialPort.inf
>> b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntime
>> DebugLibSerialPort.inf
>> new file mode 100644
>> index 000000000000..9f300f4f1b12
>> --- /dev/null
>> +++
>> b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntime
>> DebugLibSerialPort.inf
>> @@ -0,0 +1,46 @@
>> +## @file
>> +#
>> +#  Copyright (c) 2006 - 2015, Intel Corporation. All
>> rights reserved.<BR>
>> +#  Copyright (c) 2018, Linaro, Ltd. All rights
>> reserved.<BR>
>> +#
>> +#  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.
>> +#
>> +#
>> +##
>> +
>> +[Defines]
>> +  INF_VERSION                    = 0x00010005
>> +  BASE_NAME                      =
>> DxeRuntimeDebugLibSerialPort
>> +  MODULE_UNI_FILE                =
>> DxeRuntimeDebugLibSerialPort.uni
>> +  FILE_GUID                      = 9D914E2F-7CCB-41DB-
>> 8E74-9AFF8F3BBFBF
>> +  MODULE_TYPE                    = DXE_RUNTIME_DRIVER
>> +  VERSION_STRING                 = 1.0
>> +  LIBRARY_CLASS                  = DebugLib
>> +  CONSTRUCTOR                    =
>> DxeRuntimeDebugLibSerialPortConstructor
>> +
>> +[Sources]
>> +  DebugLib.c
>> +
>> +[Packages]
>> +  MdePkg/MdePkg.dec
>> +
>> +[LibraryClasses]
>> +  BaseLib
>> +  BaseMemoryLib
>> +  DebugPrintErrorLevelLib
>> +  PcdLib
>> +  PrintLib
>> +  SerialPortLib
>> +
>> +[Guids]
>> +  gEfiEventExitBootServicesGuid
>> +
>> +[FixedPcd]
>> +  gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue
>> ## SOMETIMES_CONSUMES
>> +  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask
>> ## CONSUMES
>> +  gEfiMdePkgTokenSpaceGuid.PcdFixedDebugPrintErrorLevel
>> ## CONSUMES
>> diff --git
>> a/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntime
>> DebugLibSerialPort.uni
>> b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntime
>> DebugLibSerialPort.uni
>> new file mode 100644
>> index 000000000000..cd65515c4177
>> --- /dev/null
>> +++
>> b/MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntime
>> DebugLibSerialPort.uni
>> @@ -0,0 +1,21 @@
>> +// /** @file
>> +// Instance of Debug Library based on Serial Port
>> Library.
>> +//
>> +// It uses Print Library to produce formatted output
>> strings to seiral port device.
>> +//
>> +// Copyright (c) 2006 - 2014, Intel Corporation. All
>> rights reserved.<BR>
>> +//
>> +// 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.
>> +//
>> +// **/
>> +
>> +
>> +#string STR_MODULE_ABSTRACT             #language en-US
>> "Instance of Debug Library based on Serial Port Library"
>> +
>> +#string STR_MODULE_DESCRIPTION          #language en-US
>> "It uses Print Library to produce formatted output
>> strings to a serial port device."
>> +
>> --
>> 2.11.0
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel

[PATCH 2/3] ArmVirtPkg: switch to DXE runtime version of DebugLib where appropriate

[Ard Biesheuvel]

Switch all users of ArmVirt.dsc.inc to the new DebugLib implementation
that was created especially for DXE_RUNTIME_DRIVER modules, ensuring
that DEBUG() calls do not touch the UART at runtime.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/ArmVirt.dsc.inc | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 0cb48f08e9bf..cde514958da2 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -231,6 +231,9 @@ [LibraryClasses.common.UEFI_DRIVER]
 [LibraryClasses.common.DXE_RUNTIME_DRIVER]
   MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
   CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
+!if $(TARGET) != RELEASE
+  DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
+!endif
 
 !if $(SECURE_BOOT_ENABLE) == TRUE
   BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
-- 
2.11.0

Re: [PATCH 2/3] ArmVirtPkg: switch to DXE runtime version of DebugLib where appropriate

[Laszlo Ersek]

On 02/20/18 12:05, Ard Biesheuvel wrote:
> Switch all users of ArmVirt.dsc.inc to the new DebugLib implementation
> that was created especially for DXE_RUNTIME_DRIVER modules, ensuring
> that DEBUG() calls do not touch the UART at runtime.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/ArmVirt.dsc.inc | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index 0cb48f08e9bf..cde514958da2 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -231,6 +231,9 @@ [LibraryClasses.common.UEFI_DRIVER]
>  [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>    MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
>    CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
> +!if $(TARGET) != RELEASE
> +  DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> +!endif
>  
>  !if $(SECURE_BOOT_ENABLE) == TRUE
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
> 

I think all occurrences of the DebugLib -->
"MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf"
resolution should be audited and possibly patched / extended in the edk2
tree. Otherwise those platforms will be broken by patch #3. But, that
can wait for the next iteration.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

[PATCH 3/3] MdePkg/BaseDebugLibSerialPort: blacklist for use by DXE runtime drivers

[Ard Biesheuvel]

BaseDebugLibSerialPort is not suitable for use by DXE_RUNTIME_DRIVER
modules, so blacklist it for use by such modules.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf b/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
index 823511b22f6b..25da1fb9363a 100644
--- a/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
+++ b/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
@@ -21,7 +21,7 @@ [Defines]
   FILE_GUID                      = BB83F95F-EDBC-4884-A520-CD42AF388FAE
   MODULE_TYPE                    = BASE
   VERSION_STRING                 = 1.0
-  LIBRARY_CLASS                  = DebugLib 
+  LIBRARY_CLASS                  = DebugLib|SEC PEI_CORE PEIM DXE_CORE DXE_DRIVER DXE_SMM_DRIVER DXE_SAL_DRIVER UEFI_DRIVER UEFI_APPLICATION SMM_CORE MM_STANDALONE MM_CORE_STANDALONE
   CONSTRUCTOR                    = BaseDebugLibSerialPortConstructor
 
 #
-- 
2.11.0

Re: [PATCH 3/3] MdePkg/BaseDebugLibSerialPort: blacklist for use by DXE runtime drivers

[Leif Lindholm]

On Tue, Feb 20, 2018 at 11:05:24AM +0000, Ard Biesheuvel wrote:
> BaseDebugLibSerialPort is not suitable for use by DXE_RUNTIME_DRIVER
> modules, so blacklist it for use by such modules.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf b/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> index 823511b22f6b..25da1fb9363a 100644
> --- a/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> +++ b/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
> @@ -21,7 +21,7 @@ [Defines]
>    FILE_GUID                      = BB83F95F-EDBC-4884-A520-CD42AF388FAE
>    MODULE_TYPE                    = BASE
>    VERSION_STRING                 = 1.0
> -  LIBRARY_CLASS                  = DebugLib 
> +  LIBRARY_CLASS                  = DebugLib|SEC PEI_CORE PEIM DXE_CORE DXE_DRIVER DXE_SMM_DRIVER DXE_SAL_DRIVER UEFI_DRIVER UEFI_APPLICATION SMM_CORE MM_STANDALONE MM_CORE_STANDALONE

Not a comment on this patch as such (which looks sensible to me), but
what you're doing here isn't blacklisting DXE_RUNTIME_DRIVER, but
rather whitelisting every other module type.

Could we use a ! operator added to the syntax?

/
    Leif

>    CONSTRUCTOR                    = BaseDebugLibSerialPortConstructor
>  
>  #
> -- 
> 2.11.0
> 

Re: [PATCH 3/3] MdePkg/BaseDebugLibSerialPort: blacklist for use by DXE runtime drivers

[Laszlo Ersek]

On 02/20/18 15:22, Leif Lindholm wrote:
> On Tue, Feb 20, 2018 at 11:05:24AM +0000, Ard Biesheuvel wrote:
>> BaseDebugLibSerialPort is not suitable for use by DXE_RUNTIME_DRIVER
>> modules, so blacklist it for use by such modules.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf b/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
>> index 823511b22f6b..25da1fb9363a 100644
>> --- a/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
>> +++ b/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
>> @@ -21,7 +21,7 @@ [Defines]
>>    FILE_GUID                      = BB83F95F-EDBC-4884-A520-CD42AF388FAE
>>    MODULE_TYPE                    = BASE
>>    VERSION_STRING                 = 1.0
>> -  LIBRARY_CLASS                  = DebugLib 
>> +  LIBRARY_CLASS                  = DebugLib|SEC PEI_CORE PEIM DXE_CORE DXE_DRIVER DXE_SMM_DRIVER DXE_SAL_DRIVER UEFI_DRIVER UEFI_APPLICATION SMM_CORE MM_STANDALONE MM_CORE_STANDALONE
> 
> Not a comment on this patch as such (which looks sensible to me), but
> what you're doing here isn't blacklisting DXE_RUNTIME_DRIVER, but
> rather whitelisting every other module type.
> 
> Could we use a ! operator added to the syntax?

No, I don't think so, based on the INF file spec.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

(

A future customization would be to give a similar treatment to SMM (or
"MM") drivers. While MM has its own set of (identity mapped) page
tables, and therefore I'd expect an MMIO serial port to "just work", we
still shouldn't mess with the serial port once the OS owns it,
regardless of the fact that we're in MM. So, for that, the lib instance
meant for MM drivers would have to catch EBS too.

Of course, that doesn't work the same way -- the SMM_CORE catches the
normal EBS notification, and "forwards" it into the MM protocol database
(see SmmExitBootServicesHandler() in
"MdeModulePkg/Core/PiSmmCore/PiSmmCore.c"). So the MM lib instance would
have to register an MM protocol notify for
"gEdkiiSmmExitBootServicesProtocolGuid".

But, that's for the future at best.

*This* lib instance should remain correct for the SMM_CORE itself, however.

)

Thanks,
Laszlo



> /
>     Leif
> 
>>    CONSTRUCTOR                    = BaseDebugLibSerialPortConstructor
>>  
>>  #
>> -- 
>> 2.11.0
>>

Re: [PATCH 3/3] MdePkg/BaseDebugLibSerialPort: blacklist for use by DXE runtime drivers

[Kinney, Michael D]

I do not agree with this change.

If the PCDs that describe the UART are for a UART
that is owned by the FW and hidden from the OS, then
this lib can work well at runtime.

Mike

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, February 20, 2018 9:00 AM
> To: Leif Lindholm <leif.lindholm@linaro.org>; Ard
> Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: edk2-devel@lists.01.org; Ni, Ruiyu
> <ruiyu.ni@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Zeng, Star
> <star.zeng@intel.com>; afish@apple.com
> Subject: Re: [PATCH 3/3] MdePkg/BaseDebugLibSerialPort:
> blacklist for use by DXE runtime drivers
> 
> On 02/20/18 15:22, Leif Lindholm wrote:
> > On Tue, Feb 20, 2018 at 11:05:24AM +0000, Ard
> Biesheuvel wrote:
> >> BaseDebugLibSerialPort is not suitable for use by
> DXE_RUNTIME_DRIVER
> >> modules, so blacklist it for use by such modules.
> >>
> >> Contributed-under: TianoCore Contribution Agreement
> 1.1
> >> Signed-off-by: Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> >> ---
> >>
> MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerial
> Port.inf | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git
> a/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSeri
> alPort.inf
> b/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSeri
> alPort.inf
> >> index 823511b22f6b..25da1fb9363a 100644
> >> ---
> a/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSeri
> alPort.inf
> >> +++
> b/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSeri
> alPort.inf
> >> @@ -21,7 +21,7 @@ [Defines]
> >>    FILE_GUID                      = BB83F95F-EDBC-
> 4884-A520-CD42AF388FAE
> >>    MODULE_TYPE                    = BASE
> >>    VERSION_STRING                 = 1.0
> >> -  LIBRARY_CLASS                  = DebugLib
> >> +  LIBRARY_CLASS                  = DebugLib|SEC
> PEI_CORE PEIM DXE_CORE DXE_DRIVER DXE_SMM_DRIVER
> DXE_SAL_DRIVER UEFI_DRIVER UEFI_APPLICATION SMM_CORE
> MM_STANDALONE MM_CORE_STANDALONE
> >
> > Not a comment on this patch as such (which looks
> sensible to me), but
> > what you're doing here isn't blacklisting
> DXE_RUNTIME_DRIVER, but
> > rather whitelisting every other module type.
> >
> > Could we use a ! operator added to the syntax?
> 
> No, I don't think so, based on the INF file spec.
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> (
> 
> A future customization would be to give a similar
> treatment to SMM (or
> "MM") drivers. While MM has its own set of (identity
> mapped) page
> tables, and therefore I'd expect an MMIO serial port to
> "just work", we
> still shouldn't mess with the serial port once the OS
> owns it,
> regardless of the fact that we're in MM. So, for that,
> the lib instance
> meant for MM drivers would have to catch EBS too.
> 
> Of course, that doesn't work the same way -- the
> SMM_CORE catches the
> normal EBS notification, and "forwards" it into the MM
> protocol database
> (see SmmExitBootServicesHandler() in
> "MdeModulePkg/Core/PiSmmCore/PiSmmCore.c"). So the MM
> lib instance would
> have to register an MM protocol notify for
> "gEdkiiSmmExitBootServicesProtocolGuid".
> 
> But, that's for the future at best.
> 
> *This* lib instance should remain correct for the
> SMM_CORE itself, however.
> 
> )
> 
> Thanks,
> Laszlo
> 
> 
> 
> > /
> >     Leif
> >
> >>    CONSTRUCTOR                    =
> BaseDebugLibSerialPortConstructor
> >>
> >>  #
> >> --
> >> 2.11.0
> >>

Re: [PATCH 3/3] MdePkg/BaseDebugLibSerialPort: blacklist for use by DXE runtime drivers

[Laszlo Ersek]

On 02/20/18 19:02, Kinney, Michael D wrote:
> I do not agree with this change.
> 
> If the PCDs that describe the UART are for a UART
> that is owned by the FW and hidden from the OS, then
> this lib can work well at runtime.

Does that imply that we should do the runtime disablement in the
SerialPortLib instance that underlies BaseDebugLibSerialPort?

I think it is an integral part of the feature (or, well, fix) that the
runtime incompatibility be caught at edk2 platform build time. In other
words, *some* library instance in edk2 should blacklist
DXE_RUNTIME_DRIVER modules (and the counterpart library instance should
be appropriate for DXE_RUNTIME_DRIVER modules only, and handle EBS, to
dynamically disable use of the serial port).

Are you suggesting that we implement this at the PL011 SerialPortLib
instance level?

Thanks!
Laszlo


>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Tuesday, February 20, 2018 9:00 AM
>> To: Leif Lindholm <leif.lindholm@linaro.org>; Ard
>> Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: edk2-devel@lists.01.org; Ni, Ruiyu
>> <ruiyu.ni@intel.com>; Kinney, Michael D
>> <michael.d.kinney@intel.com>; Gao, Liming
>> <liming.gao@intel.com>; Zeng, Star
>> <star.zeng@intel.com>; afish@apple.com
>> Subject: Re: [PATCH 3/3] MdePkg/BaseDebugLibSerialPort:
>> blacklist for use by DXE runtime drivers
>>
>> On 02/20/18 15:22, Leif Lindholm wrote:
>>> On Tue, Feb 20, 2018 at 11:05:24AM +0000, Ard
>> Biesheuvel wrote:
>>>> BaseDebugLibSerialPort is not suitable for use by
>> DXE_RUNTIME_DRIVER
>>>> modules, so blacklist it for use by such modules.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement
>> 1.1
>>>> Signed-off-by: Ard Biesheuvel
>> <ard.biesheuvel@linaro.org>
>>>> ---
>>>>
>> MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerial
>> Port.inf | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git
>> a/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSeri
>> alPort.inf
>> b/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSeri
>> alPort.inf
>>>> index 823511b22f6b..25da1fb9363a 100644
>>>> ---
>> a/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSeri
>> alPort.inf
>>>> +++
>> b/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSeri
>> alPort.inf
>>>> @@ -21,7 +21,7 @@ [Defines]
>>>>    FILE_GUID                      = BB83F95F-EDBC-
>> 4884-A520-CD42AF388FAE
>>>>    MODULE_TYPE                    = BASE
>>>>    VERSION_STRING                 = 1.0
>>>> -  LIBRARY_CLASS                  = DebugLib
>>>> +  LIBRARY_CLASS                  = DebugLib|SEC
>> PEI_CORE PEIM DXE_CORE DXE_DRIVER DXE_SMM_DRIVER
>> DXE_SAL_DRIVER UEFI_DRIVER UEFI_APPLICATION SMM_CORE
>> MM_STANDALONE MM_CORE_STANDALONE
>>>
>>> Not a comment on this patch as such (which looks
>> sensible to me), but
>>> what you're doing here isn't blacklisting
>> DXE_RUNTIME_DRIVER, but
>>> rather whitelisting every other module type.
>>>
>>> Could we use a ! operator added to the syntax?
>>
>> No, I don't think so, based on the INF file spec.
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> (
>>
>> A future customization would be to give a similar
>> treatment to SMM (or
>> "MM") drivers. While MM has its own set of (identity
>> mapped) page
>> tables, and therefore I'd expect an MMIO serial port to
>> "just work", we
>> still shouldn't mess with the serial port once the OS
>> owns it,
>> regardless of the fact that we're in MM. So, for that,
>> the lib instance
>> meant for MM drivers would have to catch EBS too.
>>
>> Of course, that doesn't work the same way -- the
>> SMM_CORE catches the
>> normal EBS notification, and "forwards" it into the MM
>> protocol database
>> (see SmmExitBootServicesHandler() in
>> "MdeModulePkg/Core/PiSmmCore/PiSmmCore.c"). So the MM
>> lib instance would
>> have to register an MM protocol notify for
>> "gEdkiiSmmExitBootServicesProtocolGuid".
>>
>> But, that's for the future at best.
>>
>> *This* lib instance should remain correct for the
>> SMM_CORE itself, however.
>>
>> )
>>
>> Thanks,
>> Laszlo
>>
>>
>>
>>> /
>>>     Leif
>>>
>>>>    CONSTRUCTOR                    =
>> BaseDebugLibSerialPortConstructor
>>>>
>>>>  #
>>>> --
>>>> 2.11.0
>>>>
>