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

["Wu, Hao A" <hao.a.wu@intel.com>]

Hello,

Besides the comments in patch 14/17 to remove the 'PeiServices' & 'This'
parameters from the PPI services, some minor comments below:

> -----Original Message-----
> From: Gao, Zhichao
> Sent: Tuesday, March 19, 2019 11:26 PM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming; Wang, Jian J; Wu, Hao A; Ni, Ray; Zeng, Star; Sean Brogan;
> Michael Turner; Bret Barkelew
> Subject: [PATCH V3 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         | 469
> +++++++++++++++++++++
>  .../PeiDebugLibDebugPpi/PeiDebugLibDebugPpi.inf    |  55 +++
>  2 files changed, 524 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..839dff5268
> --- /dev/null
> +++ b/MdeModulePkg/Library/PeiDebugLibDebugPpi/DebugLib.c
> @@ -0,0 +1,469 @@
> +/** @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>
> +#include <Library/PeiServicesTablePointerLib.h>
> +
> +EDKII_DEBUG_PPI             *mDebugPpi = NULL;
> +CONST EFI_PEI_SERVICES      **mPeiServicesTablePointer = NULL;

If the EDKII_DEBUG_PPI interface changes, the above variable can be
removed.


> +
> +/**
> +  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
> +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
> +                );

The indention seems not consistent for the above several lines.


> +    if (EFI_ERROR (Status)) {
> +      CpuDeadLoop();
> +    }
> +  }
> +
> +  if (mPeiServicesTablePointer == NULL) {
> +    mPeiServicesTablePointer = GetPeiServicesTablePointer ();
> +  }

If the EDKII_DEBUG_PPI interface changes, the above lines can be removed.


> +
> +  mDebugPpi->DebugBPrint (
> +              mPeiServicesTablePointer,
> +              mDebugPpi,
> +              ErrorLevel,
> +              Format,
> +              BaseListMarker
> +              );

The indention seems not consistent for the above several lines.


> +}
> +
> +
> +/**
> +  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
> +                );

The indention seems not consistent for the above several lines.


> +    if (EFI_ERROR (Status)) {
> +      CpuDeadLoop();
> +    }
> +  }
> +
> +  if (mPeiServicesTablePointer == NULL) {
> +    mPeiServicesTablePointer = GetPeiServicesTablePointer ();
> +  }

If the EDKII_DEBUG_PPI interface changes, the above lines can be removed.


> +
> +  mDebugPpi->DebugAssert (
> +              mPeiServicesTablePointer,
> +              mDebugPpi,
> +              FileName,
> +              LineNumber,
> +              Description
> +              );

The indention seems not consistent for the above several lines.


> +}
> +
> +
> +/**
> +  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.

Please help to correct the above description for the INF file.

Best Regards,
Hao Wu

> +#
> +#  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