From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (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 F285E21E79018 for ; Thu, 17 Aug 2017 20:28:53 -0700 (PDT) Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga105.jf.intel.com with ESMTP; 17 Aug 2017 20:31:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,391,1498546800"; d="png'150?scan'150,208,217,150";a="120285076" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga004.jf.intel.com with ESMTP; 17 Aug 2017 20:31:21 -0700 Received: from fmsmsx122.amr.corp.intel.com (10.18.125.37) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 17 Aug 2017 20:31:21 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx122.amr.corp.intel.com (10.18.125.37) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 17 Aug 2017 20:31:20 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.183]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.25]) with mapi id 14.03.0319.002; Fri, 18 Aug 2017 11:31:18 +0800 From: "Dong, Eric" To: "Kinney, Michael D" , "edk2-devel@lists.01.org" CC: "Ni, Ruiyu" Thread-Topic: [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data structure when change MSR value. Thread-Index: AQHTFy3eivhu+yq6Z0KiqWbDB3/37KKIMeJQgAAXSICAASyygA== Date: Fri, 18 Aug 2017 03:31:18 +0000 Message-ID: References: <1502951747-10848-1-git-send-email-eric.dong@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: yes X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 X-Content-Filtered-By: Mailman/MimeDel 2.1.22 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: Fri, 18 Aug 2017 03:28:54 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 ; edk2-devel@lists.01.org; Kinney, Mich= ael D Cc: Ni, Ruiyu Subject: RE: [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data structur= e when change MSR value. Eric, You should not us UINT64 for bitfields. Please use UINT32 instead, and mak= e sure bitfields do not cross 32-bit boundaries. Other than that, adding th= is 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 >; edk2-devel@lists.01.org Cc: Ni, Ruiyu >; Dong, Eric <= eric.dong@intel.com> Subject: RE: [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data structur= e 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 proce= ssor'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 reserve= d 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 co= de more user friendly, so I defined it in my local file. But I'm not sure w= hether other cases can use this definition if I put this definition to Arch= itecturalMsr.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 >; edk2-deve= l@lists.01.org; Kinney, Michael D > Cc: Ni, Ruiyu > Subject: RE: [Patch] UefiCpuPkg/CpuCommonFeaturesLib: Use MSR data structur= e when change MSR value. Hi Eric, I recommend that PROC_TRACE_TOPA_ENTRY be added to UefiCpuPkg\Include\Regis= ter\ArchitecturalMsr.h after MSR_IA32_RTIT_CTL_REGISTER that contains the T= OPA 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 Archi= tecturalMsr.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. > > 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 > > 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(-) > > 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 =3D (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]=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 > ); > > /// > /// 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 > ); > } > > @@ -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 > ); > > // > // 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 > ); > > // > // 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 > ); > > } > @@ -408,55 +453,66 @@ ProcTraceInitialize ( > } > > 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; > > // > // 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 > ); > > // > // 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 > ); > } > > /// > /// 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 > ); > > return RETURN_SUCCESS; > -- > 2.7.0.windows.1