From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.93, mailfrom: ray.ni@intel.com) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by groups.io with SMTP; Mon, 19 Aug 2019 15:57:32 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 Aug 2019 15:57:31 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,406,1559545200"; d="scan'208";a="195635726" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga001.fm.intel.com with ESMTP; 19 Aug 2019 15:57:31 -0700 Received: from fmsmsx155.amr.corp.intel.com (10.18.116.71) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 19 Aug 2019 15:57:31 -0700 Received: from shsmsx105.ccr.corp.intel.com (10.239.4.158) by FMSMSX155.amr.corp.intel.com (10.18.116.71) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 19 Aug 2019 15:57:31 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.112]) by SHSMSX105.ccr.corp.intel.com ([169.254.11.15]) with mapi id 14.03.0439.000; Tue, 20 Aug 2019 06:57:29 +0800 From: "Ni, Ray" To: "Dong, Eric" , "devel@edk2.groups.io" CC: Laszlo Ersek Subject: Re: [Patch v4 0/6] Add "test then write" mechanism Thread-Topic: [Patch v4 0/6] Add "test then write" mechanism Thread-Index: AQHVU+a+PBmPs4DBqEuU6gQEUGHQxqcDGrQA Date: Mon, 19 Aug 2019 22:57:29 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C28D902@SHSMSX104.ccr.corp.intel.com> References: <20190816035730.3252-1-eric.dong@intel.com> In-Reply-To: <20190816035730.3252-1-eric.dong@intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNDFiODk3MGQtMDA0ZC00MDBlLTg3NGEtODNkNzNhOTY1M2FhIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiT2lzd09hRXprcUlGYXMwRzlnXC9NbkhuRGJHRU94cjNnT0lqeGFRWFwvRzV0R1FRcW5KXC9QUTNLblwvZk5vWHoyTGEifQ== x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Eric, The whole patch series are very clean and easy to understand. Reviewed-by: Ray Ni > -----Original Message----- > From: Dong, Eric > Sent: Thursday, August 15, 2019 8:57 PM > To: devel@edk2.groups.io > Cc: Ni, Ray ; Laszlo Ersek > Subject: [Patch v4 0/6] Add "test then write" mechanism >=20 > v4 changes: > 1. Split Reserved field and use one byte as TestThenWrite field. >=20 > v3 changes: > 1. Avoid changing exist API CpuRegisterTableWrite, add new API CpuRegiste= rTableTestThenWrite which align new adds macros. > Only 1/6 patch been changed in v3. >=20 > V2 changes: > 1. Split CR read/write action in to one discrete patch 2. Keep the old lo= gic which continue the process if error found. >=20 > Below code is current implementation: > if (MsrRegister[ProcessorNumber].Bits.Lock =3D=3D 0) { > CPU_REGISTER_TABLE_WRITE_FIELD ( > ProcessorNumber, > Msr, > MSR_IA32_FEATURE_CONTROL, > MSR_IA32_FEATURE_CONTROL_REGISTER, > Bits.Lock, > 1 > ); > } >=20 > 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 alw= ays 1 after booting or resuming from S3. >=20 > 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. >=20 > 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. >=20 > Cc: Ray Ni > Cc: Laszlo Ersek >=20 >=20 > Eric Dong (6): > UefiCpuPkg/RegisterCpuFeaturesLib: Add "Test Then Write" Macros. > UefiCpuPkg/PiSmmCpuDxeSmm: Combine CR read/write action. > UefiCpuPkg/PiSmmCpuDxeSmm: Supports test then write new value logic. > UefiCpuPkg/RegisterCpuFeaturesLib: Combine CR read/write action. > UefiCpuPkg/RegisterCpuFeaturesLib: Supports test then write new value > logic. > UefiCpuPkg/CpuCommonFeaturesLib: Use new macros. >=20 > UefiCpuPkg/Include/AcpiCpuData.h | 1 + > .../Include/Library/RegisterCpuFeaturesLib.h | 91 +++++++++++ > .../CpuCommonFeaturesLib/CpuCommonFeatures.h | 15 -- > .../CpuCommonFeaturesLib.c | 8 +- > .../CpuCommonFeaturesLib/FeatureControl.c | 141 ++++++------------ > .../CpuCommonFeaturesLib/MachineCheck.c | 23 ++- > .../CpuFeaturesInitialize.c | 139 +++++++++++------ > .../RegisterCpuFeaturesLib.c | 45 +++++- > UefiCpuPkg/PiSmmCpuDxeSmm/CpuS3.c | 133 +++++++++++------ > 9 files changed, 375 insertions(+), 221 deletions(-) >=20 > -- > 2.21.0.windows.1