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>
Subject: Re: [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data structure when change MSR value.
Date: Fri, 18 Aug 2017 03:31:18 +0000 [thread overview]
Message-ID: <ED077930C258884BBCB450DB737E66224A9C1EEC@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <E92EE9817A31E24EB0585FDF735412F5A7D81E5F@ORSMSX113.amr.corp.intel.com>
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 <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.
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 <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com<mailto:ruiyu.ni@intel.com>>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>
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:image001.png@01D31815.819100E0]
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<mailto:eric.dong@intel.com>>; edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com<mailto: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-18 3:28 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
2017-08-17 17:32 ` Kinney, Michael D
2017-08-18 3:31 ` Dong, Eric [this message]
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=ED077930C258884BBCB450DB737E66224A9C1EEC@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