Re: [Patch 2/3] UefiCpuPkg/RegisterCpuFeaturesLib: Optimize PCD PcdCpuFeaturesUserConfiguration.

["Dong, Eric" <eric.dong@intel.com>]

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, February 13, 2019 10:44 AM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: Re: [Patch 2/3] UefiCpuPkg/RegisterCpuFeaturesLib: Optimize PCD
> PcdCpuFeaturesUserConfiguration.
> 
> On 02/13/19 03:04, Eric Dong wrote:
> > In current implementation, PCD PcdCpuFeaturesUserConfiguration used as
> > user enabled CPU features list. It is initialzied in platform driver
> > and as an input for CpuFeatures driver. PCD PcdCpuFeaturesSetting used
> > as an output for the final enabled CPU features list. For now,
> > PcdCpuFeaturesUserConfiguration is only used as an input and
> > PcdCpuFeaturesSetting only used as an output.
> >
> > This change merge PcdCpuFeaturesUserConfiguration into
> > PcdCpuFeaturesSetting.
> > Use PcdCpuFeaturesSetting as input for the user input feature setting
> > Use PcdCpuFeaturesSetting as output for the final CPU feature setting
> >
> > BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=1375
> >
> > Cc: Ray Ni <Ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> >  .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 37 ++++++++++++----
> ------
> >  .../DxeRegisterCpuFeaturesLib.inf                  |  1 -
> >  .../PeiRegisterCpuFeaturesLib.inf                  |  1 -
> >  .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h   |  1 -
> >  UefiCpuPkg/UefiCpuPkg.dec                          |  7 ++--
> >  5 files changed, 22 insertions(+), 25 deletions(-)
> >
> > diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > index bae92b89a6..4ebd0025b4 100644
> > ---
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.
> > +++ c
> > @@ -54,41 +54,41 @@ SetSettingPcd (
> >  }
> >
> >  /**
> > -  Worker function to get PcdCpuFeaturesSupport.
> > +  Worker function to get PcdCpuFeaturesSetting.
> >
> >    @return  The pointer to CPU feature bits mask buffer.
> >  **/
> >  UINT8 *
> > -GetSupportPcd (
> > +GetSettingPcd (
> >    VOID
> >    )
> >  {
> > -  UINT8                  *SupportBitMask;
> > +  UINT8                  *SettingBitMask;
> >
> > -  SupportBitMask = AllocateCopyPool (
> > -          PcdGetSize (PcdCpuFeaturesSupport),
> > -          PcdGetPtr (PcdCpuFeaturesSupport)
> > +  SettingBitMask = AllocateCopyPool (
> > +          PcdGetSize (PcdCpuFeaturesSetting),
> > +          PcdGetPtr (PcdCpuFeaturesSetting)
> >            );
> 
> (1) The indentation of the AllocateCopyPool() arguments is not idiomatic. I
> suggest fixing that, even if it means diverging from the old code.

Agree, will include this change in next version patches.

> 
> > -  ASSERT (SupportBitMask != NULL);
> > +  ASSERT (SettingBitMask != NULL);
> >
> > -  return SupportBitMask;
> > +  return SettingBitMask;
> >  }
> >
> >  /**
> > -  Worker function to get PcdCpuFeaturesUserConfiguration.
> > +  Worker function to get PcdCpuFeaturesSupport.
> >
> >    @return  The pointer to CPU feature bits mask buffer.
> >  **/
> >  UINT8 *
> > -GetConfigurationPcd (
> > +GetSupportPcd (
> >    VOID
> >    )
> >  {
> >    UINT8                  *SupportBitMask;
> >
> >    SupportBitMask = AllocateCopyPool (
> > -          PcdGetSize (PcdCpuFeaturesUserConfiguration),
> > -          PcdGetPtr (PcdCpuFeaturesUserConfiguration)
> > +          PcdGetSize (PcdCpuFeaturesSupport),
> > +          PcdGetPtr (PcdCpuFeaturesSupport)
> >            );
> >    ASSERT (SupportBitMask != NULL);
> >
> > @@ -287,7 +287,6 @@ CpuInitDataInitialize (
> >    // Get support and configuration PCDs
> >    //
> >    CpuFeaturesData->SupportPcd       = GetSupportPcd ();
> > -  CpuFeaturesData->ConfigurationPcd = GetConfigurationPcd ();  }
> >
> >  /**
> > @@ -595,6 +594,9 @@ AnalysisProcessorFeatures (
> >    CPU_FEATURE_DEPENDENCE_TYPE          AfterDep;
> >    CPU_FEATURE_DEPENDENCE_TYPE          NoneNeibBeforeDep;
> >    CPU_FEATURE_DEPENDENCE_TYPE          NoneNeibAfterDep;
> > +  UINT8                                *ConfigurationPcd;
> > +
> > +  ConfigurationPcd = NULL;
> >
> >    CpuFeaturesData = GetCpuFeaturesData ();
> >    CpuFeaturesData->CapabilityPcd = AllocatePool
> > (CpuFeaturesData->BitMaskSize); @@ -610,10 +612,13 @@
> AnalysisProcessorFeatures (
> >    //
> >    // Calculate the last setting
> >    //
> > -
> >    CpuFeaturesData->SettingPcd = AllocateCopyPool (CpuFeaturesData-
> >BitMaskSize, CpuFeaturesData->CapabilityPcd);
> >    ASSERT (CpuFeaturesData->SettingPcd != NULL);
> > -  SupportedMaskAnd (CpuFeaturesData->SettingPcd,
> > CpuFeaturesData->ConfigurationPcd);
> > +  ConfigurationPcd = GetSettingPcd ();  SupportedMaskAnd
> > + (CpuFeaturesData->SettingPcd, ConfigurationPcd);  if
> > + (ConfigurationPcd != NULL) {
> > +    FreePool (ConfigurationPcd);
> > +  }
> 
> (2) Why is it necessary to set ConfigurationPcd to NULL at the beginning of
> the function? And why is the NULL check necessary here?
> GetSettingPcd() can never return NULL.
> 
> (I don't just mean the ASSERT, but also the fact that we pass
> ConfigurationPcd to SupportedMaskAnd() without checking
> ConfigurationPcd for NULL.)
> 

My original purpose is in release mode the ASSERT will not work, so NULL maybe return and we need to check it before using. 
But right, if the NULL is return, the SupportedMaskAnd function will failed first. so I think here we can ignore the NULL check.
Will remove the NULL related check in my next version changes.

> >
> >    //
> >    // Save PCDs and display CPU PCDs
> > @@ -643,8 +648,6 @@ AnalysisProcessorFeatures (
> >      }
> >      DEBUG ((DEBUG_INFO, "PcdCpuFeaturesSupport:\n"));
> >      DumpCpuFeatureMask (CpuFeaturesData->SupportPcd);
> > -    DEBUG ((DEBUG_INFO, "PcdCpuFeaturesUserConfiguration:\n"));
> > -    DumpCpuFeatureMask (CpuFeaturesData->ConfigurationPcd);
> >      DEBUG ((DEBUG_INFO, "PcdCpuFeaturesCapability:\n"));
> >      DumpCpuFeatureMask (CpuFeaturesData->CapabilityPcd);
> >      DEBUG ((DEBUG_INFO, "PcdCpuFeaturesSetting:\n")); diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.
> > inf
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.
> > inf
> > index 362e0c6cd1..b7dc70808f 100644
> > ---
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.
> > inf
> > +++
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeatures
> > +++ Lib.inf
> > @@ -56,7 +56,6 @@
> >  [Pcd]
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress                        ##
> CONSUMES
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSupport                      ##
> CONSUMES
> > -  gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesUserConfiguration
> ## CONSUMES
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesCapability                   ##
> PRODUCES
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSetting                      ##
> PRODUCES
> >
> > diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.
> > inf
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.
> > inf
> > index f3907e25d3..cd69721a2d 100644
> > ---
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.
> > inf
> > +++
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeatures
> > +++ Lib.inf
> > @@ -57,7 +57,6 @@
> >  [Pcd]
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress                        ##
> CONSUMES
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSupport                      ##
> CONSUMES
> > -  gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesUserConfiguration
> ## CONSUMES
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesCapability                   ##
> PRODUCES
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSetting                      ##
> PRODUCES
> >
> > diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> > index 21dd5773a6..3e0a342fd1 100644
> > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> > @@ -83,7 +83,6 @@ typedef struct {
> >    CPU_FEATURES_INIT_ORDER  *InitOrder;
> >    UINT8                    *SupportPcd;
> >    UINT8                    *CapabilityPcd;
> > -  UINT8                    *ConfigurationPcd;
> >    UINT8                    *SettingPcd;
> >
> >    UINT32                   NumberOfCpus;
> > diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec
> > index cb05fa2660..793490872f 100644
> > --- a/UefiCpuPkg/UefiCpuPkg.dec
> > +++ b/UefiCpuPkg/UefiCpuPkg.dec
> > @@ -261,10 +261,6 @@
> >    # @Prompt SMM CPU Synchronization Method.
> >
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x00|UINT8|0x60000
> 014
> >
> > -  ## Specifies user's desired settings for enabling/disabling processor
> features.
> > -  # @Prompt User settings for enabling/disabling processor features.
> > -  gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesUserConfiguration|{0x00,
> > 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}|VOID*|0x00000017
> > -
> >    ## Specifies the On-demand clock modulation duty cycle when ACPI
> feature is enabled.
> >    # @Prompt The encoded values for target duty cycle modulation.
> >    # @ValidRange  0x80000001 | 0 - 15
> > @@ -292,7 +288,8 @@
> >    # @ValidList   0x80000001 | 0
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesCapability|{0x00, 0x00,
> > 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}|VOID*|0x00000018
> >
> > -  ## Specifies actual settings for processor features, each bit corresponding
> to a specific feature.
> > +  ## As input, specifies user's desired settings for enabling/disabling
> processor features.
> > +  ## As output, specifies actual settings for processor features, each bit
> corresponding to a specific feature.
> >    # @Prompt Actual processor feature settings.
> 
> (3) Shouldn't you update @Prompt as well?

Agree, will include this change in my next version changes.

> 
> >    # @ValidList   0x80000001 | 0
> >    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSetting|{0x00, 0x00, 0x00,
> > 0x00, 0x00, 0x00, 0x00, 0x00}|VOID*|0x00000019
> >
> 
> Thanks
> Laszlo