[Patch 0/2] Add comments for new Pcds

[Patch 0/2] Add comments for new Pcds

[Eric Dong]

Add missed Pcds definition and comments in dec and inf files.

Eric Dong (2):
  UefiCpuPkg: Add comments for PCDs definition.
  UefiCpuPkg UefiCpupkg.uni: Add new pcds comments.

 UefiCpuPkg/UefiCpuPkg.dec | 22 ++++++++++++++++++++++
 UefiCpuPkg/UefiCpuPkg.uni |  8 ++++++++
 2 files changed, 30 insertions(+)

-- 
2.10.1.windows.1

[Patch 1/2] UefiCpuPkg: Add comments for PCDs definition.

[Eric Dong]

Cc: Liming Gao <liming.gao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
 UefiCpuPkg/UefiCpuPkg.dec | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
index 2ddeab4..1f3a992 100644
--- a/UefiCpuPkg/UefiCpuPkg.dec
+++ b/UefiCpuPkg/UefiCpuPkg.dec
@@ -287,10 +287,32 @@
 
   ## Contains the size of memory required when CPU processor trace is enabled.
   # @Prompt The memory size used for 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
+  # @ValidRange  0x80000001 | 0 - 0x10
   gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize|0x10|UINT32|0x60000012
 
   ## Contains the processor trace output scheme when CPU processor trace is enabled.
   # @Prompt The processor trace output scheme.
+  #  0: Single Range output scheme.<BR>
+  #  1: ToPA(Table of physical address) scheme.<BR>
+  #  2: Invalid scheme.<BR>
+  # @ValidRange  0x80000001 | 0 - 2
   gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme|0x2|UINT8|0x60000015
 
 [UserExtensions.TianoCore."ExtraFiles"]
-- 
2.10.1.windows.1

[Patch 2/2] UefiCpuPkg UefiCpupkg.uni: Add new pcds comments.

[Eric Dong]

Cc: Liming Gao <liming.gao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
 UefiCpuPkg/UefiCpuPkg.uni | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni
index cd0ecab..dd62fc0 100644
--- a/UefiCpuPkg/UefiCpuPkg.uni
+++ b/UefiCpuPkg/UefiCpuPkg.uni
@@ -194,3 +194,11 @@
 #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuFeaturesSetting_PROMPT  #language en-US "Actual processor feature settings."
 
 #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuFeaturesSetting_HELP  #language en-US "Specifies actual settings for processor features, each bit corresponding to a specific feature."
+
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuProcTraceMemSize_PROMPT  #language en-US "Memory size used by Processor Trace feature."
+
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuProcTraceMemSize_HELP  #language en-US "User input the memory size can be used by processor trace feature."
+
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuProcTraceOutputScheme_PROMPT  #language en-US "Processor Trace output scheme type."
+
+#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuProcTraceOutputScheme_HELP  #language en-US "User input the processor trace output scheme type."
\ No newline at end of file
-- 
2.10.1.windows.1

Re: [Patch 0/2] Add comments for new Pcds

[Laszlo Ersek]

Eric,

On 08/09/17 08:32, Eric Dong wrote:
> Add missed Pcds definition and comments in dec and inf files.
> 
> Eric Dong (2):
>   UefiCpuPkg: Add comments for PCDs definition.
>   UefiCpuPkg UefiCpupkg.uni: Add new pcds comments.
> 
>  UefiCpuPkg/UefiCpuPkg.dec | 22 ++++++++++++++++++++++
>  UefiCpuPkg/UefiCpuPkg.uni |  8 ++++++++
>  2 files changed, 30 insertions(+)
> 

can you please write actual commit messages for your patches?

In this series, neither patch has a commit message body. Minimally
PcdCpuProcTraceMemSize and PcdCpuProcTraceOutputScheme should be named
in both commit messages.

I've noticed this as a tendency, and I feel it's now time for me to
speak up. Consider the following earlier patch sets:

* [edk2] [Patch v4 0/3] Enable Processor Trace feature
  https://lists.01.org/pipermail/edk2-devel/2017-August/012902.html

No commit message body on either of the three patches.

* [edk2] [Patch 0/3] Enable LMCE feature
  https://lists.01.org/pipermail/edk2-devel/2017-August/012731.html

No commit message body on either of the three patches.

Especially in the LMCE case, I tried to figure out how things fit
together, but the commit messages gave zero information.

IMO, without a commit message body, a patch cannot be considered
complete (unless the patch is trivial). I feel that reviewers shouldn't
accept empty commit messages either.

Commit messages are not there (just) for the author, but for everyone
else (too). Just because a patch is for UefiCpuPkg, someone else might
stumble upon it when bisecting a regression, and they will want to read
the justification. Commit messages also educate other project participants.

It's not necessary to repeat the same old background in every single
commit message, yes. But please provide pointers then, to
specifications, text files, earlier commits (that do have detailed
messages), and so on.

Yes, writing commit messages takes time; sometimes even *more* time than
writing the actual code. That's fine, it's part of open source
development. Please invest the time.

Thank you,
Laszlo

Re: [Patch 0/2] Add comments for new Pcds

[Dong, Eric]

Laszlo,

Thanks for your advice. I will pay more attention to the check in log later. 

Thanks,
Eric

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Wednesday, August 9, 2017 7:15 PM
To: Dong, Eric <eric.dong@intel.com>
Cc: edk2-devel@lists.01.org; Justen, Jordan L <jordan.l.justen@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Fan, Jeff <jeff.fan@intel.com>
Subject: Re: [edk2] [Patch 0/2] Add comments for new Pcds

Eric,

On 08/09/17 08:32, Eric Dong wrote:
> Add missed Pcds definition and comments in dec and inf files.
> 
> Eric Dong (2):
>   UefiCpuPkg: Add comments for PCDs definition.
>   UefiCpuPkg UefiCpupkg.uni: Add new pcds comments.
> 
>  UefiCpuPkg/UefiCpuPkg.dec | 22 ++++++++++++++++++++++  
> UefiCpuPkg/UefiCpuPkg.uni |  8 ++++++++
>  2 files changed, 30 insertions(+)
> 

can you please write actual commit messages for your patches?

In this series, neither patch has a commit message body. Minimally PcdCpuProcTraceMemSize and PcdCpuProcTraceOutputScheme should be named in both commit messages.

I've noticed this as a tendency, and I feel it's now time for me to speak up. Consider the following earlier patch sets:

* [edk2] [Patch v4 0/3] Enable Processor Trace feature
  https://lists.01.org/pipermail/edk2-devel/2017-August/012902.html

No commit message body on either of the three patches.

* [edk2] [Patch 0/3] Enable LMCE feature
  https://lists.01.org/pipermail/edk2-devel/2017-August/012731.html

No commit message body on either of the three patches.

Especially in the LMCE case, I tried to figure out how things fit together, but the commit messages gave zero information.

IMO, without a commit message body, a patch cannot be considered complete (unless the patch is trivial). I feel that reviewers shouldn't accept empty commit messages either.

Commit messages are not there (just) for the author, but for everyone else (too). Just because a patch is for UefiCpuPkg, someone else might stumble upon it when bisecting a regression, and they will want to read the justification. Commit messages also educate other project participants.

It's not necessary to repeat the same old background in every single commit message, yes. But please provide pointers then, to specifications, text files, earlier commits (that do have detailed messages), and so on.

Yes, writing commit messages takes time; sometimes even *more* time than writing the actual code. That's fine, it's part of open source development. Please invest the time.

Thank you,
Laszlo

Re: [Patch 1/2] UefiCpuPkg: Add comments for PCDs definition.

[Kinney, Michael D]

Eric,

Please add the default value settings to the detailed description.

For example, PcdCpuProcTraceMemSize should say that the default
value is 0x10 which disables the processor memory trace.

Also, the .uni file _HELP should contains the same information
as the detailed description, including the set of valid values
and their meanings.  Please see
gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel in 
MdePkg/MdePkg.uni for an example to follow.

Thanks,

Mike

> -----Original Message-----
> From: Dong, Eric
> Sent: Tuesday, August 8, 2017 11:32 PM
> To: edk2-devel@lists.01.org
> Cc: Gao, Liming <liming.gao@intel.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: [Patch 1/2] UefiCpuPkg: Add comments for PCDs
> definition.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/UefiCpuPkg.dec | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec
> b/UefiCpuPkg/UefiCpuPkg.dec
> index 2ddeab4..1f3a992 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -287,10 +287,32 @@
> 
>    ## Contains the size of memory required when CPU processor
> trace is enabled.
>    # @Prompt The memory size used for 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
> +  # @ValidRange  0x80000001 | 0 - 0x10
> 
> gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize|0x10|UINT32|0
> x60000012
> 
>    ## Contains the processor trace output scheme when CPU
> processor trace is enabled.
>    # @Prompt The processor trace output scheme.
> +  #  0: Single Range output scheme.<BR>
> +  #  1: ToPA(Table of physical address) scheme.<BR>
> +  #  2: Invalid scheme.<BR>
> +  # @ValidRange  0x80000001 | 0 - 2
> 
> gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme|0x2|UINT
> 8|0x60000015
> 
>  [UserExtensions.TianoCore."ExtraFiles"]
> --
> 2.10.1.windows.1

[Patch 1/2] UefiCpuPkg: Add comments for PCDs definition.

[Eric Dong]

Add valid/default values for PCD PcdCpuProcTraceMemSize and
PcdCpuProcTraceOutputScheme in the comment part.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
 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.<BR><BR>
+  #  Default value is 0x10 which disables the processor trace.<BR>
+  #  0x0  -  4K.<BR>
+  #  0x1  -  8K.<BR>
+  #  0x2  -  16K.<BR>
+  #  0x3  -  32K.<BR>
+  #  0x4  -  64K.<BR>
+  #  0x5  -  128K.<BR>
+  #  0x6  -  256K.<BR>
+  #  0x7  -  512K.<BR>
+  #  0x8  -  1M.<BR>
+  #  0x9  -  2M.<BR>
+  #  0xA  -  4M.<BR>
+  #  0xB  -  8M.<BR>
+  #  0xC  -  16M.<BR>
+  #  0xD  -  32M.<BR>
+  #  0xE  -  64M.<BR>
+  #  0xF  -  128M.<BR>
+  #  0x10 -  ProcTraceMemDisable.<BR>
   # @Prompt The memory size used for processor trace.
+  # @ValidRange  0x80000001 | 0 - 0x10
   gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize|0x10|UINT32|0x60000012
 
-  ## Contains the processor trace output scheme when CPU processor trace is enabled.
+  ## Contains the processor trace output scheme when CPU processor trace is enabled.<BR><BR>
+  #  Default value is 2 which disables the processor trace.<BR>
+  #  0 - Single Range output scheme.<BR>
+  #  1 - ToPA(Table of physical address) scheme.<BR>
+  #  2 - Invalid scheme.<BR>
   # @Prompt The processor trace output scheme.
+  # @ValidRange  0x80000001 | 0 - 2
   gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme|0x2|UINT8|0x60000015
 
 [UserExtensions.TianoCore."ExtraFiles"]
-- 
2.7.0.windows.1

Re: [Patch 1/2] UefiCpuPkg: Add comments for PCDs definition.

[Kinney, Michael D]

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 <ruiyu.ni@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
> 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 <liming.gao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  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.<BR><BR>
> +  #  Default value is 0x10 which disables the processor
> trace.<BR>
> +  #  0x0  -  4K.<BR>
> +  #  0x1  -  8K.<BR>
> +  #  0x2  -  16K.<BR>
> +  #  0x3  -  32K.<BR>
> +  #  0x4  -  64K.<BR>
> +  #  0x5  -  128K.<BR>
> +  #  0x6  -  256K.<BR>
> +  #  0x7  -  512K.<BR>
> +  #  0x8  -  1M.<BR>
> +  #  0x9  -  2M.<BR>
> +  #  0xA  -  4M.<BR>
> +  #  0xB  -  8M.<BR>
> +  #  0xC  -  16M.<BR>
> +  #  0xD  -  32M.<BR>
> +  #  0xE  -  64M.<BR>
> +  #  0xF  -  128M.<BR>
> +  #  0x10 -  ProcTraceMemDisable.<BR>
>    # @Prompt The memory size used for processor trace.
> +  # @ValidRange  0x80000001 | 0 - 0x10
> 
> gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize|0x10|UINT32|0x6
> 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.<BR><BR>
> +  #  Default value is 2 which disables the processor trace.<BR>
> +  #  0 - Single Range output scheme.<BR>
> +  #  1 - ToPA(Table of physical address) scheme.<BR>
> +  #  2 - Invalid scheme.<BR>
>    # @Prompt The processor trace output scheme.
> +  # @ValidRange  0x80000001 | 0 - 2
> 
> gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme|0x2|UINT8|
> 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

Re: [Patch 1/2] UefiCpuPkg: Add comments for PCDs definition.

[Dong, Eric]

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 <eric.dong@intel.com>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Gao, Liming <liming.gao@intel.com>
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 <ruiyu.ni@intel.com>; Kinney, Michael D 
> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
> 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 <liming.gao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  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.<BR><BR>
> +  #  Default value is 0x10 which disables the processor
> trace.<BR>
> +  #  0x0  -  4K.<BR>
> +  #  0x1  -  8K.<BR>
> +  #  0x2  -  16K.<BR>
> +  #  0x3  -  32K.<BR>
> +  #  0x4  -  64K.<BR>
> +  #  0x5  -  128K.<BR>
> +  #  0x6  -  256K.<BR>
> +  #  0x7  -  512K.<BR>
> +  #  0x8  -  1M.<BR>
> +  #  0x9  -  2M.<BR>
> +  #  0xA  -  4M.<BR>
> +  #  0xB  -  8M.<BR>
> +  #  0xC  -  16M.<BR>
> +  #  0xD  -  32M.<BR>
> +  #  0xE  -  64M.<BR>
> +  #  0xF  -  128M.<BR>
> +  #  0x10 -  ProcTraceMemDisable.<BR>
>    # @Prompt The memory size used for processor trace.
> +  # @ValidRange  0x80000001 | 0 - 0x10
> 
> gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize|0x10|UINT32|0x6
> 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.<BR><BR>
> +  #  Default value is 2 which disables the processor trace.<BR>  #  0 
> + - Single Range output scheme.<BR>  #  1 - ToPA(Table of physical 
> + address) scheme.<BR>  #  2 - Invalid scheme.<BR>
>    # @Prompt The processor trace output scheme.
> +  # @ValidRange  0x80000001 | 0 - 2
> 
> gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme|0x2|UINT8|
> 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

Re: [Patch 1/2] UefiCpuPkg: Add comments for PCDs definition.

[Kinney, Michael D]

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 <michael.d.kinney@intel.com>; edk2-
> devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Gao, Liming
> <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>
> 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 <eric.dong@intel.com>; edk2-devel@lists.01.org;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> 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 <ruiyu.ni@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> > 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 <liming.gao@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> >  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.<BR><BR>
> > +  #  Default value is 0x10 which disables the processor
> > trace.<BR>
> > +  #  0x0  -  4K.<BR>
> > +  #  0x1  -  8K.<BR>
> > +  #  0x2  -  16K.<BR>
> > +  #  0x3  -  32K.<BR>
> > +  #  0x4  -  64K.<BR>
> > +  #  0x5  -  128K.<BR>
> > +  #  0x6  -  256K.<BR>
> > +  #  0x7  -  512K.<BR>
> > +  #  0x8  -  1M.<BR>
> > +  #  0x9  -  2M.<BR>
> > +  #  0xA  -  4M.<BR>
> > +  #  0xB  -  8M.<BR>
> > +  #  0xC  -  16M.<BR>
> > +  #  0xD  -  32M.<BR>
> > +  #  0xE  -  64M.<BR>
> > +  #  0xF  -  128M.<BR>
> > +  #  0x10 -  ProcTraceMemDisable.<BR>
> >    # @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.<BR><BR>
> > +  #  Default value is 2 which disables the processor
> trace.<BR>  #  0
> > + - Single Range output scheme.<BR>  #  1 - ToPA(Table of
> physical
> > + address) scheme.<BR>  #  2 - Invalid scheme.<BR>
> >    # @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

Re: [Patch 1/2] UefiCpuPkg: Add comments for PCDs definition.

[Dong, Eric]

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 >= EnumProcTraceMemDisable) ||
      (ProcTraceData->ProcTraceOutputScheme >= 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 <eric.dong@intel.com>; edk2-devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Gao, Liming <liming.gao@intel.com>
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 <michael.d.kinney@intel.com>; edk2- 
> devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Gao, Liming 
> <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>
> 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 <eric.dong@intel.com>; edk2-devel@lists.01.org; Kinney, 
> Michael D <michael.d.kinney@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Gao, Liming <liming.gao@intel.com>
> 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 <ruiyu.ni@intel.com>; Kinney, Michael D 
> > <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> > 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 <liming.gao@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> >  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.<BR><BR>
> > +  #  Default value is 0x10 which disables the processor
> > trace.<BR>
> > +  #  0x0  -  4K.<BR>
> > +  #  0x1  -  8K.<BR>
> > +  #  0x2  -  16K.<BR>
> > +  #  0x3  -  32K.<BR>
> > +  #  0x4  -  64K.<BR>
> > +  #  0x5  -  128K.<BR>
> > +  #  0x6  -  256K.<BR>
> > +  #  0x7  -  512K.<BR>
> > +  #  0x8  -  1M.<BR>
> > +  #  0x9  -  2M.<BR>
> > +  #  0xA  -  4M.<BR>
> > +  #  0xB  -  8M.<BR>
> > +  #  0xC  -  16M.<BR>
> > +  #  0xD  -  32M.<BR>
> > +  #  0xE  -  64M.<BR>
> > +  #  0xF  -  128M.<BR>
> > +  #  0x10 -  ProcTraceMemDisable.<BR>
> >    # @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.<BR><BR>
> > +  #  Default value is 2 which disables the processor
> trace.<BR>  #  0
> > + - Single Range output scheme.<BR>  #  1 - ToPA(Table of
> physical
> > + address) scheme.<BR>  #  2 - Invalid scheme.<BR>
> >    # @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

Re: [Patch 1/2] UefiCpuPkg: Add comments for PCDs definition.

[Kinney, Michael D]

Eric,

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.

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 <michael.d.kinney@intel.com>; edk2-
> devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> 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 >=
> EnumProcTraceMemDisable) ||
>       (ProcTraceData->ProcTraceOutputScheme >=
> 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 <eric.dong@intel.com>; edk2-devel@lists.01.org;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> 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 <michael.d.kinney@intel.com>; edk2-
> > devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Gao, Liming
> > <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>
> > 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 <eric.dong@intel.com>; edk2-
> devel@lists.01.org; Kinney,
> > Michael D <michael.d.kinney@intel.com>
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> > 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 <ruiyu.ni@intel.com>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>; Gao, Liming
> > <liming.gao@intel.com>
> > > 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 <liming.gao@intel.com>
> > > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > > ---
> > >  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.<BR><BR>
> > > +  #  Default value is 0x10 which disables the processor
> > > trace.<BR>
> > > +  #  0x0  -  4K.<BR>
> > > +  #  0x1  -  8K.<BR>
> > > +  #  0x2  -  16K.<BR>
> > > +  #  0x3  -  32K.<BR>
> > > +  #  0x4  -  64K.<BR>
> > > +  #  0x5  -  128K.<BR>
> > > +  #  0x6  -  256K.<BR>
> > > +  #  0x7  -  512K.<BR>
> > > +  #  0x8  -  1M.<BR>
> > > +  #  0x9  -  2M.<BR>
> > > +  #  0xA  -  4M.<BR>
> > > +  #  0xB  -  8M.<BR>
> > > +  #  0xC  -  16M.<BR>
> > > +  #  0xD  -  32M.<BR>
> > > +  #  0xE  -  64M.<BR>
> > > +  #  0xF  -  128M.<BR>
> > > +  #  0x10 -  ProcTraceMemDisable.<BR>
> > >    # @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.<BR><BR>
> > > +  #  Default value is 2 which disables the processor
> > trace.<BR>  #  0
> > > + - Single Range output scheme.<BR>  #  1 - ToPA(Table of
> > physical
> > > + address) scheme.<BR>  #  2 - Invalid scheme.<BR>
> > >    # @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

Re: [Patch 1/2] UefiCpuPkg: Add comments for PCDs definition.

[Kinney, Michael D]

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 <michael.d.kinney@intel.com>; edk2-
> devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: RE: [edk2] [Patch 1/2] UefiCpuPkg: Add comments for
> PCDs definition.
> 
> Mike,
> 
> I think you misuse PcdCpuFeaturesSetting for
> PcdCpuProcTraceMemSize, right?
> ProcTrace feature is checked by BIT44(CPU_FEATURE_PROC_TRACE) in
> PcdCpuFeaturesSetting.
> 
> 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?
> 
> Thanks,
> Eric
> -----Original Message-----
> From: Kinney, Michael D
> Sent: Friday, August 18, 2017 1:39 AM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Subject: RE: [edk2] [Patch 1/2] UefiCpuPkg: Add comments for
> PCDs definition.
> 
> Eric,
> 
> 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.
> 
> 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 <michael.d.kinney@intel.com>; edk2-
> > devel@lists.01.org
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> > 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 >=
> > EnumProcTraceMemDisable) ||
> >       (ProcTraceData->ProcTraceOutputScheme >=
> > 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 <eric.dong@intel.com>; edk2-devel@lists.01.org;
> Kinney,
> > Michael D <michael.d.kinney@intel.com>
> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> > 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 <michael.d.kinney@intel.com>; edk2-
> > > devel@lists.01.org
> > > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Gao, Liming
> > > <liming.gao@intel.com>; Dong, Eric <eric.dong@intel.com>
> > > 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 <eric.dong@intel.com>; edk2-
> > devel@lists.01.org; Kinney,
> > > Michael D <michael.d.kinney@intel.com>
> > > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Gao, Liming
> > <liming.gao@intel.com>
> > > 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 <ruiyu.ni@intel.com>; Kinney, Michael D
> > > > <michael.d.kinney@intel.com>; Gao, Liming
> > > <liming.gao@intel.com>
> > > > 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 <liming.gao@intel.com>
> > > > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > > Contributed-under: TianoCore Contribution Agreement 1.0
> > > > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > > > ---
> > > >  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.<BR><BR>
> > > > +  #  Default value is 0x10 which disables the processor
> > > > trace.<BR>
> > > > +  #  0x0  -  4K.<BR>
> > > > +  #  0x1  -  8K.<BR>
> > > > +  #  0x2  -  16K.<BR>
> > > > +  #  0x3  -  32K.<BR>
> > > > +  #  0x4  -  64K.<BR>
> > > > +  #  0x5  -  128K.<BR>
> > > > +  #  0x6  -  256K.<BR>
> > > > +  #  0x7  -  512K.<BR>
> > > > +  #  0x8  -  1M.<BR>
> > > > +  #  0x9  -  2M.<BR>
> > > > +  #  0xA  -  4M.<BR>
> > > > +  #  0xB  -  8M.<BR>
> > > > +  #  0xC  -  16M.<BR>
> > > > +  #  0xD  -  32M.<BR>
> > > > +  #  0xE  -  64M.<BR>
> > > > +  #  0xF  -  128M.<BR>
> > > > +  #  0x10 -  ProcTraceMemDisable.<BR>
> > > >    # @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.<BR><BR>
> > > > +  #  Default value is 2 which disables the processor
> > > trace.<BR>  #  0
> > > > + - Single Range output scheme.<BR>  #  1 - ToPA(Table of
> > > physical
> > > > + address) scheme.<BR>  #  2 - Invalid scheme.<BR>
> > > >    # @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