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; Tue, 13 Aug 2019 03:28:23 -0700 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 52A3230EA18D; Tue, 13 Aug 2019 10:28:23 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-193.ams2.redhat.com [10.36.117.193]) by smtp.corp.redhat.com (Postfix) with ESMTP id 22F8A7E10F; Tue, 13 Aug 2019 10:28:21 +0000 (UTC) Subject: Re: [edk2-devel] [Patch v2 6/6] UefiCpuPkg/CpuCommonFeaturesLib: Use new macros. To: devel@edk2.groups.io, eric.dong@intel.com Cc: Ray Ni References: <20190812103152.35164-1-eric.dong@intel.com> <20190812103152.35164-7-eric.dong@intel.com> From: "Laszlo Ersek" Message-ID: <98fc100d-429a-b60b-301b-4818d0204c40@redhat.com> Date: Tue, 13 Aug 2019 12:28:21 +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-7-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.40]); Tue, 13 Aug 2019 10:28:23 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/12/19 12:31, Dong, Eric wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2040 > > 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 > ); > } > > 1. In first 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, so it's still 0 after resume. This > is not an expect behavior. The expect value 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. > > The solution for this issue is using new added macros for the MSRs which > preserve value during warm reboot. > > Signed-off-by: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > --- > .../CpuCommonFeaturesLib/CpuCommonFeatures.h | 15 -- > .../CpuCommonFeaturesLib.c | 8 +- > .../CpuCommonFeaturesLib/FeatureControl.c | 141 ++++++------------ > .../CpuCommonFeaturesLib/MachineCheck.c | 23 ++- > 4 files changed, 58 insertions(+), 129 deletions(-) > > diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h > index 25d0174727..b2390e6c39 100644 > --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h > +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeatures.h > @@ -848,21 +848,6 @@ X2ApicInitialize ( > IN BOOLEAN State > ); > > -/** > - Prepares for the data used by CPU feature detection and initialization. > - > - @param[in] NumberOfProcessors The number of CPUs in the platform. > - > - @return Pointer to a buffer of CPU related configuration data. > - > - @note This service could be called by BSP only. > -**/ > -VOID * > -EFIAPI > -FeatureControlGetConfigData ( > - IN UINTN NumberOfProcessors > - ); > - > /** > Prepares for the data used by CPU feature detection and initialization. > > diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c > index fd43b8d662..f0dd3a3b43 100644 > --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c > +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c > @@ -91,7 +91,7 @@ CpuCommonFeaturesLibConstructor ( > if (IsCpuFeatureSupported (CPU_FEATURE_LOCK_FEATURE_CONTROL_REGISTER)) { > Status = RegisterCpuFeature ( > "Lock Feature Control Register", > - FeatureControlGetConfigData, > + NULL, > LockFeatureControlRegisterSupport, > LockFeatureControlRegisterInitialize, > CPU_FEATURE_LOCK_FEATURE_CONTROL_REGISTER, > @@ -102,7 +102,7 @@ CpuCommonFeaturesLibConstructor ( > if (IsCpuFeatureSupported (CPU_FEATURE_SMX)) { > Status = RegisterCpuFeature ( > "SMX", > - FeatureControlGetConfigData, > + NULL, > SmxSupport, > SmxInitialize, > CPU_FEATURE_SMX, > @@ -114,7 +114,7 @@ CpuCommonFeaturesLibConstructor ( > if (IsCpuFeatureSupported (CPU_FEATURE_VMX)) { > Status = RegisterCpuFeature ( > "VMX", > - FeatureControlGetConfigData, > + NULL, > VmxSupport, > VmxInitialize, > CPU_FEATURE_VMX, > @@ -214,7 +214,7 @@ CpuCommonFeaturesLibConstructor ( > if (IsCpuFeatureSupported (CPU_FEATURE_LMCE)) { > Status = RegisterCpuFeature ( > "LMCE", > - FeatureControlGetConfigData, > + NULL, > LmceSupport, > LmceInitialize, > CPU_FEATURE_LMCE, > diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/FeatureControl.c b/UefiCpuPkg/Library/CpuCommonFeaturesLib/FeatureControl.c > index 3712ef1e5c..6679df8ba4 100644 > --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/FeatureControl.c > +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/FeatureControl.c > @@ -8,28 +8,6 @@ > > #include "CpuCommonFeatures.h" > > -/** > - Prepares for the data used by CPU feature detection and initialization. > - > - @param[in] NumberOfProcessors The number of CPUs in the platform. > - > - @return Pointer to a buffer of CPU related configuration data. > - > - @note This service could be called by BSP only. > -**/ > -VOID * > -EFIAPI > -FeatureControlGetConfigData ( > - IN UINTN NumberOfProcessors > - ) > -{ > - VOID *ConfigData; > - > - ConfigData = AllocateZeroPool (sizeof (MSR_IA32_FEATURE_CONTROL_REGISTER) * NumberOfProcessors); > - ASSERT (ConfigData != NULL); > - return ConfigData; > -} > - > /** > Detects if VMX feature supported on current processor. > > @@ -54,11 +32,6 @@ VmxSupport ( > IN VOID *ConfigData OPTIONAL > ) > { > - MSR_IA32_FEATURE_CONTROL_REGISTER *MsrRegister; > - > - ASSERT (ConfigData != NULL); > - MsrRegister = (MSR_IA32_FEATURE_CONTROL_REGISTER *) ConfigData; > - MsrRegister[ProcessorNumber].Uint64 = AsmReadMsr64 (MSR_IA32_FEATURE_CONTROL); > return (CpuInfo->CpuIdVersionInfoEcx.Bits.VMX == 1); > } > > @@ -88,8 +61,6 @@ VmxInitialize ( > IN BOOLEAN State > ) > { > - MSR_IA32_FEATURE_CONTROL_REGISTER *MsrRegister; > - > // > // The scope of EnableVmxOutsideSmx bit in the MSR_IA32_FEATURE_CONTROL is core for > // below processor type, only program MSR_IA32_FEATURE_CONTROL for thread 0 in each > @@ -103,18 +74,15 @@ VmxInitialize ( > } > } > > - ASSERT (ConfigData != NULL); > - MsrRegister = (MSR_IA32_FEATURE_CONTROL_REGISTER *) ConfigData; > - if (MsrRegister[ProcessorNumber].Bits.Lock == 0) { > - CPU_REGISTER_TABLE_WRITE_FIELD ( > - ProcessorNumber, > - Msr, > - MSR_IA32_FEATURE_CONTROL, > - MSR_IA32_FEATURE_CONTROL_REGISTER, > - Bits.EnableVmxOutsideSmx, > - (State) ? 1 : 0 > - ); > - } > + CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD ( > + ProcessorNumber, > + Msr, > + MSR_IA32_FEATURE_CONTROL, > + MSR_IA32_FEATURE_CONTROL_REGISTER, > + Bits.EnableVmxOutsideSmx, > + (State) ? 1 : 0 > + ); > + > return RETURN_SUCCESS; > } > > @@ -142,11 +110,6 @@ LockFeatureControlRegisterSupport ( > IN VOID *ConfigData OPTIONAL > ) > { > - MSR_IA32_FEATURE_CONTROL_REGISTER *MsrRegister; > - > - ASSERT (ConfigData != NULL); > - MsrRegister = (MSR_IA32_FEATURE_CONTROL_REGISTER *) ConfigData; > - MsrRegister[ProcessorNumber].Uint64 = AsmReadMsr64 (MSR_IA32_FEATURE_CONTROL); > return TRUE; > } > > @@ -176,8 +139,6 @@ LockFeatureControlRegisterInitialize ( > IN BOOLEAN State > ) > { > - MSR_IA32_FEATURE_CONTROL_REGISTER *MsrRegister; > - > // > // The scope of Lock bit in the MSR_IA32_FEATURE_CONTROL is core for > // below processor type, only program MSR_IA32_FEATURE_CONTROL for thread 0 in each > @@ -191,18 +152,15 @@ LockFeatureControlRegisterInitialize ( > } > } > > - ASSERT (ConfigData != NULL); > - MsrRegister = (MSR_IA32_FEATURE_CONTROL_REGISTER *) ConfigData; > - 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 > - ); > - } > + CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD ( > + ProcessorNumber, > + Msr, > + MSR_IA32_FEATURE_CONTROL, > + MSR_IA32_FEATURE_CONTROL_REGISTER, > + Bits.Lock, > + 1 > + ); > + > return RETURN_SUCCESS; > } > > @@ -230,11 +188,6 @@ SmxSupport ( > IN VOID *ConfigData OPTIONAL > ) > { > - MSR_IA32_FEATURE_CONTROL_REGISTER *MsrRegister; > - > - ASSERT (ConfigData != NULL); > - MsrRegister = (MSR_IA32_FEATURE_CONTROL_REGISTER *) ConfigData; > - MsrRegister[ProcessorNumber].Uint64 = AsmReadMsr64 (MSR_IA32_FEATURE_CONTROL); > return (CpuInfo->CpuIdVersionInfoEcx.Bits.SMX == 1); > } > > @@ -265,7 +218,6 @@ SmxInitialize ( > IN BOOLEAN State > ) > { > - MSR_IA32_FEATURE_CONTROL_REGISTER *MsrRegister; > RETURN_STATUS Status; > > // > @@ -288,35 +240,32 @@ SmxInitialize ( > Status = RETURN_UNSUPPORTED; > } > > - ASSERT (ConfigData != NULL); > - MsrRegister = (MSR_IA32_FEATURE_CONTROL_REGISTER *) ConfigData; > - if (MsrRegister[ProcessorNumber].Bits.Lock == 0) { > - CPU_REGISTER_TABLE_WRITE_FIELD ( > - ProcessorNumber, > - Msr, > - MSR_IA32_FEATURE_CONTROL, > - MSR_IA32_FEATURE_CONTROL_REGISTER, > - Bits.SenterLocalFunctionEnables, > - (State) ? 0x7F : 0 > - ); > - > - CPU_REGISTER_TABLE_WRITE_FIELD ( > - ProcessorNumber, > - Msr, > - MSR_IA32_FEATURE_CONTROL, > - MSR_IA32_FEATURE_CONTROL_REGISTER, > - Bits.SenterGlobalEnable, > - (State) ? 1 : 0 > - ); > - > - CPU_REGISTER_TABLE_WRITE_FIELD ( > - ProcessorNumber, > - Msr, > - MSR_IA32_FEATURE_CONTROL, > - MSR_IA32_FEATURE_CONTROL_REGISTER, > - Bits.EnableVmxInsideSmx, > - (State) ? 1 : 0 > - ); > - } > + CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD ( > + ProcessorNumber, > + Msr, > + MSR_IA32_FEATURE_CONTROL, > + MSR_IA32_FEATURE_CONTROL_REGISTER, > + Bits.SenterLocalFunctionEnables, > + (State) ? 0x7F : 0 > + ); > + > + CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD ( > + ProcessorNumber, > + Msr, > + MSR_IA32_FEATURE_CONTROL, > + MSR_IA32_FEATURE_CONTROL_REGISTER, > + Bits.SenterGlobalEnable, > + (State) ? 1 : 0 > + ); > + > + CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD ( > + ProcessorNumber, > + Msr, > + MSR_IA32_FEATURE_CONTROL, > + MSR_IA32_FEATURE_CONTROL_REGISTER, > + Bits.EnableVmxInsideSmx, > + (State) ? 1 : 0 > + ); > + > return Status; > } > diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/MachineCheck.c b/UefiCpuPkg/Library/CpuCommonFeaturesLib/MachineCheck.c > index 2528e0044e..01fd6bb54d 100644 > --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/MachineCheck.c > +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/MachineCheck.c > @@ -319,8 +319,6 @@ LmceInitialize ( > IN BOOLEAN State > ) > { > - MSR_IA32_FEATURE_CONTROL_REGISTER *MsrRegister; > - > // > // The scope of LcmeOn bit in the MSR_IA32_MISC_ENABLE is core for below processor type, only program > // MSR_IA32_MISC_ENABLE for thread 0 in each core. > @@ -333,17 +331,14 @@ LmceInitialize ( > } > } > > - ASSERT (ConfigData != NULL); > - MsrRegister = (MSR_IA32_FEATURE_CONTROL_REGISTER *) ConfigData; > - if (MsrRegister[ProcessorNumber].Bits.Lock == 0) { > - CPU_REGISTER_TABLE_WRITE_FIELD ( > - ProcessorNumber, > - Msr, > - MSR_IA32_FEATURE_CONTROL, > - MSR_IA32_FEATURE_CONTROL_REGISTER, > - Bits.LmceOn, > - (State) ? 1 : 0 > - ); > - } > + CPU_REGISTER_TABLE_TEST_THEN_WRITE_FIELD ( > + ProcessorNumber, > + Msr, > + MSR_IA32_FEATURE_CONTROL, > + MSR_IA32_FEATURE_CONTROL_REGISTER, > + Bits.LmceOn, > + (State) ? 1 : 0 > + ); > + > return RETURN_SUCCESS; > } > Acked-by: Laszlo Ersek