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

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

Minor comments:

1. Please add "CONST" before AndFeatureBitMask to declare that it won't be changed in the routine.
SupportedMaskAnd (
  IN UINT8               *SupportedFeatureMask,
  IN CONST UINT8         *AndFeatureBitMask
  )
2. Please update the year in the copyright.

With these comments addressed, Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Eric Dong
> Sent: Friday, March 1, 2019 1:40 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [Patch v2 2/4] UefiCpuPkg/RegisterCpuFeaturesLib: Optimize
> PCD PcdCpuFeaturesUserConfiguration.
> 
> 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
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
>  .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 71 +++++-----------------
>  .../DxeRegisterCpuFeaturesLib.inf                  |  3 +-
>  .../PeiRegisterCpuFeaturesLib.inf                  |  3 +-
>  .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h   |  1 -
>  UefiCpuPkg/UefiCpuPkg.dec                          |  9 +--
>  5 files changed, 21 insertions(+), 66 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> index bae92b89a6..d877caff74 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> @@ -21,16 +21,21 @@ CHAR16 *mRegisterTypeStr[] = {L"MSR", L"CR",
> L"MMIO", L"CACHE", L"SEMAP", L"INVA
>    Worker function to save PcdCpuFeaturesCapability.
> 
>    @param[in]  SupportedFeatureMask  The pointer to CPU feature bits mask
> buffer
> +  @param[in]  FeatureMaskSize       CPU feature bits mask buffer size.
> +
>  **/
>  VOID
>  SetCapabilityPcd (
> -  IN UINT8               *SupportedFeatureMask
> +  IN UINT8               *SupportedFeatureMask,
> +  IN UINT32              FeatureMaskSize
>    )
>  {
>    EFI_STATUS             Status;
>    UINTN                  BitMaskSize;
> 
>    BitMaskSize = PcdGetSize (PcdCpuFeaturesCapability);
> +  ASSERT (FeatureMaskSize == BitMaskSize);
> +
>    Status = PcdSetPtrS (PcdCpuFeaturesCapability, &BitMaskSize,
> SupportedFeatureMask);
>    ASSERT_EFI_ERROR (Status);
>  }
> @@ -53,48 +58,6 @@ SetSettingPcd (
>    ASSERT_EFI_ERROR (Status);
>  }
> 
> -/**
> -  Worker function to get PcdCpuFeaturesSupport.
> -
> -  @return  The pointer to CPU feature bits mask buffer.
> -**/
> -UINT8 *
> -GetSupportPcd (
> -  VOID
> -  )
> -{
> -  UINT8                  *SupportBitMask;
> -
> -  SupportBitMask = AllocateCopyPool (
> -          PcdGetSize (PcdCpuFeaturesSupport),
> -          PcdGetPtr (PcdCpuFeaturesSupport)
> -          );
> -  ASSERT (SupportBitMask != NULL);
> -
> -  return SupportBitMask;
> -}
> -
> -/**
> -  Worker function to get PcdCpuFeaturesUserConfiguration.
> -
> -  @return  The pointer to CPU feature bits mask buffer.
> -**/
> -UINT8 *
> -GetConfigurationPcd (
> -  VOID
> -  )
> -{
> -  UINT8                  *SupportBitMask;
> -
> -  SupportBitMask = AllocateCopyPool (
> -          PcdGetSize (PcdCpuFeaturesUserConfiguration),
> -          PcdGetPtr (PcdCpuFeaturesUserConfiguration)
> -          );
> -  ASSERT (SupportBitMask != NULL);
> -
> -  return SupportBitMask;
> -}
> -
>  /**
>    Collects CPU type and feature information.
> 
> @@ -287,7 +250,6 @@ CpuInitDataInitialize (
>    // Get support and configuration PCDs
>    //
>    CpuFeaturesData->SupportPcd       = GetSupportPcd ();
> -  CpuFeaturesData->ConfigurationPcd = GetConfigurationPcd ();  }
> 
>  /**
> @@ -610,16 +572,9 @@ AnalysisProcessorFeatures (
>    //
>    // Calculate the last setting
>    //
> -
>    CpuFeaturesData->SettingPcd = AllocateCopyPool (CpuFeaturesData-
> >BitMaskSize, CpuFeaturesData->CapabilityPcd);
>    ASSERT (CpuFeaturesData->SettingPcd != NULL);
> -  SupportedMaskAnd (CpuFeaturesData->SettingPcd, CpuFeaturesData-
> >ConfigurationPcd);
> -
> -  //
> -  // Save PCDs and display CPU PCDs
> -  //
> -  SetCapabilityPcd (CpuFeaturesData->CapabilityPcd);
> -  SetSettingPcd (CpuFeaturesData->SettingPcd);
> +  SupportedMaskAnd (CpuFeaturesData->SettingPcd, PcdGetPtr
> + (PcdCpuFeaturesSetting));
> 
>    //
>    // Dump the last CPU feature list
> @@ -643,14 +598,20 @@ 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"));
> +    DEBUG ((DEBUG_INFO, "Origin PcdCpuFeaturesSetting:\n"));
> +    DumpCpuFeatureMask (PcdGetPtr (PcdCpuFeaturesSetting));
> +    DEBUG ((DEBUG_INFO, "Final PcdCpuFeaturesSetting:\n"));
>      DumpCpuFeatureMask (CpuFeaturesData->SettingPcd);
>    );
> 
> +  //
> +  // Save PCDs and display CPU PCDs
> +  //
> +  SetCapabilityPcd (CpuFeaturesData->CapabilityPcd,
> + CpuFeaturesData->BitMaskSize);  SetSettingPcd
> + (CpuFeaturesData->SettingPcd);
> +
>    for (ProcessorNumber = 0; ProcessorNumber < NumberOfCpus;
> ProcessorNumber++) {
>      CpuInitOrder = &CpuFeaturesData->InitOrder[ProcessorNumber];
>      Entry = GetFirstNode (&CpuFeaturesData->FeatureList); diff --git
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.inf
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.inf
> index 362e0c6cd1..957ca87ff1 100644
> ---
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.inf
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLi
> +++ b.inf
> @@ -56,9 +56,8 @@
>  [Pcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress                        ##
> CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSupport                      ##
> CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesUserConfiguration            ##
> CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesCapability                   ##
> PRODUCES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSetting                      ##
> PRODUCES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSetting                      ##
> PRODUCES ## CONSUMES
> 
>  [Depex]
>    gEfiMpServiceProtocolGuid AND gEdkiiCpuFeaturesSetDoneGuid diff --git
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.inf
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.inf
> index f3907e25d3..659aa9eaaf 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.inf
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLi
> +++ b.inf
> @@ -57,9 +57,8 @@
>  [Pcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuS3DataAddress                        ##
> CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSupport                      ##
> CONSUMES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesUserConfiguration            ##
> CONSUMES
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesCapability                   ##
> PRODUCES
> -  gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSetting                      ##
> PRODUCES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSetting                      ##
> CONSUMES ## PRODUCES
> 
>  [Depex]
>    gEfiPeiMpServicesPpiGuid AND gEdkiiCpuFeaturesSetDoneGuid 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..cd9b2d1b03 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dec
> +++ b/UefiCpuPkg/UefiCpuPkg.dec
> @@ -261,10 +261,6 @@
>    # @Prompt SMM CPU Synchronization Method.
> 
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x00|UINT8|0x60000014
> 
> -  ## 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,8 +288,9 @@
>    # @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.
> -  # @Prompt Actual processor feature settings.
> +  ## 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 As input, specifies user's desired processor feature settings. As
> output, specifies actual processor feature settings.
>    # @ValidList   0x80000001 | 0
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSetting|{0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00}|VOID*|0x00000019
> 
> --
> 2.15.0.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel