Re: [Patch 2/2] UefiCpuPkg: Update default for PcdCpuProcTraceMemSize/PcdCpuProcTraceOutputScheme.

["Kinney, Michael D" <michael.d.kinney@intel.com>]

Eric,

Some Comments embedded below.  

Please make sure the UNI file is updated to match the DEC file.

For the enum PROC_TRACE_MEM_SIZE, I would prefer the enum values
have more than just "Enum" in the beginning.  It would be better
to use names like "ProcTraceMemSize" so the first enum value would
be "ProcTraceMemSize4KB".

Same for PROC_TRACE_OUTPUT_SCHEME.  Use "ProcTraceOutputScheme".

Thanks,

Mike

> -----Original Message-----
> From: Dong, Eric
> Sent: Tuesday, August 22, 2017 7:31 PM
> To: edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Ni, Ruiyu
> <ruiyu.ni@intel.com>
> Subject: [Patch 2/2] UefiCpuPkg: Update default for
> PcdCpuProcTraceMemSize/PcdCpuProcTraceOutputScheme.
> 
> These two definitions have redundant definition which can be
> handle by code.
> This patch update them to follow new code definitions.
> 
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  UefiCpuPkg/UefiCpuPkg.dec | 14 ++++++--------
>  UefiCpuPkg/UefiCpuPkg.uni |  8 +++-----
>  2 files changed, 9 insertions(+), 13 deletions(-)
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dec
> b/UefiCpuPkg/UefiCpuPkg.dec
> index b4e099d..51ae0e2 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -286,7 +286,7 @@
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSetting|{0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}|VOID*|0x00000019
> 
>    ## Contains the size of memory required when CPU processor
> trace is enabled.<BR><BR>

Add a reference to the PCD and bit that enables/disables CPU 
Processor trace and describe that this PCD is ignored if
CPU processor trace is disabled.


> -  #  Default value is 0x10 which disables the processor
> trace.<BR>
> +  #  Default value is 0x0 which use 4K memory size.<BR>

"Default value is 0x00 which means 4KB of memory is allocated if CPU processor trace is enabled."


>    #  0x0  -  4K.<BR>
>    #  0x1  -  8K.<BR>
>    #  0x2  -  16K.<BR>
> @@ -303,19 +303,17 @@
>    #  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|0x
> 60000012
> +  # @ValidRange  0x80000001 | 0 - 0xF
> +
> gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceMemSize|0x0|UINT32|0x6
> 0000012
> 
>    ## Contains the processor trace output scheme when CPU
> processor trace is enabled.<BR><BR>

Add a reference to the PCD and bit that enables/disable CPU 
Processor trace and describe that this PCD is ignored if
CPU processor trace is disabled.

> -  #  Default value is 2 which disables the processor
> trace.<BR>
> +  #  Default value is 1 which use single range output
> scheme.<BR>

The default value in the statement below is 0, not 1.
The UNI file looks correct.

>    #  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
> +  # @ValidRange  0x80000001 | 0 - 1
> +
> gUefiCpuPkgTokenSpaceGuid.PcdCpuProcTraceOutputScheme|0x0|UINT8
> |0x60000015
> 
>  [UserExtensions.TianoCore."ExtraFiles"]
>    UefiCpuPkgExtra.uni
> diff --git a/UefiCpuPkg/UefiCpuPkg.uni
> b/UefiCpuPkg/UefiCpuPkg.uni
> index 858e4a7..f3be041 100644
> --- a/UefiCpuPkg/UefiCpuPkg.uni
> +++ b/UefiCpuPkg/UefiCpuPkg.uni
> @@ -198,7 +198,7 @@
>  #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.<BR><BR>\n"
> -
> "Default value is 0x10 which disables the processor memory
> trace.<BR>\n"
> +
> "Default value is 0x0 which use 4K memory size.<BR>\n"
> 
> "0x0  -  4K.<BR>\n"
> 
> "0x1  -  8K.<BR>\n"
> 
> "0x2  -  16K.<BR>\n"
> @@ -215,12 +215,10 @@
> 
> "0xD  -  32M.<BR>\n"
> 
> "0xE  -  64M.<BR>\n"
> 
> "0xF  -  128M.<BR>\n"
> -
> "0x10 -  ProcTraceMemDisable.<BR>\n"
> 
>  #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuProcTraceOutputScheme_PROMP
> T  #language en-US "Processor Trace output scheme type."
> 
>  #string
> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuProcTraceOutputScheme_HELP
> #language en-US "User input the processor trace output scheme
> type.<BR><BR>\n"
> -
> "Default value is 2 which disables the processor memory
> trace.<BR>\n"
> 
> +
> "Default value is 0 which use single range output
> scheme.<BR>\n"
> 
> 
> "0 - Single Range output scheme.<BR>\n"
> 
> -
> "1 - ToPA(Table of physical address) scheme.<BR>\n"
> 
> -
> "2 - Invalid scheme.<BR>\n"
> \ No newline at end of file
> +
> "1 - ToPA(Table of physical address) scheme.<BR>\n"
> \ No newline at end of file
> --
> 2.7.0.windows.1