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; Mon, 12 Aug 2019 07:15:30 -0700 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D0D5AC059758; Mon, 12 Aug 2019 14:15:29 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-247.ams2.redhat.com [10.36.116.247]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0CF905D6B2; Mon, 12 Aug 2019 14:15:28 +0000 (UTC) Subject: Re: [Patch v2 0/6] Add "test then write" mechanism. To: Eric Dong , devel@edk2.groups.io Cc: Ray Ni References: <20190812103152.35164-1-eric.dong@intel.com> From: "Laszlo Ersek" Message-ID: <3d4a7318-f955-f05f-e7a2-b189b2a02246@redhat.com> Date: Mon, 12 Aug 2019 16:15:28 +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: <20190812103152.35164-1-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Mon, 12 Aug 2019 14:15:29 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/12/19 12:31, Eric Dong wrote: > V2 changes: > 1. Split CR read/write action in to one discrete patch > 2. Keep the old logic which continue the process if error found. > > 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 > Cc: Laszlo Ersek > > Eric Dong (6): > UefiCpuPkg/RegisterCpuFeaturesLib: Add "Test Then Write" Macros. > UefiCpuPkg/PiSmmCpuDxeSmm: Combine CR read/write action in one > function. > UefiCpuPkg/PiSmmCpuDxeSmm: Supports test then write new value logic. > UefiCpuPkg/RegisterCpuFeaturesLib: Combine CR read/write action in one > function. > UefiCpuPkg/RegisterCpuFeaturesLib: Supports test then write new value > logic. > UefiCpuPkg/CpuCommonFeaturesLib: Use new macros. > > 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(-) > Please don't forget to build-test this series for IA32 too. Thanks Laszlo