From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=fail (domain: intel.com, ip: , mailfrom: star.zeng@intel.com) Received: from mga06.intel.com (mga06.intel.com []) by groups.io with SMTP; Sat, 01 Jun 2019 01:03:47 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 01 Jun 2019 01:03:47 -0700 X-ExtLoop1: 1 Received: from shwdeopenpsi068.ccr.corp.intel.com ([10.239.158.82]) by fmsmga008.fm.intel.com with ESMTP; 01 Jun 2019 01:03:46 -0700 From: "Zeng, Star" To: devel@edk2.groups.io Cc: Star Zeng , Laszlo Ersek , Eric Dong , Ruiyu Ni , Chandana Kumar , Kevin Li Subject: [PATCH 2/2] UefiCpuPkg CpuCommFeaturesLib: Fix GP fault issue about ProcTrace Date: Sat, 1 Jun 2019 16:03:41 +0800 Message-Id: <20190601080341.107760-3-star.zeng@intel.com> X-Mailer: git-send-email 2.21.0.windows.1 In-Reply-To: <20190601080341.107760-1-star.zeng@intel.com> References: <20190601080341.107760-1-star.zeng@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit 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 Cc: Eric Dong Cc: Ruiyu Ni Cc: Chandana Kumar Cc: Kevin Li Signed-off-by: Star Zeng --- .../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