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

["Ni, Ray" <ray.ni@intel.com>]

> -----Original Message-----
> From: Dong, Eric <eric.dong@intel.com>
> Sent: Wednesday, February 13, 2019 10:04 AM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [Patch 2/3] UefiCpuPkg/RegisterCpuFeaturesLib: Optimize PCD
> PcdCpuFeaturesUserConfiguration.
> 
> 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)
>            );
> -  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;

1. Can we use the name "SettingPcd"? To use the same term.

> +
> +  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 ();

2. SuportedMaskAnd() function doesn't change the parameter 2.
So how about we pass PcdGetPtr (PcdCpuFeaturesSetting) directly to
SupportedMaskAnd()?

> +  SupportedMaskAnd (CpuFeaturesData->SettingPcd, ConfigurationPcd);  if
> + (ConfigurationPcd != NULL) {
> +    FreePool (ConfigurationPcd);
> +  }
> 
>    //
>    // 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);

3. The debug message cannot be just deleted.
We still need to dump the original PcdCpuFeaturesSetting.

>      DEBUG ((DEBUG_INFO, "PcdCpuFeaturesCapability:\n"));
>      DumpCpuFeatureMask (CpuFeaturesData->CapabilityPcd);
>      DEBUG ((DEBUG_INFO, "PcdCpuFeaturesSetting:\n")); diff --git
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.i
> nf
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.i
> nf
> index 362e0c6cd1..b7dc70808f 100644
> ---
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.i
> nf
> +++
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLi
> +++ b.inf
> @@ -56,7 +56,6 @@
>  [Pcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress                        ##
> CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSupport                      ##
> CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesUserConfiguration            ##
> CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesCapability                   ##
> PRODUCES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSetting                      ##
> PRODUCES

4. For PcdCpuFeaturesSetting, "CONSUMES" should be mentioned in the comments.

> 
> diff --git
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.in
> f
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.in
> f
> index f3907e25d3..cd69721a2d 100644
> ---
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.in
> f
> +++
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLi
> +++ b.inf
> @@ -57,7 +57,6 @@
>  [Pcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress                        ##
> CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSupport                      ##
> CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesUserConfiguration            ##
> CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesCapability                   ##
> PRODUCES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSetting                      ##
> PRODUCES

5. Similar as #4.

> 
> 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.
>    # @ValidList   0x80000001 | 0
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSetting|{0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00}|VOID*|0x00000019
> --
> 2.15.0.windows.1