Hi, Let us clarify below items to be more clear about what we are talking about. 1. Current DP depends on platform's TimerLib, this patch series is to remove the dependency from DP by introducing debug property system table that will be installed by DxeCorePerformanceLib or SmmCorePerformanceLib. Do you agree this patch series? 2. Current PerformanceLib + DP solution requires single TimerLib instance to be used by the callers. We are focusing on this requirement, right? What problem do we want to resolve? 3. The change to PEI_PERFORMANCE_LOG_HEADER you are saying is similar with the change in patch series V2 at http://article.gmane.org/gmane.comp.bios.edk2.devel/13173? 4. Do you have any prototype code?
Thanks, Star -----Original Message----- From: edk2-devel [mailto:[email protected]] On Behalf Of Andrew Fish Sent: Wednesday, June 15, 2016 10:56 AM To: Zeng, Star <[email protected]> Cc: Kinney, Michael D <[email protected]>; Carsey, Jaben <[email protected]>; [email protected]; Yao, Jiewen <[email protected]>; Gao, Liming <[email protected]> Subject: Re: [edk2] [PATCH V3 1/3] MdeModulePkg: Define and install performance property configuration table > On Jun 14, 2016, at 7:38 PM, Zeng, Star <[email protected]> wrote: > > Andrew, > > Sorry, I am still confused about that. > Extending PEI_PERFORMANCE_LOG_HEADER can only record the timer source of > PeiCore. > Except we are going to record time instead of tick, I am not sure how > to know the caller may be using different instance of TimerLib by > current interfaces PERF_START() ->StartPerformanceMeasurement() and > PERF_END()->EndPerformanceMeasurement() > > #define PERF_START(Handle, Token, Module, TimeStamp) > > RETURN_STATUS > EFIAPI > StartPerformanceMeasurement ( > IN CONST VOID *Handle, OPTIONAL > IN CONST CHAR8 *Token, OPTIONAL > IN CONST CHAR8 *Module, OPTIONAL > IN UINT64 TimeStamp > ); > > If the Start and End PerformanceMeasurement come from different library instances I think you have an issue regardless. If the Start and End come from the same lib you just record the relative Start and Stop Timestamp as Mike pointed out. https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Include/Guid/Performance.h#L27 The information in the the Log header is how you convert the two timestamps into wall time. If there are different instances of the library (say one that does TSC and another that is logging ACPI TImer), each instance of the library would log to a different HOB/Protocol and this would be based on the extra information we place in the PEI_PERFORMANCE_LOG_HEADER. Thanks, Andrew Fish > Thanks, > Star > -----Original Message----- > From: edk2-devel [mailto:[email protected]] On Behalf Of > Andrew Fish > Sent: Wednesday, June 15, 2016 10:25 AM > To: Zeng, Star <[email protected]> > Cc: Kinney, Michael D <[email protected]>; Carsey, Jaben > <[email protected]>; [email protected]; Yao, Jiewen > <[email protected]>; Gao, Liming <[email protected]> > Subject: Re: [edk2] [PATCH V3 1/3] MdeModulePkg: Define and install > performance property configuration table > > >> On Jun 14, 2016, at 6:55 PM, Zeng, Star <[email protected]> wrote: >> >> Mike & Andrew, >> >> Could you have more detail about the idea of a record type for a time >> source? I am not so clear about that. >> > > Star, > > For example the PEI_PERFORMANCE_LOG_HEADER could be updated to included the > frequency and/or and/or an enum that identifies the source like TSC. > > https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Include/Gui > d/Performance.h#L38 > > Then each time source that was unique could be a different instance of the > MdeModulePkg/Include/Guid/Performance.h data. > > Thanks, > > Andrew Fish > >> Thanks, >> Star >> -----Original Message----- >> From: Kinney, Michael D >> Sent: Tuesday, June 14, 2016 9:27 AM >> To: [email protected]; Kinney, Michael D <[email protected]> >> Cc: Zeng, Star <[email protected]>; [email protected]; >> Carsey, Jaben <[email protected]>; Yao, Jiewen >> <[email protected]>; Gao, Liming <[email protected]> >> Subject: RE: [edk2] [PATCH V3 1/3] MdeModulePkg: Define and install >> performance property configuration table >> >> Andrew, >> >> I like the idea of a record type for a time source. A PerformanceLib >> constructor could add that record, and performance log entries could be >> tagged with a time source record indicator. >> >> I agree that any timer source that requires an algorithm to measure the >> frequency could get slightly different frequency values which would increase >> the number of time source records for the same timer. >> >> The other even more complicated issue is a performance record whose start >> value is set in one module and stop value is set in a different module. If >> those two modules use different TimerLib instances, then it won't work. >> Which is one of the reasons the single TimerLib instance solution is used. >> >> Mike >> >>> -----Original Message----- >>> From: [email protected] [mailto:[email protected]] >>> Sent: Monday, June 13, 2016 4:34 PM >>> To: Kinney, Michael D <[email protected]> >>> Cc: Zeng, Star <[email protected]>; [email protected]; >>> Carsey, Jaben <[email protected]>; Yao, Jiewen >>> <[email protected]>; Gao, Liming <[email protected]> >>> Subject: Re: [edk2] [PATCH V3 1/3] MdeModulePkg: Define and install >>> performance property configuration table >>> >>> >>>> On Jun 13, 2016, at 4:14 PM, Kinney, Michael D >>>> <[email protected]> wrote: >>>> >>>> Star, >>>> >>>> The PERFORMANCE_PROPERTY structure has a field called CpuFreq. >>>> >>>> Since there are many different types of timer sources, some not >>>> related to the CPU frequencies, should this field name just be >>>> Frequency? >>>> >>>> Also, one of the long standing issues with the DP command is that >>>> all the modules that record performance records must use the same >>>> instance of the TimerLib so this same frequency counter is used for >>>> all performance records. >>>> >>>> If we are going to make the types of changes you are proposing >>>> here, can we also evaluate if we can update the performance records >>>> to include the counter frequency information so the DP command can >>>> support evaluation of log entries that are generated using >>>> different TimerLib instances? >>>> >>> >>> Mike, >>> >>> Maybe thee should just be multiple instances of the performance >>> logs, one per time source vs tagging every entry? >>> >>> It can get really complicated as I seem to remember updating a timer >>> lib to use TSC + timer XYZ. Depending on how the Freq is calculated >>> you could get different frequencies for the TSC. For example a PCD >>> value vs. calibration with APIC could make the time sources look not >>> the same for the TSC. Thus maybe some kind of indication of the the TSC was >>> used could be useful? >>> >>> Thanks, >>> >>> Andrew Fish >>> >>>> Thanks, >>>> >>>> Mike >>>> >>>>> -----Original Message----- >>>>> From: edk2-devel [mailto:[email protected]] On >>>>> Behalf Of Star Zeng >>>>> Sent: Sunday, June 12, 2016 12:27 AM >>>>> To: [email protected] >>>>> Cc: Carsey, Jaben <[email protected]>; Yao, Jiewen >>>>> <[email protected]>; Gao, Liming <[email protected]> >>>>> Subject: [edk2] [PATCH V3 1/3] MdeModulePkg: Define and install >>>>> performance property configuration table >>>>> >>>>> Define PERFORMANCE_PROPERTY, and install performance property >>>>> configuration table in DxeCorePerformanceLib and SmmCorePerformanceLib. >>>>> >>>>> Cc: Liming Gao <[email protected]> >>>>> Cc: Jiewen Yao <[email protected]> >>>>> Cc: Cinnamon Shia <[email protected]> >>>>> Cc: Jaben Carsey <[email protected]> >>>>> Contributed-under: TianoCore Contribution Agreement 1.0 >>>>> Signed-off-by: Star Zeng <[email protected]> >>>>> --- >>>>> MdeModulePkg/Include/Guid/Performance.h | 12 ++++++++++- >>>>> .../DxeCorePerformanceLib/DxeCorePerformanceLib.c | 24 >>>>> +++++++++++++++++++++- >>>>> .../DxeCorePerformanceLib.inf | 4 +++- >>>>> .../DxeCorePerformanceLibInternal.h | 3 ++- >>>>> .../SmmCorePerformanceLib/SmmCorePerformanceLib.c | 22 >>>>> ++++++++++++++++++++ >>>>> .../SmmCorePerformanceLib.inf | 5 ++++- >>>>> 6 files changed, 65 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/MdeModulePkg/Include/Guid/Performance.h >>>>> b/MdeModulePkg/Include/Guid/Performance.h >>>>> index c40046c87811..fe6972d9bf6d 100644 >>>>> --- a/MdeModulePkg/Include/Guid/Performance.h >>>>> +++ b/MdeModulePkg/Include/Guid/Performance.h >>>>> @@ -4,7 +4,7 @@ >>>>> * performance protocol interfaces. >>>>> * performance variables. >>>>> >>>>> -Copyright (c) 2009 - 2013, Intel Corporation. All rights >>>>> reserved.<BR> >>>>> +Copyright (c) 2009 - 2016, 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 that >>>>> accompanies this distribution. >>>>> The full text of the license may be found at @@ -18,6 +18,16 @@ >>>>> WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS >>> OR >>>>> IMPLIED. >>>>> #ifndef __PERFORMANCE_DATA_H__ >>>>> #define __PERFORMANCE_DATA_H__ >>>>> >>>>> +#define PERFORMANCE_PROPERTY_REVISION 0x1 >>>>> + >>>>> +typedef struct { >>>>> + UINT32 Revision; >>>>> + UINT32 Reserved; >>>>> + UINT64 CpuFreq; >>>>> + UINT64 TimerStartValue; >>>>> + UINT64 TimerEndValue; >>>>> +} PERFORMANCE_PROPERTY; >>>>> + >>>>> // >>>>> // PEI_PERFORMANCE_STRING_SIZE must be a multiple of 8. >>>>> // >>>>> diff --git >>>>> a/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib. >>>>> c >>>>> b/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib. >>>>> c index 4739bb842661..de7f7cbff9b1 100644 >>>>> --- >>>>> a/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib. >>>>> c >>>>> +++ b/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanc >>>>> +++ e >>>>> +++ Lib.c >>>>> @@ -61,6 +61,8 @@ PERFORMANCE_EX_PROTOCOL mPerformanceExInterface >>>>> = { GetGaugeEx }; >>>>> >>>>> +PERFORMANCE_PROPERTY mPerformanceProperty; >>>>> + >>>>> /** >>>>> Searches in the gauge array with keyword Handle, Token, Module and >>>>> Identifier. >>>>> >>>>> @@ -502,6 +504,10 @@ DxeCorePerformanceLibConstructor ( >>>>> ) >>>>> { >>>>> EFI_STATUS Status; >>>>> + UINT64 Freq; >>>>> + UINT64 StartValue; >>>>> + UINT64 EndValue; >>>>> + PERFORMANCE_PROPERTY *PerformanceProperty; >>>>> >>>>> if (!PerformanceMeasurementEnabled ()) { >>>>> // >>>>> @@ -531,7 +537,23 @@ DxeCorePerformanceLibConstructor ( >>>>> >>>>> InternalGetPeiPerformance (); >>>>> >>>>> - return Status; >>>>> + Status = EfiGetSystemConfigurationTable >>>>> + (&gPerformanceProtocolGuid, >>>>> &PerformanceProperty); >>>>> + if (EFI_ERROR (Status)) { >>>>> + PerformanceProperty = &mPerformanceProperty; >>>>> + // >>>>> + // Install configuration table for performance property. >>>>> + // >>>>> + PerformanceProperty->Revision = PERFORMANCE_PROPERTY_REVISION; >>>>> + PerformanceProperty->Reserved = 0; >>>>> + Freq = GetPerformanceCounterProperties (&StartValue, &EndValue); >>>>> + PerformanceProperty->CpuFreq = Freq; >>>>> + PerformanceProperty->TimerStartValue = StartValue; >>>>> + PerformanceProperty->TimerEndValue = EndValue; >>>>> + Status = gBS->InstallConfigurationTable >>>>> + (&gPerformanceProtocolGuid, >>>>> PerformanceProperty); >>>>> + ASSERT_EFI_ERROR (Status); >>>>> + } >>>>> + >>>>> + return EFI_SUCCESS; >>>>> } >>>>> >>>>> /** >>>>> diff --git >>>>> a/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib. >>>>> inf >>>>> b/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib. >>>>> inf index f73d0a4386ad..40a5ddc1e53e 100644 >>>>> --- >>>>> a/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib. >>>>> inf >>>>> +++ b/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanc >>>>> +++ e >>>>> +++ Lib.inf >>>>> @@ -9,7 +9,7 @@ >>>>> # This library is mainly used by DxeCore to start performance >>>>> logging to ensure >>> that >>>>> # Performance and PerformanceEx Protocol are installed at the >>>>> very beginning of DXE phase. >>>>> # >>>>> -# Copyright (c) 2006 - 2014, Intel Corporation. All rights >>>>> reserved.<BR> >>>>> +# Copyright (c) 2006 - 2016, Intel Corporation. All rights >>>>> +reserved.<BR> >>>>> # (C) Copyright 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 >>>>> @@ >>>>> -56,11 +56,13 @@ [LibraryClasses] >>>>> BaseLib >>>>> HobLib >>>>> DebugLib >>>>> + UefiLib >>>>> >>>>> >>>>> [Guids] >>>>> ## SOMETIMES_CONSUMES ## HOB >>>>> ## PRODUCES ## UNDEFINED # Install protocol >>>>> + ## PRODUCES ## SystemTable >>>>> gPerformanceProtocolGuid >>>>> ## SOMETIMES_CONSUMES ## HOB >>>>> ## PRODUCES ## UNDEFINED # Install protocol >>>>> diff --git >>> a/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLibIn >>> t >>> e >>> rnal.h >>>>> b/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib >>>>> I nternal.h index 2b9ccd2fee0c..fb5c6c2395ea 100644 >>>>> --- >>>>> a/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLib >>>>> I >>>>> nternal.h >>>>> +++ b/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanc >>>>> +++ e >>>>> +++ LibInternal.h >>>>> @@ -4,7 +4,7 @@ >>>>> This header file holds the prototypes of the Performance and >>>>> PerformanceEx >>> Protocol >>>>> published by this >>>>> library instance at its constructor. >>>>> >>>>> -Copyright (c) 2006 - 2012, Intel Corporation. All rights >>>>> reserved.<BR> >>>>> +Copyright (c) 2006 - 2016, 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 @@ -32,6 +32,7 @@ WITHOUT WARRANTIES OR >>>>> REPRESENTATIONS OF ANY KIND, EITHER EXPRESS >>> OR >>>>> IMPLIED. >>>>> #include <Library/PcdLib.h> >>>>> #include <Library/UefiBootServicesTableLib.h> >>>>> #include <Library/MemoryAllocationLib.h> >>>>> +#include <Library/UefiLib.h> >>>>> >>>>> // >>>>> // Interface declarations for PerformanceEx Protocol. >>>>> diff --git >>>>> a/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib. >>>>> c >>>>> b/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib. >>>>> c index 6f8d2dd064a3..93f7a83437a4 100644 >>>>> --- >>>>> a/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib. >>>>> c >>>>> +++ b/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanc >>>>> +++ e >>>>> +++ Lib.c >>>>> @@ -69,6 +69,8 @@ PERFORMANCE_EX_PROTOCOL mPerformanceExInterface >>>>> = { GetGaugeEx }; >>>>> >>>>> +PERFORMANCE_PROPERTY mPerformanceProperty; >>>>> + >>>>> /** >>>>> Searches in the gauge array with keyword Handle, Token, Module and >>>>> Identfier. >>>>> >>>>> @@ -687,6 +689,10 @@ InitializeSmmCorePerformanceLib ( { >>>>> EFI_STATUS Status; >>>>> EFI_HANDLE Handle; >>>>> + UINT64 Freq; >>>>> + UINT64 StartValue; >>>>> + UINT64 EndValue; >>>>> + PERFORMANCE_PROPERTY *PerformanceProperty; >>>>> >>>>> // >>>>> // Initialize spin lock >>>>> @@ -725,6 +731,22 @@ InitializeSmmCorePerformanceLib ( >>>>> ASSERT_EFI_ERROR (Status); Status = gSmst->SmiHandlerRegister >>>>> (SmmPerformanceHandlerEx, &gSmmPerformanceExProtocolGuid, >>>>> &Handle); ASSERT_EFI_ERROR (Status); >>>>> + >>>>> + Status = EfiGetSystemConfigurationTable >>>>> + (&gPerformanceProtocolGuid, >>>>> &PerformanceProperty); >>>>> + if (EFI_ERROR (Status)) { >>>>> + PerformanceProperty = &mPerformanceProperty; >>>>> + // >>>>> + // Install configuration table for performance property. >>>>> + // >>>>> + PerformanceProperty->Revision = PERFORMANCE_PROPERTY_REVISION; >>>>> + PerformanceProperty->Reserved = 0; >>>>> + Freq = GetPerformanceCounterProperties (&StartValue, &EndValue); >>>>> + PerformanceProperty->CpuFreq = Freq; >>>>> + PerformanceProperty->TimerStartValue = StartValue; >>>>> + PerformanceProperty->TimerEndValue = EndValue; >>>>> + Status = gBS->InstallConfigurationTable >>>>> + (&gPerformanceProtocolGuid, >>>>> PerformanceProperty); >>>>> + ASSERT_EFI_ERROR (Status); >>>>> + } >>>>> } >>>>> >>>>> /** >>>>> diff --git >>>>> a/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib. >>>>> inf >>>>> b/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib. >>>>> inf index 160a749390e1..5b5924d41848 100644 >>>>> --- >>>>> a/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanceLib. >>>>> inf >>>>> +++ b/MdeModulePkg/Library/SmmCorePerformanceLib/SmmCorePerformanc >>>>> +++ e >>>>> +++ Lib.inf >>>>> @@ -8,7 +8,7 @@ >>>>> # This library is mainly used by SMM Core to start performance >>>>> logging to ensure >>> that >>>>> # SMM Performance and PerformanceEx Protocol are installed at the >>>>> very beginning of SMM phase. >>>>> # >>>>> -# Copyright (c) 2011 - 2015, Intel Corporation. All rights >>>>> reserved.<BR> >>>>> +# Copyright (c) 2011 - 2016, 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 >>>>> @@ -57,6 +57,7 @@ [LibraryClasses] SynchronizationLib >>>>> SmmServicesTableLib SmmMemLib >>>>> + UefiLib >>>>> >>>>> [Protocols] >>>>> gEfiSmmBase2ProtocolGuid ## CONSUMES >>>>> @@ -68,6 +69,8 @@ [Guids] >>>>> ## PRODUCES ## UNDEFINED # Install protocol ## CONSUMES ## >>>>> UNDEFINED # SmiHandlerRegister gSmmPerformanceExProtocolGuid >>>>> + ## PRODUCES ## SystemTable >>>>> + gPerformanceProtocolGuid >>>>> >>>>> [Pcd] >>>>> gEfiMdePkgTokenSpaceGuid.PcdPerformanceLibraryPropertyMask ## CONSUMES >>>>> -- >>>>> 2.7.0.windows.1 >>>>> >>>>> _______________________________________________ >>>>> edk2-devel mailing list >>>>> [email protected] >>>>> https://lists.01.org/mailman/listinfo/edk2-devel >>>> _______________________________________________ >>>> edk2-devel mailing list >>>> [email protected] >>>> https://lists.01.org/mailman/listinfo/edk2-devel >> >> _______________________________________________ >> edk2-devel mailing list >> [email protected] >> https://lists.01.org/mailman/listinfo/edk2-devel > > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
