Re: [Patch 3/4] UefiCpuPkg/PiSmmCpuDxeSmm: Supports detect before set new value logic.

["Laszlo Ersek" <lersek@redhat.com>]

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