* [PATCH 1/2] UefiCpuPkg CpuCommFeaturesLib: Disable TraceEn at the beginning
2019-06-01 8:03 [PATCH 0/2] Fix GP fault issue and enhance code about ProcTrace Zeng, Star
@ 2019-06-01 8:03 ` Zeng, Star
2019-06-01 8:03 ` [PATCH 2/2] UefiCpuPkg CpuCommFeaturesLib: Fix GP fault issue about ProcTrace Zeng, Star
1 sibling, 0 replies; 6+ messages in thread
From: Zeng, Star @ 2019-06-01 8:03 UTC (permalink / raw)
Cc: Star Zeng, Laszlo Ersek, Eric Dong, Ruiyu Ni, Chandana Kumar,
Kevin Li
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1809
Current code disables TraceEn at the end of ProcTraceInitialize(),
then there will be much memory allocated even when ProcTrace feature
is disabled.
This patch updates code to disable TraceEn and return at the beginning
of ProcTraceInitialize() when when ProcTrace feature is disabled.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Chandana Kumar <chandana.c.kumar@intel.com>
Cc: Kevin Li <kevin.y.li@intel.com>
Signed-off-by: Star Zeng <star.zeng@intel.com>
---
.../Library/CpuCommonFeaturesLib/ProcTrace.c | 42 +++++++++----------
1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
index da6f6beb9477..b98eb116b7b6 100644
--- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
+++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
@@ -199,22 +199,6 @@ ProcTraceInitialize (
ProcTraceData = (PROC_TRACE_DATA *) ConfigData;
ASSERT (ProcTraceData != NULL);
- MemRegionBaseAddr = 0;
- FirstIn = FALSE;
-
- if (ProcTraceData->ThreadMemRegionTable == NULL) {
- FirstIn = TRUE;
- DEBUG ((DEBUG_INFO, "Initialize Processor Trace\n"));
- }
-
- ///
- /// Refer to PROC_TRACE_MEM_SIZE Table for Size Encoding
- ///
- MemRegionSize = (UINT32) (1 << (ProcTraceData->ProcTraceMemSize + 12));
- if (FirstIn) {
- DEBUG ((DEBUG_INFO, "ProcTrace: MemSize requested: 0x%X \n", MemRegionSize));
- }
-
//
// Clear MSR_IA32_RTIT_CTL[0] and IA32_RTIT_STS only if MSR_IA32_RTIT_CTL[0]==1b
//
@@ -243,6 +227,26 @@ ProcTraceInitialize (
);
}
+ if (!State) {
+ return RETURN_SUCCESS;
+ }
+
+ MemRegionBaseAddr = 0;
+ FirstIn = FALSE;
+
+ if (ProcTraceData->ThreadMemRegionTable == NULL) {
+ FirstIn = TRUE;
+ DEBUG ((DEBUG_INFO, "Initialize Processor Trace\n"));
+ }
+
+ ///
+ /// Refer to PROC_TRACE_MEM_SIZE Table for Size Encoding
+ ///
+ MemRegionSize = (UINT32) (1 << (ProcTraceData->ProcTraceMemSize + 12));
+ if (FirstIn) {
+ DEBUG ((DEBUG_INFO, "ProcTrace: MemSize requested: 0x%X \n", MemRegionSize));
+ }
+
if (FirstIn) {
//
// Let BSP allocate and create the necessary memory region (Aligned to the size of
@@ -459,11 +463,7 @@ ProcTraceInitialize (
CtrlReg.Bits.OS = 1;
CtrlReg.Bits.User = 1;
CtrlReg.Bits.BranchEn = 1;
- if (!State) {
- CtrlReg.Bits.TraceEn = 0;
- } else {
- CtrlReg.Bits.TraceEn = 1;
- }
+ CtrlReg.Bits.TraceEn = 1;
CPU_REGISTER_TABLE_WRITE64 (
ProcessorNumber,
Msr,
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/2] UefiCpuPkg CpuCommFeaturesLib: Fix GP fault issue about ProcTrace
2019-06-01 8:03 [PATCH 0/2] Fix GP fault issue and enhance code about ProcTrace Zeng, Star
2019-06-01 8:03 ` [PATCH 1/2] UefiCpuPkg CpuCommFeaturesLib: Disable TraceEn at the beginning Zeng, Star
@ 2019-06-01 8:03 ` Zeng, Star
1 sibling, 0 replies; 6+ messages in thread
From: Zeng, Star @ 2019-06-01 8:03 UTC (permalink / raw)
Cc: Star Zeng, Laszlo Ersek, Eric Dong, Ruiyu Ni, Chandana Kumar,
Kevin Li
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1808
In current code, the values of TopaEntryPtr->Uint64 for TopaTable
and the values of OutputBaseReg.Uint64 and OutputMaskPtrsReg.Uint64
to register table write for RTIT_OUTPUT_BASE and RTIT_OUTPUT_MASK_PTRS
are not been initialized in whole. For example, the reserved bits in
OutputBaseReg.Uint64 are random that will cause GP fault like below
when SetProcessorRegister (in CpuFeaturesInitialize.c) sets register
based on register table.
!!!! X64 Exception Type - 0D(#GP - General Protection)
CPU Apic ID - 00000000 !!!!
ExceptionData - 0000000000000000
RIP -0000000064D69576, CS -0000000000000038, RFLAGS -0000000000010246
RAX -000000006B9F1001, RCX -0000000000000560, RDX -0000000000000000
RBX -0000000064EECA18, RSP -000000006CB82BA0, RBP -0000000000000008
RSI -0000000080000000, RDI -0000000000000011
R8 -000000006B9493D0, R9 -0000000000000010, R10 -00000000000000FF
R11 -000000006CB82A50, R12 -0000000064D70F50, R13 -0000000066547050
R14 -0000000064E3E198, R15 -0000000000000000
DS -0000000000000030, ES -0000000000000030, FS -0000000000000030
GS -0000000000000030, SS -0000000000000030
CR0 -0000000080010013, CR2 -0000000000000000, CR3 -000000006C601000
CR4 -0000000000000628, CR8 -0000000000000000
DR0 -0000000000000000, DR1 -0000000000000000, DR2 -0000000000000000
DR3 -0000000000000000, DR6 -00000000FFFF0FF0, DR7 -0000000000000400
GDTR -000000006B8CCF18 0000000000000047, LDTR -0000000000000000
IDTR -000000006687E018 0000000000000FFF, TR -0000000000000000
FXSAVE_STATE -000000006CB82800
And current code gets MSR_IA32_RTIT_CTL, MSR_IA32_RTIT_OUTPUT_BASE and
MSR_IA32_RTIT_OUTPUT_MASK_PTRS in ProcTraceInitialize() and uses their
values for all processors. But ProcTraceInitialize() is only executed
by BSP, that means the values just for BSP. For good practice, the code
should get MSR_IA32_RTIT_CTL, MSR_IA32_RTIT_OUTPUT_BASE and
MSR_IA32_RTIT_OUTPUT_MASK_PTRS in ProcTraceSupport (executed by all
processors), and then use them in ProcTraceInitialize() for all
processors. This can also resolve the issue that the values of
OutputBaseReg.Uint64 and OutputMaskPtrsReg.Uint64 are not been
initialized in whole.
For TopaEntryPtr->Uint64, this patch updates code to initialize it
in whole explicitly by TopaEntryPtr->Uint64 = 0 before updating its
fields.
At the same time, this patch also eliminates the ProcTraceSupported
field in PROC_TRACE_PROCESSOR_DATA and the TopaMemArrayCount field in
PROC_TRACE_DATA.
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Chandana Kumar <chandana.c.kumar@intel.com>
Cc: Kevin Li <kevin.y.li@intel.com>
Signed-off-by: Star Zeng <star.zeng@intel.com>
---
.../Library/CpuCommonFeaturesLib/ProcTrace.c | 62 +++++++++----------
1 file changed, 30 insertions(+), 32 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
index b98eb116b7b6..611459105d38 100644
--- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
+++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c
@@ -29,9 +29,11 @@ typedef enum {
} RTIT_OUTPUT_SCHEME;
typedef struct {
- BOOLEAN ProcTraceSupported;
- BOOLEAN TopaSupported;
- BOOLEAN SingleRangeSupported;
+ BOOLEAN TopaSupported;
+ BOOLEAN SingleRangeSupported;
+ MSR_IA32_RTIT_CTL_REGISTER RtitCtrl;
+ MSR_IA32_RTIT_OUTPUT_BASE_REGISTER RtitOutputBase;
+ MSR_IA32_RTIT_OUTPUT_MASK_PTRS_REGISTER RtitOutputMaskPtrs;
} PROC_TRACE_PROCESSOR_DATA;
typedef struct {
@@ -44,7 +46,6 @@ typedef struct {
UINTN AllocatedThreads;
UINTN *TopaMemArray;
- UINTN TopaMemArrayCount;
PROC_TRACE_PROCESSOR_DATA *ProcessorData;
} PROC_TRACE_DATA;
@@ -124,8 +125,7 @@ ProcTraceSupport (
// Check if Processor Trace is supported
//
AsmCpuidEx (CPUID_STRUCTURED_EXTENDED_FEATURE_FLAGS, 0, NULL, &Ebx.Uint32, NULL, NULL);
- ProcTraceData->ProcessorData[ProcessorNumber].ProcTraceSupported = (BOOLEAN) (Ebx.Bits.IntelProcessorTrace == 1);
- if (!ProcTraceData->ProcessorData[ProcessorNumber].ProcTraceSupported) {
+ if (Ebx.Bits.IntelProcessorTrace == 0) {
return FALSE;
}
@@ -134,6 +134,9 @@ ProcTraceSupport (
ProcTraceData->ProcessorData[ProcessorNumber].SingleRangeSupported = (BOOLEAN) (Ecx.Bits.SingleRangeOutput == 1);
if ((ProcTraceData->ProcessorData[ProcessorNumber].TopaSupported && (ProcTraceData->ProcTraceOutputScheme == RtitOutputSchemeToPA)) ||
(ProcTraceData->ProcessorData[ProcessorNumber].SingleRangeSupported && (ProcTraceData->ProcTraceOutputScheme == RtitOutputSchemeSingleRange))) {
+ ProcTraceData->ProcessorData[ProcessorNumber].RtitCtrl.Uint64 = AsmReadMsr64 (MSR_IA32_RTIT_CTL);
+ ProcTraceData->ProcessorData[ProcessorNumber].RtitOutputBase.Uint64 = AsmReadMsr64 (MSR_IA32_RTIT_OUTPUT_BASE);
+ ProcTraceData->ProcessorData[ProcessorNumber].RtitOutputMaskPtrs.Uint64 = AsmReadMsr64 (MSR_IA32_RTIT_OUTPUT_MASK_PTRS);
return TRUE;
}
@@ -202,7 +205,7 @@ ProcTraceInitialize (
//
// Clear MSR_IA32_RTIT_CTL[0] and IA32_RTIT_STS only if MSR_IA32_RTIT_CTL[0]==1b
//
- CtrlReg.Uint64 = AsmReadMsr64 (MSR_IA32_RTIT_CTL);
+ CtrlReg.Uint64 = ProcTraceData->ProcessorData[ProcessorNumber].RtitCtrl.Uint64;
if (CtrlReg.Bits.TraceEn != 0) {
///
/// Clear bit 0 in MSR IA32_RTIT_CTL (570)
@@ -251,9 +254,9 @@ ProcTraceInitialize (
//
// Let BSP allocate and create the necessary memory region (Aligned to the size of
// the memory region from setup option(ProcTraceMemSize) which is an integral multiple of 4kB)
- // for the all the enabled threads for storing Processor Trace debug data. Then Configure the trace
+ // for all the enabled threads to store Processor Trace debug data. Then Configure the trace
// 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 clear.
+ // 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) {
@@ -284,13 +287,12 @@ ProcTraceInitialize (
}
DEBUG ((DEBUG_INFO, "ProcTrace: Allocated PT mem for %d thread \n", ProcTraceData->AllocatedThreads));
- MemRegionBaseAddr = ThreadMemRegionTable[0];
+ }
+
+ if (ProcessorNumber < ProcTraceData->AllocatedThreads) {
+ MemRegionBaseAddr = ProcTraceData->ThreadMemRegionTable[ProcessorNumber];
} else {
- if (ProcessorNumber < ProcTraceData->AllocatedThreads) {
- MemRegionBaseAddr = ProcTraceData->ThreadMemRegionTable[ProcessorNumber];
- } else {
- return RETURN_SUCCESS;
- }
+ return RETURN_SUCCESS;
}
///
@@ -309,7 +311,6 @@ ProcTraceInitialize (
//
// Clear MSR IA32_RTIT_CTL (0x570) ToPA (Bit 8)
//
- CtrlReg.Uint64 = AsmReadMsr64 (MSR_IA32_RTIT_CTL);
CtrlReg.Bits.ToPA = 0;
CPU_REGISTER_TABLE_WRITE64 (
ProcessorNumber,
@@ -321,6 +322,7 @@ ProcTraceInitialize (
//
// Program MSR IA32_RTIT_OUTPUT_BASE (0x560) bits[63:7] with the allocated Memory Region
//
+ OutputBaseReg.Uint64 = ProcTraceData->ProcessorData[ProcessorNumber].RtitOutputBase.Uint64;
OutputBaseReg.Bits.Base = (MemRegionBaseAddr >> 7) & 0x01FFFFFF;
OutputBaseReg.Bits.BaseHi = RShiftU64 ((UINT64) MemRegionBaseAddr, 32) & 0xFFFFFFFF;
CPU_REGISTER_TABLE_WRITE64 (
@@ -333,6 +335,7 @@ ProcTraceInitialize (
//
// Program the Mask bits for the Memory Region to MSR IA32_RTIT_OUTPUT_MASK_PTRS (561h)
//
+ OutputMaskPtrsReg.Uint64 = ProcTraceData->ProcessorData[ProcessorNumber].RtitOutputMaskPtrs.Uint64;
OutputMaskPtrsReg.Bits.MaskOrTableOffset = ((MemRegionSize - 1) >> 7) & 0x01FFFFFF;
OutputMaskPtrsReg.Bits.OutputOffset = RShiftU64 (MemRegionSize - 1, 32) & 0xFFFFFFFF;
CPU_REGISTER_TABLE_WRITE64 (
@@ -376,10 +379,10 @@ ProcTraceInitialize (
if (Index < ProcTraceData->AllocatedThreads) {
ProcTraceData->AllocatedThreads = Index;
}
- DEBUG ((DEBUG_ERROR, "ProcTrace: Out of mem, allocating ToPA mem only for %d threads\n", ProcTraceData->AllocatedThreads));
+ 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
+ // Could not allocate for BSP even
//
FreePool ((VOID *) TopaMemArray);
TopaMemArray = NULL;
@@ -393,29 +396,24 @@ ProcTraceInitialize (
}
DEBUG ((DEBUG_INFO, "ProcTrace: Allocated ToPA mem for %d thread \n", ProcTraceData->AllocatedThreads));
- //
- // BSP gets the first block
- //
- TopaTableBaseAddr = TopaMemArray[0];
+ }
+
+ if (ProcessorNumber < ProcTraceData->AllocatedThreads) {
+ TopaTableBaseAddr = ProcTraceData->TopaMemArray[ProcessorNumber];
} else {
- //
- // Count for currently executing AP.
- //
- if (ProcessorNumber < ProcTraceData->AllocatedThreads) {
- TopaTableBaseAddr = ProcTraceData->TopaMemArray[ProcessorNumber];
- } else {
- return RETURN_SUCCESS;
- }
+ return RETURN_SUCCESS;
}
TopaTable = (PROC_TRACE_TOPA_TABLE *) TopaTableBaseAddr;
TopaEntryPtr = &TopaTable->TopaEntry[0];
+ TopaEntryPtr->Uint64 = 0;
TopaEntryPtr->Bits.Base = (MemRegionBaseAddr >> 12) & 0x000FFFFF;
TopaEntryPtr->Bits.BaseHi = RShiftU64 ((UINT64) MemRegionBaseAddr, 32) & 0xFFFFFFFF;
TopaEntryPtr->Bits.Size = ProcTraceData->ProcTraceMemSize;
TopaEntryPtr->Bits.END = 0;
TopaEntryPtr = &TopaTable->TopaEntry[1];
+ TopaEntryPtr->Uint64 = 0;
TopaEntryPtr->Bits.Base = (TopaTableBaseAddr >> 12) & 0x000FFFFF;
TopaEntryPtr->Bits.BaseHi = RShiftU64 ((UINT64) TopaTableBaseAddr, 32) & 0xFFFFFFFF;
TopaEntryPtr->Bits.END = 1;
@@ -423,6 +421,7 @@ ProcTraceInitialize (
//
// Program the MSR IA32_RTIT_OUTPUT_BASE (0x560) bits[63:7] with ToPA base
//
+ OutputBaseReg.Uint64 = ProcTraceData->ProcessorData[ProcessorNumber].RtitOutputBase.Uint64;
OutputBaseReg.Bits.Base = (TopaTableBaseAddr >> 7) & 0x01FFFFFF;
OutputBaseReg.Bits.BaseHi = RShiftU64 ((UINT64) TopaTableBaseAddr, 32) & 0xFFFFFFFF;
CPU_REGISTER_TABLE_WRITE64 (
@@ -435,6 +434,7 @@ ProcTraceInitialize (
//
// Set the MSR IA32_RTIT_OUTPUT_MASK (0x561) bits[63:7] to 0
//
+ OutputMaskPtrsReg.Uint64 = ProcTraceData->ProcessorData[ProcessorNumber].RtitOutputMaskPtrs.Uint64;
OutputMaskPtrsReg.Bits.MaskOrTableOffset = 0;
OutputMaskPtrsReg.Bits.OutputOffset = 0;
CPU_REGISTER_TABLE_WRITE64 (
@@ -446,7 +446,6 @@ ProcTraceInitialize (
//
// Enable ToPA output scheme by enabling MSR IA32_RTIT_CTL (0x570) ToPA (Bit 8)
//
- CtrlReg.Uint64 = AsmReadMsr64 (MSR_IA32_RTIT_CTL);
CtrlReg.Bits.ToPA = 1;
CPU_REGISTER_TABLE_WRITE64 (
ProcessorNumber,
@@ -459,7 +458,6 @@ ProcTraceInitialize (
///
/// Enable the Processor Trace feature from MSR IA32_RTIT_CTL (570h)
///
- CtrlReg.Uint64 = AsmReadMsr64 (MSR_IA32_RTIT_CTL);
CtrlReg.Bits.OS = 1;
CtrlReg.Bits.User = 1;
CtrlReg.Bits.BranchEn = 1;
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread