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]>; [email protected] Cc: Ni, Ruiyu <[email protected]>; Dong, Eric <[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
