* [PATCH 0/3] Update ProcTrace feature code for new requirements.
@ 2023-04-25 5:47 duntan
2023-04-25 5:47 ` [PATCH 1/3] UefiCpuPkg: Update code to support enable ProcTrace only on BSP duntan
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: duntan @ 2023-04-25 5:47 UTC (permalink / raw)
To: devel
Update ProcTrace feature code to support enable ProcTrace only on BSP and support enable collect performance.
Dun Tan (3):
UefiCpuPkg: Update code to support enable ProcTrace only on BSP
UefiCpuPkg: Update PT code to support enable collect performance
UefiCpuPkg: Disable MTC packet by default
UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf | 12 +++++++-----
UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c | 229 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----------------------------------------------------------------------------
UefiCpuPkg/UefiCpuPkg.dec | 15 +++++++++++++++
3 files changed, 174 insertions(+), 82 deletions(-)
--
2.39.1.windows.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/3] UefiCpuPkg: Update code to support enable ProcTrace only on BSP
2023-04-25 5:47 [PATCH 0/3] Update ProcTrace feature code for new requirements duntan
@ 2023-04-25 5:47 ` duntan
2023-04-25 6:03 ` Ni, Ray
[not found] ` <1759183F15FD97B2.10313@groups.io>
2023-04-25 5:47 ` [PATCH 2/3] UefiCpuPkg: Update PT code to support enable collect performance duntan
2023-04-25 5:47 ` [PATCH 3/3] UefiCpuPkg: Disable MTC packet by default duntan
2 siblings, 2 replies; 11+ messages in thread
From: duntan @ 2023-04-25 5:47 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann, Xiao X Chen
Update code to support enable ProcTrace only on BSP. Add a new
dynamic PCD to indicate if enable ProcTrace only on BSP. In
ProcTrace.c code, if this new PCD is true, only allocate buffer
and set CtrlReg.Bits.TraceEn to 1 for BSP.
Bugzila: https://bugzilla.tianocore.org/show_bug.cgi?id=4423
Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Xiao X Chen <xiao.x.chen@intel.com>
---
UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf | 11 ++++++-----
UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c | 159 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------------------
UefiCpuPkg/UefiCpuPkg.dec | 7 +++++++
3 files changed, 112 insertions(+), 65 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
index 7fbcd8da0e..319c8b4842 100644
--- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
+++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
@@ -4,7 +4,7 @@
# This library registers CPU features defined in Intel(R) 64 and IA-32
# Architectures Software Developer's Manual.
#
-# Copyright (c) 2017 - 2019, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2017 - 2023, Intel Corporation. All rights reserved.<BR>
#
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
@@ -58,7 +58,8 @@
LocalApicLib
[Pcd]
- gUefiCpuPkgTokenSpaceGuid.PcdCpuClockModulationDutyCycle ## SOMETIMES_CONSUMES
- gUefiCpuPkgTokenSpaceGuid.PcdIsPowerOnReset ## SOMETIMES_CONSUMES
- gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme ## SOMETIMES_CONSUMES
- gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize ## SOMETIMES_CONSUMES
+ gUefiCpuPkgTokenSpaceGuid.PcdCpuClockModulationDutyCycle ## SOMETIMES_CONSUMES
+ gUefiCpuPkgTokenSpaceGuid.PcdIsPowerOnReset ## SOMETIMES_CONSUMES
+ gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme ## SOMETIMES_CONSUMES
+ gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize ## SOMETIMES_CONSUMES
+ gUefiCpuPkgTokenSpaceGuid.PcdEnableProcessorTraceOnBspOnly ## SOMETIMES_CONSUMES
diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
index 04e6a60728..f57544bf7d 100644
--- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
+++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
@@ -1,7 +1,7 @@
/** @file
Intel Processor Trace feature.
- Copyright (c) 2017 - 2018, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2017 - 2023, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -188,6 +188,8 @@ ProcTraceInitialize (
MSR_IA32_RTIT_OUTPUT_BASE_REGISTER OutputBaseReg;
MSR_IA32_RTIT_OUTPUT_MASK_PTRS_REGISTER OutputMaskPtrsReg;
RTIT_TOPA_TABLE_ENTRY *TopaEntryPtr;
+ BOOLEAN IsBsp;
+ BOOLEAN EnableOnBspOnly;
//
// The scope of the MSR_IA32_RTIT_* is core for below processor type, only program
@@ -236,6 +238,13 @@ ProcTraceInitialize (
return RETURN_SUCCESS;
}
+ IsBsp = (CpuInfo->ProcessorInfo.StatusFlag & BIT0) ? TRUE : FALSE;
+ EnableOnBspOnly = (PcdGetBool (PcdEnableProcessorTraceOnBspOnly)) ? TRUE : FALSE;
+
+ if (EnableOnBspOnly && (IsBsp == FALSE)) {
+ return RETURN_SUCCESS;
+ }
+
MemRegionBaseAddr = 0;
FirstIn = FALSE;
@@ -260,43 +269,59 @@ ProcTraceInitialize (
// address base in MSR, IA32_RTIT_OUTPUT_BASE (560h) bits 47:12. Note that all regions must be
// aligned based on their size, not just 4K. Thus a 2M region must have bits 20:12 cleared.
//
- ThreadMemRegionTable = (UINTN *)AllocatePool (ProcTraceData->NumberOfProcessors * sizeof (UINTN *));
- if (ThreadMemRegionTable == NULL) {
- DEBUG ((DEBUG_ERROR, "Allocate ProcTrace ThreadMemRegionTable Failed\n"));
- return RETURN_OUT_OF_RESOURCES;
- }
-
- ProcTraceData->ThreadMemRegionTable = ThreadMemRegionTable;
- for (Index = 0; Index < ProcTraceData->NumberOfProcessors; Index++, ProcTraceData->AllocatedThreads++) {
- Pages = EFI_SIZE_TO_PAGES (MemRegionSize);
- Alignment = MemRegionSize;
+ Pages = EFI_SIZE_TO_PAGES (MemRegionSize);
+ Alignment = MemRegionSize;
+ if (EnableOnBspOnly) {
AlignedAddress = (UINTN)AllocateAlignedReservedPages (Pages, Alignment);
if (AlignedAddress == 0) {
- DEBUG ((DEBUG_ERROR, "ProcTrace: Out of mem, allocated only for %d threads\n", ProcTraceData->AllocatedThreads));
- if (Index == 0) {
- //
- // Could not allocate for BSP even
- //
- FreePool ((VOID *)ThreadMemRegionTable);
- ThreadMemRegionTable = NULL;
- return RETURN_OUT_OF_RESOURCES;
+ //
+ // Could not allocate for BSP even
+ //
+ DEBUG ((DEBUG_ERROR, "ProcTrace: Out of mem, failed to allocate buffer for BSP\n"));
+ return RETURN_OUT_OF_RESOURCES;
+ }
+
+ DEBUG ((DEBUG_INFO, "ProcTrace: Allocated PT mem for BSP only.\n"));
+ } else {
+ ThreadMemRegionTable = (UINTN *)AllocatePool (ProcTraceData->NumberOfProcessors * sizeof (UINTN *));
+ if (ThreadMemRegionTable == NULL) {
+ DEBUG ((DEBUG_ERROR, "Allocate ProcTrace ThreadMemRegionTable Failed\n"));
+ return RETURN_OUT_OF_RESOURCES;
+ }
+
+ ProcTraceData->ThreadMemRegionTable = ThreadMemRegionTable;
+
+ for (Index = 0; Index < ProcTraceData->NumberOfProcessors; Index++, ProcTraceData->AllocatedThreads++) {
+ AlignedAddress = (UINTN)AllocateAlignedReservedPages (Pages, Alignment);
+ if (AlignedAddress == 0) {
+ DEBUG ((DEBUG_ERROR, "ProcTrace: Out of mem, allocated only for %d threads\n", ProcTraceData->AllocatedThreads));
+ if (Index == 0) {
+ //
+ // Could not allocate for BSP even
+ //
+ FreePool ((VOID *)ThreadMemRegionTable);
+ ThreadMemRegionTable = NULL;
+ return RETURN_OUT_OF_RESOURCES;
+ }
+
+ break;
}
- break;
+ ThreadMemRegionTable[Index] = AlignedAddress;
+ DEBUG ((DEBUG_INFO, "ProcTrace: PT MemRegionBaseAddr(aligned) for thread %d: 0x%llX \n", Index, (UINT64)ThreadMemRegionTable[Index]));
}
- ThreadMemRegionTable[Index] = AlignedAddress;
- DEBUG ((DEBUG_INFO, "ProcTrace: PT MemRegionBaseAddr(aligned) for thread %d: 0x%llX \n", Index, (UINT64)ThreadMemRegionTable[Index]));
+ DEBUG ((DEBUG_INFO, "ProcTrace: Allocated PT mem for %d thread \n", ProcTraceData->AllocatedThreads));
}
-
- DEBUG ((DEBUG_INFO, "ProcTrace: Allocated PT mem for %d thread \n", ProcTraceData->AllocatedThreads));
}
- if (ProcessorNumber < ProcTraceData->AllocatedThreads) {
- MemRegionBaseAddr = ProcTraceData->ThreadMemRegionTable[ProcessorNumber];
- } else {
- return RETURN_SUCCESS;
+ if (!EnableOnBspOnly) {
+ if (ProcessorNumber < ProcTraceData->AllocatedThreads) {
+ MemRegionBaseAddr = ProcTraceData->ThreadMemRegionTable[ProcessorNumber];
+ } else {
+ return RETURN_SUCCESS;
+ }
}
///
@@ -367,50 +392,64 @@ ProcTraceInitialize (
//
if (FirstIn) {
DEBUG ((DEBUG_INFO, "ProcTrace: Enabling ToPA scheme \n"));
- //
- // Let BSP allocate ToPA table mem for all threads
- //
- TopaMemArray = (UINTN *)AllocatePool (ProcTraceData->AllocatedThreads * sizeof (UINTN *));
- if (TopaMemArray == NULL) {
- DEBUG ((DEBUG_ERROR, "ProcTrace: Allocate mem for ToPA Failed\n"));
- return RETURN_OUT_OF_RESOURCES;
- }
- ProcTraceData->TopaMemArray = TopaMemArray;
+ Pages = EFI_SIZE_TO_PAGES (sizeof (PROC_TRACE_TOPA_TABLE));
+ Alignment = 0x1000;
- for (Index = 0; Index < ProcTraceData->AllocatedThreads; Index++) {
- Pages = EFI_SIZE_TO_PAGES (sizeof (PROC_TRACE_TOPA_TABLE));
- Alignment = 0x1000;
+ if (EnableOnBspOnly) {
AlignedAddress = (UINTN)AllocateAlignedReservedPages (Pages, Alignment);
if (AlignedAddress == 0) {
- if (Index < ProcTraceData->AllocatedThreads) {
- ProcTraceData->AllocatedThreads = Index;
- }
+ DEBUG ((DEBUG_ERROR, "ProcTrace: Out of mem, failed to allocate ToPA mem for BSP"));
+ return RETURN_OUT_OF_RESOURCES;
+ }
- DEBUG ((DEBUG_ERROR, "ProcTrace: Out of mem, allocated ToPA mem only for %d threads\n", ProcTraceData->AllocatedThreads));
- if (Index == 0) {
- //
- // Could not allocate for BSP even
- //
- FreePool ((VOID *)TopaMemArray);
- TopaMemArray = NULL;
- return RETURN_OUT_OF_RESOURCES;
+ DEBUG ((DEBUG_INFO, "ProcTrace: Allocated ToPA mem for BSP only\n"));
+ } else {
+ //
+ // Let BSP allocate ToPA table mem for all threads
+ //
+ TopaMemArray = (UINTN *)AllocatePool (ProcTraceData->AllocatedThreads * sizeof (UINTN *));
+ if (TopaMemArray == NULL) {
+ DEBUG ((DEBUG_ERROR, "ProcTrace: Allocate mem for ToPA Failed\n"));
+ return RETURN_OUT_OF_RESOURCES;
+ }
+
+ ProcTraceData->TopaMemArray = TopaMemArray;
+
+ for (Index = 0; Index < ProcTraceData->AllocatedThreads; Index++) {
+ AlignedAddress = (UINTN)AllocateAlignedReservedPages (Pages, Alignment);
+ if (AlignedAddress == 0) {
+ if (Index < ProcTraceData->AllocatedThreads) {
+ ProcTraceData->AllocatedThreads = Index;
+ }
+
+ DEBUG ((DEBUG_ERROR, "ProcTrace: Out of mem, allocated ToPA mem only for %d threads\n", ProcTraceData->AllocatedThreads));
+ if (Index == 0) {
+ //
+ // Could not allocate for BSP even
+ //
+ FreePool ((VOID *)TopaMemArray);
+ TopaMemArray = NULL;
+ return RETURN_OUT_OF_RESOURCES;
+ }
+
+ break;
}
- break;
+ TopaMemArray[Index] = AlignedAddress;
+ DEBUG ((DEBUG_INFO, "ProcTrace: Topa table address(aligned) for thread %d is 0x%llX \n", Index, (UINT64)TopaMemArray[Index]));
}
- TopaMemArray[Index] = AlignedAddress;
- DEBUG ((DEBUG_INFO, "ProcTrace: Topa table address(aligned) for thread %d is 0x%llX \n", Index, (UINT64)TopaMemArray[Index]));
+ DEBUG ((DEBUG_INFO, "ProcTrace: Allocated ToPA mem for %d thread \n", ProcTraceData->AllocatedThreads));
}
-
- DEBUG ((DEBUG_INFO, "ProcTrace: Allocated ToPA mem for %d thread \n", ProcTraceData->AllocatedThreads));
}
- if (ProcessorNumber < ProcTraceData->AllocatedThreads) {
- TopaTableBaseAddr = ProcTraceData->TopaMemArray[ProcessorNumber];
- } else {
- return RETURN_SUCCESS;
+ if (!EnableOnBspOnly) {
+ if (ProcessorNumber < ProcTraceData->AllocatedThreads) {
+ TopaTableBaseAddr = ProcTraceData->TopaMemArray[ProcessorNumber];
+ } else {
+ return RETURN_SUCCESS;
+ }
}
TopaTable = (PROC_TRACE_TOPA_TABLE *)TopaTableBaseAddr;
diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index a5528277ff..1a4b9333ab 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -422,5 +422,12 @@
# @Prompt GHCB Hypervisor Features
gUefiCpuPkgTokenSpaceGuid.PcdGhcbHypervisorFeatures|0x0|UINT64|0x60000018
+ ## This PCD indicates whether CPU processor trace is enabled on BSP only when CPU processor trace is enabled.<BR><BR>
+ # This PCD is ignored if CPU processor trace is disabled.<BR><BR>
+ # TRUE - CPU processor trace is enabled on BSP only.<BR>
+ # FASLE - CPU processor trace is enabled on all CPU.<BR>
+ # @Prompt Enable CPU processor trace only on BSP.
+ gUefiCpuPkgTokenSpaceGuid.PcdEnableProcessorTraceOnBspOnly|FALSE|BOOLEAN|0x60000019
+
[UserExtensions.TianoCore."ExtraFiles"]
UefiCpuPkgExtra.uni
--
2.39.1.windows.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] UefiCpuPkg: Update PT code to support enable collect performance
2023-04-25 5:47 [PATCH 0/3] Update ProcTrace feature code for new requirements duntan
2023-04-25 5:47 ` [PATCH 1/3] UefiCpuPkg: Update code to support enable ProcTrace only on BSP duntan
@ 2023-04-25 5:47 ` duntan
2023-04-25 6:13 ` Ni, Ray
2023-04-25 5:47 ` [PATCH 3/3] UefiCpuPkg: Disable MTC packet by default duntan
2 siblings, 1 reply; 11+ messages in thread
From: duntan @ 2023-04-25 5:47 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann, Xiao X Chen
Update ProcTrace feature code to support enable collect performance
data by generating CYC and TSC packets. Add a new dynamic
PCD to indicate if enable performance collecting. In ProcTrace.c
code, if this new PCD is true, CYC and TSC packets will be
generated by setting the corresponding MSR bits feilds.
Bugzila: https://bugzilla.tianocore.org/show_bug.cgi?id=4423
Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Xiao X Chen <xiao.x.chen@intel.com>
---
UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf | 1 +
UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++-------------------
UefiCpuPkg/UefiCpuPkg.dec | 8 ++++++++
3 files changed, 56 insertions(+), 19 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
index 319c8b4842..e31c1e7317 100644
--- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
+++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
@@ -63,3 +63,4 @@
gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme ## SOMETIMES_CONSUMES
gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize ## SOMETIMES_CONSUMES
gUefiCpuPkgTokenSpaceGuid.PcdEnableProcessorTraceOnBspOnly ## SOMETIMES_CONSUMES
+ gUefiCpuPkgTokenSpaceGuid.ProcTraceEnablePerformanceCollecting ## SOMETIMES_CONSUMES
diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
index f57544bf7d..1a101b7288 100644
--- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
+++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
@@ -171,25 +171,26 @@ ProcTraceInitialize (
IN BOOLEAN State
)
{
- UINT32 MemRegionSize;
- UINTN Pages;
- UINTN Alignment;
- UINTN MemRegionBaseAddr;
- UINTN *ThreadMemRegionTable;
- UINTN Index;
- UINTN TopaTableBaseAddr;
- UINTN AlignedAddress;
- UINTN *TopaMemArray;
- PROC_TRACE_TOPA_TABLE *TopaTable;
- PROC_TRACE_DATA *ProcTraceData;
- BOOLEAN FirstIn;
- MSR_IA32_RTIT_CTL_REGISTER CtrlReg;
- MSR_IA32_RTIT_STATUS_REGISTER StatusReg;
- MSR_IA32_RTIT_OUTPUT_BASE_REGISTER OutputBaseReg;
- MSR_IA32_RTIT_OUTPUT_MASK_PTRS_REGISTER OutputMaskPtrsReg;
- RTIT_TOPA_TABLE_ENTRY *TopaEntryPtr;
- BOOLEAN IsBsp;
- BOOLEAN EnableOnBspOnly;
+ UINT32 MemRegionSize;
+ UINTN Pages;
+ UINTN Alignment;
+ UINTN MemRegionBaseAddr;
+ UINTN *ThreadMemRegionTable;
+ UINTN Index;
+ UINTN TopaTableBaseAddr;
+ UINTN AlignedAddress;
+ UINTN *TopaMemArray;
+ PROC_TRACE_TOPA_TABLE *TopaTable;
+ PROC_TRACE_DATA *ProcTraceData;
+ BOOLEAN FirstIn;
+ MSR_IA32_RTIT_CTL_REGISTER CtrlReg;
+ MSR_IA32_RTIT_STATUS_REGISTER StatusReg;
+ MSR_IA32_RTIT_OUTPUT_BASE_REGISTER OutputBaseReg;
+ MSR_IA32_RTIT_OUTPUT_MASK_PTRS_REGISTER OutputMaskPtrsReg;
+ RTIT_TOPA_TABLE_ENTRY *TopaEntryPtr;
+ BOOLEAN IsBsp;
+ BOOLEAN EnableOnBspOnly;
+ CPUID_INTEL_PROCESSOR_TRACE_MAIN_LEAF_EBX Ebx;
//
// The scope of the MSR_IA32_RTIT_* is core for below processor type, only program
@@ -510,6 +511,33 @@ ProcTraceInitialize (
CtrlReg.Bits.User = 1;
CtrlReg.Bits.BranchEn = 1;
CtrlReg.Bits.TraceEn = 1;
+
+ AsmCpuidEx (CPUID_INTEL_PROCESSOR_TRACE, CPUID_INTEL_PROCESSOR_TRACE_MAIN_LEAF, NULL, &Ebx.Uint32, NULL, NULL);
+
+ //
+ // Generate CYC/TSC timing packets to to collect performance data.
+ //
+ if (PcdGetBool (ProcTraceEnablePerformanceCollecting)) {
+ if (Ebx.Bits.ConfigurablePsb == 1) {
+ CtrlReg.Bits.CYCEn = 1;
+ CtrlReg.Bits.CYCThresh = 5;
+
+ //
+ // Write to TSCEn is always supported
+ //
+ CtrlReg.Bits.TSCEn = 1;
+ } else {
+ DEBUG ((DEBUG_INFO, "ProcTrace: CYC packet is not supported. Failed to enable Performance Collecting \n"));
+ }
+ } else {
+ if (Ebx.Bits.ConfigurablePsb == 1) {
+ CtrlReg.Bits.CYCEn = 0;
+ CtrlReg.Bits.CYCThresh = 0;
+ }
+
+ CtrlReg.Bits.TSCEn = 0;
+ }
+
CPU_REGISTER_TABLE_WRITE64 (
ProcessorNumber,
Msr,
diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index 1a4b9333ab..2b0de6d5c3 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -429,5 +429,13 @@
# @Prompt Enable CPU processor trace only on BSP.
gUefiCpuPkgTokenSpaceGuid.PcdEnableProcessorTraceOnBspOnly|FALSE|BOOLEAN|0x60000019
+ ## This PCD indicates if enable performance collecting when CPU processor trace is enabled.<BR><BR>
+ # CYC/TSC timing packets will be generated to collect performance data if this PCD is TRUE.
+ # This PCD is ignored if CPU processor trace is disabled.<BR><BR>
+ # TRUE - Performance collecting will be enabled in processor trace.<BR>
+ # FASLE - Performance collecting will be disabled in processor trace.<BR>
+ # @Prompt Enable performance collecting when processor trace is enabled.
+ gUefiCpuPkgTokenSpaceGuid.ProcTraceEnablePerformanceCollecting|FALSE|BOOLEAN|0x60000020
+
[UserExtensions.TianoCore."ExtraFiles"]
UefiCpuPkgExtra.uni
--
2.39.1.windows.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] UefiCpuPkg: Disable MTC packet by default
2023-04-25 5:47 [PATCH 0/3] Update ProcTrace feature code for new requirements duntan
2023-04-25 5:47 ` [PATCH 1/3] UefiCpuPkg: Update code to support enable ProcTrace only on BSP duntan
2023-04-25 5:47 ` [PATCH 2/3] UefiCpuPkg: Update PT code to support enable collect performance duntan
@ 2023-04-25 5:47 ` duntan
2023-04-25 6:14 ` [edk2-devel] " Ni, Ray
2 siblings, 1 reply; 11+ messages in thread
From: duntan @ 2023-04-25 5:47 UTC (permalink / raw)
To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar, Gerd Hoffmann, Xiao X Chen
MTC packet is not needed and disable MTC packet by default.
Bugzila: https://bugzilla.tianocore.org/show_bug.cgi?id=4423
Signed-off-by: Dun Tan <dun.tan@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Xiao X Chen <xiao.x.chen@intel.com>
---
UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
index 1a101b7288..3f109bf29c 100644
--- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
+++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
@@ -538,6 +538,14 @@ ProcTraceInitialize (
CtrlReg.Bits.TSCEn = 0;
}
+ //
+ // MTC packet is not needed. Turn it off by default.
+ //
+ if (Ebx.Bits.Mtc == 1) {
+ CtrlReg.Bits.MTCEn = 0;
+ CtrlReg.Bits.MTCFreq = 0;
+ }
+
CPU_REGISTER_TABLE_WRITE64 (
ProcessorNumber,
Msr,
--
2.39.1.windows.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] UefiCpuPkg: Update code to support enable ProcTrace only on BSP
2023-04-25 5:47 ` [PATCH 1/3] UefiCpuPkg: Update code to support enable ProcTrace only on BSP duntan
@ 2023-04-25 6:03 ` Ni, Ray
[not found] ` <1759183F15FD97B2.10313@groups.io>
1 sibling, 0 replies; 11+ messages in thread
From: Ni, Ray @ 2023-04-25 6:03 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io
Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann, Chen, Xiao X
> - gUefiCpuPkgTokenSpaceGuid.PcdCpuClockModulationDutyCycle ##
> SOMETIMES_CONSUMES
> - gUefiCpuPkgTokenSpaceGuid.PcdIsPowerOnReset ##
> SOMETIMES_CONSUMES
> - gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme ##
> SOMETIMES_CONSUMES
> - gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize ##
> SOMETIMES_CONSUMES
> + gUefiCpuPkgTokenSpaceGuid.PcdCpuClockModulationDutyCycle ##
> SOMETIMES_CONSUMES
> + gUefiCpuPkgTokenSpaceGuid.PcdIsPowerOnReset ##
> SOMETIMES_CONSUMES
> + gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme ##
> SOMETIMES_CONSUMES
> + gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize ##
> SOMETIMES_CONSUMES
> + gUefiCpuPkgTokenSpaceGuid.PcdEnableProcessorTraceOnBspOnly ##
1. PcdCpuProcTraceBspOnly? This is to use the same prefix as other ProcTrace PCDs.
>
> + IsBsp = (CpuInfo->ProcessorInfo.StatusFlag & BIT0) ? TRUE : FALSE;
2. Can you use PROCESSOR_AS_BSP_BIT instead of BIT0?
> + EnableOnBspOnly = (PcdGetBool (PcdEnableProcessorTraceOnBspOnly)) ?
> TRUE : FALSE;
3. why not directly use PcdGetBool (PcdEnableProcessorTraceOnBspOnly) in below if-check?
> +
> + if (EnableOnBspOnly && (IsBsp == FALSE)) {
4. Change "IsBsp == FALSE" to "!IsBsp".
> + if (EnableOnBspOnly) {
> AlignedAddress = (UINTN)AllocateAlignedReservedPages (Pages,
> Alignment);
> if (AlignedAddress == 0) {
> - DEBUG ((DEBUG_ERROR, "ProcTrace: Out of mem, allocated only for %d
> threads\n", ProcTraceData->AllocatedThreads));
> - if (Index == 0) {
> - //
> - // Could not allocate for BSP even
> - //
> - FreePool ((VOID *)ThreadMemRegionTable);
> - ThreadMemRegionTable = NULL;
> - return RETURN_OUT_OF_RESOURCES;
> + //
> + // Could not allocate for BSP even
> + //
> + DEBUG ((DEBUG_ERROR, "ProcTrace: Out of mem, failed to allocate
> buffer for BSP\n"));
> + return RETURN_OUT_OF_RESOURCES;
> + }
> +
> + DEBUG ((DEBUG_INFO, "ProcTrace: Allocated PT mem for BSP only.\n"));
5. AlignedAddress is the proc trace buffer for BSP. But it seems the value is not used in
later MSR_IA32_RTIT_OUTPUT_BASE programming.
Similar comments to the Topa code path.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Update code to support enable ProcTrace only on BSP
[not found] ` <1759183F15FD97B2.10313@groups.io>
@ 2023-04-25 6:05 ` Ni, Ray
2023-04-25 6:45 ` duntan
0 siblings, 1 reply; 11+ messages in thread
From: Ni, Ray @ 2023-04-25 6:05 UTC (permalink / raw)
To: devel@edk2.groups.io, Ni, Ray, Tan, Dun
Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann, Chen, Xiao X
By the way, I know that ProcTraceInitialize() runs on BSP so it's safe to access the PCD database.
But to be consistent with the old logic, can you please cache the PCD value in ProcTraceGetConfigData()?
It has slight benefit that the PCD is only accessed once not multiple times depending on cpu count.
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Tuesday, April 25, 2023 2:03 PM
> To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Chen,
> Xiao X <xiao.x.chen@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Update code to support
> enable ProcTrace only on BSP
>
> > - gUefiCpuPkgTokenSpaceGuid.PcdCpuClockModulationDutyCycle ##
> > SOMETIMES_CONSUMES
> > - gUefiCpuPkgTokenSpaceGuid.PcdIsPowerOnReset ##
> > SOMETIMES_CONSUMES
> > - gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme ##
> > SOMETIMES_CONSUMES
> > - gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize ##
> > SOMETIMES_CONSUMES
> > + gUefiCpuPkgTokenSpaceGuid.PcdCpuClockModulationDutyCycle ##
> > SOMETIMES_CONSUMES
> > + gUefiCpuPkgTokenSpaceGuid.PcdIsPowerOnReset ##
> > SOMETIMES_CONSUMES
> > + gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme ##
> > SOMETIMES_CONSUMES
> > + gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize ##
> > SOMETIMES_CONSUMES
> > + gUefiCpuPkgTokenSpaceGuid.PcdEnableProcessorTraceOnBspOnly ##
>
> 1. PcdCpuProcTraceBspOnly? This is to use the same prefix as other
> ProcTrace PCDs.
>
>
> >
> > + IsBsp = (CpuInfo->ProcessorInfo.StatusFlag & BIT0) ? TRUE : FALSE;
> 2. Can you use PROCESSOR_AS_BSP_BIT instead of BIT0?
>
>
> > + EnableOnBspOnly = (PcdGetBool (PcdEnableProcessorTraceOnBspOnly)) ?
> > TRUE : FALSE;
>
> 3. why not directly use PcdGetBool (PcdEnableProcessorTraceOnBspOnly) in
> below if-check?
>
> > +
> > + if (EnableOnBspOnly && (IsBsp == FALSE)) {
>
> 4. Change "IsBsp == FALSE" to "!IsBsp".
>
>
>
>
> > + if (EnableOnBspOnly) {
> > AlignedAddress = (UINTN)AllocateAlignedReservedPages (Pages,
> > Alignment);
> > if (AlignedAddress == 0) {
> > - DEBUG ((DEBUG_ERROR, "ProcTrace: Out of mem, allocated only
> for %d
> > threads\n", ProcTraceData->AllocatedThreads));
> > - if (Index == 0) {
> > - //
> > - // Could not allocate for BSP even
> > - //
> > - FreePool ((VOID *)ThreadMemRegionTable);
> > - ThreadMemRegionTable = NULL;
> > - return RETURN_OUT_OF_RESOURCES;
> > + //
> > + // Could not allocate for BSP even
> > + //
> > + DEBUG ((DEBUG_ERROR, "ProcTrace: Out of mem, failed to allocate
> > buffer for BSP\n"));
> > + return RETURN_OUT_OF_RESOURCES;
> > + }
> > +
> > + DEBUG ((DEBUG_INFO, "ProcTrace: Allocated PT mem for BSP
> only.\n"));
>
> 5. AlignedAddress is the proc trace buffer for BSP. But it seems the value is
> not used in
> later MSR_IA32_RTIT_OUTPUT_BASE programming.
> Similar comments to the Topa code path.
>
>
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] UefiCpuPkg: Update PT code to support enable collect performance
2023-04-25 5:47 ` [PATCH 2/3] UefiCpuPkg: Update PT code to support enable collect performance duntan
@ 2023-04-25 6:13 ` Ni, Ray
2023-04-25 6:45 ` duntan
0 siblings, 1 reply; 11+ messages in thread
From: Ni, Ray @ 2023-04-25 6:13 UTC (permalink / raw)
To: Tan, Dun, devel@edk2.groups.io
Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann, Chen, Xiao X
1. ProcessorTrace capabilities might differ in different cpu threads.
So, the capabilities detection needs to be done in ProcTraceSupport() which runs on each AP.
2. TSCEn is not guarded by Ebx.Bits.ConfigurablePsb, right?
3. Why do you need to clear the CYCEn/CYCThresh/TSCEn bit when the ProcTraceEnablePerformanceCollecting is FALSE?
4. Please use a PCD name that's consistent with existing ones. Such as: PcdCpuProcTracePerformanceCollecting?
> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Tuesday, April 25, 2023 1:48 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> Chen, Xiao X <xiao.x.chen@intel.com>
> Subject: [PATCH 2/3] UefiCpuPkg: Update PT code to support enable collect
> performance
>
> Update ProcTrace feature code to support enable collect performance
> data by generating CYC and TSC packets. Add a new dynamic
> PCD to indicate if enable performance collecting. In ProcTrace.c
> code, if this new PCD is true, CYC and TSC packets will be
> generated by setting the corresponding MSR bits feilds.
>
> Bugzila: https://bugzilla.tianocore.org/show_bug.cgi?id=4423
> Signed-off-by: Dun Tan <dun.tan@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Xiao X Chen <xiao.x.chen@intel.com>
> ---
> UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf |
> 1 +
> UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c | 66
> +++++++++++++++++++++++++++++++++++++++++++++++------------------
> -
> UefiCpuPkg/UefiCpuPkg.dec | 8 ++++++++
> 3 files changed, 56 insertions(+), 19 deletions(-)
>
> diff --git
> a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
> b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
> index 319c8b4842..e31c1e7317 100644
> ---
> a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
> +++
> b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
> @@ -63,3 +63,4 @@
> gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme ##
> SOMETIMES_CONSUMES
> gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize ##
> SOMETIMES_CONSUMES
> gUefiCpuPkgTokenSpaceGuid.PcdEnableProcessorTraceOnBspOnly ##
> SOMETIMES_CONSUMES
> + gUefiCpuPkgTokenSpaceGuid.ProcTraceEnablePerformanceCollecting ##
> SOMETIMES_CONSUMES
> diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> index f57544bf7d..1a101b7288 100644
> --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> @@ -171,25 +171,26 @@ ProcTraceInitialize (
> IN BOOLEAN State
> )
> {
> - UINT32 MemRegionSize;
> - UINTN Pages;
> - UINTN Alignment;
> - UINTN MemRegionBaseAddr;
> - UINTN *ThreadMemRegionTable;
> - UINTN Index;
> - UINTN TopaTableBaseAddr;
> - UINTN AlignedAddress;
> - UINTN *TopaMemArray;
> - PROC_TRACE_TOPA_TABLE *TopaTable;
> - PROC_TRACE_DATA *ProcTraceData;
> - BOOLEAN FirstIn;
> - MSR_IA32_RTIT_CTL_REGISTER CtrlReg;
> - MSR_IA32_RTIT_STATUS_REGISTER StatusReg;
> - MSR_IA32_RTIT_OUTPUT_BASE_REGISTER OutputBaseReg;
> - MSR_IA32_RTIT_OUTPUT_MASK_PTRS_REGISTER OutputMaskPtrsReg;
> - RTIT_TOPA_TABLE_ENTRY *TopaEntryPtr;
> - BOOLEAN IsBsp;
> - BOOLEAN EnableOnBspOnly;
> + UINT32 MemRegionSize;
> + UINTN Pages;
> + UINTN Alignment;
> + UINTN MemRegionBaseAddr;
> + UINTN *ThreadMemRegionTable;
> + UINTN Index;
> + UINTN TopaTableBaseAddr;
> + UINTN AlignedAddress;
> + UINTN *TopaMemArray;
> + PROC_TRACE_TOPA_TABLE *TopaTable;
> + PROC_TRACE_DATA *ProcTraceData;
> + BOOLEAN FirstIn;
> + MSR_IA32_RTIT_CTL_REGISTER CtrlReg;
> + MSR_IA32_RTIT_STATUS_REGISTER StatusReg;
> + MSR_IA32_RTIT_OUTPUT_BASE_REGISTER OutputBaseReg;
> + MSR_IA32_RTIT_OUTPUT_MASK_PTRS_REGISTER OutputMaskPtrsReg;
> + RTIT_TOPA_TABLE_ENTRY *TopaEntryPtr;
> + BOOLEAN IsBsp;
> + BOOLEAN EnableOnBspOnly;
> + CPUID_INTEL_PROCESSOR_TRACE_MAIN_LEAF_EBX Ebx;
>
> //
> // The scope of the MSR_IA32_RTIT_* is core for below processor type,
> only program
> @@ -510,6 +511,33 @@ ProcTraceInitialize (
> CtrlReg.Bits.User = 1;
> CtrlReg.Bits.BranchEn = 1;
> CtrlReg.Bits.TraceEn = 1;
> +
> + AsmCpuidEx (CPUID_INTEL_PROCESSOR_TRACE,
> CPUID_INTEL_PROCESSOR_TRACE_MAIN_LEAF, NULL, &Ebx.Uint32, NULL,
> NULL);
> +
> + //
> + // Generate CYC/TSC timing packets to to collect performance data.
> + //
> + if (PcdGetBool (ProcTraceEnablePerformanceCollecting)) {
> + if (Ebx.Bits.ConfigurablePsb == 1) {
> + CtrlReg.Bits.CYCEn = 1;
> + CtrlReg.Bits.CYCThresh = 5;
> +
> + //
> + // Write to TSCEn is always supported
> + //
> + CtrlReg.Bits.TSCEn = 1;
> + } else {
> + DEBUG ((DEBUG_INFO, "ProcTrace: CYC packet is not supported. Failed
> to enable Performance Collecting \n"));
> + }
> + } else {
> + if (Ebx.Bits.ConfigurablePsb == 1) {
> + CtrlReg.Bits.CYCEn = 0;
> + CtrlReg.Bits.CYCThresh = 0;
> + }
> +
> + CtrlReg.Bits.TSCEn = 0;
> + }
> +
> CPU_REGISTER_TABLE_WRITE64 (
> ProcessorNumber,
> Msr,
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index 1a4b9333ab..2b0de6d5c3 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -429,5 +429,13 @@
> # @Prompt Enable CPU processor trace only on BSP.
>
> gUefiCpuPkgTokenSpaceGuid.PcdEnableProcessorTraceOnBspOnly|FALSE|B
> OOLEAN|0x60000019
>
> + ## This PCD indicates if enable performance collecting when CPU
> processor trace is enabled.<BR><BR>
> + # CYC/TSC timing packets will be generated to collect performance data if
> this PCD is TRUE.
> + # This PCD is ignored if CPU processor trace is disabled.<BR><BR>
> + # TRUE - Performance collecting will be enabled in processor trace.<BR>
> + # FASLE - Performance collecting will be disabled in processor trace.<BR>
> + # @Prompt Enable performance collecting when processor trace is
> enabled.
> +
> gUefiCpuPkgTokenSpaceGuid.ProcTraceEnablePerformanceCollecting|FALSE
> |BOOLEAN|0x60000020
> +
> [UserExtensions.TianoCore."ExtraFiles"]
> UefiCpuPkgExtra.uni
> --
> 2.39.1.windows.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] UefiCpuPkg: Disable MTC packet by default
2023-04-25 5:47 ` [PATCH 3/3] UefiCpuPkg: Disable MTC packet by default duntan
@ 2023-04-25 6:14 ` Ni, Ray
2023-04-25 6:45 ` duntan
0 siblings, 1 reply; 11+ messages in thread
From: Ni, Ray @ 2023-04-25 6:14 UTC (permalink / raw)
To: devel@edk2.groups.io, Tan, Dun
Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann, Chen, Xiao X
Why do you need to explicitly disable MTC bit?
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of duntan
> Sent: Tuesday, April 25, 2023 1:48 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> Chen, Xiao X <xiao.x.chen@intel.com>
> Subject: [edk2-devel] [PATCH 3/3] UefiCpuPkg: Disable MTC packet by
> default
>
> MTC packet is not needed and disable MTC packet by default.
>
> Bugzila: https://bugzilla.tianocore.org/show_bug.cgi?id=4423
> Signed-off-by: Dun Tan <dun.tan@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Xiao X Chen <xiao.x.chen@intel.com>
> ---
> UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> index 1a101b7288..3f109bf29c 100644
> --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> @@ -538,6 +538,14 @@ ProcTraceInitialize (
> CtrlReg.Bits.TSCEn = 0;
> }
>
> + //
> + // MTC packet is not needed. Turn it off by default.
> + //
> + if (Ebx.Bits.Mtc == 1) {
> + CtrlReg.Bits.MTCEn = 0;
> + CtrlReg.Bits.MTCFreq = 0;
> + }
> +
> CPU_REGISTER_TABLE_WRITE64 (
> ProcessorNumber,
> Msr,
> --
> 2.39.1.windows.1
>
>
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Update code to support enable ProcTrace only on BSP
2023-04-25 6:05 ` [edk2-devel] " Ni, Ray
@ 2023-04-25 6:45 ` duntan
0 siblings, 0 replies; 11+ messages in thread
From: duntan @ 2023-04-25 6:45 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io
Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann, Chen, Xiao X
Hi Ray,
Thanks for comments. I Will update the code based your comments for this patch.
Thanks,
Dun
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, April 25, 2023 2:06 PM
To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Tan, Dun <dun.tan@intel.com>
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Chen, Xiao X <xiao.x.chen@intel.com>
Subject: RE: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Update code to support enable ProcTrace only on BSP
By the way, I know that ProcTraceInitialize() runs on BSP so it's safe to access the PCD database.
But to be consistent with the old logic, can you please cache the PCD value in ProcTraceGetConfigData()?
It has slight benefit that the PCD is only accessed once not multiple times depending on cpu count.
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Tuesday, April 25, 2023 2:03 PM
> To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Chen,
> Xiao X <xiao.x.chen@intel.com>
> Subject: Re: [edk2-devel] [PATCH 1/3] UefiCpuPkg: Update code to
> support enable ProcTrace only on BSP
>
> > - gUefiCpuPkgTokenSpaceGuid.PcdCpuClockModulationDutyCycle ##
> > SOMETIMES_CONSUMES
> > - gUefiCpuPkgTokenSpaceGuid.PcdIsPowerOnReset ##
> > SOMETIMES_CONSUMES
> > - gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme ##
> > SOMETIMES_CONSUMES
> > - gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize ##
> > SOMETIMES_CONSUMES
> > + gUefiCpuPkgTokenSpaceGuid.PcdCpuClockModulationDutyCycle ##
> > SOMETIMES_CONSUMES
> > + gUefiCpuPkgTokenSpaceGuid.PcdIsPowerOnReset ##
> > SOMETIMES_CONSUMES
> > + gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme ##
> > SOMETIMES_CONSUMES
> > + gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize ##
> > SOMETIMES_CONSUMES
> > + gUefiCpuPkgTokenSpaceGuid.PcdEnableProcessorTraceOnBspOnly ##
>
> 1. PcdCpuProcTraceBspOnly? This is to use the same prefix as other
> ProcTrace PCDs.
>
>
> >
> > + IsBsp = (CpuInfo->ProcessorInfo.StatusFlag & BIT0) ? TRUE : FALSE;
> 2. Can you use PROCESSOR_AS_BSP_BIT instead of BIT0?
>
>
> > + EnableOnBspOnly = (PcdGetBool (PcdEnableProcessorTraceOnBspOnly)) ?
> > TRUE : FALSE;
>
> 3. why not directly use PcdGetBool (PcdEnableProcessorTraceOnBspOnly)
> in below if-check?
>
> > +
> > + if (EnableOnBspOnly && (IsBsp == FALSE)) {
>
> 4. Change "IsBsp == FALSE" to "!IsBsp".
>
>
>
>
> > + if (EnableOnBspOnly) {
> > AlignedAddress = (UINTN)AllocateAlignedReservedPages (Pages,
> > Alignment);
> > if (AlignedAddress == 0) {
> > - DEBUG ((DEBUG_ERROR, "ProcTrace: Out of mem, allocated only
> for %d
> > threads\n", ProcTraceData->AllocatedThreads));
> > - if (Index == 0) {
> > - //
> > - // Could not allocate for BSP even
> > - //
> > - FreePool ((VOID *)ThreadMemRegionTable);
> > - ThreadMemRegionTable = NULL;
> > - return RETURN_OUT_OF_RESOURCES;
> > + //
> > + // Could not allocate for BSP even
> > + //
> > + DEBUG ((DEBUG_ERROR, "ProcTrace: Out of mem, failed to
> > + allocate
> > buffer for BSP\n"));
> > + return RETURN_OUT_OF_RESOURCES;
> > + }
> > +
> > + DEBUG ((DEBUG_INFO, "ProcTrace: Allocated PT mem for BSP
> only.\n"));
>
> 5. AlignedAddress is the proc trace buffer for BSP. But it seems the
> value is not used in
> later MSR_IA32_RTIT_OUTPUT_BASE programming.
> Similar comments to the Topa code path.
>
>
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] UefiCpuPkg: Update PT code to support enable collect performance
2023-04-25 6:13 ` Ni, Ray
@ 2023-04-25 6:45 ` duntan
0 siblings, 0 replies; 11+ messages in thread
From: duntan @ 2023-04-25 6:45 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io
Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann, Chen, Xiao X
Thanks for the comments. Updated inline.
Thanks,
Dun
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, April 25, 2023 2:14 PM
To: Tan, Dun <dun.tan@intel.com>; devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Chen, Xiao X <xiao.x.chen@intel.com>
Subject: RE: [PATCH 2/3] UefiCpuPkg: Update PT code to support enable collect performance
1. ProcessorTrace capabilities might differ in different cpu threads.
So, the capabilities detection needs to be done in ProcTraceSupport() which runs on each AP.
Dun: Sure, I'll add two new fields in PROC_TRACE_DATA and update the fields in ProcTraceSupport().
2. TSCEn is not guarded by Ebx.Bits.ConfigurablePsb, right?
Dun: Yes, Ebx.Bits.ConfigurablePsb is only for CYC. TSCEn is always supported. But I think if CYC is not supported, we don't need to enable CYC, right?
3. Why do you need to clear the CYCEn/CYCThresh/TSCEn bit when the ProcTraceEnablePerformanceCollecting is FALSE?
Dun: Seems this part is not needed. I'll remove the code that clear the the CYCEn/CYCThresh/TSCEn bits.
4. Please use a PCD name that's consistent with existing ones. Such as: PcdCpuProcTracePerformanceCollecting?
Dun: Ok, I'll update it.
> -----Original Message-----
> From: Tan, Dun <dun.tan@intel.com>
> Sent: Tuesday, April 25, 2023 1:48 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Chen, Xiao X <xiao.x.chen@intel.com>
> Subject: [PATCH 2/3] UefiCpuPkg: Update PT code to support enable
> collect performance
>
> Update ProcTrace feature code to support enable collect performance
> data by generating CYC and TSC packets. Add a new dynamic PCD to
> indicate if enable performance collecting. In ProcTrace.c code, if
> this new PCD is true, CYC and TSC packets will be generated by setting
> the corresponding MSR bits feilds.
>
> Bugzila: https://bugzilla.tianocore.org/show_bug.cgi?id=4423
> Signed-off-by: Dun Tan <dun.tan@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Xiao X Chen <xiao.x.chen@intel.com>
> ---
> UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf |
> 1 +
> UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c | 66
> +++++++++++++++++++++++++++++++++++++++++++++++------------------
> -
> UefiCpuPkg/UefiCpuPkg.dec | 8 ++++++++
> 3 files changed, 56 insertions(+), 19 deletions(-)
>
> diff --git
> a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
> b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
> index 319c8b4842..e31c1e7317 100644
> ---
> a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
> +++
> b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
> @@ -63,3 +63,4 @@
> gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme ##
> SOMETIMES_CONSUMES
> gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize ##
> SOMETIMES_CONSUMES
> gUefiCpuPkgTokenSpaceGuid.PcdEnableProcessorTraceOnBspOnly ##
> SOMETIMES_CONSUMES
> + gUefiCpuPkgTokenSpaceGuid.ProcTraceEnablePerformanceCollecting ##
> SOMETIMES_CONSUMES
> diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> index f57544bf7d..1a101b7288 100644
> --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> @@ -171,25 +171,26 @@ ProcTraceInitialize (
> IN BOOLEAN State
> )
> {
> - UINT32 MemRegionSize;
> - UINTN Pages;
> - UINTN Alignment;
> - UINTN MemRegionBaseAddr;
> - UINTN *ThreadMemRegionTable;
> - UINTN Index;
> - UINTN TopaTableBaseAddr;
> - UINTN AlignedAddress;
> - UINTN *TopaMemArray;
> - PROC_TRACE_TOPA_TABLE *TopaTable;
> - PROC_TRACE_DATA *ProcTraceData;
> - BOOLEAN FirstIn;
> - MSR_IA32_RTIT_CTL_REGISTER CtrlReg;
> - MSR_IA32_RTIT_STATUS_REGISTER StatusReg;
> - MSR_IA32_RTIT_OUTPUT_BASE_REGISTER OutputBaseReg;
> - MSR_IA32_RTIT_OUTPUT_MASK_PTRS_REGISTER OutputMaskPtrsReg;
> - RTIT_TOPA_TABLE_ENTRY *TopaEntryPtr;
> - BOOLEAN IsBsp;
> - BOOLEAN EnableOnBspOnly;
> + UINT32 MemRegionSize;
> + UINTN Pages;
> + UINTN Alignment;
> + UINTN MemRegionBaseAddr;
> + UINTN *ThreadMemRegionTable;
> + UINTN Index;
> + UINTN TopaTableBaseAddr;
> + UINTN AlignedAddress;
> + UINTN *TopaMemArray;
> + PROC_TRACE_TOPA_TABLE *TopaTable;
> + PROC_TRACE_DATA *ProcTraceData;
> + BOOLEAN FirstIn;
> + MSR_IA32_RTIT_CTL_REGISTER CtrlReg;
> + MSR_IA32_RTIT_STATUS_REGISTER StatusReg;
> + MSR_IA32_RTIT_OUTPUT_BASE_REGISTER OutputBaseReg;
> + MSR_IA32_RTIT_OUTPUT_MASK_PTRS_REGISTER OutputMaskPtrsReg;
> + RTIT_TOPA_TABLE_ENTRY *TopaEntryPtr;
> + BOOLEAN IsBsp;
> + BOOLEAN EnableOnBspOnly;
> + CPUID_INTEL_PROCESSOR_TRACE_MAIN_LEAF_EBX Ebx;
>
> //
> // The scope of the MSR_IA32_RTIT_* is core for below processor
> type, only program @@ -510,6 +511,33 @@ ProcTraceInitialize (
> CtrlReg.Bits.User = 1;
> CtrlReg.Bits.BranchEn = 1;
> CtrlReg.Bits.TraceEn = 1;
> +
> + AsmCpuidEx (CPUID_INTEL_PROCESSOR_TRACE,
> CPUID_INTEL_PROCESSOR_TRACE_MAIN_LEAF, NULL, &Ebx.Uint32, NULL, NULL);
> +
> + //
> + // Generate CYC/TSC timing packets to to collect performance data.
> + //
> + if (PcdGetBool (ProcTraceEnablePerformanceCollecting)) {
> + if (Ebx.Bits.ConfigurablePsb == 1) {
> + CtrlReg.Bits.CYCEn = 1;
> + CtrlReg.Bits.CYCThresh = 5;
> +
> + //
> + // Write to TSCEn is always supported
> + //
> + CtrlReg.Bits.TSCEn = 1;
> + } else {
> + DEBUG ((DEBUG_INFO, "ProcTrace: CYC packet is not supported.
> + Failed
> to enable Performance Collecting \n"));
> + }
> + } else {
> + if (Ebx.Bits.ConfigurablePsb == 1) {
> + CtrlReg.Bits.CYCEn = 0;
> + CtrlReg.Bits.CYCThresh = 0;
> + }
> +
> + CtrlReg.Bits.TSCEn = 0;
> + }
> +
> CPU_REGISTER_TABLE_WRITE64 (
> ProcessorNumber,
> Msr,
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> index 1a4b9333ab..2b0de6d5c3 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -429,5 +429,13 @@
> # @Prompt Enable CPU processor trace only on BSP.
>
> gUefiCpuPkgTokenSpaceGuid.PcdEnableProcessorTraceOnBspOnly|FALSE|B
> OOLEAN|0x60000019
>
> + ## This PCD indicates if enable performance collecting when CPU
> processor trace is enabled.<BR><BR>
> + # CYC/TSC timing packets will be generated to collect performance
> + data if
> this PCD is TRUE.
> + # This PCD is ignored if CPU processor trace is disabled.<BR><BR>
> + # TRUE - Performance collecting will be enabled in processor
> + trace.<BR> # FASLE - Performance collecting will be disabled in
> + processor trace.<BR> # @Prompt Enable performance collecting when
> + processor trace is
> enabled.
> +
> gUefiCpuPkgTokenSpaceGuid.ProcTraceEnablePerformanceCollecting|FALSE
> |BOOLEAN|0x60000020
> +
> [UserExtensions.TianoCore."ExtraFiles"]
> UefiCpuPkgExtra.uni
> --
> 2.39.1.windows.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] UefiCpuPkg: Disable MTC packet by default
2023-04-25 6:14 ` [edk2-devel] " Ni, Ray
@ 2023-04-25 6:45 ` duntan
0 siblings, 0 replies; 11+ messages in thread
From: duntan @ 2023-04-25 6:45 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io
Cc: Dong, Eric, Kumar, Rahul R, Gerd Hoffmann, Chen, Xiao X
Because we mentioned before that MTC packet is not needed by default. So I added code to disable MTC bit.
-----Original Message-----
From: Ni, Ray <ray.ni@intel.com>
Sent: Tuesday, April 25, 2023 2:14 PM
To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>
Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Chen, Xiao X <xiao.x.chen@intel.com>
Subject: RE: [edk2-devel] [PATCH 3/3] UefiCpuPkg: Disable MTC packet by default
Why do you need to explicitly disable MTC bit?
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of duntan
> Sent: Tuesday, April 25, 2023 1:48 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Kumar, Rahul R <rahul.r.kumar@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Chen, Xiao X <xiao.x.chen@intel.com>
> Subject: [edk2-devel] [PATCH 3/3] UefiCpuPkg: Disable MTC packet by
> default
>
> MTC packet is not needed and disable MTC packet by default.
>
> Bugzila: https://bugzilla.tianocore.org/show_bug.cgi?id=4423
> Signed-off-by: Dun Tan <dun.tan@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Xiao X Chen <xiao.x.chen@intel.com>
> ---
> UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> index 1a101b7288..3f109bf29c 100644
> --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
> @@ -538,6 +538,14 @@ ProcTraceInitialize (
> CtrlReg.Bits.TSCEn = 0;
> }
>
> + //
> + // MTC packet is not needed. Turn it off by default.
> + //
> + if (Ebx.Bits.Mtc == 1) {
> + CtrlReg.Bits.MTCEn = 0;
> + CtrlReg.Bits.MTCFreq = 0;
> + }
> +
> CPU_REGISTER_TABLE_WRITE64 (
> ProcessorNumber,
> Msr,
> --
> 2.39.1.windows.1
>
>
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-04-25 6:45 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-25 5:47 [PATCH 0/3] Update ProcTrace feature code for new requirements duntan
2023-04-25 5:47 ` [PATCH 1/3] UefiCpuPkg: Update code to support enable ProcTrace only on BSP duntan
2023-04-25 6:03 ` Ni, Ray
[not found] ` <1759183F15FD97B2.10313@groups.io>
2023-04-25 6:05 ` [edk2-devel] " Ni, Ray
2023-04-25 6:45 ` duntan
2023-04-25 5:47 ` [PATCH 2/3] UefiCpuPkg: Update PT code to support enable collect performance duntan
2023-04-25 6:13 ` Ni, Ray
2023-04-25 6:45 ` duntan
2023-04-25 5:47 ` [PATCH 3/3] UefiCpuPkg: Disable MTC packet by default duntan
2023-04-25 6:14 ` [edk2-devel] " Ni, Ray
2023-04-25 6:45 ` duntan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox