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

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Jiewen,

If we do this, then we would also need to update all the 
platform DSC files that build the shell from sources too.

Maybe we should consider this in a different patch series?

Mike

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Thursday, February 2, 2017 9:35 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; edk2-devel@lists.01.org
> Cc: Andrew Fish <afish@apple.com>; Gao, Liming <liming.gao@intel.com>; Carsey, Jaben
> <jaben.carsey@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH v4 3/3] ShellPkg UefiDpLib: Remove TimerLib dependency
> 
> 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