* [Patch 0/3] Simplify CPU Features solution.
@ 2019-02-13 2:04 Eric Dong
2019-02-13 2:04 ` [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Remove useless functions Eric Dong
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Eric Dong @ 2019-02-13 2:04 UTC (permalink / raw)
To: edk2-devel; +Cc: Ray Ni, Laszlo Ersek
Changes includes:
1. Optimize PCD PcdCpuFeaturesUserConfiguration
2. Limit useage of PcdCpuFeaturesSupport
3. Remove some useless APIs.
Detail explanation please check each patch's introduction.
Cc: Ray Ni <Ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Eric Dong (3):
UefiCpuPkg/RegisterCpuFeaturesLib: Remove useless functions.
UefiCpuPkg/RegisterCpuFeaturesLib: Optimize PCD
PcdCpuFeaturesUserConfiguration.
UefiCpuPkg/RegisterCpuFeaturesLib: Simplify PcdCpuFeaturesSupport.
.../Include/Library/RegisterCpuFeaturesLib.h | 34 --------
.../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 95 ++++++++--------------
.../DxeRegisterCpuFeaturesLib.inf | 1 -
.../PeiRegisterCpuFeaturesLib.inf | 1 -
.../RegisterCpuFeaturesLib/RegisterCpuFeatures.h | 2 -
.../RegisterCpuFeaturesLib.c | 60 ++------------
UefiCpuPkg/UefiCpuPkg.dec | 7 +-
7 files changed, 42 insertions(+), 158 deletions(-)
--
2.15.0.windows.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Remove useless functions.
2019-02-13 2:04 [Patch 0/3] Simplify CPU Features solution Eric Dong
@ 2019-02-13 2:04 ` Eric Dong
2019-02-13 2:23 ` Laszlo Ersek
2019-02-13 2:04 ` [Patch 2/3] UefiCpuPkg/RegisterCpuFeaturesLib: Optimize PCD PcdCpuFeaturesUserConfiguration Eric Dong
` (2 subsequent siblings)
3 siblings, 1 reply; 12+ messages in thread
From: Eric Dong @ 2019-02-13 2:04 UTC (permalink / raw)
To: edk2-devel; +Cc: Ray Ni, Laszlo Ersek
Remove useless APIs, simply the code logic.
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>
---
.../Include/Library/RegisterCpuFeaturesLib.h | 34 ---------------
.../RegisterCpuFeaturesLib.c | 50 ----------------------
2 files changed, 84 deletions(-)
diff --git a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
index 2f7e71c833..073f020d0b 100644
--- a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
+++ b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
@@ -166,40 +166,6 @@ IsCpuFeatureInSetting (
IN UINT32 Feature
);
-/**
- Determines if a CPU feature is set in PcdCpuFeaturesCapability bit mask.
-
- @param[in] Feature The bit number of the CPU feature to check in the PCD
- PcdCpuFeaturesCapability.
-
- @retval TRUE The CPU feature is set in PcdCpuFeaturesCapability.
- @retval FALSE The CPU feature is not set in PcdCpuFeaturesCapability.
-
- @note This service could be called by BSP only.
-**/
-BOOLEAN
-EFIAPI
-IsCpuFeatureCapability (
- IN UINT32 Feature
- );
-
-/**
- Determines if a CPU feature is set in PcdCpuFeaturesUserConfiguration bit mask.
-
- @param[in] Feature The bit number of the CPU feature to check in the PCD
- PcdCpuFeaturesUserConfiguration.
-
- @retval TRUE The CPU feature is set in PcdCpuFeaturesUserConfiguration.
- @retval FALSE The CPU feature is not set in PcdCpuFeaturesUserConfiguration.
-
- @note This service could be called by BSP only.
-**/
-BOOLEAN
-EFIAPI
-IsCpuFeatureUserConfiguration (
- IN UINT32 Feature
- );
-
/**
Prepares for the data used by CPU feature detection and initialization.
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
index ed8d526325..3540029079 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
@@ -1242,56 +1242,6 @@ IsCpuFeatureInSetting (
);
}
-/**
- Determines if a CPU feature is set in PcdCpuFeaturesCapability bit mask.
-
- @param[in] Feature The bit number of the CPU feature to check in the PCD
- PcdCpuFeaturesCapability
-
- @retval TRUE The CPU feature is set in PcdCpuFeaturesCapability.
- @retval FALSE The CPU feature is not set in PcdCpuFeaturesCapability.
-
- @note This service could be called by BSP only.
-**/
-BOOLEAN
-EFIAPI
-IsCpuFeatureCapability (
- IN UINT32 Feature
- )
-{
- return IsCpuFeatureSetInCpuPcd (
- (UINT8 *)PcdGetPtr (PcdCpuFeaturesCapability),
- PcdGetSize (PcdCpuFeaturesCapability),
- Feature
- );
-
-}
-
-/**
- Determines if a CPU feature is set in PcdCpuFeaturesUserConfiguration bit mask.
-
- @param[in] Feature The bit number of the CPU feature to check in the PCD
- PcdCpuFeaturesUserConfiguration
-
- @retval TRUE The CPU feature is set in PcdCpuFeaturesUserConfiguration.
- @retval FALSE The CPU feature is not set in PcdCpuFeaturesUserConfiguration.
-
- @note This service could be called by BSP only.
-**/
-BOOLEAN
-EFIAPI
-IsCpuFeatureUserConfiguration (
- IN UINT32 Feature
- )
-{
- return IsCpuFeatureSetInCpuPcd (
- (UINT8 *)PcdGetPtr (PcdCpuFeaturesUserConfiguration),
- PcdGetSize (PcdCpuFeaturesUserConfiguration),
- Feature
- );
-
-}
-
/**
Switches to assigned BSP after CPU features initialization.
--
2.15.0.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Patch 2/3] UefiCpuPkg/RegisterCpuFeaturesLib: Optimize PCD PcdCpuFeaturesUserConfiguration.
2019-02-13 2:04 [Patch 0/3] Simplify CPU Features solution Eric Dong
2019-02-13 2:04 ` [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Remove useless functions Eric Dong
@ 2019-02-13 2:04 ` Eric Dong
2019-02-13 2:43 ` Laszlo Ersek
2019-02-14 8:47 ` Ni, Ray
2019-02-13 2:04 ` [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Simplify PcdCpuFeaturesSupport Eric Dong
2019-02-14 8:57 ` [Patch 0/3] Simplify CPU Features solution Ni, Ray
3 siblings, 2 replies; 12+ messages in thread
From: Eric Dong @ 2019-02-13 2:04 UTC (permalink / raw)
To: edk2-devel; +Cc: Ray Ni, Laszlo Ersek
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;
+
+ 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);
+ }
//
// 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/DxeRegisterCpuFeaturesLib.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/PeiRegisterCpuFeaturesLib.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|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,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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Simplify PcdCpuFeaturesSupport.
2019-02-13 2:04 [Patch 0/3] Simplify CPU Features solution Eric Dong
2019-02-13 2:04 ` [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Remove useless functions Eric Dong
2019-02-13 2:04 ` [Patch 2/3] UefiCpuPkg/RegisterCpuFeaturesLib: Optimize PCD PcdCpuFeaturesUserConfiguration Eric Dong
@ 2019-02-13 2:04 ` Eric Dong
2019-02-13 3:01 ` Laszlo Ersek
2019-02-14 8:59 ` Ni, Ray
2019-02-14 8:57 ` [Patch 0/3] Simplify CPU Features solution Ni, Ray
3 siblings, 2 replies; 12+ messages in thread
From: Eric Dong @ 2019-02-13 2:04 UTC (permalink / raw)
To: edk2-devel; +Cc: Ray Ni, Laszlo Ersek
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 <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 | 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 (&CpuFeaturesData->FeatureList);
while (!IsNull (&CpuFeaturesData->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 (CpuFeature);
Entry = Entry->ForwardLink;
}
- DEBUG ((DEBUG_INFO, "PcdCpuFeaturesSupport:\n"));
- DumpCpuFeatureMask (CpuFeaturesData->SupportPcd);
DEBUG ((DEBUG_INFO, "PcdCpuFeaturesCapability:\n"));
DumpCpuFeatureMask (CpuFeaturesData->CapabilityPcd);
DEBUG ((DEBUG_INFO, "PcdCpuFeaturesSetting:\n"));
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
index 3e0a342fd1..836ed3549c 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
@@ -81,7 +81,6 @@ typedef struct {
LIST_ENTRY FeatureList;
CPU_FEATURES_INIT_ORDER *InitOrder;
- UINT8 *SupportPcd;
UINT8 *CapabilityPcd;
UINT8 *SettingPcd;
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
index 3540029079..3e8e899766 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
@@ -31,7 +31,7 @@ IsCpuFeatureMatch (
{
UINTN BitMaskSize;
- BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
+ BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
if (CompareMem (FirstFeatureMask, SecondFeatureMask, BitMaskSize) == 0) {
return TRUE;
} else {
@@ -53,7 +53,7 @@ DumpCpuFeatureMask (
UINT8 *Data8;
UINTN BitMaskSize;
- BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
+ BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
Data8 = (UINT8 *) FeatureMask;
for (Index = 0; Index < BitMaskSize; Index++) {
DEBUG ((DEBUG_INFO, " %02x ", *Data8++));
@@ -100,7 +100,7 @@ IsBitMaskMatchCheck (
UINT8 *Data1;
UINT8 *Data2;
- BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
+ BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
Data1 = FeatureMask;
Data2 = DependentBitMask;
@@ -656,7 +656,7 @@ RegisterCpuFeatureWorker (
UINTN BitMaskSize;
BOOLEAN FeatureExist;
- BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
+ BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
CpuFeaturesData = GetCpuFeaturesData ();
if (CpuFeaturesData->FeaturesCount == 0) {
InitializeListHead (&CpuFeaturesData->FeatureList);
@@ -870,7 +870,7 @@ RegisterCpuFeature (
BeforeAll = FALSE;
AfterAll = FALSE;
- BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
+ BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
VA_START (Marker, InitializeFunc);
Feature = VA_ARG (Marker, UINT32);
--
2.15.0.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Remove useless functions.
2019-02-13 2:04 ` [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Remove useless functions Eric Dong
@ 2019-02-13 2:23 ` Laszlo Ersek
0 siblings, 0 replies; 12+ messages in thread
From: Laszlo Ersek @ 2019-02-13 2:23 UTC (permalink / raw)
To: Eric Dong, edk2-devel
On 02/13/19 03:04, Eric Dong wrote:
> Remove useless APIs, simply the code logic.
s/simply/simplify/
>
> 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>
> ---
> .../Include/Library/RegisterCpuFeaturesLib.h | 34 ---------------
> .../RegisterCpuFeaturesLib.c | 50 ----------------------
> 2 files changed, 84 deletions(-)
With the typo fixed:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
> diff --git a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> index 2f7e71c833..073f020d0b 100644
> --- a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> +++ b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> @@ -166,40 +166,6 @@ IsCpuFeatureInSetting (
> IN UINT32 Feature
> );
>
> -/**
> - Determines if a CPU feature is set in PcdCpuFeaturesCapability bit mask.
> -
> - @param[in] Feature The bit number of the CPU feature to check in the PCD
> - PcdCpuFeaturesCapability.
> -
> - @retval TRUE The CPU feature is set in PcdCpuFeaturesCapability.
> - @retval FALSE The CPU feature is not set in PcdCpuFeaturesCapability.
> -
> - @note This service could be called by BSP only.
> -**/
> -BOOLEAN
> -EFIAPI
> -IsCpuFeatureCapability (
> - IN UINT32 Feature
> - );
> -
> -/**
> - Determines if a CPU feature is set in PcdCpuFeaturesUserConfiguration bit mask.
> -
> - @param[in] Feature The bit number of the CPU feature to check in the PCD
> - PcdCpuFeaturesUserConfiguration.
> -
> - @retval TRUE The CPU feature is set in PcdCpuFeaturesUserConfiguration.
> - @retval FALSE The CPU feature is not set in PcdCpuFeaturesUserConfiguration.
> -
> - @note This service could be called by BSP only.
> -**/
> -BOOLEAN
> -EFIAPI
> -IsCpuFeatureUserConfiguration (
> - IN UINT32 Feature
> - );
> -
> /**
> Prepares for the data used by CPU feature detection and initialization.
>
> diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> index ed8d526325..3540029079 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> @@ -1242,56 +1242,6 @@ IsCpuFeatureInSetting (
> );
> }
>
> -/**
> - Determines if a CPU feature is set in PcdCpuFeaturesCapability bit mask.
> -
> - @param[in] Feature The bit number of the CPU feature to check in the PCD
> - PcdCpuFeaturesCapability
> -
> - @retval TRUE The CPU feature is set in PcdCpuFeaturesCapability.
> - @retval FALSE The CPU feature is not set in PcdCpuFeaturesCapability.
> -
> - @note This service could be called by BSP only.
> -**/
> -BOOLEAN
> -EFIAPI
> -IsCpuFeatureCapability (
> - IN UINT32 Feature
> - )
> -{
> - return IsCpuFeatureSetInCpuPcd (
> - (UINT8 *)PcdGetPtr (PcdCpuFeaturesCapability),
> - PcdGetSize (PcdCpuFeaturesCapability),
> - Feature
> - );
> -
> -}
> -
> -/**
> - Determines if a CPU feature is set in PcdCpuFeaturesUserConfiguration bit mask.
> -
> - @param[in] Feature The bit number of the CPU feature to check in the PCD
> - PcdCpuFeaturesUserConfiguration
> -
> - @retval TRUE The CPU feature is set in PcdCpuFeaturesUserConfiguration.
> - @retval FALSE The CPU feature is not set in PcdCpuFeaturesUserConfiguration.
> -
> - @note This service could be called by BSP only.
> -**/
> -BOOLEAN
> -EFIAPI
> -IsCpuFeatureUserConfiguration (
> - IN UINT32 Feature
> - )
> -{
> - return IsCpuFeatureSetInCpuPcd (
> - (UINT8 *)PcdGetPtr (PcdCpuFeaturesUserConfiguration),
> - PcdGetSize (PcdCpuFeaturesUserConfiguration),
> - Feature
> - );
> -
> -}
> -
> /**
> Switches to assigned BSP after CPU features initialization.
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2/3] UefiCpuPkg/RegisterCpuFeaturesLib: Optimize PCD PcdCpuFeaturesUserConfiguration.
2019-02-13 2:04 ` [Patch 2/3] UefiCpuPkg/RegisterCpuFeaturesLib: Optimize PCD PcdCpuFeaturesUserConfiguration Eric Dong
@ 2019-02-13 2:43 ` Laszlo Ersek
2019-02-14 2:00 ` Dong, Eric
2019-02-14 8:47 ` Ni, Ray
1 sibling, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2019-02-13 2:43 UTC (permalink / raw)
To: Eric Dong, edk2-devel
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.
> - 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.)
>
> //
> // 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/DxeRegisterCpuFeaturesLib.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/PeiRegisterCpuFeaturesLib.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|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,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?
> # @ValidList 0x80000001 | 0
> gUefiCpuPkgTokenSpaceGuid.PcdCpuFeaturesSetting|{0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}|VOID*|0x00000019
>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Simplify PcdCpuFeaturesSupport.
2019-02-13 2:04 ` [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Simplify PcdCpuFeaturesSupport Eric Dong
@ 2019-02-13 3:01 ` Laszlo Ersek
2019-02-14 1:01 ` Dong, Eric
2019-02-14 8:59 ` Ni, Ray
1 sibling, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2019-02-13 3:01 UTC (permalink / raw)
To: Eric Dong, edk2-devel
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 <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 | 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 (&CpuFeaturesData->FeatureList);
> while (!IsNull (&CpuFeaturesData->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;
> }
> }
The functional effect of the change in CollectProcessorData() is too
complex for me to determine, so I'd like to leave the review of this
patch to Ray only.
(1) FWIW, I've attempted to read the BZ carefully, and it seems the
patch doesn't do all things that the BZ suggests:
- The IsCpuFeatureSupported() function is not removed. That could be
correct, but then I think it should be explained in the BZ (i.e. update
the scope).
- Is it asserted somewhere that PcdCpuFeaturesSetting and
PcdCpuFeaturesCapability have equal size?
(2) Independently, please append (or prepend) a cleanup patch to the
series. Namely, the comment block on IsCpuFeatureSetInCpuPcd()
references "PcdCpuFeaturesSupport". It should reference "CpuBitMask"
instead, IMO.
Thanks
Laszlo
> @@ -646,8 +620,6 @@ AnalysisProcessorFeatures (
> DumpCpuFeature (CpuFeature);
> Entry = Entry->ForwardLink;
> }
> - DEBUG ((DEBUG_INFO, "PcdCpuFeaturesSupport:\n"));
> - DumpCpuFeatureMask (CpuFeaturesData->SupportPcd);
> DEBUG ((DEBUG_INFO, "PcdCpuFeaturesCapability:\n"));
> DumpCpuFeatureMask (CpuFeaturesData->CapabilityPcd);
> DEBUG ((DEBUG_INFO, "PcdCpuFeaturesSetting:\n"));
> diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> index 3e0a342fd1..836ed3549c 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> @@ -81,7 +81,6 @@ typedef struct {
> LIST_ENTRY FeatureList;
>
> CPU_FEATURES_INIT_ORDER *InitOrder;
> - UINT8 *SupportPcd;
> UINT8 *CapabilityPcd;
> UINT8 *SettingPcd;
>
> diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> index 3540029079..3e8e899766 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> @@ -31,7 +31,7 @@ IsCpuFeatureMatch (
> {
> UINTN BitMaskSize;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> if (CompareMem (FirstFeatureMask, SecondFeatureMask, BitMaskSize) == 0) {
> return TRUE;
> } else {
> @@ -53,7 +53,7 @@ DumpCpuFeatureMask (
> UINT8 *Data8;
> UINTN BitMaskSize;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> Data8 = (UINT8 *) FeatureMask;
> for (Index = 0; Index < BitMaskSize; Index++) {
> DEBUG ((DEBUG_INFO, " %02x ", *Data8++));
> @@ -100,7 +100,7 @@ IsBitMaskMatchCheck (
> UINT8 *Data1;
> UINT8 *Data2;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
>
> Data1 = FeatureMask;
> Data2 = DependentBitMask;
> @@ -656,7 +656,7 @@ RegisterCpuFeatureWorker (
> UINTN BitMaskSize;
> BOOLEAN FeatureExist;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> CpuFeaturesData = GetCpuFeaturesData ();
> if (CpuFeaturesData->FeaturesCount == 0) {
> InitializeListHead (&CpuFeaturesData->FeatureList);
> @@ -870,7 +870,7 @@ RegisterCpuFeature (
> BeforeAll = FALSE;
> AfterAll = FALSE;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
>
> VA_START (Marker, InitializeFunc);
> Feature = VA_ARG (Marker, UINT32);
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Simplify PcdCpuFeaturesSupport.
2019-02-13 3:01 ` Laszlo Ersek
@ 2019-02-14 1:01 ` Dong, Eric
0 siblings, 0 replies; 12+ messages in thread
From: Dong, Eric @ 2019-02-14 1:01 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Hi Laszlo,
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, February 13, 2019 11:01 AM
> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ray <ray.ni@intel.com>
> 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 <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 | 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 (&CpuFeaturesData->FeatureList);
> > while (!IsNull (&CpuFeaturesData->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;
> > }
> > }
>
> The functional effect of the change in CollectProcessorData() is too complex
> for me to determine, so I'd like to leave the review of this patch to Ray only.
>
> (1) FWIW, I've attempted to read the BZ carefully, and it seems the patch
> doesn't do all things that the BZ suggests:
>
> - The IsCpuFeatureSupported() function is not removed. That could be
> correct, but then I think it should be explained in the BZ (i.e. update the
> scope).
Agree, added notes in the BZ to explain what real changes will do.
>
> - Is it asserted somewhere that PcdCpuFeaturesSetting and
> PcdCpuFeaturesCapability have equal size?
Sorry, I missed this part. Will include it in V2 changes.
>
> (2) Independently, please append (or prepend) a cleanup patch to the series.
> Namely, the comment block on IsCpuFeatureSetInCpuPcd() references
> "PcdCpuFeaturesSupport". It should reference "CpuBitMask"
> instead, IMO.
Agree, will include it in v2 changes.
Thanks,
Eric
>
> Thanks
> Laszlo
>
> > @@ -646,8 +620,6 @@ AnalysisProcessorFeatures (
> > DumpCpuFeature (CpuFeature);
> > Entry = Entry->ForwardLink;
> > }
> > - DEBUG ((DEBUG_INFO, "PcdCpuFeaturesSupport:\n"));
> > - DumpCpuFeatureMask (CpuFeaturesData->SupportPcd);
> > DEBUG ((DEBUG_INFO, "PcdCpuFeaturesCapability:\n"));
> > DumpCpuFeatureMask (CpuFeaturesData->CapabilityPcd);
> > DEBUG ((DEBUG_INFO, "PcdCpuFeaturesSetting:\n")); diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> > index 3e0a342fd1..836ed3549c 100644
> > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> > @@ -81,7 +81,6 @@ typedef struct {
> > LIST_ENTRY FeatureList;
> >
> > CPU_FEATURES_INIT_ORDER *InitOrder;
> > - UINT8 *SupportPcd;
> > UINT8 *CapabilityPcd;
> > UINT8 *SettingPcd;
> >
> > diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > index 3540029079..3e8e899766 100644
> > ---
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > +++
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib
> > +++ .c
> > @@ -31,7 +31,7 @@ IsCpuFeatureMatch (
> > {
> > UINTN BitMaskSize;
> >
> > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> > + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> > if (CompareMem (FirstFeatureMask, SecondFeatureMask, BitMaskSize)
> == 0) {
> > return TRUE;
> > } else {
> > @@ -53,7 +53,7 @@ DumpCpuFeatureMask (
> > UINT8 *Data8;
> > UINTN BitMaskSize;
> >
> > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> > + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> > Data8 = (UINT8 *) FeatureMask;
> > for (Index = 0; Index < BitMaskSize; Index++) {
> > DEBUG ((DEBUG_INFO, " %02x ", *Data8++)); @@ -100,7 +100,7 @@
> > IsBitMaskMatchCheck (
> > UINT8 *Data1;
> > UINT8 *Data2;
> >
> > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> > + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> >
> > Data1 = FeatureMask;
> > Data2 = DependentBitMask;
> > @@ -656,7 +656,7 @@ RegisterCpuFeatureWorker (
> > UINTN BitMaskSize;
> > BOOLEAN FeatureExist;
> >
> > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> > + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> > CpuFeaturesData = GetCpuFeaturesData ();
> > if (CpuFeaturesData->FeaturesCount == 0) {
> > InitializeListHead (&CpuFeaturesData->FeatureList); @@ -870,7
> > +870,7 @@ RegisterCpuFeature (
> > BeforeAll = FALSE;
> > AfterAll = FALSE;
> >
> > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> > + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> >
> > VA_START (Marker, InitializeFunc);
> > Feature = VA_ARG (Marker, UINT32);
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2/3] UefiCpuPkg/RegisterCpuFeaturesLib: Optimize PCD PcdCpuFeaturesUserConfiguration.
2019-02-13 2:43 ` Laszlo Ersek
@ 2019-02-14 2:00 ` Dong, Eric
0 siblings, 0 replies; 12+ messages in thread
From: Dong, Eric @ 2019-02-14 2:00 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2/3] UefiCpuPkg/RegisterCpuFeaturesLib: Optimize PCD PcdCpuFeaturesUserConfiguration.
2019-02-13 2:04 ` [Patch 2/3] UefiCpuPkg/RegisterCpuFeaturesLib: Optimize PCD PcdCpuFeaturesUserConfiguration Eric Dong
2019-02-13 2:43 ` Laszlo Ersek
@ 2019-02-14 8:47 ` Ni, Ray
1 sibling, 0 replies; 12+ messages in thread
From: Ni, Ray @ 2019-02-14 8:47 UTC (permalink / raw)
To: Dong, Eric, edk2-devel@lists.01.org; +Cc: Laszlo Ersek
> -----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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 0/3] Simplify CPU Features solution.
2019-02-13 2:04 [Patch 0/3] Simplify CPU Features solution Eric Dong
` (2 preceding siblings ...)
2019-02-13 2:04 ` [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Simplify PcdCpuFeaturesSupport Eric Dong
@ 2019-02-14 8:57 ` Ni, Ray
3 siblings, 0 replies; 12+ messages in thread
From: Ni, Ray @ 2019-02-14 8:57 UTC (permalink / raw)
To: Dong, Eric, edk2-devel@lists.01.org; +Cc: Laszlo Ersek
Eric,
Please update the copyright year to 2019 for every file you changed.
> -----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 0/3] Simplify CPU Features solution.
>
> Changes includes:
> 1. Optimize PCD PcdCpuFeaturesUserConfiguration 2. Limit useage of
> PcdCpuFeaturesSupport 3. Remove some useless APIs.
> Detail explanation please check each patch's introduction.
>
> Cc: Ray Ni <Ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
>
> Eric Dong (3):
> UefiCpuPkg/RegisterCpuFeaturesLib: Remove useless functions.
> UefiCpuPkg/RegisterCpuFeaturesLib: Optimize PCD
> PcdCpuFeaturesUserConfiguration.
> UefiCpuPkg/RegisterCpuFeaturesLib: Simplify PcdCpuFeaturesSupport.
>
> .../Include/Library/RegisterCpuFeaturesLib.h | 34 --------
> .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 95 ++++++++-------------
> -
> .../DxeRegisterCpuFeaturesLib.inf | 1 -
> .../PeiRegisterCpuFeaturesLib.inf | 1 -
> .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h | 2 -
> .../RegisterCpuFeaturesLib.c | 60 ++------------
> UefiCpuPkg/UefiCpuPkg.dec | 7 +-
> 7 files changed, 42 insertions(+), 158 deletions(-)
>
> --
> 2.15.0.windows.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Simplify PcdCpuFeaturesSupport.
2019-02-13 2:04 ` [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Simplify PcdCpuFeaturesSupport Eric Dong
2019-02-13 3:01 ` Laszlo Ersek
@ 2019-02-14 8:59 ` Ni, Ray
1 sibling, 0 replies; 12+ messages in thread
From: Ni, Ray @ 2019-02-14 8:59 UTC (permalink / raw)
To: Eric Dong, edk2-devel; +Cc: Laszlo Ersek
Reviewed-by: Ray Ni <ray.ni@intel.com>
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 <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 | 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 (&CpuFeaturesData->FeatureList);
> while (!IsNull (&CpuFeaturesData->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 (CpuFeature);
> Entry = Entry->ForwardLink;
> }
> - DEBUG ((DEBUG_INFO, "PcdCpuFeaturesSupport:\n"));
> - DumpCpuFeatureMask (CpuFeaturesData->SupportPcd);
> DEBUG ((DEBUG_INFO, "PcdCpuFeaturesCapability:\n"));
> DumpCpuFeatureMask (CpuFeaturesData->CapabilityPcd);
> DEBUG ((DEBUG_INFO, "PcdCpuFeaturesSetting:\n"));
> diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> index 3e0a342fd1..836ed3549c 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> @@ -81,7 +81,6 @@ typedef struct {
> LIST_ENTRY FeatureList;
>
> CPU_FEATURES_INIT_ORDER *InitOrder;
> - UINT8 *SupportPcd;
> UINT8 *CapabilityPcd;
> UINT8 *SettingPcd;
>
> diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> index 3540029079..3e8e899766 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> @@ -31,7 +31,7 @@ IsCpuFeatureMatch (
> {
> UINTN BitMaskSize;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> if (CompareMem (FirstFeatureMask, SecondFeatureMask, BitMaskSize) == 0) {
> return TRUE;
> } else {
> @@ -53,7 +53,7 @@ DumpCpuFeatureMask (
> UINT8 *Data8;
> UINTN BitMaskSize;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> Data8 = (UINT8 *) FeatureMask;
> for (Index = 0; Index < BitMaskSize; Index++) {
> DEBUG ((DEBUG_INFO, " %02x ", *Data8++));
> @@ -100,7 +100,7 @@ IsBitMaskMatchCheck (
> UINT8 *Data1;
> UINT8 *Data2;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
>
> Data1 = FeatureMask;
> Data2 = DependentBitMask;
> @@ -656,7 +656,7 @@ RegisterCpuFeatureWorker (
> UINTN BitMaskSize;
> BOOLEAN FeatureExist;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> CpuFeaturesData = GetCpuFeaturesData ();
> if (CpuFeaturesData->FeaturesCount == 0) {
> InitializeListHead (&CpuFeaturesData->FeatureList);
> @@ -870,7 +870,7 @@ RegisterCpuFeature (
> BeforeAll = FALSE;
> AfterAll = FALSE;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSupport);
> + BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
>
> VA_START (Marker, InitializeFunc);
> Feature = VA_ARG (Marker, UINT32);
>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-02-14 8:57 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-13 2:04 [Patch 0/3] Simplify CPU Features solution Eric Dong
2019-02-13 2:04 ` [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Remove useless functions Eric Dong
2019-02-13 2:23 ` Laszlo Ersek
2019-02-13 2:04 ` [Patch 2/3] UefiCpuPkg/RegisterCpuFeaturesLib: Optimize PCD PcdCpuFeaturesUserConfiguration Eric Dong
2019-02-13 2:43 ` Laszlo Ersek
2019-02-14 2:00 ` Dong, Eric
2019-02-14 8:47 ` Ni, Ray
2019-02-13 2:04 ` [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Simplify PcdCpuFeaturesSupport Eric Dong
2019-02-13 3:01 ` Laszlo Ersek
2019-02-14 1:01 ` Dong, Eric
2019-02-14 8:59 ` Ni, Ray
2019-02-14 8:57 ` [Patch 0/3] Simplify CPU Features solution Ni, Ray
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox