Mike, It is make sense to change CpuFreq field name to Frequency.
We ever thought about to catch the TimerLib mismatch case, but the frequency returned from TimerLib may have very very small deviation, for example the AcpiTimerLib instances in PcAtChipsetPkg that are using ACPI timer to calibrate TSC frequency. Although the very very small deviation does not impact the final duration calculation, but that makes PerformanceLib can't compare the frequencies. Another, if we want PerformanceLib to record frequency for every entry, the interface of PerformanceLib and performance protocol also needs to be changed to accept frequency from caller. Thanks, Star From: Kinney, Michael D Sent: Tuesday, June 14, 2016 8:50 AM To: Yao, Jiewen <[email protected]>; Zeng, Star <[email protected]>; [email protected]; Kinney, Michael D <[email protected]> Cc: Carsey, Jaben <[email protected]>; Gao, Liming <[email protected]> Subject: RE: [edk2] [PATCH V3 1/3] MdeModulePkg: Define and install performance property configuration table Jiewen, The reason the original design does not record time is to minimize the time required to add a performance record to reduce the overhead of doing performance measurements. Reading a counter is much simpler and takes less time than converting to a time value that usually requires multiple/divide operations. Delaying this time conversion to the DP command resolves this issue. Mike From: Yao, Jiewen Sent: Monday, June 13, 2016 5:42 PM To: Kinney, Michael D <[email protected]<mailto:[email protected]>>; Zeng, Star <[email protected]<mailto:[email protected]>>; [email protected]<mailto:[email protected]> Cc: Carsey, Jaben <[email protected]<mailto:[email protected]>>; Gao, Liming <[email protected]<mailto:[email protected]>>; Yao, Jiewen <[email protected]<mailto:[email protected]>> Subject: RE: [edk2] [PATCH V3 1/3] MdeModulePkg: Define and install performance property configuration table Thanks Mike. Another possible way I am thinking is that: Can we just record *time* instead of *tick* in the perf record? Then there will be no worry on which timer lib the module is using. Thank you Yao Jiewen From: Kinney, Michael D Sent: Tuesday, June 14, 2016 7:15 AM To: Zeng, Star <[email protected]<mailto:[email protected]>>; [email protected]<mailto:[email protected]>; Kinney, Michael D <[email protected]<mailto:[email protected]>> Cc: Carsey, Jaben <[email protected]<mailto:[email protected]>>; Yao, Jiewen <[email protected]<mailto:[email protected]>>; Gao, Liming <[email protected]<mailto:[email protected]>> Subject: RE: [edk2] [PATCH V3 1/3] MdeModulePkg: Define and install performance property configuration table 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? 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]<mailto:[email protected]> > Cc: Carsey, Jaben <[email protected]<mailto:[email protected]>>; > Yao, Jiewen <[email protected]<mailto:[email protected]>>; Gao, > Liming <[email protected]<mailto:[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]<mailto:[email protected]>> > Cc: Jiewen Yao <[email protected]<mailto:[email protected]>> > Cc: Cinnamon Shia <[email protected]<mailto:[email protected]>> > Cc: Jaben Carsey <[email protected]<mailto:[email protected]>> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Star Zeng <[email protected]<mailto:[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/DxeCorePerformanceLib.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/DxeCorePerformanceLib.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/DxeCorePerformanceLibInternal.h > b/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLibInternal.h > index 2b9ccd2fee0c..fb5c6c2395ea 100644 > --- > a/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLibInternal.h > +++ > b/MdeModulePkg/Library/DxeCorePerformanceLib/DxeCorePerformanceLibInternal.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/SmmCorePerformanceLib.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/SmmCorePerformanceLib.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]<mailto:[email protected]> > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
