* [Patch 0/4] Add "Test then Set" mechanism.
@ 2019-08-09 6:11 Dong, Eric
2019-08-09 6:11 ` [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib: Add "detect before set" Micros Dong, Eric
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Dong, Eric @ 2019-08-09 6:11 UTC (permalink / raw)
To: devel; +Cc: Ray Ni, Laszlo Ersek
Below code is current implementation:
if (MsrRegister[ProcessorNumber].Bits.Lock == 0) {
CPU_REGISTER_TABLE_WRITE_FIELD (
ProcessorNumber,
Msr,
MSR_IA32_FEATURE_CONTROL,
MSR_IA32_FEATURE_CONTROL_REGISTER,
Bits.Lock,
1
);
}
With below steps, the Bits.Lock bit will lose its value:
1. Trig normal boot, the Bits.Lock is 0. 1 will be added
into the register table and then will set to the MSR.
2. Trig warm reboot, MSR value preserves. After normal boot phase,
the Bits.Lock is 1, so it will not be added into the register
table during the warm reboot phase.
3. Trig S3 then resume, the Bits.Lock change to 0 and Bits.Lock is
not added in register table during normal boot phase. so it's
still 0 after resume.
This is not an expect behavior. The expect result is the value should
always 1 after booting or resuming from S3.
The root cause for this issue is
1. driver bases on current value to insert the "set value action" to
the register table.
2. Some MSRs may reserve their value during warm reboot. So the insert
action may be skip after warm reboot.
The solution for this issue is:
1. Always add "Test then Set" action for above referred MSRs.
2. Detect current value before set new value. Only set new value when
current value not same as new value.
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Eric Dong (4):
UefiCpuPkg/RegisterCpuFeaturesLib: Add "detect before set" Micros.
UefiCpuPkg/RegisterCpuFeaturesLib: Supports detect before set new
value logic.
UefiCpuPkg/PiSmmCpuDxeSmm: Supports detect before set new value logic.
UefiCpuPkg/CpuCommonFeaturesLib: Use new micros.
UefiCpuPkg/Include/AcpiCpuData.h | 1 +
.../Include/Library/RegisterCpuFeaturesLib.h | 77 +++++++++-
.../CpuCommonFeaturesLib/CpuCommonFeatures.h | 15 --
.../CpuCommonFeaturesLib.c | 8 +-
.../CpuCommonFeaturesLib/FeatureControl.c | 141 ++++++------------
.../CpuCommonFeaturesLib/MachineCheck.c | 23 ++-
.../CpuFeaturesInitialize.c | 141 ++++++++++++------
.../RegisterCpuFeaturesLib.c | 14 +-
UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 135 +++++++++++------
9 files changed, 323 insertions(+), 232 deletions(-)
--
2.21.0.windows.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib: Add "detect before set" Micros.
2019-08-09 6:11 [Patch 0/4] Add "Test then Set" mechanism Dong, Eric
@ 2019-08-09 6:11 ` Dong, Eric
2019-08-09 15:14 ` Laszlo Ersek
2019-08-09 15:31 ` Laszlo Ersek
2019-08-09 6:11 ` [Patch 2/4] UefiCpuPkg/RegisterCpuFeaturesLib: Supports detect before set new value logic Dong, Eric
` (2 subsequent siblings)
3 siblings, 2 replies; 12+ messages in thread
From: Dong, Eric @ 2019-08-09 6:11 UTC (permalink / raw)
To: devel; +Cc: Ray Ni, Laszlo Ersek
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2040
Add below new micros which test the current value before set
the new value. Only set new value when current value not
same as new value.
CPU_REGISTER_TABLE_TEST_THEN_WRITE32
CPU_REGISTER_TABLE_TEST_THEN_WRITE64
CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD
Signed-off-by: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
UefiCpuPkg/Include/AcpiCpuData.h | 1 +
.../Include/Library/RegisterCpuFeaturesLib.h | 77 +++++++++++++++++--
.../RegisterCpuFeaturesLib.c | 14 +++-
3 files changed, 80 insertions(+), 12 deletions(-)
diff --git a/UefiCpuPkg/Include/AcpiCpuData.h b/UefiCpuPkg/Include/AcpiCpuData.h
index b963a2f592..c764e209cf 100644
--- a/UefiCpuPkg/Include/AcpiCpuData.h
+++ b/UefiCpuPkg/Include/AcpiCpuData.h
@@ -81,6 +81,7 @@ typedef struct {
UINT16 Reserved; // offset 10 - 11
UINT32 HighIndex; // offset 12-15, only valid for MemoryMapped
UINT64 Value; // offset 16-23
+ UINT8 DetectIt; // 0ffset 24
} CPU_REGISTER_TABLE_ENTRY;
//
diff --git a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
index e420e7f075..87fe87acb7 100644
--- a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
+++ b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
@@ -335,6 +335,7 @@ SwitchBspAfterFeaturesInitialize (
@param[in] Index Index of the register to program
@param[in] ValueMask Mask of bits in register to write
@param[in] Value Value to write
+ @param[in] DetectIt Whether need to detect current Value before writing.
@note This service could be called by BSP only.
**/
@@ -345,7 +346,8 @@ CpuRegisterTableWrite (
IN REGISTER_TYPE RegisterType,
IN UINT64 Index,
IN UINT64 ValueMask,
- IN UINT64 Value
+ IN UINT64 Value,
+ IN UINT8 DetectIt
);
/**
@@ -385,9 +387,45 @@ PreSmmCpuRegisterTableWrite (
@note This service could be called by BSP only.
**/
-#define CPU_REGISTER_TABLE_WRITE32(ProcessorNumber, RegisterType, Index, Value) \
- do { \
- CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, MAX_UINT32, Value); \
+#define CPU_REGISTER_TABLE_WRITE32(ProcessorNumber, RegisterType, Index, Value) \
+ do { \
+ CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, MAX_UINT32, Value, FALSE); \
+ } while(FALSE);
+
+/**
+ Adds a 32-bit register write entry in specified register table.
+
+ This macro adds an entry in specified register table, with given register type,
+ register index, and value.
+
+ @param[in] ProcessorNumber The index of the CPU to add a register table entry.
+ @param[in] RegisterType Type of the register to program
+ @param[in] Index Index of the register to program
+ @param[in] Value Value to write
+
+ @note This service could be called by BSP only.
+**/
+#define CPU_REGISTER_TABLE_TEST_THEN_WRITE32(ProcessorNumber, RegisterType, Index, Value) \
+ do { \
+ CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, MAX_UINT32, Value, TRUE); \
+ } while(FALSE);
+
+/**
+ Adds a 64-bit register write entry in specified register table.
+
+ This macro adds an entry in specified register table, with given register type,
+ register index, and value.
+
+ @param[in] ProcessorNumber The index of the CPU to add a register table entry.
+ @param[in] RegisterType Type of the register to program
+ @param[in] Index Index of the register to program
+ @param[in] Value Value to write
+
+ @note This service could be called by BSP only.
+**/
+#define CPU_REGISTER_TABLE_WRITE64(ProcessorNumber, RegisterType, Index, Value) \
+ do { \
+ CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, MAX_UINT64, Value, FALSE); \
} while(FALSE);
/**
@@ -403,9 +441,9 @@ PreSmmCpuRegisterTableWrite (
@note This service could be called by BSP only.
**/
-#define CPU_REGISTER_TABLE_WRITE64(ProcessorNumber, RegisterType, Index, Value) \
- do { \
- CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, MAX_UINT64, Value); \
+#define CPU_REGISTER_TABLE_TEST_THEN_WRITE64(ProcessorNumber, RegisterType, Index, Value) \
+ do { \
+ CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, MAX_UINT64, Value, TRUE); \
} while(FALSE);
/**
@@ -428,7 +466,30 @@ PreSmmCpuRegisterTableWrite (
UINT64 ValueMask; \
ValueMask = MAX_UINT64; \
((Type *)(&ValueMask))->Field = 0; \
- CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, ~ValueMask, Value); \
+ CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, ~ValueMask, Value, FALSE); \
+ } while(FALSE);
+
+/**
+ Adds a bit field write entry in specified register table.
+
+ This macro adds an entry in specified register table, with given register type,
+ register index, bit field section, and value.
+
+ @param[in] ProcessorNumber The index of the CPU to add a register table entry.
+ @param[in] RegisterType Type of the register to program.
+ @param[in] Index Index of the register to program.
+ @param[in] Type The data type name of a register structure.
+ @param[in] Field The bit fiel name in register structure to write.
+ @param[in] Value Value to write to the bit field.
+
+ @note This service could be called by BSP only.
+**/
+#define CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD(ProcessorNumber, RegisterType, Index, Type, Field, Value) \
+ do { \
+ UINT64 ValueMask; \
+ ValueMask = MAX_UINT64; \
+ ((Type *)(&ValueMask))->Field = 0; \
+ CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, ~ValueMask, Value, TRUE); \
} while(FALSE);
/**
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
index 67885bf69b..152ab75988 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
@@ -1025,6 +1025,8 @@ EnlargeRegisterTable (
@param[in] ValidBitStart Start of the bit section
@param[in] ValidBitLength Length of the bit section
@param[in] Value Value to write
+ @param[in] DetectIt Whether need to detect current Value before writing.
+
**/
VOID
CpuRegisterTableWriteWorker (
@@ -1034,7 +1036,8 @@ CpuRegisterTableWriteWorker (
IN UINT64 Index,
IN UINT8 ValidBitStart,
IN UINT8 ValidBitLength,
- IN UINT64 Value
+ IN UINT64 Value,
+ IN UINT8 DetectIt
)
{
CPU_FEATURES_DATA *CpuFeaturesData;
@@ -1070,6 +1073,7 @@ CpuRegisterTableWriteWorker (
RegisterTableEntry[RegisterTable->TableLength].ValidBitStart = ValidBitStart;
RegisterTableEntry[RegisterTable->TableLength].ValidBitLength = ValidBitLength;
RegisterTableEntry[RegisterTable->TableLength].Value = Value;
+ RegisterTableEntry[RegisterTable->TableLength].DetectIt = DetectIt;
RegisterTable->TableLength++;
}
@@ -1085,6 +1089,7 @@ CpuRegisterTableWriteWorker (
@param[in] Index Index of the register to program
@param[in] ValueMask Mask of bits in register to write
@param[in] Value Value to write
+ @param[in] DetectIt Whether need to detect current Value before writing.
@note This service could be called by BSP only.
**/
@@ -1095,7 +1100,8 @@ CpuRegisterTableWrite (
IN REGISTER_TYPE RegisterType,
IN UINT64 Index,
IN UINT64 ValueMask,
- IN UINT64 Value
+ IN UINT64 Value,
+ IN UINT8 DetectIt
)
{
UINT8 Start;
@@ -1105,7 +1111,7 @@ CpuRegisterTableWrite (
Start = (UINT8)LowBitSet64 (ValueMask);
End = (UINT8)HighBitSet64 (ValueMask);
Length = End - Start + 1;
- CpuRegisterTableWriteWorker (FALSE, ProcessorNumber, RegisterType, Index, Start, Length, Value);
+ CpuRegisterTableWriteWorker (FALSE, ProcessorNumber, RegisterType, Index, Start, Length, Value, DetectIt);
}
/**
@@ -1139,7 +1145,7 @@ PreSmmCpuRegisterTableWrite (
Start = (UINT8)LowBitSet64 (ValueMask);
End = (UINT8)HighBitSet64 (ValueMask);
Length = End - Start + 1;
- CpuRegisterTableWriteWorker (TRUE, ProcessorNumber, RegisterType, Index, Start, Length, Value);
+ CpuRegisterTableWriteWorker (TRUE, ProcessorNumber, RegisterType, Index, Start, Length, Value, FALSE);
}
/**
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Patch 2/4] UefiCpuPkg/RegisterCpuFeaturesLib: Supports detect before set new value logic.
2019-08-09 6:11 [Patch 0/4] Add "Test then Set" mechanism Dong, Eric
2019-08-09 6:11 ` [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib: Add "detect before set" Micros Dong, Eric
@ 2019-08-09 6:11 ` Dong, Eric
2019-08-09 6:11 ` [Patch 3/4] UefiCpuPkg/PiSmmCpuDxeSmm: " Dong, Eric
2019-08-09 6:11 ` [Patch 4/4] UefiCpuPkg/CpuCommonFeaturesLib: Use "Test then set" action Dong, Eric
3 siblings, 0 replies; 12+ messages in thread
From: Dong, Eric @ 2019-08-09 6:11 UTC (permalink / raw)
To: devel; +Cc: Ray Ni, Laszlo Ersek
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2040
Supports new logic which detect current value before set new value.
Only set new value when current value not same as new value.
Signed-off-by: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
.../CpuFeaturesInitialize.c | 141 ++++++++++++------
1 file changed, 93 insertions(+), 48 deletions(-)
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
index fb0535edd6..2b8816e1d8 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
@@ -744,6 +744,58 @@ LibWaitForSemaphore (
) != Value);
}
+/**
+ Read / write CR value.
+
+ @param[in] CrIndex The CR index which need to read/write.
+ @param[in] Read Read or write. TRUE is read.
+ @param[in,out] CrValue CR value.
+
+ @retval EFI_SUCCESS means read/write success, else return EFI_UNSUPPORTED.
+**/
+UINTN
+ReadWriteCr (
+ IN UINT32 CrIndex,
+ IN BOOLEAN Read,
+ IN OUT UINTN *CrValue
+ )
+{
+ switch (CrIndex) {
+ case 0:
+ if (Read) {
+ *CrValue = AsmReadCr0 ();
+ } else {
+ AsmWriteCr0 (*CrValue);
+ }
+ break;
+ case 2:
+ if (Read) {
+ *CrValue = AsmReadCr2 ();
+ } else {
+ AsmWriteCr2 (*CrValue);
+ }
+ break;
+ case 3:
+ if (Read) {
+ *CrValue = AsmReadCr3 ();
+ } else {
+ AsmWriteCr3 (*CrValue);
+ }
+ break;
+ case 4:
+ if (Read) {
+ *CrValue = AsmReadCr4 ();
+ } else {
+ AsmWriteCr4 (*CrValue);
+ }
+ break;
+ default:
+ return EFI_UNSUPPORTED;;
+ }
+
+ return EFI_SUCCESS;
+}
+
/**
Initialize the CPU registers from a register table.
@@ -773,6 +825,8 @@ ProgramProcessorRegister (
UINTN ProcessorIndex;
UINTN ValidThreadCount;
UINT32 *ValidCoreCountPerPackage;
+ EFI_STATUS Status;
+ UINT64 CurrentValue;
//
// Traverse Register Table of this logical processor
@@ -791,60 +845,51 @@ ProgramProcessorRegister (
// The specified register is Control Register
//
case ControlRegister:
- switch (RegisterTableEntry->Index) {
- case 0:
- Value = AsmReadCr0 ();
- Value = (UINTN) BitFieldWrite64 (
- Value,
- RegisterTableEntry->ValidBitStart,
- RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1,
- RegisterTableEntry->Value
- );
- AsmWriteCr0 (Value);
- break;
- case 2:
- Value = AsmReadCr2 ();
- Value = (UINTN) BitFieldWrite64 (
- Value,
- RegisterTableEntry->ValidBitStart,
- RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1,
- RegisterTableEntry->Value
- );
- AsmWriteCr2 (Value);
- break;
- case 3:
- Value = AsmReadCr3 ();
- Value = (UINTN) BitFieldWrite64 (
- Value,
- RegisterTableEntry->ValidBitStart,
- RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1,
- RegisterTableEntry->Value
- );
- AsmWriteCr3 (Value);
- break;
- case 4:
- Value = AsmReadCr4 ();
- Value = (UINTN) BitFieldWrite64 (
- Value,
- RegisterTableEntry->ValidBitStart,
- RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1,
- RegisterTableEntry->Value
- );
- AsmWriteCr4 (Value);
- break;
- case 8:
- //
- // Do we need to support CR8?
- //
- break;
- default:
- break;
+ Status = ReadWriteCr(RegisterTableEntry->Index, TRUE, &Value);
+ if (EFI_ERROR (Status)) {
+ return;
+ }
+ if (RegisterTableEntry->DetectIt) {
+ CurrentValue = BitFieldRead64(
+ Value,
+ RegisterTableEntry->ValidBitStart,
+ RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1
+ );
+ if (CurrentValue == RegisterTableEntry->Value) {
+ return;
+ }
}
+ Value = (UINTN) BitFieldWrite64 (
+ Value,
+ RegisterTableEntry->ValidBitStart,
+ RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1,
+ RegisterTableEntry->Value
+ );
+ ReadWriteCr (RegisterTableEntry->Index, FALSE, &Value);
break;
+
//
// The specified register is Model Specific Register
//
case Msr:
+ if (RegisterTableEntry->DetectIt) {
+ Value = (UINTN)AsmReadMsr64 (RegisterTableEntry->Index);
+ if (RegisterTableEntry->ValidBitLength >= 64) {
+ if (Value == RegisterTableEntry->Value) {
+ return;
+ }
+ } else {
+ CurrentValue = BitFieldRead64(
+ Value,
+ RegisterTableEntry->ValidBitStart,
+ RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1
+ );
+ if (CurrentValue == RegisterTableEntry->Value) {
+ return;
+ }
+ }
+ }
+
if (RegisterTableEntry->ValidBitLength >= 64) {
//
// If length is not less than 64 bits, then directly write without reading
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Patch 3/4] UefiCpuPkg/PiSmmCpuDxeSmm: Supports detect before set new value logic.
2019-08-09 6:11 [Patch 0/4] Add "Test then Set" mechanism Dong, Eric
2019-08-09 6:11 ` [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib: Add "detect before set" Micros Dong, Eric
2019-08-09 6:11 ` [Patch 2/4] UefiCpuPkg/RegisterCpuFeaturesLib: Supports detect before set new value logic Dong, Eric
@ 2019-08-09 6:11 ` Dong, Eric
2019-08-09 15:30 ` Laszlo Ersek
2019-08-09 6:11 ` [Patch 4/4] UefiCpuPkg/CpuCommonFeaturesLib: Use "Test then set" action Dong, Eric
3 siblings, 1 reply; 12+ messages in thread
From: Dong, Eric @ 2019-08-09 6:11 UTC (permalink / raw)
To: devel; +Cc: Ray Ni, Laszlo Ersek
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2040
Supports new logic which detect current value before set new value.
Only set new value when current value not same as new value.
Signed-off-by: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 135 ++++++++++++++++++++----------
1 file changed, 92 insertions(+), 43 deletions(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
index d8c6b19ead..957f2896eb 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
@@ -159,6 +159,58 @@ S3WaitForSemaphore (
) != Value);
}
+/**
+ Read / write CR value.
+
+ @param[in] CrIndex The CR index which need to read/write.
+ @param[in] Read Read or write. TRUE is read.
+ @param[in,out] CrValue CR value.
+
+ @retval EFI_SUCCESS means read/write success, else return EFI_UNSUPPORTED.
+**/
+UINTN
+ReadWriteCr (
+ IN UINT32 CrIndex,
+ IN BOOLEAN Read,
+ IN OUT UINTN *CrValue
+ )
+{
+ switch (CrIndex) {
+ case 0:
+ if (Read) {
+ *CrValue = AsmReadCr0 ();
+ } else {
+ AsmWriteCr0 (*CrValue);
+ }
+ break;
+ case 2:
+ if (Read) {
+ *CrValue = AsmReadCr2 ();
+ } else {
+ AsmWriteCr2 (*CrValue);
+ }
+ break;
+ case 3:
+ if (Read) {
+ *CrValue = AsmReadCr3 ();
+ } else {
+ AsmWriteCr3 (*CrValue);
+ }
+ break;
+ case 4:
+ if (Read) {
+ *CrValue = AsmReadCr4 ();
+ } else {
+ AsmWriteCr4 (*CrValue);
+ }
+ break;
+ default:
+ return EFI_UNSUPPORTED;;
+ }
+
+ return EFI_SUCCESS;
+}
+
/**
Initialize the CPU registers from a register table.
@@ -188,6 +240,8 @@ ProgramProcessorRegister (
UINTN ProcessorIndex;
UINTN ValidThreadCount;
UINT32 *ValidCoreCountPerPackage;
+ EFI_STATUS Status;
+ UINT64 CurrentValue;
//
// Traverse Register Table of this logical processor
@@ -206,55 +260,50 @@ ProgramProcessorRegister (
// The specified register is Control Register
//
case ControlRegister:
- switch (RegisterTableEntry->Index) {
- case 0:
- Value = AsmReadCr0 ();
- Value = (UINTN) BitFieldWrite64 (
- Value,
- RegisterTableEntry->ValidBitStart,
- RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1,
- (UINTN) RegisterTableEntry->Value
- );
- AsmWriteCr0 (Value);
- break;
- case 2:
- Value = AsmReadCr2 ();
- Value = (UINTN) BitFieldWrite64 (
- Value,
- RegisterTableEntry->ValidBitStart,
- RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1,
- (UINTN) RegisterTableEntry->Value
- );
- AsmWriteCr2 (Value);
- break;
- case 3:
- Value = AsmReadCr3 ();
- Value = (UINTN) BitFieldWrite64 (
- Value,
- RegisterTableEntry->ValidBitStart,
- RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1,
- (UINTN) RegisterTableEntry->Value
- );
- AsmWriteCr3 (Value);
- break;
- case 4:
- Value = AsmReadCr4 ();
- Value = (UINTN) BitFieldWrite64 (
- Value,
- RegisterTableEntry->ValidBitStart,
- RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1,
- (UINTN) RegisterTableEntry->Value
- );
- AsmWriteCr4 (Value);
- break;
- default:
- break;
+ Status = ReadWriteCr(RegisterTableEntry->Index, TRUE, &Value);
+ if (EFI_ERROR (Status)) {
+ return;
+ }
+ if (RegisterTableEntry->DetectIt) {
+ CurrentValue = BitFieldRead64(
+ Value,
+ RegisterTableEntry->ValidBitStart,
+ RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1
+ );
+ if (CurrentValue == RegisterTableEntry->Value) {
+ return;
+ }
}
+ Value = (UINTN) BitFieldWrite64 (
+ Value,
+ RegisterTableEntry->ValidBitStart,
+ RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1,
+ RegisterTableEntry->Value
+ );
+ ReadWriteCr (RegisterTableEntry->Index, FALSE, &Value);
break;
//
// The specified register is Model Specific Register
//
case Msr:
+ if (RegisterTableEntry->DetectIt) {
+ Value = (UINTN)AsmReadMsr64 (RegisterTableEntry->Index);
+ if (RegisterTableEntry->ValidBitLength >= 64) {
+ if (Value == RegisterTableEntry->Value) {
+ return;
+ }
+ } else {
+ CurrentValue = BitFieldRead64(
+ Value,
+ RegisterTableEntry->ValidBitStart,
+ RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1
+ );
+ if (CurrentValue == RegisterTableEntry->Value) {
+ return;
+ }
+ }
+ }
+
//
// If this function is called to restore register setting after INIT signal,
// there is no need to restore MSRs in register table.
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [Patch 4/4] UefiCpuPkg/CpuCommonFeaturesLib: Use "Test then set" action.
2019-08-09 6:11 [Patch 0/4] Add "Test then Set" mechanism Dong, Eric
` (2 preceding siblings ...)
2019-08-09 6:11 ` [Patch 3/4] UefiCpuPkg/PiSmmCpuDxeSmm: " Dong, Eric
@ 2019-08-09 6:11 ` Dong, Eric
3 siblings, 0 replies; 12+ messages in thread
From: Dong, Eric @ 2019-08-09 6:11 UTC (permalink / raw)
To: devel; +Cc: Ray Ni, Laszlo Ersek
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2040
Below code is current implementation:
if (MsrRegister[ProcessorNumber].Bits.Lock == 0) {
CPU_REGISTER_TABLE_WRITE_FIELD (
ProcessorNumber,
Msr,
MSR_IA32_FEATURE_CONTROL,
MSR_IA32_FEATURE_CONTROL_REGISTER,
Bits.Lock,
1
);
}
With below steps, the Bits.Lock bit will lose its value:
1. In first normal boot, the Bits.Lock is 0. 1 will be added
into the register table and then will set to the MSR.
2. Trig warm reboot, MSR value preserves. After normal boot phase,
the Bits.Lock is 1, so it will not be added into the register
table during the warm reboot phase.
3. Trig S3 then resume, the Bits.Lock change to 0 and Bits.Lock is
not added in register table during normal boot phase. so it's
still 0 after resume.
This is not an expect behavior. The expect result is the value should
always 1 after booting or resuming from S3.
The root cause for this issue is
1. driver bases on current value to insert the "set value action" to
the register table.
2. Some MSRs may reserve their value during warm reboot. So the insert
action may be skip after warm reboot.
The solution for this issue is:
1. Always add "Test then Set" action for above referred MSRs.
2. Detect current value before set new value. Only set new value when
current value not same as new value.
Signed-off-by: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
---
.../CpuCommonFeaturesLib/CpuCommonFeatures.h | 15 --
.../CpuCommonFeaturesLib.c | 8 +-
.../CpuCommonFeaturesLib/FeatureControl.c | 141 ++++++------------
.../CpuCommonFeaturesLib/MachineCheck.c | 23 ++-
4 files changed, 58 insertions(+), 129 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h
index 25d0174727..b2390e6c39 100644
--- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h
+++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h
@@ -848,21 +848,6 @@ X2ApicInitialize (
IN BOOLEAN State
);
-/**
- Prepares for the data used by CPU feature detection and initialization.
-
- @param[in] NumberOfProcessors The number of CPUs in the platform.
-
- @return Pointer to a buffer of CPU related configuration data.
-
- @note This service could be called by BSP only.
-**/
-VOID *
-EFIAPI
-FeatureControlGetConfigData (
- IN UINTN NumberOfProcessors
- );
-
/**
Prepares for the data used by CPU feature detection and initialization.
diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c
index fd43b8d662..f0dd3a3b43 100644
--- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c
+++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c
@@ -91,7 +91,7 @@ CpuCommonFeaturesLibConstructor (
if (IsCpuFeatureSupported (CPU_FEATURE_LOCK_FEATURE_CONTROL_REGISTER)) {
Status = RegisterCpuFeature (
"Lock Feature Control Register",
- FeatureControlGetConfigData,
+ NULL,
LockFeatureControlRegisterSupport,
LockFeatureControlRegisterInitialize,
CPU_FEATURE_LOCK_FEATURE_CONTROL_REGISTER,
@@ -102,7 +102,7 @@ CpuCommonFeaturesLibConstructor (
if (IsCpuFeatureSupported (CPU_FEATURE_SMX)) {
Status = RegisterCpuFeature (
"SMX",
- FeatureControlGetConfigData,
+ NULL,
SmxSupport,
SmxInitialize,
CPU_FEATURE_SMX,
@@ -114,7 +114,7 @@ CpuCommonFeaturesLibConstructor (
if (IsCpuFeatureSupported (CPU_FEATURE_VMX)) {
Status = RegisterCpuFeature (
"VMX",
- FeatureControlGetConfigData,
+ NULL,
VmxSupport,
VmxInitialize,
CPU_FEATURE_VMX,
@@ -214,7 +214,7 @@ CpuCommonFeaturesLibConstructor (
if (IsCpuFeatureSupported (CPU_FEATURE_LMCE)) {
Status = RegisterCpuFeature (
"LMCE",
- FeatureControlGetConfigData,
+ NULL,
LmceSupport,
LmceInitialize,
CPU_FEATURE_LMCE,
diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/FeatureControl.c b/UefiCpuPkg/Library/CpuCommonFeaturesLib/FeatureControl.c
index 3712ef1e5c..6679df8ba4 100644
--- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/FeatureControl.c
+++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/FeatureControl.c
@@ -8,28 +8,6 @@
#include "CpuCommonFeatures.h"
-/**
- Prepares for the data used by CPU feature detection and initialization.
-
- @param[in] NumberOfProcessors The number of CPUs in the platform.
-
- @return Pointer to a buffer of CPU related configuration data.
-
- @note This service could be called by BSP only.
-**/
-VOID *
-EFIAPI
-FeatureControlGetConfigData (
- IN UINTN NumberOfProcessors
- )
-{
- VOID *ConfigData;
-
- ConfigData = AllocateZeroPool (sizeof (MSR_IA32_FEATURE_CONTROL_REGISTER) * NumberOfProcessors);
- ASSERT (ConfigData != NULL);
- return ConfigData;
-}
-
/**
Detects if VMX feature supported on current processor.
@@ -54,11 +32,6 @@ VmxSupport (
IN VOID *ConfigData OPTIONAL
)
{
- MSR_IA32_FEATURE_CONTROL_REGISTER *MsrRegister;
-
- ASSERT (ConfigData != NULL);
- MsrRegister = (MSR_IA32_FEATURE_CONTROL_REGISTER *) ConfigData;
- MsrRegister[ProcessorNumber].Uint64 = AsmReadMsr64 (MSR_IA32_FEATURE_CONTROL);
return (CpuInfo->CpuIdVersionInfoEcx.Bits.VMX == 1);
}
@@ -88,8 +61,6 @@ VmxInitialize (
IN BOOLEAN State
)
{
- MSR_IA32_FEATURE_CONTROL_REGISTER *MsrRegister;
-
//
// The scope of EnableVmxOutsideSmx bit in the MSR_IA32_FEATURE_CONTROL is core for
// below processor type, only program MSR_IA32_FEATURE_CONTROL for thread 0 in each
@@ -103,18 +74,15 @@ VmxInitialize (
}
}
- ASSERT (ConfigData != NULL);
- MsrRegister = (MSR_IA32_FEATURE_CONTROL_REGISTER *) ConfigData;
- if (MsrRegister[ProcessorNumber].Bits.Lock == 0) {
- CPU_REGISTER_TABLE_WRITE_FIELD (
- ProcessorNumber,
- Msr,
- MSR_IA32_FEATURE_CONTROL,
- MSR_IA32_FEATURE_CONTROL_REGISTER,
- Bits.EnableVmxOutsideSmx,
- (State) ? 1 : 0
- );
- }
+ CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD (
+ ProcessorNumber,
+ Msr,
+ MSR_IA32_FEATURE_CONTROL,
+ MSR_IA32_FEATURE_CONTROL_REGISTER,
+ Bits.EnableVmxOutsideSmx,
+ (State) ? 1 : 0
+ );
+
return RETURN_SUCCESS;
}
@@ -142,11 +110,6 @@ LockFeatureControlRegisterSupport (
IN VOID *ConfigData OPTIONAL
)
{
- MSR_IA32_FEATURE_CONTROL_REGISTER *MsrRegister;
-
- ASSERT (ConfigData != NULL);
- MsrRegister = (MSR_IA32_FEATURE_CONTROL_REGISTER *) ConfigData;
- MsrRegister[ProcessorNumber].Uint64 = AsmReadMsr64 (MSR_IA32_FEATURE_CONTROL);
return TRUE;
}
@@ -176,8 +139,6 @@ LockFeatureControlRegisterInitialize (
IN BOOLEAN State
)
{
- MSR_IA32_FEATURE_CONTROL_REGISTER *MsrRegister;
-
//
// The scope of Lock bit in the MSR_IA32_FEATURE_CONTROL is core for
// below processor type, only program MSR_IA32_FEATURE_CONTROL for thread 0 in each
@@ -191,18 +152,15 @@ LockFeatureControlRegisterInitialize (
}
}
- ASSERT (ConfigData != NULL);
- MsrRegister = (MSR_IA32_FEATURE_CONTROL_REGISTER *) ConfigData;
- if (MsrRegister[ProcessorNumber].Bits.Lock == 0) {
- CPU_REGISTER_TABLE_WRITE_FIELD (
- ProcessorNumber,
- Msr,
- MSR_IA32_FEATURE_CONTROL,
- MSR_IA32_FEATURE_CONTROL_REGISTER,
- Bits.Lock,
- 1
- );
- }
+ CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD (
+ ProcessorNumber,
+ Msr,
+ MSR_IA32_FEATURE_CONTROL,
+ MSR_IA32_FEATURE_CONTROL_REGISTER,
+ Bits.Lock,
+ 1
+ );
+
return RETURN_SUCCESS;
}
@@ -230,11 +188,6 @@ SmxSupport (
IN VOID *ConfigData OPTIONAL
)
{
- MSR_IA32_FEATURE_CONTROL_REGISTER *MsrRegister;
-
- ASSERT (ConfigData != NULL);
- MsrRegister = (MSR_IA32_FEATURE_CONTROL_REGISTER *) ConfigData;
- MsrRegister[ProcessorNumber].Uint64 = AsmReadMsr64 (MSR_IA32_FEATURE_CONTROL);
return (CpuInfo->CpuIdVersionInfoEcx.Bits.SMX == 1);
}
@@ -265,7 +218,6 @@ SmxInitialize (
IN BOOLEAN State
)
{
- MSR_IA32_FEATURE_CONTROL_REGISTER *MsrRegister;
RETURN_STATUS Status;
//
@@ -288,35 +240,32 @@ SmxInitialize (
Status = RETURN_UNSUPPORTED;
}
- ASSERT (ConfigData != NULL);
- MsrRegister = (MSR_IA32_FEATURE_CONTROL_REGISTER *) ConfigData;
- if (MsrRegister[ProcessorNumber].Bits.Lock == 0) {
- CPU_REGISTER_TABLE_WRITE_FIELD (
- ProcessorNumber,
- Msr,
- MSR_IA32_FEATURE_CONTROL,
- MSR_IA32_FEATURE_CONTROL_REGISTER,
- Bits.SenterLocalFunctionEnables,
- (State) ? 0x7F : 0
- );
-
- CPU_REGISTER_TABLE_WRITE_FIELD (
- ProcessorNumber,
- Msr,
- MSR_IA32_FEATURE_CONTROL,
- MSR_IA32_FEATURE_CONTROL_REGISTER,
- Bits.SenterGlobalEnable,
- (State) ? 1 : 0
- );
-
- CPU_REGISTER_TABLE_WRITE_FIELD (
- ProcessorNumber,
- Msr,
- MSR_IA32_FEATURE_CONTROL,
- MSR_IA32_FEATURE_CONTROL_REGISTER,
- Bits.EnableVmxInsideSmx,
- (State) ? 1 : 0
- );
- }
+ CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD (
+ ProcessorNumber,
+ Msr,
+ MSR_IA32_FEATURE_CONTROL,
+ MSR_IA32_FEATURE_CONTROL_REGISTER,
+ Bits.SenterLocalFunctionEnables,
+ (State) ? 0x7F : 0
+ );
+
+ CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD (
+ ProcessorNumber,
+ Msr,
+ MSR_IA32_FEATURE_CONTROL,
+ MSR_IA32_FEATURE_CONTROL_REGISTER,
+ Bits.SenterGlobalEnable,
+ (State) ? 1 : 0
+ );
+
+ CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD (
+ ProcessorNumber,
+ Msr,
+ MSR_IA32_FEATURE_CONTROL,
+ MSR_IA32_FEATURE_CONTROL_REGISTER,
+ Bits.EnableVmxInsideSmx,
+ (State) ? 1 : 0
+ );
+
return Status;
}
diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/MachineCheck.c b/UefiCpuPkg/Library/CpuCommonFeaturesLib/MachineCheck.c
index 2528e0044e..01fd6bb54d 100644
--- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/MachineCheck.c
+++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/MachineCheck.c
@@ -319,8 +319,6 @@ LmceInitialize (
IN BOOLEAN State
)
{
- MSR_IA32_FEATURE_CONTROL_REGISTER *MsrRegister;
-
//
// The scope of LcmeOn bit in the MSR_IA32_MISC_ENABLE is core for below processor type, only program
// MSR_IA32_MISC_ENABLE for thread 0 in each core.
@@ -333,17 +331,14 @@ LmceInitialize (
}
}
- ASSERT (ConfigData != NULL);
- MsrRegister = (MSR_IA32_FEATURE_CONTROL_REGISTER *) ConfigData;
- if (MsrRegister[ProcessorNumber].Bits.Lock == 0) {
- CPU_REGISTER_TABLE_WRITE_FIELD (
- ProcessorNumber,
- Msr,
- MSR_IA32_FEATURE_CONTROL,
- MSR_IA32_FEATURE_CONTROL_REGISTER,
- Bits.LmceOn,
- (State) ? 1 : 0
- );
- }
+ CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD (
+ ProcessorNumber,
+ Msr,
+ MSR_IA32_FEATURE_CONTROL,
+ MSR_IA32_FEATURE_CONTROL_REGISTER,
+ Bits.LmceOn,
+ (State) ? 1 : 0
+ );
+
return RETURN_SUCCESS;
}
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib: Add "detect before set" Micros.
2019-08-09 6:11 ` [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib: Add "detect before set" Micros Dong, Eric
@ 2019-08-09 15:14 ` Laszlo Ersek
2019-08-12 7:46 ` [edk2-devel] " Dong, Eric
2019-08-09 15:31 ` Laszlo Ersek
1 sibling, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2019-08-09 15:14 UTC (permalink / raw)
To: Eric Dong, devel; +Cc: Ray Ni
On 08/09/19 08:11, Eric Dong wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2040
>
> Add below new micros which test the current value before set
> the new value. Only set new value when current value not
> same as new value.
> CPU_REGISTER_TABLE_TEST_THEN_WRITE32
> CPU_REGISTER_TABLE_TEST_THEN_WRITE64
> CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD
>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
> UefiCpuPkg/Include/AcpiCpuData.h | 1 +
> .../Include/Library/RegisterCpuFeaturesLib.h | 77 +++++++++++++++++--
> .../RegisterCpuFeaturesLib.c | 14 +++-
> 3 files changed, 80 insertions(+), 12 deletions(-)
(1) When you format your patch sets, can you please use the following
two options:
--stat=1000 --stat-graph-width=20
Otherwise the diffstats are truncated (on the left) and hard to understand.
>
> diff --git a/UefiCpuPkg/Include/AcpiCpuData.h b/UefiCpuPkg/Include/AcpiCpuData.h
> index b963a2f592..c764e209cf 100644
> --- a/UefiCpuPkg/Include/AcpiCpuData.h
> +++ b/UefiCpuPkg/Include/AcpiCpuData.h
> @@ -81,6 +81,7 @@ typedef struct {
> UINT16 Reserved; // offset 10 - 11
> UINT32 HighIndex; // offset 12-15, only valid for MemoryMapped
> UINT64 Value; // offset 16-23
> + UINT8 DetectIt; // 0ffset 24
> } CPU_REGISTER_TABLE_ENTRY;
(2) Another quite generic comment -- "DetectIt" does not look helpful.
Somehow the verb "detect" does not communicate the right action to me.
How about using a more established name, such as:
- CompareAndSwap
- CompareAndSet
- TestAndSet
?
If you agree, then I suggest updating the parameter names, and their
comments too, below.
Thanks
Laszlo
>
> //
> diff --git a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> index e420e7f075..87fe87acb7 100644
> --- a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> +++ b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> @@ -335,6 +335,7 @@ SwitchBspAfterFeaturesInitialize (
> @param[in] Index Index of the register to program
> @param[in] ValueMask Mask of bits in register to write
> @param[in] Value Value to write
> + @param[in] DetectIt Whether need to detect current Value before writing.
>
> @note This service could be called by BSP only.
> **/
> @@ -345,7 +346,8 @@ CpuRegisterTableWrite (
> IN REGISTER_TYPE RegisterType,
> IN UINT64 Index,
> IN UINT64 ValueMask,
> - IN UINT64 Value
> + IN UINT64 Value,
> + IN UINT8 DetectIt
> );
>
> /**
> @@ -385,9 +387,45 @@ PreSmmCpuRegisterTableWrite (
>
> @note This service could be called by BSP only.
> **/
> -#define CPU_REGISTER_TABLE_WRITE32(ProcessorNumber, RegisterType, Index, Value) \
> - do { \
> - CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, MAX_UINT32, Value); \
> +#define CPU_REGISTER_TABLE_WRITE32(ProcessorNumber, RegisterType, Index, Value) \
> + do { \
> + CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, MAX_UINT32, Value, FALSE); \
> + } while(FALSE);
> +
> +/**
> + Adds a 32-bit register write entry in specified register table.
> +
> + This macro adds an entry in specified register table, with given register type,
> + register index, and value.
> +
> + @param[in] ProcessorNumber The index of the CPU to add a register table entry.
> + @param[in] RegisterType Type of the register to program
> + @param[in] Index Index of the register to program
> + @param[in] Value Value to write
> +
> + @note This service could be called by BSP only.
> +**/
> +#define CPU_REGISTER_TABLE_TEST_THEN_WRITE32(ProcessorNumber, RegisterType, Index, Value) \
> + do { \
> + CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, MAX_UINT32, Value, TRUE); \
> + } while(FALSE);
> +
> +/**
> + Adds a 64-bit register write entry in specified register table.
> +
> + This macro adds an entry in specified register table, with given register type,
> + register index, and value.
> +
> + @param[in] ProcessorNumber The index of the CPU to add a register table entry.
> + @param[in] RegisterType Type of the register to program
> + @param[in] Index Index of the register to program
> + @param[in] Value Value to write
> +
> + @note This service could be called by BSP only.
> +**/
> +#define CPU_REGISTER_TABLE_WRITE64(ProcessorNumber, RegisterType, Index, Value) \
> + do { \
> + CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, MAX_UINT64, Value, FALSE); \
> } while(FALSE);
>
> /**
> @@ -403,9 +441,9 @@ PreSmmCpuRegisterTableWrite (
>
> @note This service could be called by BSP only.
> **/
> -#define CPU_REGISTER_TABLE_WRITE64(ProcessorNumber, RegisterType, Index, Value) \
> - do { \
> - CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, MAX_UINT64, Value); \
> +#define CPU_REGISTER_TABLE_TEST_THEN_WRITE64(ProcessorNumber, RegisterType, Index, Value) \
> + do { \
> + CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, MAX_UINT64, Value, TRUE); \
> } while(FALSE);
>
> /**
> @@ -428,7 +466,30 @@ PreSmmCpuRegisterTableWrite (
> UINT64 ValueMask; \
> ValueMask = MAX_UINT64; \
> ((Type *)(&ValueMask))->Field = 0; \
> - CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, ~ValueMask, Value); \
> + CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, ~ValueMask, Value, FALSE); \
> + } while(FALSE);
> +
> +/**
> + Adds a bit field write entry in specified register table.
> +
> + This macro adds an entry in specified register table, with given register type,
> + register index, bit field section, and value.
> +
> + @param[in] ProcessorNumber The index of the CPU to add a register table entry.
> + @param[in] RegisterType Type of the register to program.
> + @param[in] Index Index of the register to program.
> + @param[in] Type The data type name of a register structure.
> + @param[in] Field The bit fiel name in register structure to write.
> + @param[in] Value Value to write to the bit field.
> +
> + @note This service could be called by BSP only.
> +**/
> +#define CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD(ProcessorNumber, RegisterType, Index, Type, Field, Value) \
> + do { \
> + UINT64 ValueMask; \
> + ValueMask = MAX_UINT64; \
> + ((Type *)(&ValueMask))->Field = 0; \
> + CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index, ~ValueMask, Value, TRUE); \
> } while(FALSE);
>
> /**
> diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> index 67885bf69b..152ab75988 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> @@ -1025,6 +1025,8 @@ EnlargeRegisterTable (
> @param[in] ValidBitStart Start of the bit section
> @param[in] ValidBitLength Length of the bit section
> @param[in] Value Value to write
> + @param[in] DetectIt Whether need to detect current Value before writing.
> +
> **/
> VOID
> CpuRegisterTableWriteWorker (
> @@ -1034,7 +1036,8 @@ CpuRegisterTableWriteWorker (
> IN UINT64 Index,
> IN UINT8 ValidBitStart,
> IN UINT8 ValidBitLength,
> - IN UINT64 Value
> + IN UINT64 Value,
> + IN UINT8 DetectIt
> )
> {
> CPU_FEATURES_DATA *CpuFeaturesData;
> @@ -1070,6 +1073,7 @@ CpuRegisterTableWriteWorker (
> RegisterTableEntry[RegisterTable->TableLength].ValidBitStart = ValidBitStart;
> RegisterTableEntry[RegisterTable->TableLength].ValidBitLength = ValidBitLength;
> RegisterTableEntry[RegisterTable->TableLength].Value = Value;
> + RegisterTableEntry[RegisterTable->TableLength].DetectIt = DetectIt;
>
> RegisterTable->TableLength++;
> }
> @@ -1085,6 +1089,7 @@ CpuRegisterTableWriteWorker (
> @param[in] Index Index of the register to program
> @param[in] ValueMask Mask of bits in register to write
> @param[in] Value Value to write
> + @param[in] DetectIt Whether need to detect current Value before writing.
>
> @note This service could be called by BSP only.
> **/
> @@ -1095,7 +1100,8 @@ CpuRegisterTableWrite (
> IN REGISTER_TYPE RegisterType,
> IN UINT64 Index,
> IN UINT64 ValueMask,
> - IN UINT64 Value
> + IN UINT64 Value,
> + IN UINT8 DetectIt
> )
> {
> UINT8 Start;
> @@ -1105,7 +1111,7 @@ CpuRegisterTableWrite (
> Start = (UINT8)LowBitSet64 (ValueMask);
> End = (UINT8)HighBitSet64 (ValueMask);
> Length = End - Start + 1;
> - CpuRegisterTableWriteWorker (FALSE, ProcessorNumber, RegisterType, Index, Start, Length, Value);
> + CpuRegisterTableWriteWorker (FALSE, ProcessorNumber, RegisterType, Index, Start, Length, Value, DetectIt);
> }
>
> /**
> @@ -1139,7 +1145,7 @@ PreSmmCpuRegisterTableWrite (
> Start = (UINT8)LowBitSet64 (ValueMask);
> End = (UINT8)HighBitSet64 (ValueMask);
> Length = End - Start + 1;
> - CpuRegisterTableWriteWorker (TRUE, ProcessorNumber, RegisterType, Index, Start, Length, Value);
> + CpuRegisterTableWriteWorker (TRUE, ProcessorNumber, RegisterType, Index, Start, Length, Value, FALSE);
> }
>
> /**
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 3/4] UefiCpuPkg/PiSmmCpuDxeSmm: Supports detect before set new value logic.
2019-08-09 6:11 ` [Patch 3/4] UefiCpuPkg/PiSmmCpuDxeSmm: " Dong, Eric
@ 2019-08-09 15:30 ` Laszlo Ersek
2019-08-12 8:37 ` [edk2-devel] " Dong, Eric
0 siblings, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2019-08-09 15:30 UTC (permalink / raw)
To: Eric Dong, devel; +Cc: Ray Ni
On 08/09/19 08:11, Eric Dong wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2040
>
> Supports new logic which detect current value before set new value.
> Only set new value when current value not same as new value.
>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 135 ++++++++++++++++++++----------
> 1 file changed, 92 insertions(+), 43 deletions(-)
I have only superficial comments, as my understanding is that
"UefiCpuPkg/CpuS3DataDxe", which is what OVMF uses, doesn't set up any
register programming for S3 resume.
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> index d8c6b19ead..957f2896eb 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> @@ -159,6 +159,58 @@ S3WaitForSemaphore (
> ) != Value);
> }
>
> +/**
> + Read / write CR value.
> +
> + @param[in] CrIndex The CR index which need to read/write.
> + @param[in] Read Read or write. TRUE is read.
> + @param[in,out] CrValue CR value.
> +
> + @retval EFI_SUCCESS means read/write success, else return EFI_UNSUPPORTED.
> +**/
> +UINTN
> +ReadWriteCr (
> + IN UINT32 CrIndex,
> + IN BOOLEAN Read,
> + IN OUT UINTN *CrValue
> + )
> +{
> + switch (CrIndex) {
> + case 0:
> + if (Read) {
> + *CrValue = AsmReadCr0 ();
> + } else {
> + AsmWriteCr0 (*CrValue);
> + }
> + break;
> + case 2:
> + if (Read) {
> + *CrValue = AsmReadCr2 ();
> + } else {
> + AsmWriteCr2 (*CrValue);
> + }
> + break;
> + case 3:
> + if (Read) {
> + *CrValue = AsmReadCr3 ();
> + } else {
> + AsmWriteCr3 (*CrValue);
> + }
> + break;
> + case 4:
> + if (Read) {
> + *CrValue = AsmReadCr4 ();
> + } else {
> + AsmWriteCr4 (*CrValue);
> + }
> + break;
> + default:
> + return EFI_UNSUPPORTED;;
> + }
> +
> + return EFI_SUCCESS;
> +}
> +
> /**
> Initialize the CPU registers from a register table.
>
> @@ -188,6 +240,8 @@ ProgramProcessorRegister (
> UINTN ProcessorIndex;
> UINTN ValidThreadCount;
> UINT32 *ValidCoreCountPerPackage;
> + EFI_STATUS Status;
> + UINT64 CurrentValue;
>
> //
> // Traverse Register Table of this logical processor
> @@ -206,55 +260,50 @@ ProgramProcessorRegister (
> // The specified register is Control Register
> //
> case ControlRegister:
> - switch (RegisterTableEntry->Index) {
> - case 0:
> - Value = AsmReadCr0 ();
> - Value = (UINTN) BitFieldWrite64 (
> - Value,
> - RegisterTableEntry->ValidBitStart,
> - RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1,
> - (UINTN) RegisterTableEntry->Value
> - );
> - AsmWriteCr0 (Value);
> - break;
> - case 2:
> - Value = AsmReadCr2 ();
> - Value = (UINTN) BitFieldWrite64 (
> - Value,
> - RegisterTableEntry->ValidBitStart,
> - RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1,
> - (UINTN) RegisterTableEntry->Value
> - );
> - AsmWriteCr2 (Value);
> - break;
> - case 3:
> - Value = AsmReadCr3 ();
> - Value = (UINTN) BitFieldWrite64 (
> - Value,
> - RegisterTableEntry->ValidBitStart,
> - RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1,
> - (UINTN) RegisterTableEntry->Value
> - );
> - AsmWriteCr3 (Value);
> - break;
> - case 4:
> - Value = AsmReadCr4 ();
> - Value = (UINTN) BitFieldWrite64 (
> - Value,
> - RegisterTableEntry->ValidBitStart,
> - RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1,
> - (UINTN) RegisterTableEntry->Value
> - );
> - AsmWriteCr4 (Value);
> - break;
> - default:
> - break;
> + Status = ReadWriteCr(RegisterTableEntry->Index, TRUE, &Value);
(1) Space missing right after "ReadWriteCr".
> + if (EFI_ERROR (Status)) {
> + return;
> + }
(2) This changes the control flow.
Previously, a CR reference different from CR0, CR2, CR3, and CR4 would
allow the loop to process further entries from the register table.
This change could be justified, but then it needs to be in a separate patch.
> + if (RegisterTableEntry->DetectIt) {
> + CurrentValue = BitFieldRead64(
> + Value,
> + RegisterTableEntry->ValidBitStart,
> + RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1
> + );
> + if (CurrentValue == RegisterTableEntry->Value) {
> + return;
> + }
(3) Same comment here -- if the value retrieved from the recognized
register diverges from the expected value, is that grounds enough for
terminating the processing?
I'd suggest splitting up this patch.
- One patch could be factoring out ReadWriteCr(), without changes in
functionality.
- Another patch could be the early return, if that is not a bug in the
present patch, but an intended change.
- Another patch could be the Compare-And-Set logic.
> }
> + Value = (UINTN) BitFieldWrite64 (
> + Value,
> + RegisterTableEntry->ValidBitStart,
> + RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1,
> + RegisterTableEntry->Value
> + );
> + ReadWriteCr (RegisterTableEntry->Index, FALSE, &Value);
> break;
> //
> // The specified register is Model Specific Register
> //
> case Msr:
> + if (RegisterTableEntry->DetectIt) {
> + Value = (UINTN)AsmReadMsr64 (RegisterTableEntry->Index);
> + if (RegisterTableEntry->ValidBitLength >= 64) {
> + if (Value == RegisterTableEntry->Value) {
> + return;
> + }
> + } else {
> + CurrentValue = BitFieldRead64(
> + Value,
> + RegisterTableEntry->ValidBitStart,
> + RegisterTableEntry->ValidBitStart + RegisterTableEntry->ValidBitLength - 1
> + );
> + if (CurrentValue == RegisterTableEntry->Value) {
> + return;
> + }
> + }
> + }
> +
> //
> // If this function is called to restore register setting after INIT signal,
> // there is no need to restore MSRs in register table.
>
(4) "early return" issue again -- if the MSR has the intended value
already, that's likely no reason for ignoring the rest of the register
table. I guess the "continue" statement could be useful.
(5) I would suggest splitting the MSR update to a separate patch as well.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib: Add "detect before set" Micros.
2019-08-09 6:11 ` [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib: Add "detect before set" Micros Dong, Eric
2019-08-09 15:14 ` Laszlo Ersek
@ 2019-08-09 15:31 ` Laszlo Ersek
2019-08-12 8:05 ` Dong, Eric
1 sibling, 1 reply; 12+ messages in thread
From: Laszlo Ersek @ 2019-08-09 15:31 UTC (permalink / raw)
To: Eric Dong, devel; +Cc: Ray Ni
On 08/09/19 08:11, Eric Dong wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2040
>
> Add below new micros which test the current value before set
Both the subject line and the commit message should say "macros", not
"micros".
Thanks
Laszlo
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib: Add "detect before set" Micros.
2019-08-09 15:14 ` Laszlo Ersek
@ 2019-08-12 7:46 ` Dong, Eric
2019-08-12 13:01 ` Laszlo Ersek
0 siblings, 1 reply; 12+ messages in thread
From: Dong, Eric @ 2019-08-12 7:46 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com; +Cc: Ni, Ray
Hi Laszlo,
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Friday, August 9, 2019 11:14 PM
> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib:
> Add "detect before set" Micros.
>
> On 08/09/19 08:11, Eric Dong wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2040
> >
> > Add below new micros which test the current value before set the new
> > value. Only set new value when current value not same as new value.
> > CPU_REGISTER_TABLE_TEST_THEN_WRITE32
> > CPU_REGISTER_TABLE_TEST_THEN_WRITE64
> > CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD
> >
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > ---
> > UefiCpuPkg/Include/AcpiCpuData.h | 1 +
> > .../Include/Library/RegisterCpuFeaturesLib.h | 77 +++++++++++++++++--
> > .../RegisterCpuFeaturesLib.c | 14 +++-
> > 3 files changed, 80 insertions(+), 12 deletions(-)
>
> (1) When you format your patch sets, can you please use the following two
> options:
>
> --stat=1000 --stat-graph-width=20
>
> Otherwise the diffstats are truncated (on the left) and hard to understand.
>
1. I use TortoiseGit to create patch, do you know how to enable these setting in TortoiseGit?
> >
> > diff --git a/UefiCpuPkg/Include/AcpiCpuData.h
> > b/UefiCpuPkg/Include/AcpiCpuData.h
> > index b963a2f592..c764e209cf 100644
> > --- a/UefiCpuPkg/Include/AcpiCpuData.h
> > +++ b/UefiCpuPkg/Include/AcpiCpuData.h
> > @@ -81,6 +81,7 @@ typedef struct {
> > UINT16 Reserved; // offset 10 - 11
> > UINT32 HighIndex; // offset 12-15, only valid for
> MemoryMapped
> > UINT64 Value; // offset 16-23
> > + UINT8 DetectIt; // 0ffset 24
> > } CPU_REGISTER_TABLE_ENTRY;
>
> (2) Another quite generic comment -- "DetectIt" does not look helpful.
> Somehow the verb "detect" does not communicate the right action to me.
>
> How about using a more established name, such as:
>
> - CompareAndSwap
> - CompareAndSet
> - TestAndSet
>
> ?
>
> If you agree, then I suggest updating the parameter names, and their
> comments too, below.
2. Yes, I change from "TestIt" to "DetectIt" because I think it's better. Seems like this is not a correct change.
I will use "TestThenWrite" as the new name which align our new macro " CPU_REGISTER_TABLE_TEST_THEN_WRITE****". What do you think?
Thanks,
Eric
>
> Thanks
> Laszlo
>
>
> >
> > //
> > diff --git a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> > b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> > index e420e7f075..87fe87acb7 100644
> > --- a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> > +++ b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> > @@ -335,6 +335,7 @@ SwitchBspAfterFeaturesInitialize (
> > @param[in] Index Index of the register to program
> > @param[in] ValueMask Mask of bits in register to write
> > @param[in] Value Value to write
> > + @param[in] DetectIt Whether need to detect current Value before
> writing.
> >
> > @note This service could be called by BSP only.
> > **/
> > @@ -345,7 +346,8 @@ CpuRegisterTableWrite (
> > IN REGISTER_TYPE RegisterType,
> > IN UINT64 Index,
> > IN UINT64 ValueMask,
> > - IN UINT64 Value
> > + IN UINT64 Value,
> > + IN UINT8 DetectIt
> > );
> >
> > /**
> > @@ -385,9 +387,45 @@ PreSmmCpuRegisterTableWrite (
> >
> > @note This service could be called by BSP only.
> > **/
> > -#define CPU_REGISTER_TABLE_WRITE32(ProcessorNumber, RegisterType,
> Index, Value) \
> > - do { \
> > - CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index,
> MAX_UINT32, Value); \
> > +#define CPU_REGISTER_TABLE_WRITE32(ProcessorNumber,
> RegisterType, Index, Value) \
> > + do { \
> > + CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index,
> > +MAX_UINT32, Value, FALSE); \
> > + } while(FALSE);
> > +
> > +/**
> > + Adds a 32-bit register write entry in specified register table.
> > +
> > + This macro adds an entry in specified register table, with given
> > + register type, register index, and value.
> > +
> > + @param[in] ProcessorNumber The index of the CPU to add a register
> table entry.
> > + @param[in] RegisterType Type of the register to program
> > + @param[in] Index Index of the register to program
> > + @param[in] Value Value to write
> > +
> > + @note This service could be called by BSP only.
> > +**/
> > +#define CPU_REGISTER_TABLE_TEST_THEN_WRITE32(ProcessorNumber,
> RegisterType, Index, Value) \
> > + do { \
> > + CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index,
> > +MAX_UINT32, Value, TRUE); \
> > + } while(FALSE);
> > +
> > +/**
> > + Adds a 64-bit register write entry in specified register table.
> > +
> > + This macro adds an entry in specified register table, with given
> > + register type, register index, and value.
> > +
> > + @param[in] ProcessorNumber The index of the CPU to add a register
> table entry.
> > + @param[in] RegisterType Type of the register to program
> > + @param[in] Index Index of the register to program
> > + @param[in] Value Value to write
> > +
> > + @note This service could be called by BSP only.
> > +**/
> > +#define CPU_REGISTER_TABLE_WRITE64(ProcessorNumber,
> RegisterType, Index, Value) \
> > + do { \
> > + CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index,
> > +MAX_UINT64, Value, FALSE); \
> > } while(FALSE);
> >
> > /**
> > @@ -403,9 +441,9 @@ PreSmmCpuRegisterTableWrite (
> >
> > @note This service could be called by BSP only.
> > **/
> > -#define CPU_REGISTER_TABLE_WRITE64(ProcessorNumber, RegisterType,
> Index, Value) \
> > - do { \
> > - CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index,
> MAX_UINT64, Value); \
> > +#define CPU_REGISTER_TABLE_TEST_THEN_WRITE64(ProcessorNumber,
> RegisterType, Index, Value) \
> > + do { \
> > + CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index,
> > +MAX_UINT64, Value, TRUE); \
> > } while(FALSE);
> >
> > /**
> > @@ -428,7 +466,30 @@ PreSmmCpuRegisterTableWrite (
> > UINT64 ValueMask; \
> > ValueMask = MAX_UINT64; \
> > ((Type *)(&ValueMask))->Field = 0; \
> > - CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index,
> ~ValueMask, Value); \
> > + CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index,
> ~ValueMask, Value, FALSE); \
> > + } while(FALSE);
> > +
> > +/**
> > + Adds a bit field write entry in specified register table.
> > +
> > + This macro adds an entry in specified register table, with given
> > + register type, register index, bit field section, and value.
> > +
> > + @param[in] ProcessorNumber The index of the CPU to add a register
> table entry.
> > + @param[in] RegisterType Type of the register to program.
> > + @param[in] Index Index of the register to program.
> > + @param[in] Type The data type name of a register structure.
> > + @param[in] Field The bit fiel name in register structure to write.
> > + @param[in] Value Value to write to the bit field.
> > +
> > + @note This service could be called by BSP only.
> > +**/
> > +#define
> CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD(ProcessorNumber,
> RegisterType, Index, Type, Field, Value) \
> > + do { \
> > + UINT64 ValueMask; \
> > + ValueMask = MAX_UINT64; \
> > + ((Type *)(&ValueMask))->Field = 0; \
> > + CpuRegisterTableWrite (ProcessorNumber, RegisterType, Index,
> ~ValueMask, Value, TRUE); \
> > } while(FALSE);
> >
> > /**
> > diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > index 67885bf69b..152ab75988 100644
> > ---
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > +++
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib
> > +++ .c
> > @@ -1025,6 +1025,8 @@ EnlargeRegisterTable (
> > @param[in] ValidBitStart Start of the bit section
> > @param[in] ValidBitLength Length of the bit section
> > @param[in] Value Value to write
> > + @param[in] DetectIt Whether need to detect current Value before
> writing.
> > +
> > **/
> > VOID
> > CpuRegisterTableWriteWorker (
> > @@ -1034,7 +1036,8 @@ CpuRegisterTableWriteWorker (
> > IN UINT64 Index,
> > IN UINT8 ValidBitStart,
> > IN UINT8 ValidBitLength,
> > - IN UINT64 Value
> > + IN UINT64 Value,
> > + IN UINT8 DetectIt
> > )
> > {
> > CPU_FEATURES_DATA *CpuFeaturesData;
> > @@ -1070,6 +1073,7 @@ CpuRegisterTableWriteWorker (
> > RegisterTableEntry[RegisterTable->TableLength].ValidBitStart =
> ValidBitStart;
> > RegisterTableEntry[RegisterTable->TableLength].ValidBitLength =
> ValidBitLength;
> > RegisterTableEntry[RegisterTable->TableLength].Value = Value;
> > + RegisterTableEntry[RegisterTable->TableLength].DetectIt = DetectIt;
> >
> > RegisterTable->TableLength++;
> > }
> > @@ -1085,6 +1089,7 @@ CpuRegisterTableWriteWorker (
> > @param[in] Index Index of the register to program
> > @param[in] ValueMask Mask of bits in register to write
> > @param[in] Value Value to write
> > + @param[in] DetectIt Whether need to detect current Value before
> writing.
> >
> > @note This service could be called by BSP only.
> > **/
> > @@ -1095,7 +1100,8 @@ CpuRegisterTableWrite (
> > IN REGISTER_TYPE RegisterType,
> > IN UINT64 Index,
> > IN UINT64 ValueMask,
> > - IN UINT64 Value
> > + IN UINT64 Value,
> > + IN UINT8 DetectIt
> > )
> > {
> > UINT8 Start;
> > @@ -1105,7 +1111,7 @@ CpuRegisterTableWrite (
> > Start = (UINT8)LowBitSet64 (ValueMask);
> > End = (UINT8)HighBitSet64 (ValueMask);
> > Length = End - Start + 1;
> > - CpuRegisterTableWriteWorker (FALSE, ProcessorNumber, RegisterType,
> > Index, Start, Length, Value);
> > + CpuRegisterTableWriteWorker (FALSE, ProcessorNumber, RegisterType,
> > + Index, Start, Length, Value, DetectIt);
> > }
> >
> > /**
> > @@ -1139,7 +1145,7 @@ PreSmmCpuRegisterTableWrite (
> > Start = (UINT8)LowBitSet64 (ValueMask);
> > End = (UINT8)HighBitSet64 (ValueMask);
> > Length = End - Start + 1;
> > - CpuRegisterTableWriteWorker (TRUE, ProcessorNumber, RegisterType,
> > Index, Start, Length, Value);
> > + CpuRegisterTableWriteWorker (TRUE, ProcessorNumber, RegisterType,
> > + Index, Start, Length, Value, FALSE);
> > }
> >
> > /**
> >
>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib: Add "detect before set" Micros.
2019-08-09 15:31 ` Laszlo Ersek
@ 2019-08-12 8:05 ` Dong, Eric
0 siblings, 0 replies; 12+ messages in thread
From: Dong, Eric @ 2019-08-12 8:05 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io; +Cc: Ni, Ray
Hi Laszlo,
Will fix it in my next version change.
Thanks,
Eric
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Friday, August 9, 2019 11:31 PM
> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: Re: [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib: Add "detect before
> set" Micros.
>
> On 08/09/19 08:11, Eric Dong wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2040
> >
> > Add below new micros which test the current value before set
>
> Both the subject line and the commit message should say "macros", not
> "micros".
>
> Thanks
> Laszlo
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [Patch 3/4] UefiCpuPkg/PiSmmCpuDxeSmm: Supports detect before set new value logic.
2019-08-09 15:30 ` Laszlo Ersek
@ 2019-08-12 8:37 ` Dong, Eric
0 siblings, 0 replies; 12+ messages in thread
From: Dong, Eric @ 2019-08-12 8:37 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com; +Cc: Ni, Ray
Hi Laszlo,
> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Laszlo Ersek
> Sent: Friday, August 9, 2019 11:31 PM
> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: Re: [edk2-devel] [Patch 3/4] UefiCpuPkg/PiSmmCpuDxeSmm:
> Supports detect before set new value logic.
>
> On 08/09/19 08:11, Eric Dong wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2040
> >
> > Supports new logic which detect current value before set new value.
> > Only set new value when current value not same as new value.
> >
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > ---
> > UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 135
> > ++++++++++++++++++++----------
> > 1 file changed, 92 insertions(+), 43 deletions(-)
>
> I have only superficial comments, as my understanding is that
> "UefiCpuPkg/CpuS3DataDxe", which is what OVMF uses, doesn't set up any
> register programming for S3 resume.
>
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > index d8c6b19ead..957f2896eb 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c
> > @@ -159,6 +159,58 @@ S3WaitForSemaphore (
> > ) != Value);
> > }
> >
> > +/**
> > + Read / write CR value.
> > +
> > + @param[in] CrIndex The CR index which need to read/write.
> > + @param[in] Read Read or write. TRUE is read.
> > + @param[in,out] CrValue CR value.
> > +
> > + @retval EFI_SUCCESS means read/write success, else return
> EFI_UNSUPPORTED.
> > +**/
> > +UINTN
> > +ReadWriteCr (
> > + IN UINT32 CrIndex,
> > + IN BOOLEAN Read,
> > + IN OUT UINTN *CrValue
> > + )
> > +{
> > + switch (CrIndex) {
> > + case 0:
> > + if (Read) {
> > + *CrValue = AsmReadCr0 ();
> > + } else {
> > + AsmWriteCr0 (*CrValue);
> > + }
> > + break;
> > + case 2:
> > + if (Read) {
> > + *CrValue = AsmReadCr2 ();
> > + } else {
> > + AsmWriteCr2 (*CrValue);
> > + }
> > + break;
> > + case 3:
> > + if (Read) {
> > + *CrValue = AsmReadCr3 ();
> > + } else {
> > + AsmWriteCr3 (*CrValue);
> > + }
> > + break;
> > + case 4:
> > + if (Read) {
> > + *CrValue = AsmReadCr4 ();
> > + } else {
> > + AsmWriteCr4 (*CrValue);
> > + }
> > + break;
> > + default:
> > + return EFI_UNSUPPORTED;;
> > + }
> > +
> > + return EFI_SUCCESS;
> > +}
> > +
> > /**
> > Initialize the CPU registers from a register table.
> >
> > @@ -188,6 +240,8 @@ ProgramProcessorRegister (
> > UINTN ProcessorIndex;
> > UINTN ValidThreadCount;
> > UINT32 *ValidCoreCountPerPackage;
> > + EFI_STATUS Status;
> > + UINT64 CurrentValue;
> >
> > //
> > // Traverse Register Table of this logical processor @@ -206,55
> > +260,50 @@ ProgramProcessorRegister (
> > // The specified register is Control Register
> > //
> > case ControlRegister:
> > - switch (RegisterTableEntry->Index) {
> > - case 0:
> > - Value = AsmReadCr0 ();
> > - Value = (UINTN) BitFieldWrite64 (
> > - Value,
> > - RegisterTableEntry->ValidBitStart,
> > - RegisterTableEntry->ValidBitStart + RegisterTableEntry-
> >ValidBitLength - 1,
> > - (UINTN) RegisterTableEntry->Value
> > - );
> > - AsmWriteCr0 (Value);
> > - break;
> > - case 2:
> > - Value = AsmReadCr2 ();
> > - Value = (UINTN) BitFieldWrite64 (
> > - Value,
> > - RegisterTableEntry->ValidBitStart,
> > - RegisterTableEntry->ValidBitStart + RegisterTableEntry-
> >ValidBitLength - 1,
> > - (UINTN) RegisterTableEntry->Value
> > - );
> > - AsmWriteCr2 (Value);
> > - break;
> > - case 3:
> > - Value = AsmReadCr3 ();
> > - Value = (UINTN) BitFieldWrite64 (
> > - Value,
> > - RegisterTableEntry->ValidBitStart,
> > - RegisterTableEntry->ValidBitStart + RegisterTableEntry-
> >ValidBitLength - 1,
> > - (UINTN) RegisterTableEntry->Value
> > - );
> > - AsmWriteCr3 (Value);
> > - break;
> > - case 4:
> > - Value = AsmReadCr4 ();
> > - Value = (UINTN) BitFieldWrite64 (
> > - Value,
> > - RegisterTableEntry->ValidBitStart,
> > - RegisterTableEntry->ValidBitStart + RegisterTableEntry-
> >ValidBitLength - 1,
> > - (UINTN) RegisterTableEntry->Value
> > - );
> > - AsmWriteCr4 (Value);
> > - break;
> > - default:
> > - break;
> > + Status = ReadWriteCr(RegisterTableEntry->Index, TRUE, &Value);
>
> (1) Space missing right after "ReadWriteCr".
1. Will fix it in my next version change.
>
> > + if (EFI_ERROR (Status)) {
> > + return;
> > + }
>
> (2) This changes the control flow.
>
> Previously, a CR reference different from CR0, CR2, CR3, and CR4 would allow
> the loop to process further entries from the register table.
>
> This change could be justified, but then it needs to be in a separate patch.
2. Good catch, this is not an expect change. Should use "continue" for it. Will update it in my next change.
>
> > + if (RegisterTableEntry->DetectIt) {
> > + CurrentValue = BitFieldRead64(
> > + Value,
> > + RegisterTableEntry->ValidBitStart,
> > + RegisterTableEntry->ValidBitStart + RegisterTableEntry-
> >ValidBitLength - 1
> > + );
> > + if (CurrentValue == RegisterTableEntry->Value) {
> > + return;
> > + }
>
> (3) Same comment here -- if the value retrieved from the recognized register
> diverges from the expected value, is that grounds enough for terminating
> the processing?
3. Good catch, here should also use "continue". Will update it in my next patch.
>
> I'd suggest splitting up this patch.
>
> - One patch could be factoring out ReadWriteCr(), without changes in
> functionality.
>
> - Another patch could be the early return, if that is not a bug in the present
> patch, but an intended change.
>
> - Another patch could be the Compare-And-Set logic.
4. I will generate the factoring out of ReadWriteCr change to a separate one in my next version changes.
>
> > }
> > + Value = (UINTN) BitFieldWrite64 (
> > + Value,
> > + RegisterTableEntry->ValidBitStart,
> > + RegisterTableEntry->ValidBitStart + RegisterTableEntry-
> >ValidBitLength - 1,
> > + RegisterTableEntry->Value
> > + );
> > + ReadWriteCr (RegisterTableEntry->Index, FALSE, &Value);
> > break;
> > //
> > // The specified register is Model Specific Register
> > //
> > case Msr:
> > + if (RegisterTableEntry->DetectIt) {
> > + Value = (UINTN)AsmReadMsr64 (RegisterTableEntry->Index);
> > + if (RegisterTableEntry->ValidBitLength >= 64) {
> > + if (Value == RegisterTableEntry->Value) {
> > + return;
> > + }
> > + } else {
> > + CurrentValue = BitFieldRead64(
> > + Value,
> > + RegisterTableEntry->ValidBitStart,
> > + RegisterTableEntry->ValidBitStart + RegisterTableEntry-
> >ValidBitLength - 1
> > + );
> > + if (CurrentValue == RegisterTableEntry->Value) {
> > + return;
> > + }
> > + }
> > + }
> > +
> > //
> > // If this function is called to restore register setting after INIT signal,
> > // there is no need to restore MSRs in register table.
> >
>
> (4) "early return" issue again -- if the MSR has the intended value already,
> that's likely no reason for ignoring the rest of the register table. I guess the
> "continue" statement could be useful.
5. yes, it should use "continue" for this case, will update it in my next version change.
>
> (5) I would suggest splitting the MSR update to a separate patch as well.
6. The logic should use "continue" instead of "return". This is follow the original logic. So I will not separate the change to different patches.
Thanks,
Eric
>
> Thanks
> Laszlo
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib: Add "detect before set" Micros.
2019-08-12 7:46 ` [edk2-devel] " Dong, Eric
@ 2019-08-12 13:01 ` Laszlo Ersek
0 siblings, 0 replies; 12+ messages in thread
From: Laszlo Ersek @ 2019-08-12 13:01 UTC (permalink / raw)
To: Dong, Eric, devel@edk2.groups.io; +Cc: Ni, Ray
On 08/12/19 09:46, Dong, Eric wrote:
> Hi Laszlo,
>
>> -----Original Message-----
>> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
>> Laszlo Ersek
>> Sent: Friday, August 9, 2019 11:14 PM
>> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
>> Cc: Ni, Ray <ray.ni@intel.com>
>> Subject: Re: [edk2-devel] [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib:
>> Add "detect before set" Micros.
>>
>> On 08/09/19 08:11, Eric Dong wrote:
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2040
>>>
>>> Add below new micros which test the current value before set the new
>>> value. Only set new value when current value not same as new value.
>>> CPU_REGISTER_TABLE_TEST_THEN_WRITE32
>>> CPU_REGISTER_TABLE_TEST_THEN_WRITE64
>>> CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD
>>>
>>> Signed-off-by: Eric Dong <eric.dong@intel.com>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>> UefiCpuPkg/Include/AcpiCpuData.h | 1 +
>>> .../Include/Library/RegisterCpuFeaturesLib.h | 77 +++++++++++++++++--
>>> .../RegisterCpuFeaturesLib.c | 14 +++-
>>> 3 files changed, 80 insertions(+), 12 deletions(-)
>>
>> (1) When you format your patch sets, can you please use the following two
>> options:
>>
>> --stat=1000 --stat-graph-width=20
>>
>> Otherwise the diffstats are truncated (on the left) and hard to understand.
>>
>
> 1. I use TortoiseGit to create patch, do you know how to enable these setting in TortoiseGit?
Sorry, no clue :(
>
>>>
>>> diff --git a/UefiCpuPkg/Include/AcpiCpuData.h
>>> b/UefiCpuPkg/Include/AcpiCpuData.h
>>> index b963a2f592..c764e209cf 100644
>>> --- a/UefiCpuPkg/Include/AcpiCpuData.h
>>> +++ b/UefiCpuPkg/Include/AcpiCpuData.h
>>> @@ -81,6 +81,7 @@ typedef struct {
>>> UINT16 Reserved; // offset 10 - 11
>>> UINT32 HighIndex; // offset 12-15, only valid for
>> MemoryMapped
>>> UINT64 Value; // offset 16-23
>>> + UINT8 DetectIt; // 0ffset 24
>>> } CPU_REGISTER_TABLE_ENTRY;
>>
>> (2) Another quite generic comment -- "DetectIt" does not look helpful.
>> Somehow the verb "detect" does not communicate the right action to me.
>>
>> How about using a more established name, such as:
>>
>> - CompareAndSwap
>> - CompareAndSet
>> - TestAndSet
>>
>> ?
>>
>> If you agree, then I suggest updating the parameter names, and their
>> comments too, below.
>
> 2. Yes, I change from "TestIt" to "DetectIt" because I think it's better. Seems like this is not a correct change.
> I will use "TestThenWrite" as the new name which align our new macro " CPU_REGISTER_TABLE_TEST_THEN_WRITE****". What do you think?
Sounds good, thanks.
Laszlo
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-08-12 13:01 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-08-09 6:11 [Patch 0/4] Add "Test then Set" mechanism Dong, Eric
2019-08-09 6:11 ` [Patch 1/4] UefiCpuPkg/RegisterCpuFeaturesLib: Add "detect before set" Micros Dong, Eric
2019-08-09 15:14 ` Laszlo Ersek
2019-08-12 7:46 ` [edk2-devel] " Dong, Eric
2019-08-12 13:01 ` Laszlo Ersek
2019-08-09 15:31 ` Laszlo Ersek
2019-08-12 8:05 ` Dong, Eric
2019-08-09 6:11 ` [Patch 2/4] UefiCpuPkg/RegisterCpuFeaturesLib: Supports detect before set new value logic Dong, Eric
2019-08-09 6:11 ` [Patch 3/4] UefiCpuPkg/PiSmmCpuDxeSmm: " Dong, Eric
2019-08-09 15:30 ` Laszlo Ersek
2019-08-12 8:37 ` [edk2-devel] " Dong, Eric
2019-08-09 6:11 ` [Patch 4/4] UefiCpuPkg/CpuCommonFeaturesLib: Use "Test then set" action Dong, Eric
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox