> On Jun 14, 2016, at 8:31 PM, Zeng, Star <[email protected]> wrote: > > 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?
Is there a hash I can diff? > 4. Do you have any prototype code? No. I've just implemented some PerformanceLIb instances and I was commenting on some of Mike's ideas. I've been pointing out for years the edk2 scheme was broken due to common assumptions about frequency. >From a design stand point you end up with the following. 1) The logging mechanism needs to define the Frequency of the timer. The Log header is a place this can be done. 2) Three is an issue with TSC vs calibration (PCD, APIC, ACPI timer etc) , so maybe some way in the header to have an enum for well known sources would be helpful vs. just showing a frequency that could vary. 3) It has to be possible to implement. For DXE an extra protocol can be added to the handle, but for PEI it might require a GUID in the the PEI_PERFORMANCE_LOG_HEADER so the implementations can log to the correct instance. Thanks, Andrew Fish > > > 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
