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 B39B02095B9E0 for ; Tue, 22 Aug 2017 15:27:55 -0700 (PDT) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 22 Aug 2017 15:30:28 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,414,1498546800"; d="scan'208";a="143438650" Received: from orsmsx109.amr.corp.intel.com ([10.22.240.7]) by fmsmga006.fm.intel.com with ESMTP; 22 Aug 2017 15:30:28 -0700 Received: from orsmsx158.amr.corp.intel.com (10.22.240.20) by ORSMSX109.amr.corp.intel.com (10.22.240.7) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 22 Aug 2017 15:30:28 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.211]) by ORSMSX158.amr.corp.intel.com ([169.254.10.102]) with mapi id 14.03.0319.002; Tue, 22 Aug 2017 15:30:27 -0700 From: "Kinney, Michael D" To: "Dong, Eric" , "edk2-devel@lists.01.org" , "Kinney, Michael D" CC: "Ni, Ruiyu" , "Gao, Liming" Thread-Topic: [edk2] [Patch 1/2] UefiCpuPkg: Add comments for PCDs definition. Thread-Index: AQHTFM46dlWLoHrjk06PgorEwxtss6KHJYaAgAFvqoD//50p8IAAhaKAgAAclACAB0cXAIAA5vbQ Date: Tue, 22 Aug 2017 22:30:26 +0000 Message-ID: References: <1502695374-11368-1-git-send-email-eric.dong@intel.com> <1502695374-11368-2-git-send-email-eric.dong@intel.com> In-Reply-To: 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.139] MIME-Version: 1.0 Subject: Re: [Patch 1/2] UefiCpuPkg: Add comments for PCDs 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: Tue, 22 Aug 2017 22:27:55 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Eric, Yes. I like the idea of removing both OutputSchemeInvalid and ProcTraceMemDisable values. Mike > -----Original Message----- > From: Dong, Eric > Sent: Monday, August 21, 2017 6:43 PM > To: Kinney, Michael D ; edk2- > devel@lists.01.org > Cc: Ni, Ruiyu ; Gao, Liming > > Subject: RE: [edk2] [Patch 1/2] UefiCpuPkg: Add comments for > PCDs definition. >=20 > Mike, >=20 > I think you misuse PcdCpuFeaturesSetting for > PcdCpuProcTraceMemSize, right? > ProcTrace feature is checked by BIT44(CPU_FEATURE_PROC_TRACE) in > PcdCpuFeaturesSetting. >=20 > If you think define OutputSchemeInvalid is redundant and propose > to remove it. I think we can also remove ProcTraceMemDisable. We > can use the first value in PcdCpuProcTraceOutputScheme and > PcdCpuProcTraceMemSize as the default value. And these values > valid only ProcTrace feature is enables in > PcdCpuFeaturesSetting. Do you think remove OutputSchemeInvalid > and ProcTraceMemDisable is ok for you? >=20 > Thanks, > Eric > -----Original Message----- > From: Kinney, Michael D > Sent: Friday, August 18, 2017 1:39 AM > To: Dong, Eric ; edk2-devel@lists.01.org; > Kinney, Michael D > Cc: Ni, Ruiyu ; Gao, Liming > > Subject: RE: [edk2] [Patch 1/2] UefiCpuPkg: Add comments for > PCDs definition. >=20 > Eric, >=20 > Define PcdCpuFeaturesSetting set to 0x10 ProcTraceMemDisable as > the default. If this PCD is 0x10, then the ProcTrace feature is > enabled. If this PCD is less than 0x10, then the ProcTrace > feature is enabled. Basically, if the PCD is configured to > allocate memory for the feature then the feature is enabled. If > no memory is allocated to the feature, then the feature is > disabled. >=20 > Define PcdCpuProcTraceOutputScheme set to 0 as the default but > also add that this PCD is ignored if PcdCpuFeaturesSetting is > set to 0x10 ProcTraceMemDisable. >=20 > You can still do range checks for these PCDs. We just don't > need an Invalid value for PcdCpuProcTraceOutputScheme >=20 > Thanks, >=20 > Mike >=20 > > -----Original Message----- > > From: Dong, Eric > > Sent: Thursday, August 17, 2017 1:52 AM > > To: Kinney, Michael D ; edk2- > > devel@lists.01.org > > Cc: Ni, Ruiyu ; Gao, Liming > > > Subject: RE: [edk2] [Patch 1/2] UefiCpuPkg: Add comments for > PCDs > > definition. > > > > Mike, > > > > My reason to defined disable options for memory size and > output scheme > > is I can use them as my default policy(all in disable status). > If not > > define disable options, I don't know which default value are > better > > than others. Also disable options also defined clearly invalid > tag and > > valuable to add user friendly code check like below: > > if ((ProcTraceData->ProcTraceMemSize >=3D > > EnumProcTraceMemDisable) || > > (ProcTraceData->ProcTraceOutputScheme >=3D > > OutputSchemeInvalid)) { > > return FALSE; > > } > > > > If you think these definitions are redundant, what's the > default value > > do you think is suitable for memory size and output scheme. > > > > Thanks, > > Eric > > -----Original Message----- > > From: Kinney, Michael D > > Sent: Thursday, August 17, 2017 3:55 PM > > To: Dong, Eric ; edk2-devel@lists.01.org; > Kinney, > > Michael D > > Cc: Ni, Ruiyu ; Gao, Liming > > > Subject: RE: [edk2] [Patch 1/2] UefiCpuPkg: Add comments for > PCDs > > definition. > > > > Eric, > > > > For a setup page, the scheme can be greyed out or hidden if > the > > feature is disabled. If the feature is disabled, it does not > make > > sense to allow the user to set the memory size or the scheme. > > > > Mike > > > > > -----Original Message----- > > > From: Dong, Eric > > > Sent: Wednesday, August 16, 2017 11:48 PM > > > To: Kinney, Michael D ; edk2- > > > devel@lists.01.org > > > Cc: Ni, Ruiyu ; Gao, Liming > > > ; Dong, Eric > > > Subject: RE: [edk2] [Patch 1/2] UefiCpuPkg: Add comments for > > PCDs > > > definition. > > > > > > Hi Mike, > > > > > > Processor trace feature need to get three inputs from end > > user. 1. > > > Enabe/disable, 2. Memory size, 3, Output scheme. > > > It will have three menus in UI to let end user to select. I > > want the > > > default value for all three menus are disabled, so I add > > invalid or > > > disable for both PCDs. Maybe I can enhance > > OutputSchemeInvalid to > > > OutputSchemeDisable for consistent with > > EnumProcTraceMemDisable. > > > > > > For the BITxx issue, I have submit a patch for it, please > > help to > > > review it. > > > > > > Thanks, > > > Eric > > > -----Original Message----- > > > From: Kinney, Michael D > > > Sent: Wednesday, August 16, 2017 11:59 PM > > > To: Dong, Eric ; edk2- > > devel@lists.01.org; Kinney, > > > Michael D > > > Cc: Ni, Ruiyu ; Gao, Liming > > > > > Subject: RE: [edk2] [Patch 1/2] UefiCpuPkg: Add comments for > > PCDs > > > definition. > > > > > > Eric, > > > > > > For the PCD PcdCpuProcTraceOutputScheme, the description of > > "Invalid > > > scheme" > > > does not seem like a good description for disable. > > > > > > Do we really need the value 0x02 for > PcdCpuProcTraceOutputScheme at > > > all? > > > Can we use PcdCpuFeaturesSetting set to 0x10 > > ProcTraceMemDisable to > > > know that the feature is disabled, and only look at > > > PcdCpuProcTraceOutputScheme if PcdCpuFeaturesSetting is set > > to a value > > > where the memory size is > 0? > > > > > > Also, when looking at where these PCD values are used in > > > UefiCpuPkg\Library\CpuCommonFeaturesLib\ProcTrace.c, I see > > use of > > > BITxx to modify the MSR_IA32_RTIT_CTL values. Please use > > the bit > > > fields in the MSR_IA32_RTIT_CTL_REGISTER structure instead. > > You can > > > work on this change as a separate patch. > > > > > > Thanks, > > > > > > Mike > > > > > > > > > > -----Original Message----- > > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] > > On > > > Behalf Of > > > > Eric Dong > > > > Sent: Monday, August 14, 2017 12:23 AM > > > > To: edk2-devel@lists.01.org > > > > Cc: Ni, Ruiyu ; Kinney, Michael D > > > > ; Gao, Liming > > > > > > > Subject: [edk2] [Patch 1/2] UefiCpuPkg: Add comments for > > > PCDs > > > > definition. > > > > > > > > Add valid/default values for PCD PcdCpuProcTraceMemSize > > and > > > > PcdCpuProcTraceOutputScheme in the comment part. > > > > > > > > Cc: Liming Gao > > > > Cc: Ruiyu Ni > > > > Cc: Michael D Kinney > > > > Contributed-under: TianoCore Contribution Agreement 1.0 > > > > Signed-off-by: Eric Dong > > > > --- > > > > UefiCpuPkg/UefiCpuPkg.dec | 28 > > ++++++++++++++++++++++++++-- > > > > 1 file changed, 26 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/UefiCpuPkg/UefiCpuPkg.dec > > > b/UefiCpuPkg/UefiCpuPkg.dec > > > > index 2ddeab4..b4e099d 100644 > > > > --- a/UefiCpuPkg/UefiCpuPkg.dec > > > > +++ b/UefiCpuPkg/UefiCpuPkg.dec > > > > @@ -285,12 +285,36 @@ > > > > # @ValidList 0x80000001 | 0 > > > > gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSetting|{0x00, > > > 0x00, 0x00, > > > > 0x00, 0x00, 0x00, 0x00, 0x00}|VOID*|0x00000019 > > > > > > > > - ## Contains the size of memory required when CPU > > > processor trace is > > > > enabled. > > > > + ## Contains the size of memory required when CPU > > > processor > > > > trace is enabled.

> > > > + # Default value is 0x10 which disables the processor > > > > trace.
> > > > + # 0x0 - 4K.
> > > > + # 0x1 - 8K.
> > > > + # 0x2 - 16K.
> > > > + # 0x3 - 32K.
> > > > + # 0x4 - 64K.
> > > > + # 0x5 - 128K.
> > > > + # 0x6 - 256K.
> > > > + # 0x7 - 512K.
> > > > + # 0x8 - 1M.
> > > > + # 0x9 - 2M.
> > > > + # 0xA - 4M.
> > > > + # 0xB - 8M.
> > > > + # 0xC - 16M.
> > > > + # 0xD - 32M.
> > > > + # 0xE - 64M.
> > > > + # 0xF - 128M.
> > > > + # 0x10 - ProcTraceMemDisable.
> > > > # @Prompt The memory size used for processor trace. > > > > + # @ValidRange 0x80000001 | 0 - 0x10 > > > > > > > > > > > > > gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize|0x10|UINT32|0 > > > x6 > > > > 0000012 > > > > > > > > - ## Contains the processor trace output scheme when CPU > > > processor > > > > trace is enabled. > > > > + ## Contains the processor trace output scheme when CPU > > > > processor trace is enabled.

> > > > + # Default value is 2 which disables the processor > > > trace.
# 0 > > > > + - Single Range output scheme.
# 1 - ToPA(Table of > > > physical > > > > + address) scheme.
# 2 - Invalid scheme.
> > > > # @Prompt The processor trace output scheme. > > > > + # @ValidRange 0x80000001 | 0 - 2 > > > > > > > > > > > > > gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme|0x2|UINT > > > 8| > > > > 0x60000015 > > > > > > > > [UserExtensions.TianoCore."ExtraFiles"] > > > > -- > > > > 2.7.0.windows.1 > > > > > > > > _______________________________________________ > > > > edk2-devel mailing list > > > > edk2-devel@lists.01.org > > > > https://lists.01.org/mailman/listinfo/edk2-devel