Re: [edk2] [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Simplify PcdCpuFeaturesSupport.

2019-02-14 Thread Ni, Ray

Reviewed-by: Ray Ni 

On 2/13/2019 10:04 AM, Eric Dong wrote:

PcdCpuFeaturesSupport used to specify the platform policy about
what CPU features this platform supports. This value is decide by
platform owner, not by hardware. After this change, this PCD will
be used in IsCpuFeatureSupported function only.

Now RegisterCpuFeaturesLib use this PCD as an template to Get the
pcd size. Update the code logic to replace it with
PcdCpuFeaturesSetting.

BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=1375

Cc: Ray Ni 
Cc: Laszlo Ersek 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Eric Dong 
---
  .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 66 +++---
  .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h   |  1 -
  .../RegisterCpuFeaturesLib.c   | 10 ++--
  3 files changed, 24 insertions(+), 53 deletions(-)

diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c 
b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
index 4ebd0025b4..762eaec277 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
@@ -74,27 +74,6 @@ GetSettingPcd (
return SettingBitMask;
  }
  
-/**

-  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;
-}
-
  /**
Collects CPU type and feature information.
  
@@ -282,11 +261,6 @@ CpuInitDataInitialize (

ASSERT (CpuFeaturesData->CpuFlags.CoreSemaphoreCount != NULL);
CpuFeaturesData->CpuFlags.PackageSemaphoreCount = AllocateZeroPool (sizeof (UINT32) 
* CpuStatus->PackageCount * CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount);
ASSERT (CpuFeaturesData->CpuFlags.PackageSemaphoreCount != NULL);
-
-  //
-  // Get support and configuration PCDs
-  //
-  CpuFeaturesData->SupportPcd   = GetSupportPcd ();
  }
  
  /**

@@ -306,7 +280,7 @@ SupportedMaskOr (
UINT8  *Data1;
UINT8  *Data2;
  
-  BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);

+  BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
Data1 = SupportedFeatureMask;
Data2 = OrFeatureBitMask;
for (Index = 0; Index < BitMaskSize; Index++) {
@@ -331,7 +305,7 @@ SupportedMaskAnd (
UINT8  *Data1;
UINT8  *Data2;
  
-  BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);

+  BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
Data1 = SupportedFeatureMask;
Data2 = AndFeatureBitMask;
for (Index = 0; Index < BitMaskSize; Index++) {
@@ -356,7 +330,7 @@ SupportedMaskCleanBit (
UINT8  *Data1;
UINT8  *Data2;
  
-  BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);

+  BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
Data1 = SupportedFeatureMask;
Data2 = AndFeatureBitMask;
for (Index = 0; Index < BitMaskSize; Index++) {
@@ -387,7 +361,7 @@ IsBitMaskMatch (
UINT8  *Data1;
UINT8  *Data2;
  
-  BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);

+  BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
  
Data1 = SupportedFeatureMask;

Data2 = ComparedFeatureBitMask;
@@ -426,22 +400,22 @@ CollectProcessorData (
Entry = GetFirstNode (>FeatureList);
while (!IsNull (>FeatureList, Entry)) {
  CpuFeature = CPU_FEATURE_ENTRY_FROM_LINK (Entry);
-if (IsBitMaskMatch (CpuFeaturesData->SupportPcd, CpuFeature->FeatureMask)) 
{
-  if (CpuFeature->SupportFunc == NULL) {
-//
-// If SupportFunc is NULL, then the feature is supported.
-//
-SupportedMaskOr (
-  CpuFeaturesData->InitOrder[ProcessorNumber].FeaturesSupportedMask,
-  CpuFeature->FeatureMask
-  );
-  } else if (CpuFeature->SupportFunc (ProcessorNumber, CpuInfo, 
CpuFeature->ConfigData)) {
-SupportedMaskOr (
-  CpuFeaturesData->InitOrder[ProcessorNumber].FeaturesSupportedMask,
-  CpuFeature->FeatureMask
-  );
-  }
+
+if (CpuFeature->SupportFunc == NULL) {
+  //
+  // If SupportFunc is NULL, then the feature is supported.
+  //
+  SupportedMaskOr (
+CpuFeaturesData->InitOrder[ProcessorNumber].FeaturesSupportedMask,
+CpuFeature->FeatureMask
+);
+} else if (CpuFeature->SupportFunc (ProcessorNumber, CpuInfo, 
CpuFeature->ConfigData)) {
+  SupportedMaskOr (
+CpuFeaturesData->InitOrder[ProcessorNumber].FeaturesSupportedMask,
+CpuFeature->FeatureMask
+);
  }
+
  Entry = Entry->ForwardLink;
}
  }
@@ -646,8 +620,6 @@ AnalysisProcessorFeatures (
DumpCpuFeature 

Re: [edk2] [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Simplify PcdCpuFeaturesSupport.

2019-02-13 Thread Dong, Eric
Hi Laszlo,

> -Original Message-
> From: Laszlo Ersek [mailto:ler...@redhat.com]
> Sent: Wednesday, February 13, 2019 11:01 AM
> To: Dong, Eric ; edk2-devel@lists.01.org
> Cc: Ni, Ray 
> Subject: Re: [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Simplify
> PcdCpuFeaturesSupport.
> 
> On 02/13/19 03:04, Eric Dong wrote:
> > PcdCpuFeaturesSupport used to specify the platform policy about what
> > CPU features this platform supports. This value is decide by platform
> > owner, not by hardware. After this change, this PCD will be used in
> > IsCpuFeatureSupported function only.
> >
> > Now RegisterCpuFeaturesLib use this PCD as an template to Get the pcd
> > size. Update the code logic to replace it with PcdCpuFeaturesSetting.
> >
> > BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=1375
> >
> > Cc: Ray Ni 
> > Cc: Laszlo Ersek 
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Eric Dong 
> > ---
> >  .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 66 +++
> ---
> >  .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h   |  1 -
> >  .../RegisterCpuFeaturesLib.c   | 10 ++--
> >  3 files changed, 24 insertions(+), 53 deletions(-)
> >
> > diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > index 4ebd0025b4..762eaec277 100644
> > ---
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.
> > +++ c
> > @@ -74,27 +74,6 @@ GetSettingPcd (
> >return SettingBitMask;
> >  }
> >
> > -/**
> > -  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;
> > -}
> > -
> >  /**
> >Collects CPU type and feature information.
> >
> > @@ -282,11 +261,6 @@ CpuInitDataInitialize (
> >ASSERT (CpuFeaturesData->CpuFlags.CoreSemaphoreCount != NULL);
> >CpuFeaturesData->CpuFlags.PackageSemaphoreCount =
> AllocateZeroPool (sizeof (UINT32) * CpuStatus->PackageCount * CpuStatus-
> >MaxCoreCount * CpuStatus->MaxThreadCount);
> >ASSERT (CpuFeaturesData->CpuFlags.PackageSemaphoreCount != NULL);
> > -
> > -  //
> > -  // Get support and configuration PCDs
> > -  //
> > -  CpuFeaturesData->SupportPcd   = GetSupportPcd ();
> >  }
> >
> >  /**
> > @@ -306,7 +280,7 @@ SupportedMaskOr (
> >UINT8  *Data1;
> >UINT8  *Data2;
> >
> > -  BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> > +  BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> >Data1 = SupportedFeatureMask;
> >Data2 = OrFeatureBitMask;
> >for (Index = 0; Index < BitMaskSize; Index++) { @@ -331,7 +305,7 @@
> > SupportedMaskAnd (
> >UINT8  *Data1;
> >UINT8  *Data2;
> >
> > -  BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> > +  BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> >Data1 = SupportedFeatureMask;
> >Data2 = AndFeatureBitMask;
> >for (Index = 0; Index < BitMaskSize; Index++) { @@ -356,7 +330,7 @@
> > SupportedMaskCleanBit (
> >UINT8  *Data1;
> >UINT8  *Data2;
> >
> > -  BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> > +  BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> >Data1 = SupportedFeatureMask;
> >Data2 = AndFeatureBitMask;
> >for (Index = 0; Index < BitMaskSize; Index++) { @@ -387,7 +361,7 @@
> > IsBitMaskMatch (
> >UINT8  *Data1;
> >UINT8  *Data2;
> >
> > -  BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> > +  BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> >
> >Data1 = SupportedFeatureMask;
> >Data2 = ComparedFeatureBitMask;
> > @@ -426,22 +400,22 @@ CollectProcessorData (
> >Entry = GetFirstNode (>FeatureList);
> >while (!IsNull (>FeatureList, Entry)) {
> >  CpuFeature = CPU_FEATURE_ENTRY_FROM_LINK (Entry);
> > -if (IsBitMaskMatch (CpuFeaturesData->SupportPcd, CpuFeature-
> >FeatureMask)) {
> > -  if (CpuFeature->SupportFunc == NULL) {
> > -//
> > -// If SupportFunc is NULL, then the feature is supported.
> > -//
> > -SupportedMaskOr (
> > -  CpuFeaturesData-
> >InitOrder[ProcessorNumber].FeaturesSupportedMask,
> > -  CpuFeature->FeatureMask
> > -  );
> > -  } else if (CpuFeature->SupportFunc (ProcessorNumber, CpuInfo,
> CpuFeature->ConfigData)) {
> > -SupportedMaskOr (
> > -  CpuFeaturesData-
> >InitOrder[ProcessorNumber].FeaturesSupportedMask,
> > -  

Re: [edk2] [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Simplify PcdCpuFeaturesSupport.

2019-02-12 Thread Laszlo Ersek
On 02/13/19 03:04, Eric Dong wrote:
> PcdCpuFeaturesSupport used to specify the platform policy about
> what CPU features this platform supports. This value is decide by
> platform owner, not by hardware. After this change, this PCD will
> be used in IsCpuFeatureSupported function only.
> 
> Now RegisterCpuFeaturesLib use this PCD as an template to Get the
> pcd size. Update the code logic to replace it with
> PcdCpuFeaturesSetting.
> 
> BZ:https://bugzilla.tianocore.org/show_bug.cgi?id=1375
> 
> Cc: Ray Ni 
> Cc: Laszlo Ersek 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong 
> ---
>  .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 66 
> +++---
>  .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h   |  1 -
>  .../RegisterCpuFeaturesLib.c   | 10 ++--
>  3 files changed, 24 insertions(+), 53 deletions(-)
> 
> diff --git 
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c 
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> index 4ebd0025b4..762eaec277 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> @@ -74,27 +74,6 @@ GetSettingPcd (
>return SettingBitMask;
>  }
>  
> -/**
> -  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;
> -}
> -
>  /**
>Collects CPU type and feature information.
>  
> @@ -282,11 +261,6 @@ CpuInitDataInitialize (
>ASSERT (CpuFeaturesData->CpuFlags.CoreSemaphoreCount != NULL);
>CpuFeaturesData->CpuFlags.PackageSemaphoreCount = AllocateZeroPool (sizeof 
> (UINT32) * CpuStatus->PackageCount * CpuStatus->MaxCoreCount * 
> CpuStatus->MaxThreadCount);
>ASSERT (CpuFeaturesData->CpuFlags.PackageSemaphoreCount != NULL);
> -
> -  //
> -  // Get support and configuration PCDs
> -  //
> -  CpuFeaturesData->SupportPcd   = GetSupportPcd ();
>  }
>  
>  /**
> @@ -306,7 +280,7 @@ SupportedMaskOr (
>UINT8  *Data1;
>UINT8  *Data2;
>  
> -  BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> +  BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
>Data1 = SupportedFeatureMask;
>Data2 = OrFeatureBitMask;
>for (Index = 0; Index < BitMaskSize; Index++) {
> @@ -331,7 +305,7 @@ SupportedMaskAnd (
>UINT8  *Data1;
>UINT8  *Data2;
>  
> -  BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> +  BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
>Data1 = SupportedFeatureMask;
>Data2 = AndFeatureBitMask;
>for (Index = 0; Index < BitMaskSize; Index++) {
> @@ -356,7 +330,7 @@ SupportedMaskCleanBit (
>UINT8  *Data1;
>UINT8  *Data2;
>  
> -  BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> +  BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
>Data1 = SupportedFeatureMask;
>Data2 = AndFeatureBitMask;
>for (Index = 0; Index < BitMaskSize; Index++) {
> @@ -387,7 +361,7 @@ IsBitMaskMatch (
>UINT8  *Data1;
>UINT8  *Data2;
>  
> -  BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> +  BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
>  
>Data1 = SupportedFeatureMask;
>Data2 = ComparedFeatureBitMask;
> @@ -426,22 +400,22 @@ CollectProcessorData (
>Entry = GetFirstNode (>FeatureList);
>while (!IsNull (>FeatureList, Entry)) {
>  CpuFeature = CPU_FEATURE_ENTRY_FROM_LINK (Entry);
> -if (IsBitMaskMatch (CpuFeaturesData->SupportPcd, 
> CpuFeature->FeatureMask)) {
> -  if (CpuFeature->SupportFunc == NULL) {
> -//
> -// If SupportFunc is NULL, then the feature is supported.
> -//
> -SupportedMaskOr (
> -  CpuFeaturesData->InitOrder[ProcessorNumber].FeaturesSupportedMask,
> -  CpuFeature->FeatureMask
> -  );
> -  } else if (CpuFeature->SupportFunc (ProcessorNumber, CpuInfo, 
> CpuFeature->ConfigData)) {
> -SupportedMaskOr (
> -  CpuFeaturesData->InitOrder[ProcessorNumber].FeaturesSupportedMask,
> -  CpuFeature->FeatureMask
> -  );
> -  }
> +
> +if (CpuFeature->SupportFunc == NULL) {
> +  //
> +  // If SupportFunc is NULL, then the feature is supported.
> +  //
> +  SupportedMaskOr (
> +CpuFeaturesData->InitOrder[ProcessorNumber].FeaturesSupportedMask,
> +CpuFeature->FeatureMask
> +);
> +} else if (CpuFeature->SupportFunc (ProcessorNumber, CpuInfo, 
> CpuFeature->ConfigData)) {
> +  SupportedMaskOr (
> +