Hi Mike, I have updated the patch to follow your suggest. Please help to review the new patches.
BTW, why we can't use UINT64 bit fields? Does it have potential issue in 32 bits system? Thanks, Eric From: Kinney, Michael D Sent: Friday, August 18, 2017 1:33 AM To: Dong, Eric <[email protected]>; [email protected]; Kinney, Michael D <[email protected]> Cc: Ni, Ruiyu <[email protected]> Subject: RE: [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data structure when change MSR value. Eric, You should not us UINT64 for bitfields. Please use UINT32 instead, and make sure bitfields do not cross 32-bit boundaries. Other than that, adding this structure to ArchitectureMsrs.h with the comments you have looks correct to me. I agree that MAX_TOPA_ENTRY_COUNT should remain in the CpuCommonFeaturesLib implementation. Thanks, Mike From: Dong, Eric Sent: Thursday, August 17, 2017 1:27 AM To: Kinney, Michael D <[email protected]<mailto:[email protected]>>; [email protected]<mailto:[email protected]> Cc: Ni, Ruiyu <[email protected]<mailto:[email protected]>>; Dong, Eric <[email protected]<mailto:[email protected]>> Subject: RE: [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data structure when change MSR value. Mike, The PROC_TRACE_TOPA_ENTRY definition has a little difference with the spec definition, as I have raised in the comments for "Address" field. /// /// [Bit 12:63] Output Region Base Physical Address. /// ATTENTION: The size of the address field is determined by the processor's /// physical-address width (MAXPHYADDR) in bits, as reported in /// CPUID.80000008H:EAX[7:0]. the above part of address reserved. /// True address field is [12:MAXPHYADDR-1], [MAXPHYADDR:63] is reserved part. /// Detail see Section 35.2.6.2, "Table of Physical Addresses (ToPA)". /// UINT64 Address:52; The spec description for this entry like below: [cid:[email protected]] For my use case in this file, I think this definition is ok and make the code more user friendly, so I defined it in my local file. But I'm not sure whether other cases can use this definition if I put this definition to ArchitecturalMsr.h. For the MAX_TOPA_ENTRY_COUNT macro, it's implement choice. So it can't move to ArchitecturalMsr.h. Thanks, Eric -----Original Message----- From: Kinney, Michael D Sent: Thursday, August 17, 2017 3:53 PM To: Dong, Eric <[email protected]<mailto:[email protected]>>; [email protected]<mailto:[email protected]>; Kinney, Michael D <[email protected]<mailto:[email protected]>> Cc: Ni, Ruiyu <[email protected]<mailto:[email protected]>> Subject: RE: [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data structure when change MSR value. Hi Eric, I recommend that PROC_TRACE_TOPA_ENTRY be added to UefiCpuPkg\Include\Register\ArchitecturalMsr.h after MSR_IA32_RTIT_CTL_REGISTER that contains the TOPA enable bit. How is the value of MAX_TOPA_ENTRY_COUNT determined? If that value of 2 is from the SDM, we may want to add this define to ArchitecturalMsr.h too. Mike > -----Original Message----- > From: Dong, Eric > Sent: Wednesday, August 16, 2017 11:36 PM > To: [email protected]<mailto:[email protected]> > Cc: Kinney, Michael D > <[email protected]<mailto:[email protected]>>; Ni, Ruiyu > <[email protected]<mailto:[email protected]>> > Subject: [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data > structure when change MSR value. > > When update MSR values, current code use BITxx to modify the MSR > values. Enhance the code to use corresponding MSR's data structures to > make it more user friendly. > > Cc: Michael Kinney > <[email protected]<mailto:[email protected]>> > Cc: Ruiyu Ni <[email protected]<mailto:[email protected]>> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong <[email protected]<mailto:[email protected]>> > --- > .../Library/CpuCommonFeaturesLib/ProcTrace.c | 112 > +++++++++++++++------ > 1 file changed, 84 insertions(+), 28 deletions(-) > > diff --git > a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c > b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c > index 40e6321..fa458ab 100644 > --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c > +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ProcTrace.c > @@ -69,8 +69,50 @@ typedef struct { > PROC_TRACE_PROCESSOR_DATA *ProcessorData; > } PROC_TRACE_DATA; > > +typedef union { > + /// > + /// Individual bit fields > + /// > + struct { > + /// > + /// [Bit 0] END. See Section 35.2.6.2, "Table of Physical > Addresses (ToPA)". > + /// > + UINT64 END:1; > + UINT64 Reserved1:1; > + /// > + /// [Bit 2] INT. See Section 35.2.6.2, "Table of Physical > Addresses (ToPA)". > + /// > + UINT64 INT:1; > + UINT64 Reserved2:1; > + /// > + /// [Bit 4] STOP. See Section 35.2.6.2, "Table of > Physical Addresses (ToPA)". > + /// > + UINT64 STOP:1; > + UINT64 Reserved3:1; > + /// > + /// [Bit 6:9] Indicates the size of the associated output > region. See Section > + /// 35.2.6.2, "Table of Physical Addresses (ToPA)". > + /// > + UINT64 Size:4; > + UINT64 Reserved4:2; > + /// > + /// [Bit 12:63] Output Region Base Physical Address. > + /// ATTENTION: The size of the address field is > determined by the processor's > + /// physical-address width (MAXPHYADDR) in bits, as > reported in > + /// CPUID.80000008H:EAX[7:0]. the above part of address > reserved. > + /// True address field is [12:MAXPHYADDR-1], > [MAXPHYADDR:63] is reserved part. > + /// Detail see Section 35.2.6.2, "Table of Physical > Addresses (ToPA)". > + /// > + UINT64 Address:52; > + } Bits; > + /// > + /// All bit fields as a 64-bit value > + /// > + UINT64 Uint64; > +} PROC_TRACE_TOPA_ENTRY; > + > typedef struct { > - UINT64 TopaEntry[MAX_TOPA_ENTRY_COUNT]; > + PROC_TRACE_TOPA_ENTRY TopaEntry[MAX_TOPA_ENTRY_COUNT]; > } PROC_TRACE_TOPA_TABLE; > > /** > @@ -186,7 +228,6 @@ ProcTraceInitialize ( > IN BOOLEAN State > ) > { > - UINT64 MsrValue; > UINT32 MemRegionSize; > UINTN Pages; > UINTN Alignment; > @@ -199,6 +240,11 @@ ProcTraceInitialize ( > 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; > + PROC_TRACE_TOPA_ENTRY *TopaEntryPtr; > > ProcTraceData = (PROC_TRACE_DATA *) ConfigData; > > @@ -221,29 +267,28 @@ ProcTraceInitialize ( > // > // Clear MSR_IA32_RTIT_CTL[0] and IA32_RTIT_STS only if > MSR_IA32_RTIT_CTL[0]==1b > // > - MsrValue = AsmReadMsr64 (MSR_IA32_RTIT_CTL); > - if ((MsrValue & BIT0) != 0) { > + CtrlReg.Uint64 = AsmReadMsr64 (MSR_IA32_RTIT_CTL); if > + (CtrlReg.Bits.TraceEn != 0) { > /// > /// Clear bit 0 in MSR IA32_RTIT_CTL (570) > /// > - MsrValue &= (UINT64) ~BIT0; > + CtrlReg.Bits.TraceEn = 0; > CPU_REGISTER_TABLE_WRITE64 ( > ProcessorNumber, > Msr, > MSR_IA32_RTIT_CTL, > - MsrValue > + CtrlReg.Uint64 > ); > > /// > /// Clear MSR IA32_RTIT_STS (571h) to all zeros > /// > - MsrValue = AsmReadMsr64 (MSR_IA32_RTIT_STATUS); > - MsrValue &= 0x0; > + StatusReg.Uint64 = 0x0; > CPU_REGISTER_TABLE_WRITE64 ( > ProcessorNumber, > Msr, > MSR_IA32_RTIT_STATUS, > - MsrValue > + StatusReg.Uint64 > ); > } > > @@ -309,35 +354,35 @@ ProcTraceInitialize ( > // > // Clear MSR IA32_RTIT_CTL (0x570) ToPA (Bit 8) > // > - MsrValue = AsmReadMsr64 (MSR_IA32_RTIT_CTL); > - MsrValue &= (UINT64) ~BIT8; > + CtrlReg.Uint64 = AsmReadMsr64 (MSR_IA32_RTIT_CTL); > + CtrlReg.Bits.ToPA = 0; > CPU_REGISTER_TABLE_WRITE64 ( > ProcessorNumber, > Msr, > MSR_IA32_RTIT_CTL, > - MsrValue > + CtrlReg.Uint64 > ); > > // > // Program MSR IA32_RTIT_OUTPUT_BASE (0x560) bits[47:12] with the > allocated Memory Region > // > - MsrValue = (UINT64) MemRegionBaseAddr; > + OutputBaseReg.Uint64 = (UINT64) MemRegionBaseAddr; > CPU_REGISTER_TABLE_WRITE64 ( > ProcessorNumber, > Msr, > MSR_IA32_RTIT_OUTPUT_BASE, > - MsrValue > + OutputBaseReg.Uint64 > ); > > // > // Program the Mask bits for the Memory Region to MSR > IA32_RTIT_OUTPUT_MASK_PTRS (561h) > // > - MsrValue = (UINT64) MemRegionSize - 1; > + OutputMaskPtrsReg.Uint64 = (UINT64) MemRegionSize - 1; > CPU_REGISTER_TABLE_WRITE64 ( > ProcessorNumber, > Msr, > MSR_IA32_RTIT_OUTPUT_MASK_PTRS, > - MsrValue > + OutputMaskPtrsReg.Uint64 > ); > > } > @@ -408,55 +453,66 @@ ProcTraceInitialize ( > } > > TopaTable = (PROC_TRACE_TOPA_TABLE *) TopaTableBaseAddr; > - TopaTable->TopaEntry[0] = (UINT64) (MemRegionBaseAddr | > ((ProcTraceData->ProcTraceMemSize) << 6)) & ~BIT0; > - TopaTable->TopaEntry[1] = (UINT64) TopaTableBaseAddr | > BIT0; > + TopaEntryPtr = &TopaTable->TopaEntry[0]; > + TopaEntryPtr->Bits.Address = MemRegionBaseAddr; > + TopaEntryPtr->Bits.Size = ProcTraceData- > >ProcTraceMemSize; > + TopaEntryPtr->Bits.END = 0; > + > + TopaEntryPtr = &TopaTable->TopaEntry[1]; > + TopaEntryPtr->Bits.Address = TopaTableBaseAddr; > + TopaEntryPtr->Bits.END = 1; > > // > // Program the MSR IA32_RTIT_OUTPUT_BASE (0x560) bits[47:12] with > ToPA base > // > - MsrValue = (UINT64) TopaTableBaseAddr; > + OutputBaseReg.Uint64 = (UINT64) TopaTableBaseAddr; > CPU_REGISTER_TABLE_WRITE64 ( > ProcessorNumber, > Msr, > MSR_IA32_RTIT_OUTPUT_BASE, > - MsrValue > + OutputBaseReg.Uint64 > ); > > // > // Set the MSR IA32_RTIT_OUTPUT_MASK (0x561) bits[63:7] to 0 > // > + OutputMaskPtrsReg.Uint64 = (UINT64) 0x7F; > CPU_REGISTER_TABLE_WRITE64 ( > ProcessorNumber, > Msr, > MSR_IA32_RTIT_OUTPUT_MASK_PTRS, > - 0x7F > + OutputMaskPtrsReg.Uint64 > ); > // > // Enable ToPA output scheme by enabling MSR IA32_RTIT_CTL > (0x570) ToPA (Bit 8) > // > - MsrValue = AsmReadMsr64 (MSR_IA32_RTIT_CTL); > - MsrValue |= BIT8; > + CtrlReg.Uint64 = AsmReadMsr64 (MSR_IA32_RTIT_CTL); > + CtrlReg.Bits.ToPA = 1; > CPU_REGISTER_TABLE_WRITE64 ( > ProcessorNumber, > Msr, > MSR_IA32_RTIT_CTL, > - MsrValue > + CtrlReg.Uint64 > ); > } > > /// > /// Enable the Processor Trace feature from MSR IA32_RTIT_CTL > (570h) > /// > - MsrValue = AsmReadMsr64 (MSR_IA32_RTIT_CTL); > - MsrValue |= (UINT64) BIT0 + BIT2 + BIT3 + BIT13; > + CtrlReg.Uint64 = AsmReadMsr64 (MSR_IA32_RTIT_CTL); CtrlReg.Bits.OS > + = 1; CtrlReg.Bits.User = 1; CtrlReg.Bits.BranchEn = 1; > if (!State) { > - MsrValue &= (UINT64) ~BIT0; > + CtrlReg.Bits.TraceEn = 0; > + } else { > + CtrlReg.Bits.TraceEn = 1; > } > CPU_REGISTER_TABLE_WRITE64 ( > ProcessorNumber, > Msr, > MSR_IA32_RTIT_CTL, > - MsrValue > + CtrlReg.Uint64 > ); > > return RETURN_SUCCESS; > -- > 2.7.0.windows.1 _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
