From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Fri, 09 Aug 2019 08:30:42 -0700 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0B9B530031F3; Fri, 9 Aug 2019 15:30:42 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-119.ams2.redhat.com [10.36.117.119]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0036519C70; Fri, 9 Aug 2019 15:30:40 +0000 (UTC) Subject: Re: [Patch 3/4] UefiCpuPkg/PiSmmCpuDxeSmm: Supports detect before set new value logic. To: Eric Dong , devel@edk2.groups.io Cc: Ray Ni References: <20190809061159.40248-1-eric.dong@intel.com> <20190809061159.40248-4-eric.dong@intel.com> From: "Laszlo Ersek" Message-ID: <0aefcf17-4050-e3fb-86a2-f721feb67760@redhat.com> Date: Fri, 9 Aug 2019 17:30:39 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190809061159.40248-4-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.48]); Fri, 09 Aug 2019 15:30:42 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Ray Ni > Cc: Laszlo Ersek > --- > 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