From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 376D821E2DA62 for ; Thu, 17 Aug 2017 00:50:42 -0700 (PDT) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Aug 2017 00:53:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,386,1498546800"; d="scan'208";a="124639648" Received: from orsmsx107.amr.corp.intel.com ([10.22.240.5]) by orsmga002.jf.intel.com with ESMTP; 17 Aug 2017 00:53:08 -0700 Received: from orsmsx155.amr.corp.intel.com (10.22.240.21) by ORSMSX107.amr.corp.intel.com (10.22.240.5) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 17 Aug 2017 00:53:08 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.211]) by ORSMSX155.amr.corp.intel.com ([169.254.7.162]) with mapi id 14.03.0319.002; Thu, 17 Aug 2017 00:53:07 -0700 From: "Kinney, Michael D" To: "Dong, Eric" , "edk2-devel@lists.01.org" , "Kinney, Michael D" CC: "Ni, Ruiyu" Thread-Topic: [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data structure when change MSR value. Thread-Index: AQHTFyMWb7KuabphbUu2CU+6sqLaAaKIK/VQ Date: Thu, 17 Aug 2017 07:53:07 +0000 Message-ID: References: <1502951747-10848-1-git-send-email-eric.dong@intel.com> In-Reply-To: <1502951747-10848-1-git-send-email-eric.dong@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [10.22.254.140] MIME-Version: 1.0 Subject: Re: [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data structure when change MSR value. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 Aug 2017 07:50:42 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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=20 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=20 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 > Cc: Kinney, Michael D ; Ni, Ruiyu > > Subject: [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data > structure when change MSR value. >=20 > 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. >=20 > Cc: Michael Kinney > Cc: Ruiyu Ni > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong > --- > .../Library/CpuCommonFeaturesLib/ProcTrace.c | 112 > +++++++++++++++------ > 1 file changed, 84 insertions(+), 28 deletions(-) >=20 > 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; >=20 > +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; >=20 > /** > @@ -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; >=20 > ProcTraceData =3D (PROC_TRACE_DATA *) ConfigData; >=20 > @@ -221,29 +267,28 @@ ProcTraceInitialize ( > // > // Clear MSR_IA32_RTIT_CTL[0] and IA32_RTIT_STS only if > MSR_IA32_RTIT_CTL[0]=3D=3D1b > // > - MsrValue =3D AsmReadMsr64 (MSR_IA32_RTIT_CTL); > - if ((MsrValue & BIT0) !=3D 0) { > + CtrlReg.Uint64 =3D AsmReadMsr64 (MSR_IA32_RTIT_CTL); > + if (CtrlReg.Bits.TraceEn !=3D 0) { > /// > /// Clear bit 0 in MSR IA32_RTIT_CTL (570) > /// > - MsrValue &=3D (UINT64) ~BIT0; > + CtrlReg.Bits.TraceEn =3D 0; > CPU_REGISTER_TABLE_WRITE64 ( > ProcessorNumber, > Msr, > MSR_IA32_RTIT_CTL, > - MsrValue > + CtrlReg.Uint64 > ); >=20 > /// > /// Clear MSR IA32_RTIT_STS (571h) to all zeros > /// > - MsrValue =3D AsmReadMsr64 (MSR_IA32_RTIT_STATUS); > - MsrValue &=3D 0x0; > + StatusReg.Uint64 =3D 0x0; > CPU_REGISTER_TABLE_WRITE64 ( > ProcessorNumber, > Msr, > MSR_IA32_RTIT_STATUS, > - MsrValue > + StatusReg.Uint64 > ); > } >=20 > @@ -309,35 +354,35 @@ ProcTraceInitialize ( > // > // Clear MSR IA32_RTIT_CTL (0x570) ToPA (Bit 8) > // > - MsrValue =3D AsmReadMsr64 (MSR_IA32_RTIT_CTL); > - MsrValue &=3D (UINT64) ~BIT8; > + CtrlReg.Uint64 =3D AsmReadMsr64 (MSR_IA32_RTIT_CTL); > + CtrlReg.Bits.ToPA =3D 0; > CPU_REGISTER_TABLE_WRITE64 ( > ProcessorNumber, > Msr, > MSR_IA32_RTIT_CTL, > - MsrValue > + CtrlReg.Uint64 > ); >=20 > // > // Program MSR IA32_RTIT_OUTPUT_BASE (0x560) bits[47:12] > with the allocated Memory Region > // > - MsrValue =3D (UINT64) MemRegionBaseAddr; > + OutputBaseReg.Uint64 =3D (UINT64) MemRegionBaseAddr; > CPU_REGISTER_TABLE_WRITE64 ( > ProcessorNumber, > Msr, > MSR_IA32_RTIT_OUTPUT_BASE, > - MsrValue > + OutputBaseReg.Uint64 > ); >=20 > // > // Program the Mask bits for the Memory Region to MSR > IA32_RTIT_OUTPUT_MASK_PTRS (561h) > // > - MsrValue =3D (UINT64) MemRegionSize - 1; > + OutputMaskPtrsReg.Uint64 =3D (UINT64) MemRegionSize - 1; > CPU_REGISTER_TABLE_WRITE64 ( > ProcessorNumber, > Msr, > MSR_IA32_RTIT_OUTPUT_MASK_PTRS, > - MsrValue > + OutputMaskPtrsReg.Uint64 > ); >=20 > } > @@ -408,55 +453,66 @@ ProcTraceInitialize ( > } >=20 > TopaTable =3D (PROC_TRACE_TOPA_TABLE *) TopaTableBaseAddr; > - TopaTable->TopaEntry[0] =3D (UINT64) (MemRegionBaseAddr | > ((ProcTraceData->ProcTraceMemSize) << 6)) & ~BIT0; > - TopaTable->TopaEntry[1] =3D (UINT64) TopaTableBaseAddr | > BIT0; > + TopaEntryPtr =3D &TopaTable->TopaEntry[0]; > + TopaEntryPtr->Bits.Address =3D MemRegionBaseAddr; > + TopaEntryPtr->Bits.Size =3D ProcTraceData- > >ProcTraceMemSize; > + TopaEntryPtr->Bits.END =3D 0; > + > + TopaEntryPtr =3D &TopaTable->TopaEntry[1]; > + TopaEntryPtr->Bits.Address =3D TopaTableBaseAddr; > + TopaEntryPtr->Bits.END =3D 1; >=20 > // > // Program the MSR IA32_RTIT_OUTPUT_BASE (0x560) > bits[47:12] with ToPA base > // > - MsrValue =3D (UINT64) TopaTableBaseAddr; > + OutputBaseReg.Uint64 =3D (UINT64) TopaTableBaseAddr; > CPU_REGISTER_TABLE_WRITE64 ( > ProcessorNumber, > Msr, > MSR_IA32_RTIT_OUTPUT_BASE, > - MsrValue > + OutputBaseReg.Uint64 > ); >=20 > // > // Set the MSR IA32_RTIT_OUTPUT_MASK (0x561) bits[63:7] > to 0 > // > + OutputMaskPtrsReg.Uint64 =3D (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 =3D AsmReadMsr64 (MSR_IA32_RTIT_CTL); > - MsrValue |=3D BIT8; > + CtrlReg.Uint64 =3D AsmReadMsr64 (MSR_IA32_RTIT_CTL); > + CtrlReg.Bits.ToPA =3D 1; > CPU_REGISTER_TABLE_WRITE64 ( > ProcessorNumber, > Msr, > MSR_IA32_RTIT_CTL, > - MsrValue > + CtrlReg.Uint64 > ); > } >=20 > /// > /// Enable the Processor Trace feature from MSR > IA32_RTIT_CTL (570h) > /// > - MsrValue =3D AsmReadMsr64 (MSR_IA32_RTIT_CTL); > - MsrValue |=3D (UINT64) BIT0 + BIT2 + BIT3 + BIT13; > + CtrlReg.Uint64 =3D AsmReadMsr64 (MSR_IA32_RTIT_CTL); > + CtrlReg.Bits.OS =3D 1; > + CtrlReg.Bits.User =3D 1; > + CtrlReg.Bits.BranchEn =3D 1; > if (!State) { > - MsrValue &=3D (UINT64) ~BIT0; > + CtrlReg.Bits.TraceEn =3D 0; > + } else { > + CtrlReg.Bits.TraceEn =3D 1; > } > CPU_REGISTER_TABLE_WRITE64 ( > ProcessorNumber, > Msr, > MSR_IA32_RTIT_CTL, > - MsrValue > + CtrlReg.Uint64 > ); >=20 > return RETURN_SUCCESS; > -- > 2.7.0.windows.1