Re: [PATCH V4 16/17] MdeModulePkg/PeiDebugLibDebugPpi: Add PEI debug lib

["Gao, Liming" <liming.gao@intel.com>]

Zhichao:
   For new library instance, I have some comments. 

1) DebugPrint(). If DebugPpi is not found, directly return instead of CpuDeadLoop().
2) DebugAssert(). If DebugPpi is not found, bases on PcdDebugPropertyMask value to trig CpuDeadLoop() or CpuBreakPoint(). 
3) Please rename mDebugPpi as the specific prefix name to avoid the name confliction. 
4) What test is done for this library instance? 

Thanks
Liming
>-----Original Message-----
>From: Gao, Zhichao
>Sent: Thursday, March 21, 2019 10:05 PM
>To: edk2-devel@lists.01.org
>Cc: Gao, Liming <liming.gao@intel.com>; Wang, Jian J
><jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
><ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Sean Brogan
><sean.brogan@microsoft.com>; Michael Turner
><Michael.Turner@microsoft.com>; Bret Barkelew
><Bret.Barkelew@microsoft.com>
>Subject: [PATCH V4 16/17] MdeModulePkg/PeiDebugLibDebugPpi: Add PEI
>debug lib
>
>From: Liming Gao <liming.gao@intel.com>
>
>REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1395
>
>Add a PEI debug library instance PeiDebugLibDebugPpi base on
>DebugPpi. Using the combination of the DebugServicePei and
>this lib instance can reduce the image size of PEI drivers.
>
>Contributed-under: TianoCore Contribution Agreement 1.1
>Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
>Cc: Jian J Wang <jian.j.wang@intel.com>
>Cc: Hao Wu <hao.a.wu@intel.com>
>Cc: Ray Ni <ray.ni@intel.com>
>Cc: Star Zeng <star.zeng@intel.com>
>Cc: Liming Gao <liming.gao@intel.com>
>Cc: Sean Brogan <sean.brogan@microsoft.com>
>Cc: Michael Turner <Michael.Turner@microsoft.com>
>Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
>---
> .../Library/PeiDebugLibDebugPpi/DebugLib.c         | 456
>+++++++++++++++++++++
> .../PeiDebugLibDebugPpi/PeiDebugLibDebugPpi.inf    |  55 +++
> 2 files changed, 511 insertions(+)
> create mode 100644
>MdeModulePkg/Library/PeiDebugLibDebugPpi/DebugLib.c
> create mode 100644
>MdeModulePkg/Library/PeiDebugLibDebugPpi/PeiDebugLibDebugPpi.inf
>
>diff --git a/MdeModulePkg/Library/PeiDebugLibDebugPpi/DebugLib.c
>b/MdeModulePkg/Library/PeiDebugLibDebugPpi/DebugLib.c
>new file mode 100644
>index 0000000000..75859163ae
>--- /dev/null
>+++ b/MdeModulePkg/Library/PeiDebugLibDebugPpi/DebugLib.c
>@@ -0,0 +1,456 @@
>+/** @file
>+  PEI debug lib instance base on DebugPpi to save size
>+
>+  Copyright (c) 2019, 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.
>+
>+**/
>+
>+#include <PiPei.h>
>+#include <Ppi/Debug.h>
>+#include <Library/DebugLib.h>
>+#include <Library/BaseMemoryLib.h>
>+#include <Library/PcdLib.h>
>+#include <Library/PeiServicesLib.h>
>+#include <Library/DebugPrintErrorLevelLib.h>
>+
>+EDKII_DEBUG_PPI             *mDebugPpi = NULL;
>+
>+/**
>+  Prints a debug message to the debug output device if the specified
>+  error level is enabled.
>+
>+  If any bit in ErrorLevel is also set in DebugPrintErrorLevelLib function
>+  GetDebugPrintErrorLevel (), then print the message specified by Format
>and
>+  the associated variable argument list to the debug output device.
>+
>+  If Format is NULL, then ASSERT().
>+
>+  @param  ErrorLevel    The error level of the debug message.
>+  @param  Format        Format string for the debug message to print.
>+  @param  ...           Variable argument list whose contents are accessed
>+                        based on the format string specified by Format.
>+
>+**/
>+VOID
>+EFIAPI
>+DebugPrint (
>+  IN  UINTN        ErrorLevel,
>+  IN  CONST CHAR8  *Format,
>+  ...
>+  )
>+{
>+  VA_LIST         Marker;
>+
>+  VA_START (Marker, Format);
>+  DebugVPrint (ErrorLevel, Format, Marker);
>+  VA_END (Marker);
>+}
>+
>+
>+/**
>+  Prints a debug message to the debug output device if the specified
>+  error level is enabled.
>+  This function use BASE_LIST which would provide a more compatible
>+  service than VA_LIST.
>+
>+  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  BaseListMarker  BASE_LIST marker for the variable argument list.
>+
>+**/
>+VOID
>+EFIAPI
>+DebugBPrint (
>+  IN  UINTN         ErrorLevel,
>+  IN  CONST CHAR8   *Format,
>+  IN  BASE_LIST     BaseListMarker
>+  )
>+{
>+  EFI_STATUS      Status;
>+
>+  //
>+  // If Format is NULL, then ASSERT().
>+  //
>+  ASSERT (Format != NULL);
>+
>+  //
>+  // Check driver Debug Level value and global debug level
>+  //
>+  if ((ErrorLevel & GetDebugPrintErrorLevel ()) == 0) {
>+    return;
>+  }
>+
>+  if (mDebugPpi == NULL) {
>+    Status = PeiServicesLocatePpi (
>+                &gEdkiiDebugPpiGuid,
>+                0,
>+                NULL,
>+                (VOID **)&mDebugPpi
>+                );
>+    if (EFI_ERROR (Status)) {
>+      CpuDeadLoop();
>+    }
>+  }
>+
>+  mDebugPpi->DebugBPrint (
>+              ErrorLevel,
>+              Format,
>+              BaseListMarker
>+              );
>+}
>+
>+
>+/**
>+  Worker function that convert a VA_LIST to a BASE_LIST based on a
>+  Null-terminated format string.
>+
>+  @param  Format          Null-terminated format string.
>+  @param  VaListMarker    VA_LIST style variable argument list consumed
>+                          by processing Format.
>+  @param  BaseListMarker  BASE_LIST style variable argument list consumed
>+                          by processing Format.
>+  @param  Size            The size, in bytes, of the BaseListMarker buffer.
>+
>+  @return TRUE   The VA_LIST has been converted to BASE_LIST.
>+  @return FALSE  The VA_LIST has not been converted to BASE_LIST.
>+
>+**/
>+BOOLEAN
>+VaListToBaseList (
>+  IN  CONST CHAR8  *Format,
>+  IN  VA_LIST      VaListMarker,
>+  OUT BASE_LIST    BaseListMarker,
>+  IN  UINTN        Size
>+  )
>+{
>+  BASE_LIST       BaseListStart;
>+  BOOLEAN         Long;
>+
>+  ASSERT (Format != NULL);
>+
>+  ASSERT (BaseListMarker != NULL);
>+
>+  BaseListStart = BaseListMarker;
>+
>+  for (; *Format != '\0'; Format++) {
>+    //
>+    // Only format with prefix % is processed.
>+    //
>+    if (*Format != '%') {
>+      continue;
>+    }
>+
>+    Long = FALSE;
>+
>+    //
>+    // Parse Flags and Width
>+    //
>+    for (Format++; TRUE; Format++) {
>+      if (*Format == '.' || *Format == '-' || *Format == '+' || *Format == ' ') {
>+        //
>+        // These characters in format field are omitted.
>+        //
>+        continue;
>+      }
>+      if (*Format >= '0' && *Format <= '9') {
>+        //
>+        // These characters in format field are omitted.
>+        //
>+        continue;
>+      }
>+      if (*Format == 'L' || *Format == 'l') {
>+        //
>+        // 'L" or "l" in format field means the number being printed is a UINT64
>+        //
>+        Long = TRUE;
>+        continue;
>+      }
>+      if (*Format == '*') {
>+        //
>+        // '*' in format field means the precision of the field is specified by
>+        // a UINTN argument in the argument list.
>+        //
>+        BASE_ARG (BaseListMarker, UINTN) = VA_ARG (VaListMarker, UINTN);
>+        continue;
>+      }
>+      if (*Format == '\0') {
>+        //
>+        // Make no output if Format string terminates unexpectedly when
>+        // looking up for flag, width, precision and type.
>+        //
>+        Format--;
>+      }
>+      //
>+      // When valid argument type detected or format string terminates
>unexpectedly,
>+      // the inner loop is done.
>+      //
>+      break;
>+    }
>+
>+    //
>+    // Pack variable arguments into the storage area following
>EFI_DEBUG_INFO.
>+    //
>+    if ((*Format == 'p') && (sizeof (VOID *) > 4)) {
>+      Long = TRUE;
>+    }
>+    if (*Format == 'p' || *Format == 'X' || *Format == 'x' || *Format == 'd' ||
>*Format == 'u') {
>+      if (Long) {
>+        BASE_ARG (BaseListMarker, INT64) = VA_ARG (VaListMarker, INT64);
>+      } else {
>+        BASE_ARG (BaseListMarker, int) = VA_ARG (VaListMarker, int);
>+      }
>+    } else if (*Format == 's' || *Format == 'S' || *Format == 'a' || *Format ==
>'g' || *Format == 't') {
>+      BASE_ARG (BaseListMarker, VOID *) = VA_ARG (VaListMarker, VOID *);
>+    } else if (*Format == 'c') {
>+      BASE_ARG (BaseListMarker, UINTN) = VA_ARG (VaListMarker, UINTN);
>+    } else if (*Format == 'r') {
>+      BASE_ARG (BaseListMarker, RETURN_STATUS) = VA_ARG (VaListMarker,
>RETURN_STATUS);
>+    }
>+
>+    //
>+    // If the converted BASE_LIST is larger than the size of BaseListMarker,
>then return FALSE
>+    //
>+    if (((UINTN)BaseListMarker - (UINTN)BaseListStart) > Size) {
>+      return FALSE;
>+    }
>+  }
>+
>+  return TRUE;
>+}
>+
>+
>+/**
>+  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  VaListMarker  VA_LIST marker for the variable argument list.
>+
>+**/
>+VOID
>+EFIAPI
>+DebugVPrint (
>+  IN  UINTN         ErrorLevel,
>+  IN  CONST CHAR8   *Format,
>+  IN  VA_LIST       VaListMarker
>+  )
>+{
>+  UINT64            BaseListMarker[256 / sizeof (UINT64)];
>+  BOOLEAN           Converted;
>+
>+  //
>+  // Convert the VaList to BaseList
>+  //
>+  Converted = VaListToBaseList (
>+                Format,
>+                VaListMarker,
>+                (BASE_LIST)BaseListMarker,
>+                sizeof (BaseListMarker) - 8
>+                );
>+
>+  if (!Converted) {
>+    return;
>+  }
>+
>+  DebugBPrint (ErrorLevel, Format, (BASE_LIST)BaseListMarker);
>+}
>+
>+
>+/**
>+  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
>+  )
>+{
>+  EFI_STATUS      Status;
>+
>+  if (mDebugPpi == NULL) {
>+    Status = PeiServicesLocatePpi (
>+                &gEdkiiDebugPpiGuid,
>+                0,
>+                NULL,
>+                (VOID **)&mDebugPpi
>+                );
>+    if (EFI_ERROR (Status)) {
>+      CpuDeadLoop();
>+    }
>+  }
>+
>+  mDebugPpi->DebugAssert (
>+              FileName,
>+              LineNumber,
>+              Description
>+              );
>+}
>+
>+
>+/**
>+  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 zeros
>PcdDebugClearMemoryValue.
>+
>+  @return  Buffer  The pointer to the target buffer filled with
>PcdDebugClearMemoryValue.
>+
>+**/
>+VOID *
>+EFIAPI
>+DebugClearMemory (
>+  OUT VOID  *Buffer,
>+  IN UINTN  Length
>+  )
>+{
>+  ASSERT (Buffer != NULL);
>+
>+  return SetMem (Buffer, Length, PcdGet8 (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 (BOOLEAN) ((PcdGet8 (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 (BOOLEAN) ((PcdGet8 (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 (BOOLEAN) ((PcdGet8 (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 (BOOLEAN) ((PcdGet8 (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 (BOOLEAN) ((ErrorLevel &
>PcdGet32(PcdFixedDebugPrintErrorLevel)) != 0);
>+}
>+
>diff --git
>a/MdeModulePkg/Library/PeiDebugLibDebugPpi/PeiDebugLibDebugPpi.inf
>b/MdeModulePkg/Library/PeiDebugLibDebugPpi/PeiDebugLibDebugPpi.inf
>new file mode 100644
>index 0000000000..4ab21e577e
>--- /dev/null
>+++
>b/MdeModulePkg/Library/PeiDebugLibDebugPpi/PeiDebugLibDebugPpi.inf
>@@ -0,0 +1,55 @@
>+## @file
>+#  Debug Lib instance through DebugServicePei for PEI phase
>+#
>+#  This module installs gEdkiiFaultTolerantWriteGuid PPI to inform the check
>for FTW last write data has been done.
>+#
>+#  Copyright (c) 2019, 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.
>+#
>+##
>+
>+[Defines]
>+  INF_VERSION                    = 0x00010005
>+  BASE_NAME                      = PeiDebugLibDebugPpi
>+  FILE_GUID                      = 2E08836C-4D1C-42F7-BBBE-EC5D25F1FDD4
>+  MODULE_TYPE                    = PEIM
>+  VERSION_STRING                 = 1.0
>+  LIBRARY_CLASS                  = DebugLib|PEIM
>+
>+#
>+# The following information is for reference only and not required by the
>build tools.
>+#
>+#  VALID_ARCHITECTURES           = IA32 X64 EBC
>+#
>+
>+[Sources]
>+  DebugLib.c
>+
>+[Packages]
>+  MdePkg/MdePkg.dec
>+  MdeModulePkg/MdeModulePkg.dec
>+
>+[LibraryClasses]
>+  PcdLib
>+  BaseMemoryLib
>+  DebugPrintErrorLevelLib
>+  PeiServicesLib
>+  PeiServicesTablePointerLib
>+
>+[Ppis]
>+  gEdkiiDebugPpiGuid                                        ## CONSUMES
>+
>+[Pcd]
>+  gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue         ##
>SOMETIMES_CONSUMES
>+  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask             ## CONSUMES
>+  gEfiMdePkgTokenSpaceGuid.PcdFixedDebugPrintErrorLevel     ##
>CONSUMES
>+
>+[Depex]
>+  gEdkiiDebugPpiGuid
>+
>--
>2.16.2.windows.1