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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#45257): https://edk2.groups.io/g/devel/message/45257
Mute This Topic: https://groups.io/mt/32807821/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#45284): https://edk2.groups.io/g/devel/message/45284
Mute This Topic: https://groups.io/mt/32807821/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#45415): https://edk2.groups.io/g/devel/message/45415
Mute This Topic: https://groups.io/mt/32807821/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.