From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) (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 D0EC921E79022 for ; Thu, 17 Aug 2017 10:36:41 -0700 (PDT) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga105.fm.intel.com with ESMTP; 17 Aug 2017 10:39:08 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,388,1498546800"; d="scan'208";a="301496268" Received: from orsmsx106.amr.corp.intel.com ([10.22.225.133]) by fmsmga004.fm.intel.com with ESMTP; 17 Aug 2017 10:39:08 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.211]) by ORSMSX106.amr.corp.intel.com ([169.254.1.25]) with mapi id 14.03.0319.002; Thu, 17 Aug 2017 10:39:08 -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//50p8IAAhaKAgAAclAA= Date: Thu, 17 Aug 2017 17:39:07 +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.140] 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: Thu, 17 Aug 2017 17:36:42 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Eric, Define PcdCpuFeaturesSetting set to 0x10 ProcTraceMemDisable as the default. If this PCD is 0x10, then the ProcTrace=20 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. Define PcdCpuProcTraceOutputScheme set to 0 as the default but also add that this PCD is ignored if PcdCpuFeaturesSetting is set to 0x10 ProcTraceMemDisable. You can still do range checks for these PCDs. We just don't need an Invalid value for PcdCpuProcTraceOutputScheme Thanks, Mike > -----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. >=20 > Mike, >=20 > 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; > } >=20 > If you think these definitions are redundant, what's the > default value do you think is suitable for memory size and > output scheme. >=20 > 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. >=20 > Eric, >=20 > 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. >=20 > Mike >=20 > > -----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