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

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

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