From: "Dong, Eric" <eric.dong@intel.com>
To: "Kinney, Michael D" <michael.d.kinney@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Ni, Ruiyu" <ruiyu.ni@intel.com>, "Dong, Eric" <eric.dong@intel.com>
Subject: Re: [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data structure when change MSR value.
Date: Thu, 17 Aug 2017 08:26:30 +0000 [thread overview]
Message-ID: <ED077930C258884BBCB450DB737E66224A9C0C30@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <E92EE9817A31E24EB0585FDF735412F5A7D81CCC@ORSMSX113.amr.corp.intel.com>
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:image001.png@01D31774.015A0BC0]
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 <eric.dong@intel.com>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>
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: edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> Cc: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Ni, Ruiyu
> <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>
> 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 <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> ---
> .../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
next prev parent reply other threads:[~2017-08-17 8:24 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-17 6:35 [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data structure when change MSR value Eric Dong
2017-08-17 7:53 ` Kinney, Michael D
2017-08-17 8:26 ` Dong, Eric [this message]
2017-08-17 17:32 ` Kinney, Michael D
2017-08-18 3:31 ` Dong, Eric
2017-08-18 7:38 ` Kinney, Michael D
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ED077930C258884BBCB450DB737E66224A9C0C30@shsmsx102.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox