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]>; [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.
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
