From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (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 7DB482095B9F5 for ; Wed, 23 Aug 2017 20:19:43 -0700 (PDT) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Aug 2017 20:22:17 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,419,1498546800"; d="jpg'145?scan'145,208,217,145";a="1187565670" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga001.fm.intel.com with ESMTP; 23 Aug 2017 20:22:17 -0700 Received: from fmsmsx119.amr.corp.intel.com (10.18.124.207) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 23 Aug 2017 20:22:15 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX119.amr.corp.intel.com (10.18.124.207) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 23 Aug 2017 20:22:14 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.183]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.128]) with mapi id 14.03.0319.002; Thu, 24 Aug 2017 11:21:44 +0800 From: "Dong, Eric" To: "Kinney, Michael D" , "edk2-devel@lists.01.org" CC: "Ni, Ruiyu" Thread-Topic: [Patch v2 1/2] UefiCpuPkg/ArchitecturalMsr.h: Add RTIT TOPA table entry definition. Thread-Index: AQHTHGnOI/i3sYMD9kyUAQ5qEhkhOqKS086A Date: Thu, 24 Aug 2017 03:21:44 +0000 Message-ID: References: <1503026917-13296-1-git-send-email-eric.dong@intel.com> <1503026917-13296-2-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 v2 1/2] UefiCpuPkg/ArchitecturalMsr.h: Add RTIT TOPA table entry definition. 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, 24 Aug 2017 03:19:43 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Mike, Add my comments below. -----Original Message----- From: Kinney, Michael D Sent: Thursday, August 24, 2017 7:45 AM To: Dong, Eric ; edk2-devel@lists.01.org; Kinney, Mich= ael D Cc: Ni, Ruiyu Subject: RE: [Patch v2 1/2] UefiCpuPkg/ArchitecturalMsr.h: Add RTIT TOPA ta= ble entry definition. Hi Eric, I see that the legal values for the Size field of RTIT_TOPA_TABLE_ENTRY are= documented in the SDM. I think we should add the enum for PROC_TRACE_MEM_SIZE into ArchitecturalMs= r.h next to RTIT_TOPA_TABLE_ENTRY. Maybe rename it to RTIT_TOPA_MEMORY_SIZE with enum names that start with "R= titTopaMemorySize". [[Eric]] agree, update in the new patch. The Copyright date in ArchitecturalMsr.h also needs to be updated. [[Eric]] agree, update in the new patch. The #define for MAX_TOPA_ENTRY_COUNT need a comment block that describes wh= at it is and why it has a value of 2. I see 2 entries are needed because the list of entries must be terminated by an e= ntry with the END bit set to 1, so 2 entries are required to use a single v= alid entry. [[Eric]] agree, update in the new patch. ProcTrace.c: Please update programming of MSR_IA32_RTIT_OUTPUT_BASE to use = the named bit fields from the structure. ProcTrace.c: Please update programming of MSR_IA32_RTIT_OUTPUT_MASK_PTRS to= use the named bit fields from the structure. [[Eric]] I think current code is easy to write but it may touch the reserve= bits. Agree to change to use the bit fields. Update in the new patches. ProcTrace.c: I am confused by the programming MSR_IA32_RTIT_OUTPUT_MASK_PTR= S to 0x7f. That sets a Reserved field to all 1s. [[Eric]] Copy from the sdm, default value is all 1. Write to it is ignored.= So I think the original code is same as set the others bits all 0. I have = update code to directly set other bits to 0. Please check the new patch. [cid:image001.jpg@01D31CCB.2A4EEA30] ProcTrace.c: I see a mix of setting TopaEntryPtr using both bit fields and = the Uint64 field. Can we change to use only the bit fields. [[Eric]] same reason as before, use Uint64 is easy to write but may touch t= he reserved bits. Update code to use bit fields. Do we really need to set the address field in entry #1? Isn't setting END = to 1 enough? [[Eric]] Copy below from sdm. This code point to the current table for a ci= rcular case. [cid:image002.jpg@01D31CCB.2A4EEA30] Thanks, Mike > -----Original Message----- > From: Dong, Eric > Sent: Thursday, August 17, 2017 8:29 PM > To: edk2-devel@lists.01.org > Cc: Kinney, Michael D >; Ni, Ruiyu > > > Subject: [Patch v2 1/2] UefiCpuPkg/ArchitecturalMsr.h: Add RTIT TOPA > table entry definition. > > Add RTIT TOPA table entry definition to architecturalMsr.h file. > > Cc: Michael Kinney > > Cc: Ruiyu Ni > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong = > > --- > UefiCpuPkg/Include/Register/ArchitecturalMsr.h | 55 > ++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > > diff --git a/UefiCpuPkg/Include/Register/ArchitecturalMsr.h > b/UefiCpuPkg/Include/Register/ArchitecturalMsr.h > index 4f9c103..40c4383 100644 > --- a/UefiCpuPkg/Include/Register/ArchitecturalMsr.h > +++ b/UefiCpuPkg/Include/Register/ArchitecturalMsr.h > @@ -4534,6 +4534,61 @@ typedef union { > UINT64 Uint64; > } MSR_IA32_RTIT_OUTPUT_MASK_PTRS_REGISTER; > > +/** > + Format of ToPA table entries. > +**/ > +typedef union { > + /// > + /// Individual bit fields > + /// > + struct { > + /// > + /// [Bit 0] END. See Section 35.2.6.2, "Table of Physical > Addresses (ToPA)". > + /// > + UINT32 END:1; > + UINT32 Reserved1:1; > + /// > + /// [Bit 2] INT. See Section 35.2.6.2, "Table of Physical > Addresses (ToPA)". > + /// > + UINT32 INT:1; > + UINT32 Reserved2:1; > + /// > + /// [Bit 4] STOP. See Section 35.2.6.2, "Table of Physical > Addresses (ToPA)". > + /// > + UINT32 STOP:1; > + UINT32 Reserved3:1; > + /// > + /// [Bit 6:9] Indicates the size of the associated output > region. See Section > + /// 35.2.6.2, "Table of Physical Addresses (ToPA)". > + /// > + UINT32 Size:4; > + UINT32 Reserved4:2; > + /// > + /// [Bit 12:31] Output Region Base Physical Address low part. > + /// [Bit 12:31] Output Region Base Physical Address [12:63] > value to match. > + /// 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)". > + /// > + UINT32 Base:20; > + /// > + /// [Bit 32:63] Output Region Base Physical Address high > part. > + /// [Bit 32:63] Output Region Base Physical Address [12:63] > value to match. > + /// 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)". > + /// > + UINT32 BaseHi:32; > + } Bits; > + /// > + /// All bit fields as a 64-bit value > + /// > + UINT64 Uint64; > +} RTIT_TOPA_TABLE_ENTRY; > > /** > Trace Control Register (R/W). If (CPUID.(EAX=3D07H, ECX=3D0):EBX[25] = =3D > 1). > -- > 2.7.0.windows.1