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

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

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