Re: [PATCH v4 3/3] ShellPkg UefiDpLib: Remove TimerLib dependency

["Yao, Jiewen" <jiewen.yao@intel.com>]

I have one more suggestion.
Can we use MdeModulePkg/Library/DxeSmmPerformanceLib/DxeSmmPerformanceLib.inf as default one in DSC?

So that the binary built with default DSC can be used on any platform.

ShellPkg.dsc:
   ShellPkg/Library/UefiDpLib/UefiDpLib.inf {
     <LibraryClasses>
-      TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate.inf
       PerformanceLib|MdePkg/Library/BasePerformanceLibNull/BasePerformanceLibNull.inf
       DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
   }


Thank you
Yao Jiewen


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Michael Kinney
> Sent: Friday, February 3, 2017 12:56 PM
> To: edk2-devel@lists.01.org
> Cc: Andrew Fish <afish@apple.com>; Gao, Liming <liming.gao@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Carsey, Jaben <jaben.carsey@intel.com>; Zeng,
> Star <star.zeng@intel.com>
> Subject: [edk2] [PATCH v4 3/3] ShellPkg UefiDpLib: Remove TimerLib dependency
> 
> From: Star Zeng <star.zeng@intel.com>
> 
> Current UefiDpLib implementation depends on TimerLib,
> as different platforms may implement and use their
> own TimerLib, it makes the dp command needs to be built
> by platform. The TimerLib dependency can be removed by
> using performance property configuration table to make
> UefiDpLib to be generic.
> 
> Cc: Andrew Fish <afish@apple.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Cinnamon Shia <cinnamon.shia@hpe.com>
> Cc: Jaben Carsey <jaben.carsey@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  ShellPkg/Library/UefiDpLib/Dp.c          | 29 +++++++++++------------------
>  ShellPkg/Library/UefiDpLib/DpInternal.h  |  6 ++----
>  ShellPkg/Library/UefiDpLib/DpProfile.c   |  3 +--
>  ShellPkg/Library/UefiDpLib/DpTrace.c     | 25 ++-----------------------
>  ShellPkg/Library/UefiDpLib/DpUtilities.c |  3 +--
>  ShellPkg/Library/UefiDpLib/Literals.c    |  3 +--
>  ShellPkg/Library/UefiDpLib/UefiDpLib.inf |  6 ++++--
>  ShellPkg/Library/UefiDpLib/UefiDpLib.uni |  3 ++-
>  ShellPkg/ShellPkg.dsc                    |  3 +--
>  9 files changed, 25 insertions(+), 56 deletions(-)
> 
> diff --git a/ShellPkg/Library/UefiDpLib/Dp.c b/ShellPkg/Library/UefiDpLib/Dp.c
> index 75c7d11..444c136 100644
> --- a/ShellPkg/Library/UefiDpLib/Dp.c
> +++ b/ShellPkg/Library/UefiDpLib/Dp.c
> @@ -13,7 +13,7 @@
>    Dp uses this information to group records in different ways.  It also uses
>    timer information to calculate elapsed time for each measurement.
> 
> -  Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.
> +  Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.
>    (C) Copyright 2015-2016 Hewlett Packard Enterprise Development LP<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD
> License
> @@ -25,16 +25,10 @@
>  **/
> 
>  #include "UefiDpLib.h"
> -#include <Guid/GlobalVariable.h>
> -#include <Library/PrintLib.h>
> -#include <Library/HandleParsingLib.h>
> -#include <Library/DevicePathLib.h>
> -
>  #include <Library/ShellLib.h>
>  #include <Library/BaseLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/DebugLib.h>
> -#include <Library/TimerLib.h>
>  #include <Library/UefiLib.h>
> 
>  #include <Guid/Performance.h>
> @@ -153,8 +147,7 @@ ShellCommandRunDp (
>    CONST CHAR16              *CmdLineArg;
>    EFI_STATUS                Status;
> 
> -  UINT64                    Freq;
> -  UINT64                    Ticker;
> +  PERFORMANCE_PROPERTY      *PerformanceProperty;
>    UINTN                     Number2Display;
> 
>    EFI_STRING                StringPtr;
> @@ -183,11 +176,6 @@ ShellCommandRunDp (
>    CustomCumulativeData = NULL;
>    ShellStatus = SHELL_SUCCESS;
> 
> -  // Get DP's entry time as soon as possible.
> -  // This is used as the Shell-Phase end time.
> -  //
> -  Ticker  = GetPerformanceCounter ();
> -
>    //
>    // initialize the shell lib (we must be in non-auto-init...)
>    //
> @@ -283,10 +271,15 @@ ShellCommandRunDp (
>    //    StartCount = Value loaded into the counter when it starts counting
>    //      EndCount = Value counter counts to before it needs to be reset
>    //
> -  Freq = GetPerformanceCounterProperties (&TimerInfo.StartCount,
> &TimerInfo.EndCount);
> +  Status = EfiGetSystemConfigurationTable (&gPerformanceProtocolGuid,
> &PerformanceProperty);
> +  if (EFI_ERROR (Status)) {
> +    ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN
> (STR_PERF_PROPERTY_NOT_FOUND), gDpHiiHandle);
> +    goto Done;
> +  }
> 
> -  // Convert the Frequency from Hz to KHz
> -  TimerInfo.Frequency = (UINT32)DivU64x32 (Freq, 1000);
> +  TimerInfo.Frequency  = (UINT32)DivU64x32
> (PerformanceProperty->Frequency, 1000);
> +  TimerInfo.StartCount = PerformanceProperty->TimerStartValue;
> +  TimerInfo.EndCount   = PerformanceProperty->TimerEndValue;
> 
>    // Determine in which direction the performance counter counts.
>    TimerInfo.CountUp = (BOOLEAN) (TimerInfo.EndCount >=
> TimerInfo.StartCount);
> @@ -362,7 +355,7 @@ ShellCommandRunDp (
>    } else {
>      //------------- Begin Cooked Mode Processing
>      if (TraceMode) {
> -      ProcessPhases ( Ticker );
> +      ProcessPhases ();
>        if ( ! SummaryMode) {
>          Status = ProcessHandles ( ExcludeMode);
>          if (Status == EFI_ABORTED) {
> diff --git a/ShellPkg/Library/UefiDpLib/DpInternal.h
> b/ShellPkg/Library/UefiDpLib/DpInternal.h
> index b5ec5f0..6de194a 100644
> --- a/ShellPkg/Library/UefiDpLib/DpInternal.h
> +++ b/ShellPkg/Library/UefiDpLib/DpInternal.h
> @@ -6,7 +6,7 @@
>    Dp application.  In addition to global data, function declarations for
>    DpUtilities.c, DpTrace.c, and DpProfile.c are included here.
> 
> -  Copyright (c) 2009 - 2013, Intel Corporation. All rights reserved.
> +  Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.
>    (C) Copyright 2015-2016 Hewlett Packard Enterprise Development LP<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD
> License
> @@ -236,12 +236,10 @@ DumpRawTrace(
>  /**
>    Gather and print Major Phase metrics.
> 
> -  @param[in]    Ticker      The timer value for the END of Shell phase
> -
>  **/
>  VOID
>  ProcessPhases(
> -  IN UINT64 Ticker
> +  VOID
>    );
> 
> 
> diff --git a/ShellPkg/Library/UefiDpLib/DpProfile.c
> b/ShellPkg/Library/UefiDpLib/DpProfile.c
> index 6458398..e443403 100644
> --- a/ShellPkg/Library/UefiDpLib/DpProfile.c
> +++ b/ShellPkg/Library/UefiDpLib/DpProfile.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Measured Profiling reporting for the Dp utility.
> 
> -  Copyright (c) 2009 - 2013, Intel Corporation. All rights reserved.
> +  Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.
>    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
> @@ -16,7 +16,6 @@
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
> -#include <Library/TimerLib.h>
>  #include <Library/PeCoffGetEntryPointLib.h>
>  #include <Library/PerformanceLib.h>
>  #include <Library/PrintLib.h>
> diff --git a/ShellPkg/Library/UefiDpLib/DpTrace.c
> b/ShellPkg/Library/UefiDpLib/DpTrace.c
> index eca2ef3..90a71ad 100644
> --- a/ShellPkg/Library/UefiDpLib/DpTrace.c
> +++ b/ShellPkg/Library/UefiDpLib/DpTrace.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Trace reporting for the Dp utility.
> 
> -  Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.
> +  Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.
>    (C) Copyright 2015-2016 Hewlett Packard Enterprise Development LP<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD
> License
> @@ -17,7 +17,6 @@
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
> -#include <Library/TimerLib.h>
>  #include <Library/PeCoffGetEntryPointLib.h>
>  #include <Library/PerformanceLib.h>
>  #include <Library/PrintLib.h>
> @@ -386,12 +385,10 @@ DumpRawTrace(
>  /**
>    Gather and print Major Phase metrics.
> 
> -  @param[in]    Ticker      The timer value for the END of Shell phase
> -
>  **/
>  VOID
>  ProcessPhases(
> -  IN UINT64            Ticker
> +  VOID
>    )
>  {
>    MEASUREMENT_RECORD        Measurement;
> @@ -400,7 +397,6 @@ ProcessPhases(
>    UINT64                    PeiTime;
>    UINT64                    DxeTime;
>    UINT64                    BdsTime;
> -  UINT64                    ShellTime;
>    UINT64                    ElapsedTime;
>    UINT64                    Duration;
>    UINT64                    Total;
> @@ -413,7 +409,6 @@ ProcessPhases(
>    PeiTime         = 0;
>    DxeTime         = 0;
>    BdsTime         = 0;
> -  ShellTime       = 0;
>    //
>    // Get Execution Phase Statistics
>    //
> @@ -434,9 +429,6 @@ ProcessPhases(
>                            &Measurement.EndTimeStamp,
>                            &Measurement.Identifier)) != 0)
>    {
> -    if (AsciiStrnCmp (Measurement.Token, ALit_SHELL, PERF_TOKEN_LENGTH)
> == 0) {
> -      Measurement.EndTimeStamp = Ticker;
> -    }
>      if (Measurement.EndTimeStamp == 0) { // Skip "incomplete" records
>        continue;
>      }
> @@ -454,8 +446,6 @@ ProcessPhases(
>        DxeTime      = Duration;
>      } else if (AsciiStrnCmp (Measurement.Token, ALit_BDS,
> PERF_TOKEN_LENGTH) == 0) {
>        BdsTime      = Duration;
> -    } else if (AsciiStrnCmp (Measurement.Token, ALit_SHELL,
> PERF_TOKEN_LENGTH) == 0) {
> -      ShellTime    = Duration;
>      }
>    }
> 
> @@ -510,17 +500,6 @@ ProcessPhases(
>      ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_DP_PHASE_BDSTO),
> gDpHiiHandle, ALit_BdsTO, ElapsedTime);
>    }
> 
> -  // print SHELL phase duration time
> -  //
> -  if (ShellTime > 0) {
> -    ElapsedTime = DivU64x32 (
> -                    ShellTime,
> -                    (UINT32)TimerInfo.Frequency
> -                    );
> -    Total += ElapsedTime;
> -    ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_DP_PHASE_DURATION),
> gDpHiiHandle, ALit_SHELL, ElapsedTime);
> -  }
> -
>    ShellPrintHiiEx (-1, -1, NULL, STRING_TOKEN (STR_DP_TOTAL_DURATION),
> gDpHiiHandle, Total);
>  }
> 
> diff --git a/ShellPkg/Library/UefiDpLib/DpUtilities.c
> b/ShellPkg/Library/UefiDpLib/DpUtilities.c
> index fbdd938..64f1830 100644
> --- a/ShellPkg/Library/UefiDpLib/DpUtilities.c
> +++ b/ShellPkg/Library/UefiDpLib/DpUtilities.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Utility functions used by the Dp application.
> 
> -  Copyright (c) 2009 - 2016, Intel Corporation. All rights reserved.
> +  Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.
>    (C) Copyright 2015-2016 Hewlett Packard Enterprise Development LP<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD
> License
> @@ -17,7 +17,6 @@
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
> -#include <Library/TimerLib.h>
>  #include <Library/PeCoffGetEntryPointLib.h>
>  #include <Library/PrintLib.h>
>  #include <Library/HiiLib.h>
> diff --git a/ShellPkg/Library/UefiDpLib/Literals.c
> b/ShellPkg/Library/UefiDpLib/Literals.c
> index 68de0fb..c1cddfb 100644
> --- a/ShellPkg/Library/UefiDpLib/Literals.c
> +++ b/ShellPkg/Library/UefiDpLib/Literals.c
> @@ -1,7 +1,7 @@
>  /** @file
>    Definitions of ASCII string literals used by DP.
> 
> -  Copyright (c) 2009 - 2010, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2009 - 2017, 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
> @@ -16,7 +16,6 @@
>  CHAR8 const ALit_TimerLibError[] = "Timer library instance error!\n";
>  CHAR8 const ALit_SEC[]    = SEC_TOK;
>  CHAR8 const ALit_DXE[]    = DXE_TOK;
> -CHAR8 const ALit_SHELL[]  = SHELL_TOK;
>  CHAR8 const ALit_PEI[]    = PEI_TOK;
>  CHAR8 const ALit_BDS[]    = BDS_TOK;
>  CHAR8 const ALit_BdsTO[]  = "BdsTimeOut";
> diff --git a/ShellPkg/Library/UefiDpLib/UefiDpLib.inf
> b/ShellPkg/Library/UefiDpLib/UefiDpLib.inf
> index 70d2163..0bce0ce 100644
> --- a/ShellPkg/Library/UefiDpLib/UefiDpLib.inf
> +++ b/ShellPkg/Library/UefiDpLib/UefiDpLib.inf
> @@ -1,7 +1,7 @@
>  ##  @file
>  #  Display Performance Application, Module information file.
>  #
> -# Copyright (c) 2009 - 2015, Intel Corporation. All rights reserved.
> +# Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.
>  # 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
> @@ -48,7 +48,6 @@
>    MdeModulePkg/MdeModulePkg.dec
> 
>  [LibraryClasses]
> -  TimerLib
>    PerformanceLib
>    DxeServicesLib
>    MemoryAllocationLib
> @@ -64,6 +63,9 @@
>    PrintLib
>    DevicePathLib
> 
> +[Guids]
> +  gPerformanceProtocolGuid                                ##
> CONSUMES ## SystemTable
> +
>  [Protocols]
>    gEfiLoadedImageProtocolGuid                             ##
> CONSUMES
>    gEfiDriverBindingProtocolGuid                           ##
> SOMETIMES_CONSUMES
> diff --git a/ShellPkg/Library/UefiDpLib/UefiDpLib.uni
> b/ShellPkg/Library/UefiDpLib/UefiDpLib.uni
> index b092274..b77c507 100644
> --- a/ShellPkg/Library/UefiDpLib/UefiDpLib.uni
> +++ b/ShellPkg/Library/UefiDpLib/UefiDpLib.uni
> @@ -1,7 +1,7 @@
>  // *++
>  //
>  // (C) Copyright 2014-2015 Hewlett-Packard Development Company, L.P.<BR>
> -// Copyright (c) 2009 - 2013, Intel Corporation. All rights reserved.<BR>
> +// Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.<BR>
>  // (C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR>
>  // This program and the accompanying materials
>  // are licensed and made available under the terms and conditions of the BSD
> License
> @@ -35,6 +35,7 @@
>  #string STR_DP_INVALID_ARG             #language en-US  "Invalid
> argument(s)\n"
>  #string STR_DP_HANDLES_ERROR           #language en-US  "Locate all
> handles error - %r\n"
>  #string STR_DP_ERROR_NAME              #language en-US  "Unknown
> driver name"
> +#string STR_PERF_PROPERTY_NOT_FOUND    #language en-US
> "Performance property not found\n"
>  #string STR_DP_BUILD_REVISION          #language en-US  "\nDP Build
> Version:       %d.%d\n"
>  #string STR_DP_KHZ                     #language en-US  "System
> Performance Timer Frequency:   %,8d (KHz)\n"
>  #string STR_DP_TIMER_PROPERTIES        #language en-US  "System
> Performance Timer counts %s from 0x%Lx to 0x%Lx\n"
> diff --git a/ShellPkg/ShellPkg.dsc b/ShellPkg/ShellPkg.dsc
> index 6b7864b..5c01933 100644
> --- a/ShellPkg/ShellPkg.dsc
> +++ b/ShellPkg/ShellPkg.dsc
> @@ -1,7 +1,7 @@
>  ##  @file
>  # Shell Package
>  #
> -# Copyright (c) 2007 - 2016, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2007 - 2017, 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
> @@ -99,7 +99,6 @@
> 
>    ShellPkg/Library/UefiDpLib/UefiDpLib.inf {
>      <LibraryClasses>
> -
> TimerLib|MdePkg/Library/BaseTimerLibNullTemplate/BaseTimerLibNullTemplate
> .inf
> 
> PerformanceLib|MdePkg/Library/BasePerformanceLibNull/BasePerformanceLib
> Null.inf
>        DxeServicesLib|MdePkg/Library/DxeServicesLib/DxeServicesLib.inf
>    }
> --
> 2.6.3.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel