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.d...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Laszlo Ersek <ler...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
