* [Patch 0/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls gBS service.
@ 2019-07-12 1:53 Dong, Eric
2019-07-12 1:53 ` [Patch 1/2] " Dong, Eric
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Dong, Eric @ 2019-07-12 1:53 UTC (permalink / raw)
To: devel; +Cc: Ray Ni, Laszlo Ersek, Chandana Kumar, Star Zeng
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1972
AP calls CollectProcessorData() to collect processor info.
CollectProcessorData function finally calls PcdGetSize function to
get DynamicPCD PcdCpuFeaturesSetting value. PcdGetSize will use gBS
which caused ASSERT.
This patch serial fixes the issue and enhances the related code to avoid
later report this issue again.
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Chandana Kumar <chandana.c.kumar@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Eric Dong (2):
UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls gBS service.
UefiCpuPkg/Library/RegisterCpuFeaturesLib: avoid use dynamic PCD.
.../CpuFeaturesInitialize.c | 77 ++++++-------
.../RegisterCpuFeatures.h | 10 +-
.../RegisterCpuFeaturesLib.c | 109 +++++++-----------
3 files changed, 85 insertions(+), 111 deletions(-)
--
2.21.0.windows.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Patch 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls gBS service.
2019-07-12 1:53 [Patch 0/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls gBS service Dong, Eric
@ 2019-07-12 1:53 ` Dong, Eric
2019-07-12 10:53 ` Zeng, Star
2019-07-12 1:53 ` [Patch 2/2] UefiCpuPkg/Library/RegisterCpuFeaturesLib: avoid use dynamic PCD Dong, Eric
2019-07-12 22:16 ` [Patch 0/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls gBS service Laszlo Ersek
2 siblings, 1 reply; 9+ messages in thread
From: Dong, Eric @ 2019-07-12 1:53 UTC (permalink / raw)
To: devel; +Cc: Ray Ni, Laszlo Ersek, Chandana Kumar, Star Zeng
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1972
AP calls CollectProcessorData() to collect processor info.
CollectProcessorData function finally calls PcdGetSize function to
get DynamicPCD PcdCpuFeaturesSetting value. PcdGetSize will use gBS
which caused below assert info:
Processor Info: Package: 1, MaxCore : 4, MaxThread: 1
Package: 0, Valid Core : 4
ASSERT [CpuFeaturesPei] c:\projects\jsl\jsl_v1193\Edk2\MdePkg\Library
\PeiServicesTablePointerLibIdt\PeiServicesTablePointer.c(48):
PeiServices != ((void *) 0)
This change uses saved global pcd size instead of calls PcdGetSize to
fix this issue.
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Chandana Kumar <chandana.c.kumar@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
.../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 13 ++++++++-----
.../RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c | 5 +++++
2 files changed, 13 insertions(+), 5 deletions(-)
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
index aff7ad600c..87bfc64250 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
@@ -246,19 +246,20 @@ CpuInitDataInitialize (
@param[in] SupportedFeatureMask The pointer to CPU feature bits mask buffer
@param[in] OrFeatureBitMask The feature bit mask to do OR operation
+ @param[in] BitMaskSize The CPU feature bits mask buffer size.
+
**/
VOID
SupportedMaskOr (
IN UINT8 *SupportedFeatureMask,
- IN UINT8 *OrFeatureBitMask
+ IN UINT8 *OrFeatureBitMask,
+ IN UINT32 BitMaskSize
)
{
UINTN Index;
- UINTN BitMaskSize;
UINT8 *Data1;
UINT8 *Data2;
- BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
Data1 = SupportedFeatureMask;
Data2 = OrFeatureBitMask;
for (Index = 0; Index < BitMaskSize; Index++) {
@@ -384,12 +385,14 @@ CollectProcessorData (
//
SupportedMaskOr (
CpuFeaturesData->InitOrder[ProcessorNumber].FeaturesSupportedMask,
- CpuFeature->FeatureMask
+ CpuFeature->FeatureMask,
+ CpuFeaturesData->BitMaskSize
);
} else if (CpuFeature->SupportFunc (ProcessorNumber, CpuInfo, CpuFeature->ConfigData)) {
SupportedMaskOr (
CpuFeaturesData->InitOrder[ProcessorNumber].FeaturesSupportedMask,
- CpuFeature->FeatureMask
+ CpuFeature->FeatureMask,
+ CpuFeaturesData->BitMaskSize
);
}
Entry = Entry->ForwardLink;
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
index fa0f0b41e2..36aabd7267 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
@@ -658,6 +658,11 @@ RegisterCpuFeatureWorker (
InitializeListHead (&CpuFeaturesData->FeatureList);
InitializeSpinLock (&CpuFeaturesData->CpuFlags.MemoryMappedLock);
InitializeSpinLock (&CpuFeaturesData->CpuFlags.ConsoleLogLock);
+ //
+ // Driver has assumption that these three PCD should has same buffer size.
+ //
+ ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize (PcdCpuFeaturesCapability));
+ ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize (PcdCpuFeaturesSupport));
CpuFeaturesData->BitMaskSize = (UINT32) BitMaskSize;
}
ASSERT (CpuFeaturesData->BitMaskSize == BitMaskSize);
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Patch 2/2] UefiCpuPkg/Library/RegisterCpuFeaturesLib: avoid use dynamic PCD.
2019-07-12 1:53 [Patch 0/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls gBS service Dong, Eric
2019-07-12 1:53 ` [Patch 1/2] " Dong, Eric
@ 2019-07-12 1:53 ` Dong, Eric
2019-07-12 11:10 ` [edk2-devel] " Zeng, Star
2019-07-15 4:57 ` Ni, Ray
2019-07-12 22:16 ` [Patch 0/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls gBS service Laszlo Ersek
2 siblings, 2 replies; 9+ messages in thread
From: Dong, Eric @ 2019-07-12 1:53 UTC (permalink / raw)
To: devel; +Cc: Ray Ni, Laszlo Ersek, Chandana Kumar, Star Zeng
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1972
Function in this library may be used by APs. Assert will be trig if AP uses
dynamic pcd.
This patch enhance the current code, remove the unnecessary usage of dynamic
PCD. This change try to avoid report this issue again later.
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Chandana Kumar <chandana.c.kumar@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
.../CpuFeaturesInitialize.c | 64 +++++-----
.../RegisterCpuFeatures.h | 10 +-
.../RegisterCpuFeaturesLib.c | 114 ++++++------------
3 files changed, 77 insertions(+), 111 deletions(-)
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
index 87bfc64250..16b99c0c27 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
@@ -21,16 +21,12 @@ CHAR16 *mRegisterTypeStr[] = {L"MSR", L"CR", L"MMIO", L"CACHE", L"SEMAP", L"INVA
VOID
SetCapabilityPcd (
IN UINT8 *SupportedFeatureMask,
- IN UINT32 FeatureMaskSize
+ IN UINTN FeatureMaskSize
)
{
EFI_STATUS Status;
- UINTN BitMaskSize;
- BitMaskSize = PcdGetSize (PcdCpuFeaturesCapability);
- ASSERT (FeatureMaskSize == BitMaskSize);
-
- Status = PcdSetPtrS (PcdCpuFeaturesCapability, &BitMaskSize, SupportedFeatureMask);
+ Status = PcdSetPtrS (PcdCpuFeaturesCapability, &FeatureMaskSize, SupportedFeatureMask);
ASSERT_EFI_ERROR (Status);
}
@@ -38,16 +34,16 @@ SetCapabilityPcd (
Worker function to save PcdCpuFeaturesSetting.
@param[in] SupportedFeatureMask The pointer to CPU feature bits mask buffer
+ @param[in] FeatureMaskSize CPU feature bits mask buffer size.
**/
VOID
SetSettingPcd (
- IN UINT8 *SupportedFeatureMask
+ IN UINT8 *SupportedFeatureMask,
+ IN UINTN BitMaskSize
)
{
EFI_STATUS Status;
- UINTN BitMaskSize;
- BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
Status = PcdSetPtrS (PcdCpuFeaturesSetting, &BitMaskSize, SupportedFeatureMask);
ASSERT_EFI_ERROR (Status);
}
@@ -272,19 +268,20 @@ SupportedMaskOr (
@param[in] SupportedFeatureMask The pointer to CPU feature bits mask buffer
@param[in] AndFeatureBitMask The feature bit mask to do AND operation
+ @param[in] BitMaskSize CPU feature bits mask buffer size.
+
**/
VOID
SupportedMaskAnd (
IN UINT8 *SupportedFeatureMask,
- IN CONST UINT8 *AndFeatureBitMask
+ IN CONST UINT8 *AndFeatureBitMask,
+ IN UINT32 BitMaskSize
)
{
UINTN Index;
- UINTN BitMaskSize;
UINT8 *Data1;
CONST UINT8 *Data2;
- BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
Data1 = SupportedFeatureMask;
Data2 = AndFeatureBitMask;
for (Index = 0; Index < BitMaskSize; Index++) {
@@ -297,19 +294,19 @@ SupportedMaskAnd (
@param[in] SupportedFeatureMask The pointer to CPU feature bits mask buffer
@param[in] AndFeatureBitMask The feature bit mask to do XOR operation
+ @param[in] BitMaskSize CPU feature bits mask buffer size.
**/
VOID
SupportedMaskCleanBit (
IN UINT8 *SupportedFeatureMask,
- IN UINT8 *AndFeatureBitMask
+ IN UINT8 *AndFeatureBitMask,
+ IN UINT32 BitMaskSize
)
{
UINTN Index;
- UINTN BitMaskSize;
UINT8 *Data1;
UINT8 *Data2;
- BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
Data1 = SupportedFeatureMask;
Data2 = AndFeatureBitMask;
for (Index = 0; Index < BitMaskSize; Index++) {
@@ -323,6 +320,7 @@ SupportedMaskCleanBit (
@param[in] SupportedFeatureMask The pointer to CPU feature bits mask buffer
@param[in] ComparedFeatureBitMask The feature bit mask to be compared
+ @param[in] BitMaskSize CPU feature bits mask buffer size.
@retval TRUE The ComparedFeatureBitMask is set in CPU feature supported bits
mask buffer.
@@ -332,16 +330,14 @@ SupportedMaskCleanBit (
BOOLEAN
IsBitMaskMatch (
IN UINT8 *SupportedFeatureMask,
- IN UINT8 *ComparedFeatureBitMask
+ IN UINT8 *ComparedFeatureBitMask,
+ IN UINT32 BitMaskSize
)
{
UINTN Index;
- UINTN BitMaskSize;
UINT8 *Data1;
UINT8 *Data2;
- BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
-
Data1 = SupportedFeatureMask;
Data2 = ComparedFeatureBitMask;
for (Index = 0; Index < BitMaskSize; Index++) {
@@ -557,14 +553,14 @@ AnalysisProcessorFeatures (
//
// Calculate the last capability on all processors
//
- SupportedMaskAnd (CpuFeaturesData->CapabilityPcd, CpuInitOrder->FeaturesSupportedMask);
+ SupportedMaskAnd (CpuFeaturesData->CapabilityPcd, CpuInitOrder->FeaturesSupportedMask, CpuFeaturesData->BitMaskSize);
}
//
// Calculate the last setting
//
CpuFeaturesData->SettingPcd = AllocateCopyPool (CpuFeaturesData->BitMaskSize, CpuFeaturesData->CapabilityPcd);
ASSERT (CpuFeaturesData->SettingPcd != NULL);
- SupportedMaskAnd (CpuFeaturesData->SettingPcd, PcdGetPtr (PcdCpuFeaturesSetting));
+ SupportedMaskAnd (CpuFeaturesData->SettingPcd, PcdGetPtr (PcdCpuFeaturesSetting), CpuFeaturesData->BitMaskSize);
//
// Dump the last CPU feature list
@@ -574,8 +570,8 @@ AnalysisProcessorFeatures (
Entry = GetFirstNode (&CpuFeaturesData->FeatureList);
while (!IsNull (&CpuFeaturesData->FeatureList, Entry)) {
CpuFeature = CPU_FEATURE_ENTRY_FROM_LINK (Entry);
- if (IsBitMaskMatch (CpuFeature->FeatureMask, CpuFeaturesData->CapabilityPcd)) {
- if (IsBitMaskMatch (CpuFeature->FeatureMask, CpuFeaturesData->SettingPcd)) {
+ if (IsBitMaskMatch (CpuFeature->FeatureMask, CpuFeaturesData->CapabilityPcd, CpuFeaturesData->BitMaskSize)) {
+ if (IsBitMaskMatch (CpuFeature->FeatureMask, CpuFeaturesData->SettingPcd, CpuFeaturesData->BitMaskSize)) {
DEBUG ((DEBUG_INFO, "[Enable ] "));
} else {
DEBUG ((DEBUG_INFO, "[Disable ] "));
@@ -583,22 +579,22 @@ AnalysisProcessorFeatures (
} else {
DEBUG ((DEBUG_INFO, "[Unsupport] "));
}
- DumpCpuFeature (CpuFeature);
+ DumpCpuFeature (CpuFeature, CpuFeaturesData->BitMaskSize);
Entry = Entry->ForwardLink;
}
DEBUG ((DEBUG_INFO, "PcdCpuFeaturesCapability:\n"));
- DumpCpuFeatureMask (CpuFeaturesData->CapabilityPcd);
+ DumpCpuFeatureMask (CpuFeaturesData->CapabilityPcd, CpuFeaturesData->BitMaskSize);
DEBUG ((DEBUG_INFO, "Origin PcdCpuFeaturesSetting:\n"));
- DumpCpuFeatureMask (PcdGetPtr (PcdCpuFeaturesSetting));
+ DumpCpuFeatureMask (PcdGetPtr (PcdCpuFeaturesSetting), CpuFeaturesData->BitMaskSize);
DEBUG ((DEBUG_INFO, "Final PcdCpuFeaturesSetting:\n"));
- DumpCpuFeatureMask (CpuFeaturesData->SettingPcd);
+ DumpCpuFeatureMask (CpuFeaturesData->SettingPcd, CpuFeaturesData->BitMaskSize);
);
//
// Save PCDs and display CPU PCDs
//
SetCapabilityPcd (CpuFeaturesData->CapabilityPcd, CpuFeaturesData->BitMaskSize);
- SetSettingPcd (CpuFeaturesData->SettingPcd);
+ SetSettingPcd (CpuFeaturesData->SettingPcd, CpuFeaturesData->BitMaskSize);
for (ProcessorNumber = 0; ProcessorNumber < NumberOfCpus; ProcessorNumber++) {
CpuInitOrder = &CpuFeaturesData->InitOrder[ProcessorNumber];
@@ -608,7 +604,7 @@ AnalysisProcessorFeatures (
// Insert each feature into processor's order list
//
CpuFeature = CPU_FEATURE_ENTRY_FROM_LINK (Entry);
- if (IsBitMaskMatch (CpuFeature->FeatureMask, CpuFeaturesData->CapabilityPcd)) {
+ if (IsBitMaskMatch (CpuFeature->FeatureMask, CpuFeaturesData->CapabilityPcd, CpuFeaturesData->BitMaskSize)) {
CpuFeatureInOrder = AllocateCopyPool (sizeof (CPU_FEATURES_ENTRY), CpuFeature);
ASSERT (CpuFeatureInOrder != NULL);
InsertTailList (&CpuInitOrder->OrderList, &CpuFeatureInOrder->Link);
@@ -624,18 +620,18 @@ AnalysisProcessorFeatures (
CpuFeatureInOrder = CPU_FEATURE_ENTRY_FROM_LINK (Entry);
Success = FALSE;
- if (IsBitMaskMatch (CpuFeatureInOrder->FeatureMask, CpuFeaturesData->SettingPcd)) {
+ if (IsBitMaskMatch (CpuFeatureInOrder->FeatureMask, CpuFeaturesData->SettingPcd, CpuFeaturesData->BitMaskSize)) {
Status = CpuFeatureInOrder->InitializeFunc (ProcessorNumber, CpuInfo, CpuFeatureInOrder->ConfigData, TRUE);
if (EFI_ERROR (Status)) {
//
// Clean the CpuFeatureInOrder->FeatureMask in setting PCD.
//
- SupportedMaskCleanBit (CpuFeaturesData->SettingPcd, CpuFeatureInOrder->FeatureMask);
+ SupportedMaskCleanBit (CpuFeaturesData->SettingPcd, CpuFeatureInOrder->FeatureMask, CpuFeaturesData->BitMaskSize);
if (CpuFeatureInOrder->FeatureName != NULL) {
DEBUG ((DEBUG_WARN, "Warning :: Failed to enable Feature: Name = %a.\n", CpuFeatureInOrder->FeatureName));
} else {
DEBUG ((DEBUG_WARN, "Warning :: Failed to enable Feature: Mask = "));
- DumpCpuFeatureMask (CpuFeatureInOrder->FeatureMask);
+ DumpCpuFeatureMask (CpuFeatureInOrder->FeatureMask, CpuFeaturesData->BitMaskSize);
}
} else {
Success = TRUE;
@@ -647,7 +643,7 @@ AnalysisProcessorFeatures (
DEBUG ((DEBUG_WARN, "Warning :: Failed to disable Feature: Name = %a.\n", CpuFeatureInOrder->FeatureName));
} else {
DEBUG ((DEBUG_WARN, "Warning :: Failed to disable Feature: Mask = "));
- DumpCpuFeatureMask (CpuFeatureInOrder->FeatureMask);
+ DumpCpuFeatureMask (CpuFeatureInOrder->FeatureMask, CpuFeaturesData->BitMaskSize);
}
} else {
Success = TRUE;
@@ -699,7 +695,7 @@ AnalysisProcessorFeatures (
// again during initialize the features.
//
DEBUG ((DEBUG_INFO, "Dump final value for PcdCpuFeaturesSetting:\n"));
- DumpCpuFeatureMask (CpuFeaturesData->SettingPcd);
+ DumpCpuFeatureMask (CpuFeaturesData->SettingPcd, CpuFeaturesData->BitMaskSize);
//
// Dump the RegisterTable
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
index 5c546ee153..a18f926641 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
@@ -180,20 +180,26 @@ SwitchNewBsp (
Function that uses DEBUG() macros to display the contents of a a CPU feature bit mask.
@param[in] FeatureMask A pointer to the CPU feature bit mask.
+ @param[in] BitMaskSize CPU feature bits mask buffer size.
+
**/
VOID
DumpCpuFeatureMask (
- IN UINT8 *FeatureMask
+ IN UINT8 *FeatureMask,
+ IN UINT32 BitMaskSize
);
/**
Dump CPU feature name or CPU feature bit mask.
@param[in] CpuFeature Pointer to CPU_FEATURES_ENTRY
+ @param[in] BitMaskSize CPU feature bits mask buffer size.
+
**/
VOID
DumpCpuFeature (
- IN CPU_FEATURES_ENTRY *CpuFeature
+ IN CPU_FEATURES_ENTRY *CpuFeature,
+ IN UINT32 BitMaskSize
);
/**
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
index 36aabd7267..283e9d6539 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
@@ -18,36 +18,34 @@
@retval FALSE Two CPU feature bit masks are not equal.
**/
BOOLEAN
-IsCpuFeatureMatch (
+IsBitMaskMatchCheck (
IN UINT8 *FirstFeatureMask,
IN UINT8 *SecondFeatureMask
)
{
- UINTN BitMaskSize;
+ CPU_FEATURES_DATA *CpuFeaturesData;
- BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
- if (CompareMem (FirstFeatureMask, SecondFeatureMask, BitMaskSize) == 0) {
- return TRUE;
- } else {
- return FALSE;
- }
+ CpuFeaturesData = GetCpuFeaturesData ();
+
+ return (CompareMem (FirstFeatureMask, SecondFeatureMask, CpuFeaturesData->BitMaskSize) == 0);
}
/**
Function that uses DEBUG() macros to display the contents of a a CPU feature bit mask.
@param[in] FeatureMask A pointer to the CPU feature bit mask.
+ @param[in] BitMaskSize CPU feature bits mask buffer size.
+
**/
VOID
DumpCpuFeatureMask (
- IN UINT8 *FeatureMask
+ IN UINT8 *FeatureMask,
+ IN UINT32 BitMaskSize
)
{
UINTN Index;
UINT8 *Data8;
- UINTN BitMaskSize;
- BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
Data8 = (UINT8 *) FeatureMask;
for (Index = 0; Index < BitMaskSize; Index++) {
DEBUG ((DEBUG_INFO, " %02x ", *Data8++));
@@ -59,10 +57,13 @@ DumpCpuFeatureMask (
Dump CPU feature name or CPU feature bit mask.
@param[in] CpuFeature Pointer to CPU_FEATURES_ENTRY
+ @param[in] BitMaskSize CPU feature bits mask buffer size.
+
**/
VOID
DumpCpuFeature (
- IN CPU_FEATURES_ENTRY *CpuFeature
+ IN CPU_FEATURES_ENTRY *CpuFeature,
+ IN UINT32 BitMaskSize
)
{
@@ -70,42 +71,10 @@ DumpCpuFeature (
DEBUG ((DEBUG_INFO, "FeatureName: %a\n", CpuFeature->FeatureName));
} else {
DEBUG ((DEBUG_INFO, "FeatureMask = "));
- DumpCpuFeatureMask (CpuFeature->FeatureMask);
+ DumpCpuFeatureMask (CpuFeature->FeatureMask, BitMaskSize);
}
}
-/**
- Determines if the feature bit mask is in dependent CPU feature bit mask buffer.
-
- @param[in] FeatureMask Pointer to CPU feature bit mask
- @param[in] DependentBitMask Pointer to dependent CPU feature bit mask buffer
-
- @retval TRUE The feature bit mask is in dependent CPU feature bit mask buffer.
- @retval FALSE The feature bit mask is not in dependent CPU feature bit mask buffer.
-**/
-BOOLEAN
-IsBitMaskMatchCheck (
- IN UINT8 *FeatureMask,
- IN UINT8 *DependentBitMask
- )
-{
- UINTN Index;
- UINTN BitMaskSize;
- UINT8 *Data1;
- UINT8 *Data2;
-
- BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
-
- Data1 = FeatureMask;
- Data2 = DependentBitMask;
- for (Index = 0; Index < BitMaskSize; Index++) {
- if (((*(Data1++)) & (*(Data2++))) != 0) {
- return TRUE;
- }
- }
- return FALSE;
-}
-
/**
Try to find the specify cpu featuren in former/after feature list.
@@ -642,37 +611,21 @@ CheckCpuFeaturesDependency (
**/
RETURN_STATUS
RegisterCpuFeatureWorker (
+ IN CPU_FEATURES_DATA *CpuFeaturesData,
IN CPU_FEATURES_ENTRY *CpuFeature
)
{
EFI_STATUS Status;
- CPU_FEATURES_DATA *CpuFeaturesData;
CPU_FEATURES_ENTRY *CpuFeatureEntry;
LIST_ENTRY *Entry;
- UINTN BitMaskSize;
BOOLEAN FeatureExist;
- BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
- CpuFeaturesData = GetCpuFeaturesData ();
- if (CpuFeaturesData->FeaturesCount == 0) {
- InitializeListHead (&CpuFeaturesData->FeatureList);
- InitializeSpinLock (&CpuFeaturesData->CpuFlags.MemoryMappedLock);
- InitializeSpinLock (&CpuFeaturesData->CpuFlags.ConsoleLogLock);
- //
- // Driver has assumption that these three PCD should has same buffer size.
- //
- ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize (PcdCpuFeaturesCapability));
- ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize (PcdCpuFeaturesSupport));
- CpuFeaturesData->BitMaskSize = (UINT32) BitMaskSize;
- }
- ASSERT (CpuFeaturesData->BitMaskSize == BitMaskSize);
-
FeatureExist = FALSE;
CpuFeatureEntry = NULL;
Entry = GetFirstNode (&CpuFeaturesData->FeatureList);
while (!IsNull (&CpuFeaturesData->FeatureList, Entry)) {
CpuFeatureEntry = CPU_FEATURE_ENTRY_FROM_LINK (Entry);
- if (IsCpuFeatureMatch (CpuFeature->FeatureMask, CpuFeatureEntry->FeatureMask)) {
+ if (IsBitMaskMatchCheck (CpuFeature->FeatureMask, CpuFeatureEntry->FeatureMask)) {
//
// If this feature already registered
//
@@ -684,12 +637,12 @@ RegisterCpuFeatureWorker (
if (!FeatureExist) {
DEBUG ((DEBUG_INFO, "[NEW] "));
- DumpCpuFeature (CpuFeature);
+ DumpCpuFeature (CpuFeature, CpuFeaturesData->BitMaskSize);
InsertTailList (&CpuFeaturesData->FeatureList, &CpuFeature->Link);
CpuFeaturesData->FeaturesCount++;
} else {
DEBUG ((DEBUG_INFO, "[OVERRIDE] "));
- DumpCpuFeature (CpuFeature);
+ DumpCpuFeature (CpuFeature, CpuFeaturesData->BitMaskSize);
ASSERT (CpuFeatureEntry != NULL);
//
// Overwrite original parameters of CPU feature
@@ -849,7 +802,6 @@ RegisterCpuFeature (
EFI_STATUS Status;
VA_LIST Marker;
UINT32 Feature;
- UINTN BitMaskSize;
CPU_FEATURES_ENTRY *CpuFeature;
UINT8 *FeatureMask;
UINT8 *BeforeFeatureBitMask;
@@ -860,6 +812,7 @@ RegisterCpuFeature (
UINT8 *PackageAfterFeatureBitMask;
BOOLEAN BeforeAll;
BOOLEAN AfterAll;
+ CPU_FEATURES_DATA *CpuFeaturesData;
FeatureMask = NULL;
BeforeFeatureBitMask = NULL;
@@ -871,7 +824,18 @@ RegisterCpuFeature (
BeforeAll = FALSE;
AfterAll = FALSE;
- BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
+ CpuFeaturesData = GetCpuFeaturesData ();
+ if (CpuFeaturesData->FeaturesCount == 0) {
+ InitializeListHead (&CpuFeaturesData->FeatureList);
+ InitializeSpinLock (&CpuFeaturesData->CpuFlags.MemoryMappedLock);
+ InitializeSpinLock (&CpuFeaturesData->CpuFlags.ConsoleLogLock);
+ //
+ // Driver has assumption that below three PCDs should has same buffer size.
+ //
+ ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize (PcdCpuFeaturesCapability));
+ ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize (PcdCpuFeaturesSupport));
+ CpuFeaturesData->BitMaskSize = (UINT32) PcdGetSize (PcdCpuFeaturesSetting);
+ }
VA_START (Marker, InitializeFunc);
Feature = VA_ARG (Marker, UINT32);
@@ -889,19 +853,19 @@ RegisterCpuFeature (
AfterAll = ((Feature & CPU_FEATURE_AFTER_ALL) != 0) ? TRUE : FALSE;
Feature &= ~(CPU_FEATURE_BEFORE_ALL | CPU_FEATURE_AFTER_ALL);
ASSERT (FeatureMask == NULL);
- SetCpuFeaturesBitMask (&FeatureMask, Feature, BitMaskSize);
+ SetCpuFeaturesBitMask (&FeatureMask, Feature, CpuFeaturesData->BitMaskSize);
} else if ((Feature & CPU_FEATURE_BEFORE) != 0) {
- SetCpuFeaturesBitMask (&BeforeFeatureBitMask, Feature & ~CPU_FEATURE_BEFORE, BitMaskSize);
+ SetCpuFeaturesBitMask (&BeforeFeatureBitMask, Feature & ~CPU_FEATURE_BEFORE, CpuFeaturesData->BitMaskSize);
} else if ((Feature & CPU_FEATURE_AFTER) != 0) {
- SetCpuFeaturesBitMask (&AfterFeatureBitMask, Feature & ~CPU_FEATURE_AFTER, BitMaskSize);
+ SetCpuFeaturesBitMask (&AfterFeatureBitMask, Feature & ~CPU_FEATURE_AFTER, CpuFeaturesData->BitMaskSize);
} else if ((Feature & CPU_FEATURE_CORE_BEFORE) != 0) {
- SetCpuFeaturesBitMask (&CoreBeforeFeatureBitMask, Feature & ~CPU_FEATURE_CORE_BEFORE, BitMaskSize);
+ SetCpuFeaturesBitMask (&CoreBeforeFeatureBitMask, Feature & ~CPU_FEATURE_CORE_BEFORE, CpuFeaturesData->BitMaskSize);
} else if ((Feature & CPU_FEATURE_CORE_AFTER) != 0) {
- SetCpuFeaturesBitMask (&CoreAfterFeatureBitMask, Feature & ~CPU_FEATURE_CORE_AFTER, BitMaskSize);
+ SetCpuFeaturesBitMask (&CoreAfterFeatureBitMask, Feature & ~CPU_FEATURE_CORE_AFTER, CpuFeaturesData->BitMaskSize);
} else if ((Feature & CPU_FEATURE_PACKAGE_BEFORE) != 0) {
- SetCpuFeaturesBitMask (&PackageBeforeFeatureBitMask, Feature & ~CPU_FEATURE_PACKAGE_BEFORE, BitMaskSize);
+ SetCpuFeaturesBitMask (&PackageBeforeFeatureBitMask, Feature & ~CPU_FEATURE_PACKAGE_BEFORE, CpuFeaturesData->BitMaskSize);
} else if ((Feature & CPU_FEATURE_PACKAGE_AFTER) != 0) {
- SetCpuFeaturesBitMask (&PackageAfterFeatureBitMask, Feature & ~CPU_FEATURE_PACKAGE_AFTER, BitMaskSize);
+ SetCpuFeaturesBitMask (&PackageAfterFeatureBitMask, Feature & ~CPU_FEATURE_PACKAGE_AFTER, CpuFeaturesData->BitMaskSize);
}
Feature = VA_ARG (Marker, UINT32);
}
@@ -929,7 +893,7 @@ RegisterCpuFeature (
ASSERT_EFI_ERROR (Status);
}
- Status = RegisterCpuFeatureWorker (CpuFeature);
+ Status = RegisterCpuFeatureWorker (CpuFeaturesData, CpuFeature);
ASSERT_EFI_ERROR (Status);
return RETURN_SUCCESS;
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Patch 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls gBS service.
2019-07-12 1:53 ` [Patch 1/2] " Dong, Eric
@ 2019-07-12 10:53 ` Zeng, Star
2019-07-15 4:59 ` Ni, Ray
0 siblings, 1 reply; 9+ messages in thread
From: Zeng, Star @ 2019-07-12 10:53 UTC (permalink / raw)
To: Dong, Eric, devel@edk2.groups.io
Cc: Ni, Ray, Laszlo Ersek, Kumar, Chandana C, Zeng, Star
Some minor comments inline.
> -----Original Message-----
> From: Dong, Eric
> Sent: Friday, July 12, 2019 9:53 AM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar,
> Chandana C <chandana.c.kumar@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: [Patch 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls gBS
> service.
"gBS service" is not match with the assertion information, gBS is the concept in DXE phase.
So here, please "PEI service" to be accurate.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1972
>
> AP calls CollectProcessorData() to collect processor info.
> CollectProcessorData function finally calls PcdGetSize function to
> get DynamicPCD PcdCpuFeaturesSetting value. PcdGetSize will use gBS
Same comments with above.
> which caused below assert info:
> Processor Info: Package: 1, MaxCore : 4, MaxThread: 1
> Package: 0, Valid Core : 4
> ASSERT [CpuFeaturesPei] c:\projects\jsl\jsl_v1193\Edk2\MdePkg\Library
> \PeiServicesTablePointerLibIdt\PeiServicesTablePointer.c(48):
> PeiServices != ((void *) 0)
>
> This change uses saved global pcd size instead of calls PcdGetSize to
> fix this issue.
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Chandana Kumar <chandana.c.kumar@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 13 ++++++++-----
> .../RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c | 5 +++++
> 2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> index aff7ad600c..87bfc64250 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> @@ -246,19 +246,20 @@ CpuInitDataInitialize (
>
> @param[in] SupportedFeatureMask The pointer to CPU feature bits mask
> buffer
> @param[in] OrFeatureBitMask The feature bit mask to do OR operation
> + @param[in] BitMaskSize The CPU feature bits mask buffer size.
> +
> **/
> VOID
> SupportedMaskOr (
> IN UINT8 *SupportedFeatureMask,
> - IN UINT8 *OrFeatureBitMask
> + IN UINT8 *OrFeatureBitMask,
> + IN UINT32 BitMaskSize
> )
> {
> UINTN Index;
> - UINTN BitMaskSize;
> UINT8 *Data1;
> UINT8 *Data2;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> Data1 = SupportedFeatureMask;
> Data2 = OrFeatureBitMask;
> for (Index = 0; Index < BitMaskSize; Index++) {
> @@ -384,12 +385,14 @@ CollectProcessorData (
> //
> SupportedMaskOr (
> CpuFeaturesData-
> >InitOrder[ProcessorNumber].FeaturesSupportedMask,
> - CpuFeature->FeatureMask
> + CpuFeature->FeatureMask,
> + CpuFeaturesData->BitMaskSize
> );
> } else if (CpuFeature->SupportFunc (ProcessorNumber, CpuInfo,
> CpuFeature->ConfigData)) {
> SupportedMaskOr (
> CpuFeaturesData-
> >InitOrder[ProcessorNumber].FeaturesSupportedMask,
> - CpuFeature->FeatureMask
> + CpuFeature->FeatureMask,
> + CpuFeaturesData->BitMaskSize
> );
> }
> Entry = Entry->ForwardLink;
> diff --git
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> index fa0f0b41e2..36aabd7267 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> +++
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> @@ -658,6 +658,11 @@ RegisterCpuFeatureWorker (
> InitializeListHead (&CpuFeaturesData->FeatureList);
> InitializeSpinLock (&CpuFeaturesData->CpuFlags.MemoryMappedLock);
> InitializeSpinLock (&CpuFeaturesData->CpuFlags.ConsoleLogLock);
> + //
> + // Driver has assumption that these three PCD should has same buffer
> size.
It is library, not driver. So how about "The code has assumption that these three PCDs should have same buffer size."?
The proposed sentence also uses 'PCDs' and 'have'.
With the comments handled, Reviewed-by: Star Zeng <star.zeng@intel.com>
Thanks,
Star
> + //
> + ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize
> (PcdCpuFeaturesCapability));
> + ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize
> (PcdCpuFeaturesSupport));
> CpuFeaturesData->BitMaskSize = (UINT32) BitMaskSize;
> }
> ASSERT (CpuFeaturesData->BitMaskSize == BitMaskSize);
> --
> 2.21.0.windows.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [Patch 2/2] UefiCpuPkg/Library/RegisterCpuFeaturesLib: avoid use dynamic PCD.
2019-07-12 1:53 ` [Patch 2/2] UefiCpuPkg/Library/RegisterCpuFeaturesLib: avoid use dynamic PCD Dong, Eric
@ 2019-07-12 11:10 ` Zeng, Star
2019-07-15 4:57 ` Ni, Ray
1 sibling, 0 replies; 9+ messages in thread
From: Zeng, Star @ 2019-07-12 11:10 UTC (permalink / raw)
To: devel@edk2.groups.io, Dong, Eric
Cc: Ni, Ray, Laszlo Ersek, Kumar, Chandana C, Zeng, Star
4 comments are added inline.
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Dong, Eric
> Sent: Friday, July 12, 2019 9:53 AM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar,
> Chandana C <chandana.c.kumar@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: [edk2-devel] [Patch 2/2]
> UefiCpuPkg/Library/RegisterCpuFeaturesLib: avoid use dynamic PCD.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1972
>
> Function in this library may be used by APs. Assert will be trig if AP uses
> dynamic pcd.
> This patch enhance the current code, remove the unnecessary usage of
> dynamic PCD. This change try to avoid report this issue again later.
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Chandana Kumar <chandana.c.kumar@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> .../CpuFeaturesInitialize.c | 64 +++++-----
> .../RegisterCpuFeatures.h | 10 +-
> .../RegisterCpuFeaturesLib.c | 114 ++++++------------
> 3 files changed, 77 insertions(+), 111 deletions(-)
>
> diff --git
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> index 87bfc64250..16b99c0c27 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> @@ -21,16 +21,12 @@ CHAR16 *mRegisterTypeStr[] = {L"MSR", L"CR",
> L"MMIO", L"CACHE", L"SEMAP", L"INVA VOID SetCapabilityPcd (
> IN UINT8 *SupportedFeatureMask,
> - IN UINT32 FeatureMaskSize
> + IN UINTN FeatureMaskSize
1. How about using BitMaskSize to be aligned with other places? Notice, the function header also needs to be updated if using BitMaskSize.
> )
> {
> EFI_STATUS Status;
> - UINTN BitMaskSize;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesCapability);
> - ASSERT (FeatureMaskSize == BitMaskSize);
> -
> - Status = PcdSetPtrS (PcdCpuFeaturesCapability, &BitMaskSize,
> SupportedFeatureMask);
> + Status = PcdSetPtrS (PcdCpuFeaturesCapability, &FeatureMaskSize,
> + SupportedFeatureMask);
> ASSERT_EFI_ERROR (Status);
> }
>
> @@ -38,16 +34,16 @@ SetCapabilityPcd (
> Worker function to save PcdCpuFeaturesSetting.
>
> @param[in] SupportedFeatureMask The pointer to CPU feature bits mask
> buffer
> + @param[in] FeatureMaskSize CPU feature bits mask buffer size.
2. Need use BitMaskSize to be matched with function parameter name.
> **/
> VOID
> SetSettingPcd (
> - IN UINT8 *SupportedFeatureMask
> + IN UINT8 *SupportedFeatureMask,
> + IN UINTN BitMaskSize
> )
> {
> EFI_STATUS Status;
> - UINTN BitMaskSize;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> Status = PcdSetPtrS (PcdCpuFeaturesSetting, &BitMaskSize,
> SupportedFeatureMask);
> ASSERT_EFI_ERROR (Status);
> }
> @@ -272,19 +268,20 @@ SupportedMaskOr (
>
> @param[in] SupportedFeatureMask The pointer to CPU feature bits mask
> buffer
> @param[in] AndFeatureBitMask The feature bit mask to do AND
> operation
> + @param[in] BitMaskSize CPU feature bits mask buffer size.
> +
> **/
> VOID
> SupportedMaskAnd (
> IN UINT8 *SupportedFeatureMask,
> - IN CONST UINT8 *AndFeatureBitMask
> + IN CONST UINT8 *AndFeatureBitMask,
> + IN UINT32 BitMaskSize
> )
> {
> UINTN Index;
> - UINTN BitMaskSize;
> UINT8 *Data1;
> CONST UINT8 *Data2;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> Data1 = SupportedFeatureMask;
> Data2 = AndFeatureBitMask;
> for (Index = 0; Index < BitMaskSize; Index++) { @@ -297,19 +294,19 @@
> SupportedMaskAnd (
>
> @param[in] SupportedFeatureMask The pointer to CPU feature bits mask
> buffer
> @param[in] AndFeatureBitMask The feature bit mask to do XOR
> operation
> + @param[in] BitMaskSize CPU feature bits mask buffer size.
> **/
> VOID
> SupportedMaskCleanBit (
> IN UINT8 *SupportedFeatureMask,
> - IN UINT8 *AndFeatureBitMask
> + IN UINT8 *AndFeatureBitMask,
> + IN UINT32 BitMaskSize
> )
> {
> UINTN Index;
> - UINTN BitMaskSize;
> UINT8 *Data1;
> UINT8 *Data2;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> Data1 = SupportedFeatureMask;
> Data2 = AndFeatureBitMask;
> for (Index = 0; Index < BitMaskSize; Index++) { @@ -323,6 +320,7 @@
> SupportedMaskCleanBit (
>
> @param[in] SupportedFeatureMask The pointer to CPU feature bits mask
> buffer
> @param[in] ComparedFeatureBitMask The feature bit mask to be
> compared
> + @param[in] BitMaskSize CPU feature bits mask buffer size.
>
> @retval TRUE The ComparedFeatureBitMask is set in CPU feature
> supported bits
> mask buffer.
> @@ -332,16 +330,14 @@ SupportedMaskCleanBit ( BOOLEAN
> IsBitMaskMatch (
> IN UINT8 *SupportedFeatureMask,
> - IN UINT8 *ComparedFeatureBitMask
> + IN UINT8 *ComparedFeatureBitMask,
> + IN UINT32 BitMaskSize
> )
> {
> UINTN Index;
> - UINTN BitMaskSize;
> UINT8 *Data1;
> UINT8 *Data2;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> -
> Data1 = SupportedFeatureMask;
> Data2 = ComparedFeatureBitMask;
> for (Index = 0; Index < BitMaskSize; Index++) { @@ -557,14 +553,14 @@
> AnalysisProcessorFeatures (
> //
> // Calculate the last capability on all processors
> //
> - SupportedMaskAnd (CpuFeaturesData->CapabilityPcd, CpuInitOrder-
> >FeaturesSupportedMask);
> + SupportedMaskAnd (CpuFeaturesData->CapabilityPcd,
> + CpuInitOrder->FeaturesSupportedMask, CpuFeaturesData->BitMaskSize);
> }
> //
> // Calculate the last setting
> //
> CpuFeaturesData->SettingPcd = AllocateCopyPool (CpuFeaturesData-
> >BitMaskSize, CpuFeaturesData->CapabilityPcd);
> ASSERT (CpuFeaturesData->SettingPcd != NULL);
> - SupportedMaskAnd (CpuFeaturesData->SettingPcd, PcdGetPtr
> (PcdCpuFeaturesSetting));
> + SupportedMaskAnd (CpuFeaturesData->SettingPcd, PcdGetPtr
> + (PcdCpuFeaturesSetting), CpuFeaturesData->BitMaskSize);
>
> //
> // Dump the last CPU feature list
> @@ -574,8 +570,8 @@ AnalysisProcessorFeatures (
> Entry = GetFirstNode (&CpuFeaturesData->FeatureList);
> while (!IsNull (&CpuFeaturesData->FeatureList, Entry)) {
> CpuFeature = CPU_FEATURE_ENTRY_FROM_LINK (Entry);
> - if (IsBitMaskMatch (CpuFeature->FeatureMask, CpuFeaturesData-
> >CapabilityPcd)) {
> - if (IsBitMaskMatch (CpuFeature->FeatureMask, CpuFeaturesData-
> >SettingPcd)) {
> + if (IsBitMaskMatch (CpuFeature->FeatureMask, CpuFeaturesData-
> >CapabilityPcd, CpuFeaturesData->BitMaskSize)) {
> + if (IsBitMaskMatch (CpuFeature->FeatureMask,
> + CpuFeaturesData->SettingPcd, CpuFeaturesData->BitMaskSize)) {
> DEBUG ((DEBUG_INFO, "[Enable ] "));
> } else {
> DEBUG ((DEBUG_INFO, "[Disable ] ")); @@ -583,22 +579,22 @@
> AnalysisProcessorFeatures (
> } else {
> DEBUG ((DEBUG_INFO, "[Unsupport] "));
> }
> - DumpCpuFeature (CpuFeature);
> + DumpCpuFeature (CpuFeature, CpuFeaturesData->BitMaskSize);
> Entry = Entry->ForwardLink;
> }
> DEBUG ((DEBUG_INFO, "PcdCpuFeaturesCapability:\n"));
> - DumpCpuFeatureMask (CpuFeaturesData->CapabilityPcd);
> + DumpCpuFeatureMask (CpuFeaturesData->CapabilityPcd,
> + CpuFeaturesData->BitMaskSize);
> DEBUG ((DEBUG_INFO, "Origin PcdCpuFeaturesSetting:\n"));
> - DumpCpuFeatureMask (PcdGetPtr (PcdCpuFeaturesSetting));
> + DumpCpuFeatureMask (PcdGetPtr (PcdCpuFeaturesSetting),
> + CpuFeaturesData->BitMaskSize);
> DEBUG ((DEBUG_INFO, "Final PcdCpuFeaturesSetting:\n"));
> - DumpCpuFeatureMask (CpuFeaturesData->SettingPcd);
> + DumpCpuFeatureMask (CpuFeaturesData->SettingPcd,
> + CpuFeaturesData->BitMaskSize);
> );
>
> //
> // Save PCDs and display CPU PCDs
> //
> SetCapabilityPcd (CpuFeaturesData->CapabilityPcd, CpuFeaturesData-
> >BitMaskSize);
> - SetSettingPcd (CpuFeaturesData->SettingPcd);
> + SetSettingPcd (CpuFeaturesData->SettingPcd,
> + CpuFeaturesData->BitMaskSize);
>
> for (ProcessorNumber = 0; ProcessorNumber < NumberOfCpus;
> ProcessorNumber++) {
> CpuInitOrder = &CpuFeaturesData->InitOrder[ProcessorNumber];
> @@ -608,7 +604,7 @@ AnalysisProcessorFeatures (
> // Insert each feature into processor's order list
> //
> CpuFeature = CPU_FEATURE_ENTRY_FROM_LINK (Entry);
> - if (IsBitMaskMatch (CpuFeature->FeatureMask, CpuFeaturesData-
> >CapabilityPcd)) {
> + if (IsBitMaskMatch (CpuFeature->FeatureMask,
> + CpuFeaturesData->CapabilityPcd, CpuFeaturesData->BitMaskSize)) {
> CpuFeatureInOrder = AllocateCopyPool (sizeof (CPU_FEATURES_ENTRY),
> CpuFeature);
> ASSERT (CpuFeatureInOrder != NULL);
> InsertTailList (&CpuInitOrder->OrderList, &CpuFeatureInOrder->Link);
> @@ -624,18 +620,18 @@ AnalysisProcessorFeatures (
> CpuFeatureInOrder = CPU_FEATURE_ENTRY_FROM_LINK (Entry);
>
> Success = FALSE;
> - if (IsBitMaskMatch (CpuFeatureInOrder->FeatureMask,
> CpuFeaturesData->SettingPcd)) {
> + if (IsBitMaskMatch (CpuFeatureInOrder->FeatureMask,
> + CpuFeaturesData->SettingPcd, CpuFeaturesData->BitMaskSize)) {
> Status = CpuFeatureInOrder->InitializeFunc (ProcessorNumber, CpuInfo,
> CpuFeatureInOrder->ConfigData, TRUE);
> if (EFI_ERROR (Status)) {
> //
> // Clean the CpuFeatureInOrder->FeatureMask in setting PCD.
> //
> - SupportedMaskCleanBit (CpuFeaturesData->SettingPcd,
> CpuFeatureInOrder->FeatureMask);
> + SupportedMaskCleanBit (CpuFeaturesData->SettingPcd,
> + CpuFeatureInOrder->FeatureMask, CpuFeaturesData->BitMaskSize);
> if (CpuFeatureInOrder->FeatureName != NULL) {
> DEBUG ((DEBUG_WARN, "Warning :: Failed to enable Feature: Name
> = %a.\n", CpuFeatureInOrder->FeatureName));
> } else {
> DEBUG ((DEBUG_WARN, "Warning :: Failed to enable Feature: Mask =
> "));
> - DumpCpuFeatureMask (CpuFeatureInOrder->FeatureMask);
> + DumpCpuFeatureMask (CpuFeatureInOrder->FeatureMask,
> + CpuFeaturesData->BitMaskSize);
> }
> } else {
> Success = TRUE;
> @@ -647,7 +643,7 @@ AnalysisProcessorFeatures (
> DEBUG ((DEBUG_WARN, "Warning :: Failed to disable Feature: Name
> = %a.\n", CpuFeatureInOrder->FeatureName));
> } else {
> DEBUG ((DEBUG_WARN, "Warning :: Failed to disable Feature: Mask =
> "));
> - DumpCpuFeatureMask (CpuFeatureInOrder->FeatureMask);
> + DumpCpuFeatureMask (CpuFeatureInOrder->FeatureMask,
> + CpuFeaturesData->BitMaskSize);
> }
> } else {
> Success = TRUE;
> @@ -699,7 +695,7 @@ AnalysisProcessorFeatures (
> // again during initialize the features.
> //
> DEBUG ((DEBUG_INFO, "Dump final value for
> PcdCpuFeaturesSetting:\n"));
> - DumpCpuFeatureMask (CpuFeaturesData->SettingPcd);
> + DumpCpuFeatureMask (CpuFeaturesData->SettingPcd,
> + CpuFeaturesData->BitMaskSize);
>
> //
> // Dump the RegisterTable
> diff --git
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> index 5c546ee153..a18f926641 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> @@ -180,20 +180,26 @@ SwitchNewBsp (
> Function that uses DEBUG() macros to display the contents of a a CPU
> feature bit mask.
>
> @param[in] FeatureMask A pointer to the CPU feature bit mask.
> + @param[in] BitMaskSize CPU feature bits mask buffer size.
> +
> **/
> VOID
> DumpCpuFeatureMask (
> - IN UINT8 *FeatureMask
> + IN UINT8 *FeatureMask,
> + IN UINT32 BitMaskSize
> );
>
> /**
> Dump CPU feature name or CPU feature bit mask.
>
> @param[in] CpuFeature Pointer to CPU_FEATURES_ENTRY
> + @param[in] BitMaskSize CPU feature bits mask buffer size.
> +
> **/
> VOID
> DumpCpuFeature (
> - IN CPU_FEATURES_ENTRY *CpuFeature
> + IN CPU_FEATURES_ENTRY *CpuFeature,
> + IN UINT32 BitMaskSize
> );
>
> /**
> diff --git
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> index 36aabd7267..283e9d6539 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> +++
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> @@ -18,36 +18,34 @@
> @retval FALSE Two CPU feature bit masks are not equal.
> **/
> BOOLEAN
> -IsCpuFeatureMatch (
> +IsBitMaskMatchCheck (
3. Seemingly, original IsCpuFeatureMatch() and IsBitMaskMatchCheck() have different purpose. How they can be merged? Please double confirm that.
And even, they can be merged, it will be better to do that in a separated patch.
> IN UINT8 *FirstFeatureMask,
> IN UINT8 *SecondFeatureMask
> )
> {
> - UINTN BitMaskSize;
> + CPU_FEATURES_DATA *CpuFeaturesData;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> - if (CompareMem (FirstFeatureMask, SecondFeatureMask, BitMaskSize) ==
> 0) {
> - return TRUE;
> - } else {
> - return FALSE;
> - }
> + CpuFeaturesData = GetCpuFeaturesData ();
> +
> + return (CompareMem (FirstFeatureMask, SecondFeatureMask,
> + CpuFeaturesData->BitMaskSize) == 0);
> }
>
> /**
> Function that uses DEBUG() macros to display the contents of a a CPU
> feature bit mask.
>
> @param[in] FeatureMask A pointer to the CPU feature bit mask.
> + @param[in] BitMaskSize CPU feature bits mask buffer size.
> +
> **/
> VOID
> DumpCpuFeatureMask (
> - IN UINT8 *FeatureMask
> + IN UINT8 *FeatureMask,
> + IN UINT32 BitMaskSize
> )
> {
> UINTN Index;
> UINT8 *Data8;
> - UINTN BitMaskSize;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> Data8 = (UINT8 *) FeatureMask;
> for (Index = 0; Index < BitMaskSize; Index++) {
> DEBUG ((DEBUG_INFO, " %02x ", *Data8++)); @@ -59,10 +57,13 @@
> DumpCpuFeatureMask (
> Dump CPU feature name or CPU feature bit mask.
>
> @param[in] CpuFeature Pointer to CPU_FEATURES_ENTRY
> + @param[in] BitMaskSize CPU feature bits mask buffer size.
> +
> **/
> VOID
> DumpCpuFeature (
> - IN CPU_FEATURES_ENTRY *CpuFeature
> + IN CPU_FEATURES_ENTRY *CpuFeature,
> + IN UINT32 BitMaskSize
> )
> {
>
> @@ -70,42 +71,10 @@ DumpCpuFeature (
> DEBUG ((DEBUG_INFO, "FeatureName: %a\n", CpuFeature-
> >FeatureName));
> } else {
> DEBUG ((DEBUG_INFO, "FeatureMask = "));
> - DumpCpuFeatureMask (CpuFeature->FeatureMask);
> + DumpCpuFeatureMask (CpuFeature->FeatureMask, BitMaskSize);
> }
> }
>
> -/**
> - Determines if the feature bit mask is in dependent CPU feature bit mask
> buffer.
> -
> - @param[in] FeatureMask Pointer to CPU feature bit mask
> - @param[in] DependentBitMask Pointer to dependent CPU feature bit
> mask buffer
> -
> - @retval TRUE The feature bit mask is in dependent CPU feature bit mask
> buffer.
> - @retval FALSE The feature bit mask is not in dependent CPU feature bit
> mask buffer.
> -**/
> -BOOLEAN
> -IsBitMaskMatchCheck (
> - IN UINT8 *FeatureMask,
> - IN UINT8 *DependentBitMask
> - )
> -{
> - UINTN Index;
> - UINTN BitMaskSize;
> - UINT8 *Data1;
> - UINT8 *Data2;
> -
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> -
> - Data1 = FeatureMask;
> - Data2 = DependentBitMask;
> - for (Index = 0; Index < BitMaskSize; Index++) {
> - if (((*(Data1++)) & (*(Data2++))) != 0) {
> - return TRUE;
> - }
> - }
> - return FALSE;
> -}
> -
> /**
> Try to find the specify cpu featuren in former/after feature list.
>
> @@ -642,37 +611,21 @@ CheckCpuFeaturesDependency ( **/
> RETURN_STATUS RegisterCpuFeatureWorker (
> + IN CPU_FEATURES_DATA *CpuFeaturesData,
4. New parameter is added, so the function header will need to be updated.
Thanks,
Star
> IN CPU_FEATURES_ENTRY *CpuFeature
> )
> {
> EFI_STATUS Status;
> - CPU_FEATURES_DATA *CpuFeaturesData;
> CPU_FEATURES_ENTRY *CpuFeatureEntry;
> LIST_ENTRY *Entry;
> - UINTN BitMaskSize;
> BOOLEAN FeatureExist;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> - CpuFeaturesData = GetCpuFeaturesData ();
> - if (CpuFeaturesData->FeaturesCount == 0) {
> - InitializeListHead (&CpuFeaturesData->FeatureList);
> - InitializeSpinLock (&CpuFeaturesData->CpuFlags.MemoryMappedLock);
> - InitializeSpinLock (&CpuFeaturesData->CpuFlags.ConsoleLogLock);
> - //
> - // Driver has assumption that these three PCD should has same buffer size.
> - //
> - ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize
> (PcdCpuFeaturesCapability));
> - ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize
> (PcdCpuFeaturesSupport));
> - CpuFeaturesData->BitMaskSize = (UINT32) BitMaskSize;
> - }
> - ASSERT (CpuFeaturesData->BitMaskSize == BitMaskSize);
> -
> FeatureExist = FALSE;
> CpuFeatureEntry = NULL;
> Entry = GetFirstNode (&CpuFeaturesData->FeatureList);
> while (!IsNull (&CpuFeaturesData->FeatureList, Entry)) {
> CpuFeatureEntry = CPU_FEATURE_ENTRY_FROM_LINK (Entry);
> - if (IsCpuFeatureMatch (CpuFeature->FeatureMask, CpuFeatureEntry-
> >FeatureMask)) {
> + if (IsBitMaskMatchCheck (CpuFeature->FeatureMask,
> + CpuFeatureEntry->FeatureMask)) {
> //
> // If this feature already registered
> //
> @@ -684,12 +637,12 @@ RegisterCpuFeatureWorker (
>
> if (!FeatureExist) {
> DEBUG ((DEBUG_INFO, "[NEW] "));
> - DumpCpuFeature (CpuFeature);
> + DumpCpuFeature (CpuFeature, CpuFeaturesData->BitMaskSize);
> InsertTailList (&CpuFeaturesData->FeatureList, &CpuFeature->Link);
> CpuFeaturesData->FeaturesCount++;
> } else {
> DEBUG ((DEBUG_INFO, "[OVERRIDE] "));
> - DumpCpuFeature (CpuFeature);
> + DumpCpuFeature (CpuFeature, CpuFeaturesData->BitMaskSize);
> ASSERT (CpuFeatureEntry != NULL);
> //
> // Overwrite original parameters of CPU feature @@ -849,7 +802,6 @@
> RegisterCpuFeature (
> EFI_STATUS Status;
> VA_LIST Marker;
> UINT32 Feature;
> - UINTN BitMaskSize;
> CPU_FEATURES_ENTRY *CpuFeature;
> UINT8 *FeatureMask;
> UINT8 *BeforeFeatureBitMask;
> @@ -860,6 +812,7 @@ RegisterCpuFeature (
> UINT8 *PackageAfterFeatureBitMask;
> BOOLEAN BeforeAll;
> BOOLEAN AfterAll;
> + CPU_FEATURES_DATA *CpuFeaturesData;
>
> FeatureMask = NULL;
> BeforeFeatureBitMask = NULL;
> @@ -871,7 +824,18 @@ RegisterCpuFeature (
> BeforeAll = FALSE;
> AfterAll = FALSE;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> + CpuFeaturesData = GetCpuFeaturesData (); if
> + (CpuFeaturesData->FeaturesCount == 0) {
> + InitializeListHead (&CpuFeaturesData->FeatureList);
> + InitializeSpinLock (&CpuFeaturesData->CpuFlags.MemoryMappedLock);
> + InitializeSpinLock (&CpuFeaturesData->CpuFlags.ConsoleLogLock);
> + //
> + // Driver has assumption that below three PCDs should has same buffer
> size.
> + //
> + ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize
> (PcdCpuFeaturesCapability));
> + ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize
> (PcdCpuFeaturesSupport));
> + CpuFeaturesData->BitMaskSize = (UINT32) PcdGetSize
> + (PcdCpuFeaturesSetting); }
>
> VA_START (Marker, InitializeFunc);
> Feature = VA_ARG (Marker, UINT32);
> @@ -889,19 +853,19 @@ RegisterCpuFeature (
> AfterAll = ((Feature & CPU_FEATURE_AFTER_ALL) != 0) ? TRUE : FALSE;
> Feature &= ~(CPU_FEATURE_BEFORE_ALL | CPU_FEATURE_AFTER_ALL);
> ASSERT (FeatureMask == NULL);
> - SetCpuFeaturesBitMask (&FeatureMask, Feature, BitMaskSize);
> + SetCpuFeaturesBitMask (&FeatureMask, Feature,
> + CpuFeaturesData->BitMaskSize);
> } else if ((Feature & CPU_FEATURE_BEFORE) != 0) {
> - SetCpuFeaturesBitMask (&BeforeFeatureBitMask, Feature &
> ~CPU_FEATURE_BEFORE, BitMaskSize);
> + SetCpuFeaturesBitMask (&BeforeFeatureBitMask, Feature &
> + ~CPU_FEATURE_BEFORE, CpuFeaturesData->BitMaskSize);
> } else if ((Feature & CPU_FEATURE_AFTER) != 0) {
> - SetCpuFeaturesBitMask (&AfterFeatureBitMask, Feature &
> ~CPU_FEATURE_AFTER, BitMaskSize);
> + SetCpuFeaturesBitMask (&AfterFeatureBitMask, Feature &
> + ~CPU_FEATURE_AFTER, CpuFeaturesData->BitMaskSize);
> } else if ((Feature & CPU_FEATURE_CORE_BEFORE) != 0) {
> - SetCpuFeaturesBitMask (&CoreBeforeFeatureBitMask, Feature &
> ~CPU_FEATURE_CORE_BEFORE, BitMaskSize);
> + SetCpuFeaturesBitMask (&CoreBeforeFeatureBitMask, Feature &
> + ~CPU_FEATURE_CORE_BEFORE, CpuFeaturesData->BitMaskSize);
> } else if ((Feature & CPU_FEATURE_CORE_AFTER) != 0) {
> - SetCpuFeaturesBitMask (&CoreAfterFeatureBitMask, Feature &
> ~CPU_FEATURE_CORE_AFTER, BitMaskSize);
> + SetCpuFeaturesBitMask (&CoreAfterFeatureBitMask, Feature &
> + ~CPU_FEATURE_CORE_AFTER, CpuFeaturesData->BitMaskSize);
> } else if ((Feature & CPU_FEATURE_PACKAGE_BEFORE) != 0) {
> - SetCpuFeaturesBitMask (&PackageBeforeFeatureBitMask, Feature &
> ~CPU_FEATURE_PACKAGE_BEFORE, BitMaskSize);
> + SetCpuFeaturesBitMask (&PackageBeforeFeatureBitMask, Feature &
> + ~CPU_FEATURE_PACKAGE_BEFORE, CpuFeaturesData->BitMaskSize);
> } else if ((Feature & CPU_FEATURE_PACKAGE_AFTER) != 0) {
> - SetCpuFeaturesBitMask (&PackageAfterFeatureBitMask, Feature &
> ~CPU_FEATURE_PACKAGE_AFTER, BitMaskSize);
> + SetCpuFeaturesBitMask (&PackageAfterFeatureBitMask, Feature &
> + ~CPU_FEATURE_PACKAGE_AFTER, CpuFeaturesData->BitMaskSize);
> }
> Feature = VA_ARG (Marker, UINT32);
> }
> @@ -929,7 +893,7 @@ RegisterCpuFeature (
> ASSERT_EFI_ERROR (Status);
> }
>
> - Status = RegisterCpuFeatureWorker (CpuFeature);
> + Status = RegisterCpuFeatureWorker (CpuFeaturesData, CpuFeature);
> ASSERT_EFI_ERROR (Status);
>
> return RETURN_SUCCESS;
> --
> 2.21.0.windows.1
>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Patch 0/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls gBS service.
2019-07-12 1:53 [Patch 0/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls gBS service Dong, Eric
2019-07-12 1:53 ` [Patch 1/2] " Dong, Eric
2019-07-12 1:53 ` [Patch 2/2] UefiCpuPkg/Library/RegisterCpuFeaturesLib: avoid use dynamic PCD Dong, Eric
@ 2019-07-12 22:16 ` Laszlo Ersek
2 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2019-07-12 22:16 UTC (permalink / raw)
To: Eric Dong, devel; +Cc: Ray Ni, Chandana Kumar, Star Zeng
On 07/12/19 03:53, Eric Dong wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1972
>
> AP calls CollectProcessorData() to collect processor info.
> CollectProcessorData function finally calls PcdGetSize function to
> get DynamicPCD PcdCpuFeaturesSetting value. PcdGetSize will use gBS
> which caused ASSERT.
> This patch serial fixes the issue and enhances the related code to avoid
> later report this issue again.
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Chandana Kumar <chandana.c.kumar@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
>
> Eric Dong (2):
> UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls gBS service.
> UefiCpuPkg/Library/RegisterCpuFeaturesLib: avoid use dynamic PCD.
>
> .../CpuFeaturesInitialize.c | 77 ++++++-------
> .../RegisterCpuFeatures.h | 10 +-
> .../RegisterCpuFeaturesLib.c | 109 +++++++-----------
> 3 files changed, 85 insertions(+), 111 deletions(-)
>
Will have to skip this one too.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [Patch 2/2] UefiCpuPkg/Library/RegisterCpuFeaturesLib: avoid use dynamic PCD.
2019-07-12 1:53 ` [Patch 2/2] UefiCpuPkg/Library/RegisterCpuFeaturesLib: avoid use dynamic PCD Dong, Eric
2019-07-12 11:10 ` [edk2-devel] " Zeng, Star
@ 2019-07-15 4:57 ` Ni, Ray
2019-07-15 7:04 ` Dong, Eric
1 sibling, 1 reply; 9+ messages in thread
From: Ni, Ray @ 2019-07-15 4:57 UTC (permalink / raw)
To: devel@edk2.groups.io, Dong, Eric
Cc: Laszlo Ersek, Kumar, Chandana C, Zeng, Star
1. FeatureMaskSize can be BitMaskSize. It clearly tells the caller that this function will assume bit size of SupportedFeatureMask is the same as that of PcdCpuFeaturesCapability.
SetCapabilityPcd (
IN UINT8 *SupportedFeatureMask,
IN UINTN FeatureMaskSize
2. IsBitMaskMatchCheck () returns TRUE when the bits in FeatureMask and DependentBitMask overlap. We cannot change its behavior using CompareMem.
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dong,
> Eric
> Sent: Friday, July 12, 2019 9:53 AM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar,
> Chandana C <chandana.c.kumar@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: [edk2-devel] [Patch 2/2]
> UefiCpuPkg/Library/RegisterCpuFeaturesLib: avoid use dynamic PCD.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1972
>
> Function in this library may be used by APs. Assert will be trig if AP uses
> dynamic pcd.
> This patch enhance the current code, remove the unnecessary usage of
> dynamic PCD. This change try to avoid report this issue again later.
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Chandana Kumar <chandana.c.kumar@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> .../CpuFeaturesInitialize.c | 64 +++++-----
> .../RegisterCpuFeatures.h | 10 +-
> .../RegisterCpuFeaturesLib.c | 114 ++++++------------
> 3 files changed, 77 insertions(+), 111 deletions(-)
>
> diff --git
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> index 87bfc64250..16b99c0c27 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> @@ -21,16 +21,12 @@ CHAR16 *mRegisterTypeStr[] = {L"MSR", L"CR",
> L"MMIO", L"CACHE", L"SEMAP", L"INVA VOID SetCapabilityPcd (
> IN UINT8 *SupportedFeatureMask,
> - IN UINT32 FeatureMaskSize
> + IN UINTN FeatureMaskSize
> )
> {
> EFI_STATUS Status;
> - UINTN BitMaskSize;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesCapability);
> - ASSERT (FeatureMaskSize == BitMaskSize);
> -
> - Status = PcdSetPtrS (PcdCpuFeaturesCapability, &BitMaskSize,
> SupportedFeatureMask);
> + Status = PcdSetPtrS (PcdCpuFeaturesCapability, &FeatureMaskSize,
> + SupportedFeatureMask);
> ASSERT_EFI_ERROR (Status);
> }
>
> @@ -38,16 +34,16 @@ SetCapabilityPcd (
> Worker function to save PcdCpuFeaturesSetting.
>
> @param[in] SupportedFeatureMask The pointer to CPU feature bits mask
> buffer
> + @param[in] FeatureMaskSize CPU feature bits mask buffer size.
> **/
> VOID
> SetSettingPcd (
> - IN UINT8 *SupportedFeatureMask
> + IN UINT8 *SupportedFeatureMask,
> + IN UINTN BitMaskSize
> )
> {
> EFI_STATUS Status;
> - UINTN BitMaskSize;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> Status = PcdSetPtrS (PcdCpuFeaturesSetting, &BitMaskSize,
> SupportedFeatureMask);
> ASSERT_EFI_ERROR (Status);
> }
> @@ -272,19 +268,20 @@ SupportedMaskOr (
>
> @param[in] SupportedFeatureMask The pointer to CPU feature bits mask
> buffer
> @param[in] AndFeatureBitMask The feature bit mask to do AND
> operation
> + @param[in] BitMaskSize CPU feature bits mask buffer size.
> +
> **/
> VOID
> SupportedMaskAnd (
> IN UINT8 *SupportedFeatureMask,
> - IN CONST UINT8 *AndFeatureBitMask
> + IN CONST UINT8 *AndFeatureBitMask,
> + IN UINT32 BitMaskSize
> )
> {
> UINTN Index;
> - UINTN BitMaskSize;
> UINT8 *Data1;
> CONST UINT8 *Data2;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> Data1 = SupportedFeatureMask;
> Data2 = AndFeatureBitMask;
> for (Index = 0; Index < BitMaskSize; Index++) { @@ -297,19 +294,19 @@
> SupportedMaskAnd (
>
> @param[in] SupportedFeatureMask The pointer to CPU feature bits mask
> buffer
> @param[in] AndFeatureBitMask The feature bit mask to do XOR
> operation
> + @param[in] BitMaskSize CPU feature bits mask buffer size.
> **/
> VOID
> SupportedMaskCleanBit (
> IN UINT8 *SupportedFeatureMask,
> - IN UINT8 *AndFeatureBitMask
> + IN UINT8 *AndFeatureBitMask,
> + IN UINT32 BitMaskSize
> )
> {
> UINTN Index;
> - UINTN BitMaskSize;
> UINT8 *Data1;
> UINT8 *Data2;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> Data1 = SupportedFeatureMask;
> Data2 = AndFeatureBitMask;
> for (Index = 0; Index < BitMaskSize; Index++) { @@ -323,6 +320,7 @@
> SupportedMaskCleanBit (
>
> @param[in] SupportedFeatureMask The pointer to CPU feature bits mask
> buffer
> @param[in] ComparedFeatureBitMask The feature bit mask to be
> compared
> + @param[in] BitMaskSize CPU feature bits mask buffer size.
>
> @retval TRUE The ComparedFeatureBitMask is set in CPU feature
> supported bits
> mask buffer.
> @@ -332,16 +330,14 @@ SupportedMaskCleanBit ( BOOLEAN
> IsBitMaskMatch (
> IN UINT8 *SupportedFeatureMask,
> - IN UINT8 *ComparedFeatureBitMask
> + IN UINT8 *ComparedFeatureBitMask,
> + IN UINT32 BitMaskSize
> )
> {
> UINTN Index;
> - UINTN BitMaskSize;
> UINT8 *Data1;
> UINT8 *Data2;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> -
> Data1 = SupportedFeatureMask;
> Data2 = ComparedFeatureBitMask;
> for (Index = 0; Index < BitMaskSize; Index++) { @@ -557,14 +553,14 @@
> AnalysisProcessorFeatures (
> //
> // Calculate the last capability on all processors
> //
> - SupportedMaskAnd (CpuFeaturesData->CapabilityPcd, CpuInitOrder-
> >FeaturesSupportedMask);
> + SupportedMaskAnd (CpuFeaturesData->CapabilityPcd,
> + CpuInitOrder->FeaturesSupportedMask, CpuFeaturesData->BitMaskSize);
> }
> //
> // Calculate the last setting
> //
> CpuFeaturesData->SettingPcd = AllocateCopyPool (CpuFeaturesData-
> >BitMaskSize, CpuFeaturesData->CapabilityPcd);
> ASSERT (CpuFeaturesData->SettingPcd != NULL);
> - SupportedMaskAnd (CpuFeaturesData->SettingPcd, PcdGetPtr
> (PcdCpuFeaturesSetting));
> + SupportedMaskAnd (CpuFeaturesData->SettingPcd, PcdGetPtr
> + (PcdCpuFeaturesSetting), CpuFeaturesData->BitMaskSize);
>
> //
> // Dump the last CPU feature list
> @@ -574,8 +570,8 @@ AnalysisProcessorFeatures (
> Entry = GetFirstNode (&CpuFeaturesData->FeatureList);
> while (!IsNull (&CpuFeaturesData->FeatureList, Entry)) {
> CpuFeature = CPU_FEATURE_ENTRY_FROM_LINK (Entry);
> - if (IsBitMaskMatch (CpuFeature->FeatureMask, CpuFeaturesData-
> >CapabilityPcd)) {
> - if (IsBitMaskMatch (CpuFeature->FeatureMask, CpuFeaturesData-
> >SettingPcd)) {
> + if (IsBitMaskMatch (CpuFeature->FeatureMask, CpuFeaturesData-
> >CapabilityPcd, CpuFeaturesData->BitMaskSize)) {
> + if (IsBitMaskMatch (CpuFeature->FeatureMask,
> + CpuFeaturesData->SettingPcd, CpuFeaturesData->BitMaskSize)) {
> DEBUG ((DEBUG_INFO, "[Enable ] "));
> } else {
> DEBUG ((DEBUG_INFO, "[Disable ] ")); @@ -583,22 +579,22 @@
> AnalysisProcessorFeatures (
> } else {
> DEBUG ((DEBUG_INFO, "[Unsupport] "));
> }
> - DumpCpuFeature (CpuFeature);
> + DumpCpuFeature (CpuFeature, CpuFeaturesData->BitMaskSize);
> Entry = Entry->ForwardLink;
> }
> DEBUG ((DEBUG_INFO, "PcdCpuFeaturesCapability:\n"));
> - DumpCpuFeatureMask (CpuFeaturesData->CapabilityPcd);
> + DumpCpuFeatureMask (CpuFeaturesData->CapabilityPcd,
> + CpuFeaturesData->BitMaskSize);
> DEBUG ((DEBUG_INFO, "Origin PcdCpuFeaturesSetting:\n"));
> - DumpCpuFeatureMask (PcdGetPtr (PcdCpuFeaturesSetting));
> + DumpCpuFeatureMask (PcdGetPtr (PcdCpuFeaturesSetting),
> + CpuFeaturesData->BitMaskSize);
> DEBUG ((DEBUG_INFO, "Final PcdCpuFeaturesSetting:\n"));
> - DumpCpuFeatureMask (CpuFeaturesData->SettingPcd);
> + DumpCpuFeatureMask (CpuFeaturesData->SettingPcd,
> + CpuFeaturesData->BitMaskSize);
> );
>
> //
> // Save PCDs and display CPU PCDs
> //
> SetCapabilityPcd (CpuFeaturesData->CapabilityPcd, CpuFeaturesData-
> >BitMaskSize);
> - SetSettingPcd (CpuFeaturesData->SettingPcd);
> + SetSettingPcd (CpuFeaturesData->SettingPcd,
> + CpuFeaturesData->BitMaskSize);
>
> for (ProcessorNumber = 0; ProcessorNumber < NumberOfCpus;
> ProcessorNumber++) {
> CpuInitOrder = &CpuFeaturesData->InitOrder[ProcessorNumber];
> @@ -608,7 +604,7 @@ AnalysisProcessorFeatures (
> // Insert each feature into processor's order list
> //
> CpuFeature = CPU_FEATURE_ENTRY_FROM_LINK (Entry);
> - if (IsBitMaskMatch (CpuFeature->FeatureMask, CpuFeaturesData-
> >CapabilityPcd)) {
> + if (IsBitMaskMatch (CpuFeature->FeatureMask,
> + CpuFeaturesData->CapabilityPcd, CpuFeaturesData->BitMaskSize)) {
> CpuFeatureInOrder = AllocateCopyPool (sizeof (CPU_FEATURES_ENTRY),
> CpuFeature);
> ASSERT (CpuFeatureInOrder != NULL);
> InsertTailList (&CpuInitOrder->OrderList, &CpuFeatureInOrder->Link);
> @@ -624,18 +620,18 @@ AnalysisProcessorFeatures (
> CpuFeatureInOrder = CPU_FEATURE_ENTRY_FROM_LINK (Entry);
>
> Success = FALSE;
> - if (IsBitMaskMatch (CpuFeatureInOrder->FeatureMask,
> CpuFeaturesData->SettingPcd)) {
> + if (IsBitMaskMatch (CpuFeatureInOrder->FeatureMask,
> + CpuFeaturesData->SettingPcd, CpuFeaturesData->BitMaskSize)) {
> Status = CpuFeatureInOrder->InitializeFunc (ProcessorNumber, CpuInfo,
> CpuFeatureInOrder->ConfigData, TRUE);
> if (EFI_ERROR (Status)) {
> //
> // Clean the CpuFeatureInOrder->FeatureMask in setting PCD.
> //
> - SupportedMaskCleanBit (CpuFeaturesData->SettingPcd,
> CpuFeatureInOrder->FeatureMask);
> + SupportedMaskCleanBit (CpuFeaturesData->SettingPcd,
> + CpuFeatureInOrder->FeatureMask, CpuFeaturesData->BitMaskSize);
> if (CpuFeatureInOrder->FeatureName != NULL) {
> DEBUG ((DEBUG_WARN, "Warning :: Failed to enable Feature: Name
> = %a.\n", CpuFeatureInOrder->FeatureName));
> } else {
> DEBUG ((DEBUG_WARN, "Warning :: Failed to enable Feature: Mask =
> "));
> - DumpCpuFeatureMask (CpuFeatureInOrder->FeatureMask);
> + DumpCpuFeatureMask (CpuFeatureInOrder->FeatureMask,
> + CpuFeaturesData->BitMaskSize);
> }
> } else {
> Success = TRUE;
> @@ -647,7 +643,7 @@ AnalysisProcessorFeatures (
> DEBUG ((DEBUG_WARN, "Warning :: Failed to disable Feature: Name
> = %a.\n", CpuFeatureInOrder->FeatureName));
> } else {
> DEBUG ((DEBUG_WARN, "Warning :: Failed to disable Feature: Mask =
> "));
> - DumpCpuFeatureMask (CpuFeatureInOrder->FeatureMask);
> + DumpCpuFeatureMask (CpuFeatureInOrder->FeatureMask,
> + CpuFeaturesData->BitMaskSize);
> }
> } else {
> Success = TRUE;
> @@ -699,7 +695,7 @@ AnalysisProcessorFeatures (
> // again during initialize the features.
> //
> DEBUG ((DEBUG_INFO, "Dump final value for
> PcdCpuFeaturesSetting:\n"));
> - DumpCpuFeatureMask (CpuFeaturesData->SettingPcd);
> + DumpCpuFeatureMask (CpuFeaturesData->SettingPcd,
> + CpuFeaturesData->BitMaskSize);
>
> //
> // Dump the RegisterTable
> diff --git
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> index 5c546ee153..a18f926641 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> @@ -180,20 +180,26 @@ SwitchNewBsp (
> Function that uses DEBUG() macros to display the contents of a a CPU
> feature bit mask.
>
> @param[in] FeatureMask A pointer to the CPU feature bit mask.
> + @param[in] BitMaskSize CPU feature bits mask buffer size.
> +
> **/
> VOID
> DumpCpuFeatureMask (
> - IN UINT8 *FeatureMask
> + IN UINT8 *FeatureMask,
> + IN UINT32 BitMaskSize
> );
>
> /**
> Dump CPU feature name or CPU feature bit mask.
>
> @param[in] CpuFeature Pointer to CPU_FEATURES_ENTRY
> + @param[in] BitMaskSize CPU feature bits mask buffer size.
> +
> **/
> VOID
> DumpCpuFeature (
> - IN CPU_FEATURES_ENTRY *CpuFeature
> + IN CPU_FEATURES_ENTRY *CpuFeature,
> + IN UINT32 BitMaskSize
> );
>
> /**
> diff --git
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> index 36aabd7267..283e9d6539 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> +++
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> @@ -18,36 +18,34 @@
> @retval FALSE Two CPU feature bit masks are not equal.
> **/
> BOOLEAN
> -IsCpuFeatureMatch (
> +IsBitMaskMatchCheck (
> IN UINT8 *FirstFeatureMask,
> IN UINT8 *SecondFeatureMask
> )
> {
> - UINTN BitMaskSize;
> + CPU_FEATURES_DATA *CpuFeaturesData;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> - if (CompareMem (FirstFeatureMask, SecondFeatureMask, BitMaskSize) ==
> 0) {
> - return TRUE;
> - } else {
> - return FALSE;
> - }
> + CpuFeaturesData = GetCpuFeaturesData ();
> +
> + return (CompareMem (FirstFeatureMask, SecondFeatureMask,
> + CpuFeaturesData->BitMaskSize) == 0);
> }
>
> /**
> Function that uses DEBUG() macros to display the contents of a a CPU
> feature bit mask.
>
> @param[in] FeatureMask A pointer to the CPU feature bit mask.
> + @param[in] BitMaskSize CPU feature bits mask buffer size.
> +
> **/
> VOID
> DumpCpuFeatureMask (
> - IN UINT8 *FeatureMask
> + IN UINT8 *FeatureMask,
> + IN UINT32 BitMaskSize
> )
> {
> UINTN Index;
> UINT8 *Data8;
> - UINTN BitMaskSize;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> Data8 = (UINT8 *) FeatureMask;
> for (Index = 0; Index < BitMaskSize; Index++) {
> DEBUG ((DEBUG_INFO, " %02x ", *Data8++)); @@ -59,10 +57,13 @@
> DumpCpuFeatureMask (
> Dump CPU feature name or CPU feature bit mask.
>
> @param[in] CpuFeature Pointer to CPU_FEATURES_ENTRY
> + @param[in] BitMaskSize CPU feature bits mask buffer size.
> +
> **/
> VOID
> DumpCpuFeature (
> - IN CPU_FEATURES_ENTRY *CpuFeature
> + IN CPU_FEATURES_ENTRY *CpuFeature,
> + IN UINT32 BitMaskSize
> )
> {
>
> @@ -70,42 +71,10 @@ DumpCpuFeature (
> DEBUG ((DEBUG_INFO, "FeatureName: %a\n", CpuFeature-
> >FeatureName));
> } else {
> DEBUG ((DEBUG_INFO, "FeatureMask = "));
> - DumpCpuFeatureMask (CpuFeature->FeatureMask);
> + DumpCpuFeatureMask (CpuFeature->FeatureMask, BitMaskSize);
> }
> }
>
> -/**
> - Determines if the feature bit mask is in dependent CPU feature bit mask
> buffer.
> -
> - @param[in] FeatureMask Pointer to CPU feature bit mask
> - @param[in] DependentBitMask Pointer to dependent CPU feature bit
> mask buffer
> -
> - @retval TRUE The feature bit mask is in dependent CPU feature bit mask
> buffer.
> - @retval FALSE The feature bit mask is not in dependent CPU feature bit
> mask buffer.
> -**/
> -BOOLEAN
> -IsBitMaskMatchCheck (
> - IN UINT8 *FeatureMask,
> - IN UINT8 *DependentBitMask
> - )
> -{
> - UINTN Index;
> - UINTN BitMaskSize;
> - UINT8 *Data1;
> - UINT8 *Data2;
> -
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> -
> - Data1 = FeatureMask;
> - Data2 = DependentBitMask;
> - for (Index = 0; Index < BitMaskSize; Index++) {
> - if (((*(Data1++)) & (*(Data2++))) != 0) {
> - return TRUE;
> - }
> - }
> - return FALSE;
> -}
> -
> /**
> Try to find the specify cpu featuren in former/after feature list.
>
> @@ -642,37 +611,21 @@ CheckCpuFeaturesDependency ( **/
> RETURN_STATUS RegisterCpuFeatureWorker (
> + IN CPU_FEATURES_DATA *CpuFeaturesData,
> IN CPU_FEATURES_ENTRY *CpuFeature
> )
> {
> EFI_STATUS Status;
> - CPU_FEATURES_DATA *CpuFeaturesData;
> CPU_FEATURES_ENTRY *CpuFeatureEntry;
> LIST_ENTRY *Entry;
> - UINTN BitMaskSize;
> BOOLEAN FeatureExist;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> - CpuFeaturesData = GetCpuFeaturesData ();
> - if (CpuFeaturesData->FeaturesCount == 0) {
> - InitializeListHead (&CpuFeaturesData->FeatureList);
> - InitializeSpinLock (&CpuFeaturesData->CpuFlags.MemoryMappedLock);
> - InitializeSpinLock (&CpuFeaturesData->CpuFlags.ConsoleLogLock);
> - //
> - // Driver has assumption that these three PCD should has same buffer size.
> - //
> - ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize
> (PcdCpuFeaturesCapability));
> - ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize
> (PcdCpuFeaturesSupport));
> - CpuFeaturesData->BitMaskSize = (UINT32) BitMaskSize;
> - }
> - ASSERT (CpuFeaturesData->BitMaskSize == BitMaskSize);
> -
> FeatureExist = FALSE;
> CpuFeatureEntry = NULL;
> Entry = GetFirstNode (&CpuFeaturesData->FeatureList);
> while (!IsNull (&CpuFeaturesData->FeatureList, Entry)) {
> CpuFeatureEntry = CPU_FEATURE_ENTRY_FROM_LINK (Entry);
> - if (IsCpuFeatureMatch (CpuFeature->FeatureMask, CpuFeatureEntry-
> >FeatureMask)) {
> + if (IsBitMaskMatchCheck (CpuFeature->FeatureMask,
> + CpuFeatureEntry->FeatureMask)) {
> //
> // If this feature already registered
> //
> @@ -684,12 +637,12 @@ RegisterCpuFeatureWorker (
>
> if (!FeatureExist) {
> DEBUG ((DEBUG_INFO, "[NEW] "));
> - DumpCpuFeature (CpuFeature);
> + DumpCpuFeature (CpuFeature, CpuFeaturesData->BitMaskSize);
> InsertTailList (&CpuFeaturesData->FeatureList, &CpuFeature->Link);
> CpuFeaturesData->FeaturesCount++;
> } else {
> DEBUG ((DEBUG_INFO, "[OVERRIDE] "));
> - DumpCpuFeature (CpuFeature);
> + DumpCpuFeature (CpuFeature, CpuFeaturesData->BitMaskSize);
> ASSERT (CpuFeatureEntry != NULL);
> //
> // Overwrite original parameters of CPU feature @@ -849,7 +802,6 @@
> RegisterCpuFeature (
> EFI_STATUS Status;
> VA_LIST Marker;
> UINT32 Feature;
> - UINTN BitMaskSize;
> CPU_FEATURES_ENTRY *CpuFeature;
> UINT8 *FeatureMask;
> UINT8 *BeforeFeatureBitMask;
> @@ -860,6 +812,7 @@ RegisterCpuFeature (
> UINT8 *PackageAfterFeatureBitMask;
> BOOLEAN BeforeAll;
> BOOLEAN AfterAll;
> + CPU_FEATURES_DATA *CpuFeaturesData;
>
> FeatureMask = NULL;
> BeforeFeatureBitMask = NULL;
> @@ -871,7 +824,18 @@ RegisterCpuFeature (
> BeforeAll = FALSE;
> AfterAll = FALSE;
>
> - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> + CpuFeaturesData = GetCpuFeaturesData (); if
> + (CpuFeaturesData->FeaturesCount == 0) {
> + InitializeListHead (&CpuFeaturesData->FeatureList);
> + InitializeSpinLock (&CpuFeaturesData->CpuFlags.MemoryMappedLock);
> + InitializeSpinLock (&CpuFeaturesData->CpuFlags.ConsoleLogLock);
> + //
> + // Driver has assumption that below three PCDs should has same buffer
> size.
> + //
> + ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize
> (PcdCpuFeaturesCapability));
> + ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize
> (PcdCpuFeaturesSupport));
> + CpuFeaturesData->BitMaskSize = (UINT32) PcdGetSize
> + (PcdCpuFeaturesSetting); }
>
> VA_START (Marker, InitializeFunc);
> Feature = VA_ARG (Marker, UINT32);
> @@ -889,19 +853,19 @@ RegisterCpuFeature (
> AfterAll = ((Feature & CPU_FEATURE_AFTER_ALL) != 0) ? TRUE : FALSE;
> Feature &= ~(CPU_FEATURE_BEFORE_ALL | CPU_FEATURE_AFTER_ALL);
> ASSERT (FeatureMask == NULL);
> - SetCpuFeaturesBitMask (&FeatureMask, Feature, BitMaskSize);
> + SetCpuFeaturesBitMask (&FeatureMask, Feature,
> + CpuFeaturesData->BitMaskSize);
> } else if ((Feature & CPU_FEATURE_BEFORE) != 0) {
> - SetCpuFeaturesBitMask (&BeforeFeatureBitMask, Feature &
> ~CPU_FEATURE_BEFORE, BitMaskSize);
> + SetCpuFeaturesBitMask (&BeforeFeatureBitMask, Feature &
> + ~CPU_FEATURE_BEFORE, CpuFeaturesData->BitMaskSize);
> } else if ((Feature & CPU_FEATURE_AFTER) != 0) {
> - SetCpuFeaturesBitMask (&AfterFeatureBitMask, Feature &
> ~CPU_FEATURE_AFTER, BitMaskSize);
> + SetCpuFeaturesBitMask (&AfterFeatureBitMask, Feature &
> + ~CPU_FEATURE_AFTER, CpuFeaturesData->BitMaskSize);
> } else if ((Feature & CPU_FEATURE_CORE_BEFORE) != 0) {
> - SetCpuFeaturesBitMask (&CoreBeforeFeatureBitMask, Feature &
> ~CPU_FEATURE_CORE_BEFORE, BitMaskSize);
> + SetCpuFeaturesBitMask (&CoreBeforeFeatureBitMask, Feature &
> + ~CPU_FEATURE_CORE_BEFORE, CpuFeaturesData->BitMaskSize);
> } else if ((Feature & CPU_FEATURE_CORE_AFTER) != 0) {
> - SetCpuFeaturesBitMask (&CoreAfterFeatureBitMask, Feature &
> ~CPU_FEATURE_CORE_AFTER, BitMaskSize);
> + SetCpuFeaturesBitMask (&CoreAfterFeatureBitMask, Feature &
> + ~CPU_FEATURE_CORE_AFTER, CpuFeaturesData->BitMaskSize);
> } else if ((Feature & CPU_FEATURE_PACKAGE_BEFORE) != 0) {
> - SetCpuFeaturesBitMask (&PackageBeforeFeatureBitMask, Feature &
> ~CPU_FEATURE_PACKAGE_BEFORE, BitMaskSize);
> + SetCpuFeaturesBitMask (&PackageBeforeFeatureBitMask, Feature &
> + ~CPU_FEATURE_PACKAGE_BEFORE, CpuFeaturesData->BitMaskSize);
> } else if ((Feature & CPU_FEATURE_PACKAGE_AFTER) != 0) {
> - SetCpuFeaturesBitMask (&PackageAfterFeatureBitMask, Feature &
> ~CPU_FEATURE_PACKAGE_AFTER, BitMaskSize);
> + SetCpuFeaturesBitMask (&PackageAfterFeatureBitMask, Feature &
> + ~CPU_FEATURE_PACKAGE_AFTER, CpuFeaturesData->BitMaskSize);
> }
> Feature = VA_ARG (Marker, UINT32);
> }
> @@ -929,7 +893,7 @@ RegisterCpuFeature (
> ASSERT_EFI_ERROR (Status);
> }
>
> - Status = RegisterCpuFeatureWorker (CpuFeature);
> + Status = RegisterCpuFeatureWorker (CpuFeaturesData, CpuFeature);
> ASSERT_EFI_ERROR (Status);
>
> return RETURN_SUCCESS;
> --
> 2.21.0.windows.1
>
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Patch 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls gBS service.
2019-07-12 10:53 ` Zeng, Star
@ 2019-07-15 4:59 ` Ni, Ray
0 siblings, 0 replies; 9+ messages in thread
From: Ni, Ray @ 2019-07-15 4:59 UTC (permalink / raw)
To: Zeng, Star, Dong, Eric, devel@edk2.groups.io
Cc: Laszlo Ersek, Kumar, Chandana C
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Zeng, Star
> Sent: Friday, July 12, 2019 6:53 PM
> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Kumar,
> Chandana C <chandana.c.kumar@intel.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: RE: [Patch 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls
> gBS service.
>
> Some minor comments inline.
>
> > -----Original Message-----
> > From: Dong, Eric
> > Sent: Friday, July 12, 2019 9:53 AM
> > To: devel@edk2.groups.io
> > Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> > Kumar, Chandana C <chandana.c.kumar@intel.com>; Zeng, Star
> > <star.zeng@intel.com>
> > Subject: [Patch 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls
> > gBS service.
>
> "gBS service" is not match with the assertion information, gBS is the concept
> in DXE phase.
> So here, please "PEI service" to be accurate.
>
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1972
> >
> > AP calls CollectProcessorData() to collect processor info.
> > CollectProcessorData function finally calls PcdGetSize function to get
> > DynamicPCD PcdCpuFeaturesSetting value. PcdGetSize will use gBS
>
> Same comments with above.
>
> > which caused below assert info:
> > Processor Info: Package: 1, MaxCore : 4, MaxThread: 1
> > Package: 0, Valid Core : 4
> > ASSERT [CpuFeaturesPei] c:\projects\jsl\jsl_v1193\Edk2\MdePkg\Library
> > \PeiServicesTablePointerLibIdt\PeiServicesTablePointer.c(48):
> > PeiServices != ((void *) 0)
> >
> > This change uses saved global pcd size instead of calls PcdGetSize to
> > fix this issue.
> >
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Chandana Kumar <chandana.c.kumar@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> > .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 13
> > ++++++++----- .../RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c |
> > 5 +++++
> > 2 files changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > index aff7ad600c..87bfc64250 100644
> > ---
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.
> > +++ c
> > @@ -246,19 +246,20 @@ CpuInitDataInitialize (
> >
> > @param[in] SupportedFeatureMask The pointer to CPU feature bits
> > mask buffer
> > @param[in] OrFeatureBitMask The feature bit mask to do OR
> operation
> > + @param[in] BitMaskSize The CPU feature bits mask buffer size.
> > +
> > **/
> > VOID
> > SupportedMaskOr (
> > IN UINT8 *SupportedFeatureMask,
> > - IN UINT8 *OrFeatureBitMask
> > + IN UINT8 *OrFeatureBitMask,
> > + IN UINT32 BitMaskSize
> > )
> > {
> > UINTN Index;
> > - UINTN BitMaskSize;
> > UINT8 *Data1;
> > UINT8 *Data2;
> >
> > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> > Data1 = SupportedFeatureMask;
> > Data2 = OrFeatureBitMask;
> > for (Index = 0; Index < BitMaskSize; Index++) { @@ -384,12 +385,14
> > @@ CollectProcessorData (
> > //
> > SupportedMaskOr (
> > CpuFeaturesData-
> > >InitOrder[ProcessorNumber].FeaturesSupportedMask,
> > - CpuFeature->FeatureMask
> > + CpuFeature->FeatureMask,
> > + CpuFeaturesData->BitMaskSize
> > );
> > } else if (CpuFeature->SupportFunc (ProcessorNumber, CpuInfo,
> > CpuFeature->ConfigData)) {
> > SupportedMaskOr (
> > CpuFeaturesData-
> > >InitOrder[ProcessorNumber].FeaturesSupportedMask,
> > - CpuFeature->FeatureMask
> > + CpuFeature->FeatureMask,
> > + CpuFeaturesData->BitMaskSize
> > );
> > }
> > Entry = Entry->ForwardLink;
> > diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > index fa0f0b41e2..36aabd7267 100644
> > ---
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > +++
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > @@ -658,6 +658,11 @@ RegisterCpuFeatureWorker (
> > InitializeListHead (&CpuFeaturesData->FeatureList);
> > InitializeSpinLock (&CpuFeaturesData->CpuFlags.MemoryMappedLock);
> > InitializeSpinLock (&CpuFeaturesData->CpuFlags.ConsoleLogLock);
> > + //
> > + // Driver has assumption that these three PCD should has same
> > + buffer
> > size.
>
> It is library, not driver. So how about "The code has assumption that these
> three PCDs should have same buffer size."?
> The proposed sentence also uses 'PCDs' and 'have'.
>
>
> With the comments handled, Reviewed-by: Star Zeng <star.zeng@intel.com>
>
> Thanks,
> Star
>
> > + //
> > + ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize
> > (PcdCpuFeaturesCapability));
> > + ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize
> > (PcdCpuFeaturesSupport));
> > CpuFeaturesData->BitMaskSize = (UINT32) BitMaskSize;
> > }
> > ASSERT (CpuFeaturesData->BitMaskSize == BitMaskSize);
> > --
> > 2.21.0.windows.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [Patch 2/2] UefiCpuPkg/Library/RegisterCpuFeaturesLib: avoid use dynamic PCD.
2019-07-15 4:57 ` Ni, Ray
@ 2019-07-15 7:04 ` Dong, Eric
0 siblings, 0 replies; 9+ messages in thread
From: Dong, Eric @ 2019-07-15 7:04 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io, Zeng, Star; +Cc: Laszlo Ersek, Kumar, Chandana C
Hi Star & Ray,
Thanks for your comments, updated the related code. Please check my v2 changes.
Thanks,
Eric
> -----Original Message-----
> From: Ni, Ray
> Sent: Monday, July 15, 2019 12:58 PM
> To: devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>; Kumar, Chandana C
> <chandana.c.kumar@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2-devel] [Patch 2/2]
> UefiCpuPkg/Library/RegisterCpuFeaturesLib: avoid use dynamic PCD.
>
> 1. FeatureMaskSize can be BitMaskSize. It clearly tells the caller that this
> function will assume bit size of SupportedFeatureMask is the same as that of
> PcdCpuFeaturesCapability.
>
> SetCapabilityPcd (
> IN UINT8 *SupportedFeatureMask,
> IN UINTN FeatureMaskSize
>
> 2. IsBitMaskMatchCheck () returns TRUE when the bits in FeatureMask and
> DependentBitMask overlap. We cannot change its behavior using
> CompareMem.
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dong,
> > Eric
> > Sent: Friday, July 12, 2019 9:53 AM
> > To: devel@edk2.groups.io
> > Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> > Kumar, Chandana C <chandana.c.kumar@intel.com>; Zeng, Star
> > <star.zeng@intel.com>
> > Subject: [edk2-devel] [Patch 2/2]
> > UefiCpuPkg/Library/RegisterCpuFeaturesLib: avoid use dynamic PCD.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1972
> >
> > Function in this library may be used by APs. Assert will be trig if AP
> > uses dynamic pcd.
> > This patch enhance the current code, remove the unnecessary usage of
> > dynamic PCD. This change try to avoid report this issue again later.
> >
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Chandana Kumar <chandana.c.kumar@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> > .../CpuFeaturesInitialize.c | 64 +++++-----
> > .../RegisterCpuFeatures.h | 10 +-
> > .../RegisterCpuFeaturesLib.c | 114 ++++++------------
> > 3 files changed, 77 insertions(+), 111 deletions(-)
> >
> > diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > index 87bfc64250..16b99c0c27 100644
> > ---
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.
> > +++ c
> > @@ -21,16 +21,12 @@ CHAR16 *mRegisterTypeStr[] = {L"MSR", L"CR",
> > L"MMIO", L"CACHE", L"SEMAP", L"INVA VOID SetCapabilityPcd (
> > IN UINT8 *SupportedFeatureMask,
> > - IN UINT32 FeatureMaskSize
> > + IN UINTN FeatureMaskSize
> > )
> > {
> > EFI_STATUS Status;
> > - UINTN BitMaskSize;
> >
> > - BitMaskSize = PcdGetSize (PcdCpuFeaturesCapability);
> > - ASSERT (FeatureMaskSize == BitMaskSize);
> > -
> > - Status = PcdSetPtrS (PcdCpuFeaturesCapability, &BitMaskSize,
> > SupportedFeatureMask);
> > + Status = PcdSetPtrS (PcdCpuFeaturesCapability, &FeatureMaskSize,
> > + SupportedFeatureMask);
> > ASSERT_EFI_ERROR (Status);
> > }
> >
> > @@ -38,16 +34,16 @@ SetCapabilityPcd (
> > Worker function to save PcdCpuFeaturesSetting.
> >
> > @param[in] SupportedFeatureMask The pointer to CPU feature bits
> > mask buffer
> > + @param[in] FeatureMaskSize CPU feature bits mask buffer size.
> > **/
> > VOID
> > SetSettingPcd (
> > - IN UINT8 *SupportedFeatureMask
> > + IN UINT8 *SupportedFeatureMask,
> > + IN UINTN BitMaskSize
> > )
> > {
> > EFI_STATUS Status;
> > - UINTN BitMaskSize;
> >
> > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> > Status = PcdSetPtrS (PcdCpuFeaturesSetting, &BitMaskSize,
> > SupportedFeatureMask);
> > ASSERT_EFI_ERROR (Status);
> > }
> > @@ -272,19 +268,20 @@ SupportedMaskOr (
> >
> > @param[in] SupportedFeatureMask The pointer to CPU feature bits
> > mask buffer
> > @param[in] AndFeatureBitMask The feature bit mask to do AND
> > operation
> > + @param[in] BitMaskSize CPU feature bits mask buffer size.
> > +
> > **/
> > VOID
> > SupportedMaskAnd (
> > IN UINT8 *SupportedFeatureMask,
> > - IN CONST UINT8 *AndFeatureBitMask
> > + IN CONST UINT8 *AndFeatureBitMask,
> > + IN UINT32 BitMaskSize
> > )
> > {
> > UINTN Index;
> > - UINTN BitMaskSize;
> > UINT8 *Data1;
> > CONST UINT8 *Data2;
> >
> > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> > Data1 = SupportedFeatureMask;
> > Data2 = AndFeatureBitMask;
> > for (Index = 0; Index < BitMaskSize; Index++) { @@ -297,19 +294,19
> > @@ SupportedMaskAnd (
> >
> > @param[in] SupportedFeatureMask The pointer to CPU feature bits
> > mask buffer
> > @param[in] AndFeatureBitMask The feature bit mask to do XOR
> > operation
> > + @param[in] BitMaskSize CPU feature bits mask buffer size.
> > **/
> > VOID
> > SupportedMaskCleanBit (
> > IN UINT8 *SupportedFeatureMask,
> > - IN UINT8 *AndFeatureBitMask
> > + IN UINT8 *AndFeatureBitMask,
> > + IN UINT32 BitMaskSize
> > )
> > {
> > UINTN Index;
> > - UINTN BitMaskSize;
> > UINT8 *Data1;
> > UINT8 *Data2;
> >
> > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> > Data1 = SupportedFeatureMask;
> > Data2 = AndFeatureBitMask;
> > for (Index = 0; Index < BitMaskSize; Index++) { @@ -323,6 +320,7 @@
> > SupportedMaskCleanBit (
> >
> > @param[in] SupportedFeatureMask The pointer to CPU feature bits
> mask
> > buffer
> > @param[in] ComparedFeatureBitMask The feature bit mask to be
> > compared
> > + @param[in] BitMaskSize CPU feature bits mask buffer size.
> >
> > @retval TRUE The ComparedFeatureBitMask is set in CPU feature
> > supported bits
> > mask buffer.
> > @@ -332,16 +330,14 @@ SupportedMaskCleanBit ( BOOLEAN
> IsBitMaskMatch
> > (
> > IN UINT8 *SupportedFeatureMask,
> > - IN UINT8 *ComparedFeatureBitMask
> > + IN UINT8 *ComparedFeatureBitMask,
> > + IN UINT32 BitMaskSize
> > )
> > {
> > UINTN Index;
> > - UINTN BitMaskSize;
> > UINT8 *Data1;
> > UINT8 *Data2;
> >
> > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> > -
> > Data1 = SupportedFeatureMask;
> > Data2 = ComparedFeatureBitMask;
> > for (Index = 0; Index < BitMaskSize; Index++) { @@ -557,14 +553,14
> > @@ AnalysisProcessorFeatures (
> > //
> > // Calculate the last capability on all processors
> > //
> > - SupportedMaskAnd (CpuFeaturesData->CapabilityPcd, CpuInitOrder-
> > >FeaturesSupportedMask);
> > + SupportedMaskAnd (CpuFeaturesData->CapabilityPcd,
> > + CpuInitOrder->FeaturesSupportedMask, CpuFeaturesData-
> >BitMaskSize);
> > }
> > //
> > // Calculate the last setting
> > //
> > CpuFeaturesData->SettingPcd = AllocateCopyPool (CpuFeaturesData-
> > >BitMaskSize, CpuFeaturesData->CapabilityPcd);
> > ASSERT (CpuFeaturesData->SettingPcd != NULL);
> > - SupportedMaskAnd (CpuFeaturesData->SettingPcd, PcdGetPtr
> > (PcdCpuFeaturesSetting));
> > + SupportedMaskAnd (CpuFeaturesData->SettingPcd, PcdGetPtr
> > + (PcdCpuFeaturesSetting), CpuFeaturesData->BitMaskSize);
> >
> > //
> > // Dump the last CPU feature list
> > @@ -574,8 +570,8 @@ AnalysisProcessorFeatures (
> > Entry = GetFirstNode (&CpuFeaturesData->FeatureList);
> > while (!IsNull (&CpuFeaturesData->FeatureList, Entry)) {
> > CpuFeature = CPU_FEATURE_ENTRY_FROM_LINK (Entry);
> > - if (IsBitMaskMatch (CpuFeature->FeatureMask, CpuFeaturesData-
> > >CapabilityPcd)) {
> > - if (IsBitMaskMatch (CpuFeature->FeatureMask, CpuFeaturesData-
> > >SettingPcd)) {
> > + if (IsBitMaskMatch (CpuFeature->FeatureMask, CpuFeaturesData-
> > >CapabilityPcd, CpuFeaturesData->BitMaskSize)) {
> > + if (IsBitMaskMatch (CpuFeature->FeatureMask,
> > + CpuFeaturesData->SettingPcd, CpuFeaturesData->BitMaskSize)) {
> > DEBUG ((DEBUG_INFO, "[Enable ] "));
> > } else {
> > DEBUG ((DEBUG_INFO, "[Disable ] ")); @@ -583,22 +579,22 @@
> > AnalysisProcessorFeatures (
> > } else {
> > DEBUG ((DEBUG_INFO, "[Unsupport] "));
> > }
> > - DumpCpuFeature (CpuFeature);
> > + DumpCpuFeature (CpuFeature, CpuFeaturesData->BitMaskSize);
> > Entry = Entry->ForwardLink;
> > }
> > DEBUG ((DEBUG_INFO, "PcdCpuFeaturesCapability:\n"));
> > - DumpCpuFeatureMask (CpuFeaturesData->CapabilityPcd);
> > + DumpCpuFeatureMask (CpuFeaturesData->CapabilityPcd,
> > + CpuFeaturesData->BitMaskSize);
> > DEBUG ((DEBUG_INFO, "Origin PcdCpuFeaturesSetting:\n"));
> > - DumpCpuFeatureMask (PcdGetPtr (PcdCpuFeaturesSetting));
> > + DumpCpuFeatureMask (PcdGetPtr (PcdCpuFeaturesSetting),
> > + CpuFeaturesData->BitMaskSize);
> > DEBUG ((DEBUG_INFO, "Final PcdCpuFeaturesSetting:\n"));
> > - DumpCpuFeatureMask (CpuFeaturesData->SettingPcd);
> > + DumpCpuFeatureMask (CpuFeaturesData->SettingPcd,
> > + CpuFeaturesData->BitMaskSize);
> > );
> >
> > //
> > // Save PCDs and display CPU PCDs
> > //
> > SetCapabilityPcd (CpuFeaturesData->CapabilityPcd, CpuFeaturesData-
> > >BitMaskSize);
> > - SetSettingPcd (CpuFeaturesData->SettingPcd);
> > + SetSettingPcd (CpuFeaturesData->SettingPcd,
> > + CpuFeaturesData->BitMaskSize);
> >
> > for (ProcessorNumber = 0; ProcessorNumber < NumberOfCpus;
> > ProcessorNumber++) {
> > CpuInitOrder = &CpuFeaturesData->InitOrder[ProcessorNumber];
> > @@ -608,7 +604,7 @@ AnalysisProcessorFeatures (
> > // Insert each feature into processor's order list
> > //
> > CpuFeature = CPU_FEATURE_ENTRY_FROM_LINK (Entry);
> > - if (IsBitMaskMatch (CpuFeature->FeatureMask, CpuFeaturesData-
> > >CapabilityPcd)) {
> > + if (IsBitMaskMatch (CpuFeature->FeatureMask,
> > + CpuFeaturesData->CapabilityPcd, CpuFeaturesData->BitMaskSize)) {
> > CpuFeatureInOrder = AllocateCopyPool (sizeof
> > (CPU_FEATURES_ENTRY), CpuFeature);
> > ASSERT (CpuFeatureInOrder != NULL);
> > InsertTailList (&CpuInitOrder->OrderList,
> > &CpuFeatureInOrder->Link); @@ -624,18 +620,18 @@
> AnalysisProcessorFeatures (
> > CpuFeatureInOrder = CPU_FEATURE_ENTRY_FROM_LINK (Entry);
> >
> > Success = FALSE;
> > - if (IsBitMaskMatch (CpuFeatureInOrder->FeatureMask,
> > CpuFeaturesData->SettingPcd)) {
> > + if (IsBitMaskMatch (CpuFeatureInOrder->FeatureMask,
> > + CpuFeaturesData->SettingPcd, CpuFeaturesData->BitMaskSize)) {
> > Status = CpuFeatureInOrder->InitializeFunc (ProcessorNumber,
> > CpuInfo,
> > CpuFeatureInOrder->ConfigData, TRUE);
> > if (EFI_ERROR (Status)) {
> > //
> > // Clean the CpuFeatureInOrder->FeatureMask in setting PCD.
> > //
> > - SupportedMaskCleanBit (CpuFeaturesData->SettingPcd,
> > CpuFeatureInOrder->FeatureMask);
> > + SupportedMaskCleanBit (CpuFeaturesData->SettingPcd,
> > + CpuFeatureInOrder->FeatureMask, CpuFeaturesData->BitMaskSize);
> > if (CpuFeatureInOrder->FeatureName != NULL) {
> > DEBUG ((DEBUG_WARN, "Warning :: Failed to enable Feature:
> > Name = %a.\n", CpuFeatureInOrder->FeatureName));
> > } else {
> > DEBUG ((DEBUG_WARN, "Warning :: Failed to enable Feature:
> > Mask = "));
> > - DumpCpuFeatureMask (CpuFeatureInOrder->FeatureMask);
> > + DumpCpuFeatureMask (CpuFeatureInOrder->FeatureMask,
> > + CpuFeaturesData->BitMaskSize);
> > }
> > } else {
> > Success = TRUE;
> > @@ -647,7 +643,7 @@ AnalysisProcessorFeatures (
> > DEBUG ((DEBUG_WARN, "Warning :: Failed to disable
> > Feature: Name = %a.\n", CpuFeatureInOrder->FeatureName));
> > } else {
> > DEBUG ((DEBUG_WARN, "Warning :: Failed to disable
> > Feature: Mask = "));
> > - DumpCpuFeatureMask (CpuFeatureInOrder->FeatureMask);
> > + DumpCpuFeatureMask (CpuFeatureInOrder->FeatureMask,
> > + CpuFeaturesData->BitMaskSize);
> > }
> > } else {
> > Success = TRUE;
> > @@ -699,7 +695,7 @@ AnalysisProcessorFeatures (
> > // again during initialize the features.
> > //
> > DEBUG ((DEBUG_INFO, "Dump final value for
> > PcdCpuFeaturesSetting:\n"));
> > - DumpCpuFeatureMask (CpuFeaturesData->SettingPcd);
> > + DumpCpuFeatureMask (CpuFeaturesData->SettingPcd,
> > + CpuFeaturesData->BitMaskSize);
> >
> > //
> > // Dump the RegisterTable
> > diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> > index 5c546ee153..a18f926641 100644
> > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> > @@ -180,20 +180,26 @@ SwitchNewBsp (
> > Function that uses DEBUG() macros to display the contents of a a
> > CPU feature bit mask.
> >
> > @param[in] FeatureMask A pointer to the CPU feature bit mask.
> > + @param[in] BitMaskSize CPU feature bits mask buffer size.
> > +
> > **/
> > VOID
> > DumpCpuFeatureMask (
> > - IN UINT8 *FeatureMask
> > + IN UINT8 *FeatureMask,
> > + IN UINT32 BitMaskSize
> > );
> >
> > /**
> > Dump CPU feature name or CPU feature bit mask.
> >
> > @param[in] CpuFeature Pointer to CPU_FEATURES_ENTRY
> > + @param[in] BitMaskSize CPU feature bits mask buffer size.
> > +
> > **/
> > VOID
> > DumpCpuFeature (
> > - IN CPU_FEATURES_ENTRY *CpuFeature
> > + IN CPU_FEATURES_ENTRY *CpuFeature,
> > + IN UINT32 BitMaskSize
> > );
> >
> > /**
> > diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > index 36aabd7267..283e9d6539 100644
> > ---
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > +++
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > @@ -18,36 +18,34 @@
> > @retval FALSE Two CPU feature bit masks are not equal.
> > **/
> > BOOLEAN
> > -IsCpuFeatureMatch (
> > +IsBitMaskMatchCheck (
> > IN UINT8 *FirstFeatureMask,
> > IN UINT8 *SecondFeatureMask
> > )
> > {
> > - UINTN BitMaskSize;
> > + CPU_FEATURES_DATA *CpuFeaturesData;
> >
> > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> > - if (CompareMem (FirstFeatureMask, SecondFeatureMask, BitMaskSize)
> > ==
> > 0) {
> > - return TRUE;
> > - } else {
> > - return FALSE;
> > - }
> > + CpuFeaturesData = GetCpuFeaturesData ();
> > +
> > + return (CompareMem (FirstFeatureMask, SecondFeatureMask,
> > + CpuFeaturesData->BitMaskSize) == 0);
> > }
> >
> > /**
> > Function that uses DEBUG() macros to display the contents of a a
> > CPU feature bit mask.
> >
> > @param[in] FeatureMask A pointer to the CPU feature bit mask.
> > + @param[in] BitMaskSize CPU feature bits mask buffer size.
> > +
> > **/
> > VOID
> > DumpCpuFeatureMask (
> > - IN UINT8 *FeatureMask
> > + IN UINT8 *FeatureMask,
> > + IN UINT32 BitMaskSize
> > )
> > {
> > UINTN Index;
> > UINT8 *Data8;
> > - UINTN BitMaskSize;
> >
> > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> > Data8 = (UINT8 *) FeatureMask;
> > for (Index = 0; Index < BitMaskSize; Index++) {
> > DEBUG ((DEBUG_INFO, " %02x ", *Data8++)); @@ -59,10 +57,13 @@
> > DumpCpuFeatureMask (
> > Dump CPU feature name or CPU feature bit mask.
> >
> > @param[in] CpuFeature Pointer to CPU_FEATURES_ENTRY
> > + @param[in] BitMaskSize CPU feature bits mask buffer size.
> > +
> > **/
> > VOID
> > DumpCpuFeature (
> > - IN CPU_FEATURES_ENTRY *CpuFeature
> > + IN CPU_FEATURES_ENTRY *CpuFeature,
> > + IN UINT32 BitMaskSize
> > )
> > {
> >
> > @@ -70,42 +71,10 @@ DumpCpuFeature (
> > DEBUG ((DEBUG_INFO, "FeatureName: %a\n", CpuFeature-
> > >FeatureName));
> > } else {
> > DEBUG ((DEBUG_INFO, "FeatureMask = "));
> > - DumpCpuFeatureMask (CpuFeature->FeatureMask);
> > + DumpCpuFeatureMask (CpuFeature->FeatureMask, BitMaskSize);
> > }
> > }
> >
> > -/**
> > - Determines if the feature bit mask is in dependent CPU feature bit
> > mask buffer.
> > -
> > - @param[in] FeatureMask Pointer to CPU feature bit mask
> > - @param[in] DependentBitMask Pointer to dependent CPU feature bit
> > mask buffer
> > -
> > - @retval TRUE The feature bit mask is in dependent CPU feature bit
> > mask buffer.
> > - @retval FALSE The feature bit mask is not in dependent CPU feature
> > bit mask buffer.
> > -**/
> > -BOOLEAN
> > -IsBitMaskMatchCheck (
> > - IN UINT8 *FeatureMask,
> > - IN UINT8 *DependentBitMask
> > - )
> > -{
> > - UINTN Index;
> > - UINTN BitMaskSize;
> > - UINT8 *Data1;
> > - UINT8 *Data2;
> > -
> > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> > -
> > - Data1 = FeatureMask;
> > - Data2 = DependentBitMask;
> > - for (Index = 0; Index < BitMaskSize; Index++) {
> > - if (((*(Data1++)) & (*(Data2++))) != 0) {
> > - return TRUE;
> > - }
> > - }
> > - return FALSE;
> > -}
> > -
> > /**
> > Try to find the specify cpu featuren in former/after feature list.
> >
> > @@ -642,37 +611,21 @@ CheckCpuFeaturesDependency ( **/
> RETURN_STATUS
> > RegisterCpuFeatureWorker (
> > + IN CPU_FEATURES_DATA *CpuFeaturesData,
> > IN CPU_FEATURES_ENTRY *CpuFeature
> > )
> > {
> > EFI_STATUS Status;
> > - CPU_FEATURES_DATA *CpuFeaturesData;
> > CPU_FEATURES_ENTRY *CpuFeatureEntry;
> > LIST_ENTRY *Entry;
> > - UINTN BitMaskSize;
> > BOOLEAN FeatureExist;
> >
> > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> > - CpuFeaturesData = GetCpuFeaturesData ();
> > - if (CpuFeaturesData->FeaturesCount == 0) {
> > - InitializeListHead (&CpuFeaturesData->FeatureList);
> > - InitializeSpinLock (&CpuFeaturesData->CpuFlags.MemoryMappedLock);
> > - InitializeSpinLock (&CpuFeaturesData->CpuFlags.ConsoleLogLock);
> > - //
> > - // Driver has assumption that these three PCD should has same buffer
> size.
> > - //
> > - ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize
> > (PcdCpuFeaturesCapability));
> > - ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize
> > (PcdCpuFeaturesSupport));
> > - CpuFeaturesData->BitMaskSize = (UINT32) BitMaskSize;
> > - }
> > - ASSERT (CpuFeaturesData->BitMaskSize == BitMaskSize);
> > -
> > FeatureExist = FALSE;
> > CpuFeatureEntry = NULL;
> > Entry = GetFirstNode (&CpuFeaturesData->FeatureList);
> > while (!IsNull (&CpuFeaturesData->FeatureList, Entry)) {
> > CpuFeatureEntry = CPU_FEATURE_ENTRY_FROM_LINK (Entry);
> > - if (IsCpuFeatureMatch (CpuFeature->FeatureMask, CpuFeatureEntry-
> > >FeatureMask)) {
> > + if (IsBitMaskMatchCheck (CpuFeature->FeatureMask,
> > + CpuFeatureEntry->FeatureMask)) {
> > //
> > // If this feature already registered
> > //
> > @@ -684,12 +637,12 @@ RegisterCpuFeatureWorker (
> >
> > if (!FeatureExist) {
> > DEBUG ((DEBUG_INFO, "[NEW] "));
> > - DumpCpuFeature (CpuFeature);
> > + DumpCpuFeature (CpuFeature, CpuFeaturesData->BitMaskSize);
> > InsertTailList (&CpuFeaturesData->FeatureList, &CpuFeature->Link);
> > CpuFeaturesData->FeaturesCount++;
> > } else {
> > DEBUG ((DEBUG_INFO, "[OVERRIDE] "));
> > - DumpCpuFeature (CpuFeature);
> > + DumpCpuFeature (CpuFeature, CpuFeaturesData->BitMaskSize);
> > ASSERT (CpuFeatureEntry != NULL);
> > //
> > // Overwrite original parameters of CPU feature @@ -849,7 +802,6
> > @@ RegisterCpuFeature (
> > EFI_STATUS Status;
> > VA_LIST Marker;
> > UINT32 Feature;
> > - UINTN BitMaskSize;
> > CPU_FEATURES_ENTRY *CpuFeature;
> > UINT8 *FeatureMask;
> > UINT8 *BeforeFeatureBitMask;
> > @@ -860,6 +812,7 @@ RegisterCpuFeature (
> > UINT8 *PackageAfterFeatureBitMask;
> > BOOLEAN BeforeAll;
> > BOOLEAN AfterAll;
> > + CPU_FEATURES_DATA *CpuFeaturesData;
> >
> > FeatureMask = NULL;
> > BeforeFeatureBitMask = NULL;
> > @@ -871,7 +824,18 @@ RegisterCpuFeature (
> > BeforeAll = FALSE;
> > AfterAll = FALSE;
> >
> > - BitMaskSize = PcdGetSize (PcdCpuFeaturesSetting);
> > + CpuFeaturesData = GetCpuFeaturesData (); if
> > + (CpuFeaturesData->FeaturesCount == 0) {
> > + InitializeListHead (&CpuFeaturesData->FeatureList);
> > + InitializeSpinLock (&CpuFeaturesData->CpuFlags.MemoryMappedLock);
> > + InitializeSpinLock (&CpuFeaturesData->CpuFlags.ConsoleLogLock);
> > + //
> > + // Driver has assumption that below three PCDs should has same
> > + buffer
> > size.
> > + //
> > + ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize
> > (PcdCpuFeaturesCapability));
> > + ASSERT (PcdGetSize (PcdCpuFeaturesSetting) == PcdGetSize
> > (PcdCpuFeaturesSupport));
> > + CpuFeaturesData->BitMaskSize = (UINT32) PcdGetSize
> > + (PcdCpuFeaturesSetting); }
> >
> > VA_START (Marker, InitializeFunc);
> > Feature = VA_ARG (Marker, UINT32);
> > @@ -889,19 +853,19 @@ RegisterCpuFeature (
> > AfterAll = ((Feature & CPU_FEATURE_AFTER_ALL) != 0) ? TRUE : FALSE;
> > Feature &= ~(CPU_FEATURE_BEFORE_ALL |
> CPU_FEATURE_AFTER_ALL);
> > ASSERT (FeatureMask == NULL);
> > - SetCpuFeaturesBitMask (&FeatureMask, Feature, BitMaskSize);
> > + SetCpuFeaturesBitMask (&FeatureMask, Feature,
> > + CpuFeaturesData->BitMaskSize);
> > } else if ((Feature & CPU_FEATURE_BEFORE) != 0) {
> > - SetCpuFeaturesBitMask (&BeforeFeatureBitMask, Feature &
> > ~CPU_FEATURE_BEFORE, BitMaskSize);
> > + SetCpuFeaturesBitMask (&BeforeFeatureBitMask, Feature &
> > + ~CPU_FEATURE_BEFORE, CpuFeaturesData->BitMaskSize);
> > } else if ((Feature & CPU_FEATURE_AFTER) != 0) {
> > - SetCpuFeaturesBitMask (&AfterFeatureBitMask, Feature &
> > ~CPU_FEATURE_AFTER, BitMaskSize);
> > + SetCpuFeaturesBitMask (&AfterFeatureBitMask, Feature &
> > + ~CPU_FEATURE_AFTER, CpuFeaturesData->BitMaskSize);
> > } else if ((Feature & CPU_FEATURE_CORE_BEFORE) != 0) {
> > - SetCpuFeaturesBitMask (&CoreBeforeFeatureBitMask, Feature &
> > ~CPU_FEATURE_CORE_BEFORE, BitMaskSize);
> > + SetCpuFeaturesBitMask (&CoreBeforeFeatureBitMask, Feature &
> > + ~CPU_FEATURE_CORE_BEFORE, CpuFeaturesData->BitMaskSize);
> > } else if ((Feature & CPU_FEATURE_CORE_AFTER) != 0) {
> > - SetCpuFeaturesBitMask (&CoreAfterFeatureBitMask, Feature &
> > ~CPU_FEATURE_CORE_AFTER, BitMaskSize);
> > + SetCpuFeaturesBitMask (&CoreAfterFeatureBitMask, Feature &
> > + ~CPU_FEATURE_CORE_AFTER, CpuFeaturesData->BitMaskSize);
> > } else if ((Feature & CPU_FEATURE_PACKAGE_BEFORE) != 0) {
> > - SetCpuFeaturesBitMask (&PackageBeforeFeatureBitMask, Feature &
> > ~CPU_FEATURE_PACKAGE_BEFORE, BitMaskSize);
> > + SetCpuFeaturesBitMask (&PackageBeforeFeatureBitMask, Feature &
> > + ~CPU_FEATURE_PACKAGE_BEFORE, CpuFeaturesData->BitMaskSize);
> > } else if ((Feature & CPU_FEATURE_PACKAGE_AFTER) != 0) {
> > - SetCpuFeaturesBitMask (&PackageAfterFeatureBitMask, Feature &
> > ~CPU_FEATURE_PACKAGE_AFTER, BitMaskSize);
> > + SetCpuFeaturesBitMask (&PackageAfterFeatureBitMask, Feature &
> > + ~CPU_FEATURE_PACKAGE_AFTER, CpuFeaturesData->BitMaskSize);
> > }
> > Feature = VA_ARG (Marker, UINT32);
> > }
> > @@ -929,7 +893,7 @@ RegisterCpuFeature (
> > ASSERT_EFI_ERROR (Status);
> > }
> >
> > - Status = RegisterCpuFeatureWorker (CpuFeature);
> > + Status = RegisterCpuFeatureWorker (CpuFeaturesData, CpuFeature);
> > ASSERT_EFI_ERROR (Status);
> >
> > return RETURN_SUCCESS;
> > --
> > 2.21.0.windows.1
> >
> >
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-07-15 7:04 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-07-12 1:53 [Patch 0/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls gBS service Dong, Eric
2019-07-12 1:53 ` [Patch 1/2] " Dong, Eric
2019-07-12 10:53 ` Zeng, Star
2019-07-15 4:59 ` Ni, Ray
2019-07-12 1:53 ` [Patch 2/2] UefiCpuPkg/Library/RegisterCpuFeaturesLib: avoid use dynamic PCD Dong, Eric
2019-07-12 11:10 ` [edk2-devel] " Zeng, Star
2019-07-15 4:57 ` Ni, Ray
2019-07-15 7:04 ` Dong, Eric
2019-07-12 22:16 ` [Patch 0/2] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls gBS service Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox