Re: [PATCH V3 14/17] MdeModulePkg: Add definitions for EDKII DEBUG PPI

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

> -----Original Message-----
> From: Gao, Zhichao
> Sent: Tuesday, March 19, 2019 11:26 PM
> To: edk2-devel@lists.01.org
> Cc: Wang, Jian J; Wu, Hao A; Ni, Ray; Zeng, Star; Gao, Liming; Sean Brogan;
> Michael Turner; Bret Barkelew
> Subject: [PATCH V3 14/17] MdeModulePkg: Add definitions for EDKII DEBUG
> PPI
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1395
> 
> Add a debug PPI for PEI phase. This PPI will provide basic
> services of debug. PEI debug lib instance can use these
> services to implement debug function to reduce the PEIMs
> which consume the debug lib.
> 
> 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>
> ---
>  MdeModulePkg/Include/Ppi/Debug.h | 90
> ++++++++++++++++++++++++++++++++++++++++
>  MdeModulePkg/MdeModulePkg.dec    |  3 ++
>  2 files changed, 93 insertions(+)
>  create mode 100644 MdeModulePkg/Include/Ppi/Debug.h
> 
> diff --git a/MdeModulePkg/Include/Ppi/Debug.h
> b/MdeModulePkg/Include/Ppi/Debug.h
> new file mode 100644
> index 0000000000..40db304f3d
> --- /dev/null
> +++ b/MdeModulePkg/Include/Ppi/Debug.h
> @@ -0,0 +1,90 @@
> +/** @file
> +  Define the EDKII_DEBUG_PPI that PEIMs can use to dump info to debug
> port.
> +
> +  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.
> +
> +**/
> +
> +#ifndef __EDKII_DEBUG_PPI_H__
> +#define __EDKII_DEBUG_PPI_H__
> +
> +#include <Pi/PiPeiCis.h>
> +
> +//
> +// Global ID for the EDKII_DEBUG_PPI
> +//
> +#define EDKII_DEBUG_PPI_GUID \
> +  { \
> +    0x999e699c, 0xb013, 0x475e, {0xb1, 0x7b, 0xf3, 0xa8, 0xae, 0x5c, 0x48,
> 0x75} \
> +  }
> +
> +///
> +/// Forward declaration for the PEI_DEBUG_LIB_DEBUG_PPI
> EDKII_DEBUG_PPI
> +///
> +typedef struct _EDKII_DEBUG_PPI EDKII_DEBUG_PPI;
> +
> +/**
> +  Print a debug message to debug output device if the specified error level
> +  is enabled.
> +
> +  @param[in] PeiServices              The pointer to the PEI Services Table.
> +  @param[in] This                     The pointer to this instance of
> EDKII_DEBUG_PPI
> +  @param[in] ErrorLevel               The error level of the debug message.
> +  @param[in] Format                   Format string for the debug message to print.
> +  @param[in] Marker                   BASE_LIST marker for the variable argument
> list.
> +
> +**/
> +typedef
> +VOID
> +(EFIAPI *EDKII_DEBUG_BPRINT)(
> +  IN CONST EFI_PEI_SERVICES         **PeiServices,
> +  IN EDKII_DEBUG_PPI                *This,
> +  IN UINTN                          ErrorLevel,
> +  IN CONST CHAR8                    *Format,
> +  IN BASE_LIST                      Marker
> +  );

Hello,

I checked the PEIM that implements this PPI (patch 15/17), and I did not
find the parameters 'PeiServices' & 'This' are being used there.

Thus, I suggest to remove those 2 parameters from the interfaces of
EFIAPI *EDKII_DEBUG_BPRINT
EFIAPI *EDKII_DEBUG_ASSERT

Best Regards,
Hao Wu

> +
> +/**
> +  Print an assert message containing a filename, line number, and
> description.
> +  This may be followed by a breakpoint or a dead loop.
> +
> +  @param[in] PeiServices              The pointer to the PEI Services Table.
> +  @param[in] This                     The pointer to this instance of
> EDKII_DEBUG_PPI
> +  @param[in] FileName                 The pointer to the name of the source file
> that
> +                                      generated the assert condition.
> +  @param[in] LineNumber               The line number in the source file that
> generated
> +                                      the assert condition
> +  @param[in] Description              The pointer to the description of the assert
> condition.
> +
> +**/
> +typedef
> +VOID
> +(EFIAPI *EDKII_DEBUG_ASSERT)(
> +  IN CONST EFI_PEI_SERVICES         **PeiServices,
> +  IN EDKII_DEBUG_PPI                *This,
> +  IN CONST CHAR8                    *FileName,
> +  IN UINTN                          LineNumber,
> +  IN CONST CHAR8                    *Description
> +  );
> +
> +///
> +/// This PPI contains a set of services to print message to debug output
> device
> +///
> +struct _EDKII_DEBUG_PPI {
> +  EDKII_DEBUG_BPRINT                DebugBPrint;
> +  EDKII_DEBUG_ASSERT                DebugAssert;
> +};
> +
> +extern EFI_GUID gEdkiiDebugPpiGuid;
> +
> +#endif
> +
> diff --git a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec
> index a2130bc439..9bbd0572f5 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -492,6 +492,9 @@
>    ## Include/Ppi/AtaPassThru.h
>    gEdkiiPeiAtaPassThruPpiGuid               = { 0xa16473fd, 0xd474, 0x4c89, { 0xae,
> 0xc7, 0x90, 0xb8, 0x3c, 0x73, 0x86, 0x9  } }
> 
> +  ## Include/Ppi/Debug.h
> +  gEdkiiDebugPpiGuid                        = { 0x999e699c, 0xb013, 0x475e, { 0xb1,
> 0x7b, 0xf3, 0xa8, 0xae, 0x5c, 0x48, 0x75 } }
> +
>  [Protocols]
>    ## Load File protocol provides capability to load and unload EFI image into
> memory and execute it.
>    #  Include/Protocol/LoadPe32Image.h
> --
> 2.16.2.windows.1