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

["Ni, Ray" <ray.ni@intel.com>]

Zhichao,
For a PEIM, it may not be proper to have a global variable and update that global variable during execution.
Because sometimes the PEIM runs from read-only SPI flash, the global variable cannot be updated.
I see you have a check against NULL when using that global variable. How about just remove the global variable and always locate the PPI?

> -----Original Message-----
> From: Gao, Zhichao <zhichao.gao@intel.com>
> Sent: Thursday, March 28, 2019 4:10 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 V5 16/17] MdeModulePkg/PeiDebugLibDebugPpi: Add PEI
> debug lib
> 
> From: Liming Gao <liming.gao@intel.com>
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1549
> 
> 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>
> Reviewed-by: Hao Wu <hao.a.wu@intel.com>
> ---
>  .../Library/PeiDebugLibDebugPpi/DebugLib.c    | 456 ++++++++++++++++++
>  .../PeiDebugLibDebugPpi.inf                   |  56 +++
>  2 files changed, 512 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..349577e5ea
> --- /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             *mEdkiiDebugPpi = 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 (mEdkiiDebugPpi == NULL) {
> +    Status = PeiServicesLocatePpi (
> +                &gEdkiiDebugPpiGuid,
> +                0,
> +                NULL,
> +                (VOID **)&mEdkiiDebugPpi
> +                );
> +    if (EFI_ERROR (Status)) {
> +      return;
> +    }
> +  }
> +
> +  mEdkiiDebugPpi->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 (mEdkiiDebugPpi == NULL) {
> +    Status = PeiServicesLocatePpi (
> +                &gEdkiiDebugPpiGuid,
> +                0,
> +                NULL,
> +                (VOID **)&mEdkiiDebugPpi
> +                );
> +    if (EFI_ERROR (Status)) {
> +      return;
> +    }
> +  }
> +
> +  mEdkiiDebugPpi->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..61c7a4fea7
> --- /dev/null
> +++
> b/MdeModulePkg/Library/PeiDebugLibDebugPpi/PeiDebugLibDebugPpi.inf
> @@ -0,0 +1,56 @@
> +## @file
> +#  Debug Lib instance through DebugServicePei for PEI phase # #  This
> +module use gEdkiiDebugPpiGuid to implement the PEI DebugLib for reduce
> +#  the size of PEIMs which consume DebugLib # #  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.21.0.windows.1