Re: [edk2-devel] [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Execute CET and XD check only on BSP

["Brian J. Johnson" <brian.johnson@hpe.com>]

As others have pointed out, setting BOOLEAN values from TRUE to FALSE on 
multiple CPUs at once "should" be safe, in that the final values of the 
variables are the same no matter what order the CPUs write the data.

Now that only the BSP is writing these values, what guarantees that they 
are written before the APs need to read them?  The commit message didn't 
mention it, and I haven't traced through the SMM initialization flow.

Brian J. Johnson

-------- Original Message --------
From: Wu, Jiaxin [mailto:jiaxin.wu@intel.com]
Sent: Sunday, February 4, 2024 at 2:47 AM
To: devel@edk2.groups.io
Cc: Ray Ni <ray.ni@intel.com>, Laszlo Ersek <lersek@redhat.com>, Eric 
Dong <eric.dong@intel.com>, Zeng Star <star.zeng@intel.com>, Gerd 
Hoffmann <kraxel@redhat.com>, Rahul Kumar <rahul1.kumar@intel.com>
Subject: [edk2-devel] [PATCH v2 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Execute 
CET and XD check only on BSP

Existing CheckFeatureSupported function will check CET & XD
features on each processor.

The CPUIDs for CET & XD features are software visible domain,
which means a properly configured platform will have consistent
values for these CPUID Leafs/SubLeafs/Fields on each logical
processor. So, execute Execute CET and XD check only on BSP.This
belongs to the optimization, which can avoid unnecessary and
duplicate check on multiple processors.

Note 1: we don't split those functionality check out of the
SmmInitHandler. Because the CET & XD & BTS features might be
different in non-smm & smm environment. Keep in SMM is more safer.

As for MSR_IA32_MISC_ENABLE.BTS, it's core scope according SDM.
So, still keep it check on each processor.

Note 2: For BTS, using global variable to indicate feature
supported or not (mBtsSupported), the function only performs
variable modification from TRUE to FALSE. So even the code runs
in parallel, it's safe.

Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Zeng Star <star.zeng@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Jiaxin Wu <jiaxin.wu@intel.com>
Reviewed-by: Ray Ni <ray.ni@Intel.com>
---
   UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c |  6 +--
   UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c     | 78 
+++++++++++++++++-------------
   UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h     |  6 ++-
   3 files changed, 52 insertions(+), 38 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
index cd394826ff..15d26dd88f 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c
@@ -1,9 +1,9 @@
   /** @file
   Agent Module to load other modules to deploy SMM Entry Vector for X86 
CPU.

-Copyright (c) 2009 - 2023, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2009 - 2024, Intel Corporation. All rights reserved.<BR>
   Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
   Copyright (C) 2023 Advanced Micro Devices, Inc. All rights reserved.<BR>

   SPDX-License-Identifier: BSD-2-Clause-Patent

@@ -375,13 +375,13 @@ SmmInitHandler (
           &mCpuHotPlugData
           );

         if (!mSmmS3Flag) {
           //
-        // Check XD and BTS features on each processor on normal boot
+        // Check CET & XD & BTS features on each processor on normal boot
           //
-        CheckFeatureSupported ();
+        CheckFeatureSupported (IsBsp);
         } else if (IsBsp) {
           //
           // BSP rebase is already done above.
           // Initialize private data during S3 resume
           //
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
index 8142d3ceac..44c352ad98 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
@@ -1,9 +1,9 @@
   /** @file
   Enable SMM profile.

-Copyright (c) 2012 - 2023, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2012 - 2024, Intel Corporation. All rights reserved.<BR>
   Copyright (c) 2017 - 2020, AMD Incorporated. All rights reserved.<BR>

   SPDX-License-Identifier: BSD-2-Clause-Patent

   **/
@@ -892,62 +892,74 @@ InitSmmProfileInternal (
   }

   /**
     Check if feature is supported by a processor.

+  @param[in] IsBsp   Indicate it's called by BSP or not.
+
   **/
   VOID
   CheckFeatureSupported (
-  VOID
+  IN BOOLEAN  IsBsp
     )
   {
     UINT32                         RegEax;
     UINT32                         RegEcx;
     UINT32                         RegEdx;
     MSR_IA32_MISC_ENABLE_REGISTER  MiscEnableMsr;

-  if ((PcdGet32 (PcdControlFlowEnforcementPropertyMask) != 0) && 
mCetSupported) {
-    AsmCpuid (CPUID_SIGNATURE, &RegEax, NULL, NULL, NULL);
-    if (RegEax >= CPUID_STRUCTURED_EXTENDED_FEATURE_FLAGS) {
-      AsmCpuidEx (CPUID_STRUCTURED_EXTENDED_FEATURE_FLAGS, 
CPUID_STRUCTURED_EXTENDED_FEATURE_FLAGS_SUB_LEAF_INFO, NULL, NULL, 
&RegEcx, NULL);
-      if ((RegEcx & CPUID_CET_SS) == 0) {
+  //
+  // The feature scope is software visible domain.
+  // Only need check on BSP.
+  //
+  if (IsBsp) {
+    if ((PcdGet32 (PcdControlFlowEnforcementPropertyMask) != 0) && 
mCetSupported) {
+      AsmCpuid (CPUID_SIGNATURE, &RegEax, NULL, NULL, NULL);
+      if (RegEax >= CPUID_STRUCTURED_EXTENDED_FEATURE_FLAGS) {
+        AsmCpuidEx (CPUID_STRUCTURED_EXTENDED_FEATURE_FLAGS, 
CPUID_STRUCTURED_EXTENDED_FEATURE_FLAGS_SUB_LEAF_INFO, NULL, NULL, 
&RegEcx, NULL);
+        if ((RegEcx & CPUID_CET_SS) == 0) {
+          mCetSupported = FALSE;
+          PatchInstructionX86 (mPatchCetSupported, mCetSupported, 1);
+        }
+      } else {
           mCetSupported = FALSE;
           PatchInstructionX86 (mPatchCetSupported, mCetSupported, 1);
         }
-    } else {
-      mCetSupported = FALSE;
-      PatchInstructionX86 (mPatchCetSupported, mCetSupported, 1);
       }
-  }

-  if (mXdSupported) {
-    AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL);
-    if (RegEax <= CPUID_EXTENDED_FUNCTION) {
-      //
-      // Extended CPUID functions are not supported on this processor.
-      //
-      mXdSupported = FALSE;
-      PatchInstructionX86 (gPatchXdSupported, mXdSupported, 1);
-    }
+    if (mXdSupported) {
+      AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL);
+      if (RegEax <= CPUID_EXTENDED_FUNCTION) {
+        //
+        // Extended CPUID functions are not supported on this processor.
+        //
+        mXdSupported = FALSE;
+        PatchInstructionX86 (gPatchXdSupported, mXdSupported, 1);
+      }

-    AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL, &RegEdx);
-    if ((RegEdx & CPUID1_EDX_XD_SUPPORT) == 0) {
-      //
-      // Execute Disable Bit feature is not supported on this processor.
-      //
-      mXdSupported = FALSE;
-      PatchInstructionX86 (gPatchXdSupported, mXdSupported, 1);
-    }
+      AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL, &RegEdx);
+      if ((RegEdx & CPUID1_EDX_XD_SUPPORT) == 0) {
+        //
+        // Execute Disable Bit feature is not supported on this processor.
+        //
+        mXdSupported = FALSE;
+        PatchInstructionX86 (gPatchXdSupported, mXdSupported, 1);
+      }

-    if (StandardSignatureIsAuthenticAMD ()) {
-      //
-      // AMD processors do not support MSR_IA32_MISC_ENABLE
-      //
-      PatchInstructionX86 (gPatchMsrIa32MiscEnableSupported, FALSE, 1);
+      if (StandardSignatureIsAuthenticAMD ()) {
+        //
+        // AMD processors do not support MSR_IA32_MISC_ENABLE
+        //
+        PatchInstructionX86 (gPatchMsrIa32MiscEnableSupported, FALSE, 1);
+      }
       }
     }

+  //
+  // The feature scope is core.
+  // Need check on each processor.
+  //
     if (mBtsSupported) {
       AsmCpuid (CPUID_VERSION_INFO, NULL, NULL, NULL, &RegEdx);
       if ((RegEdx & CPUID1_EDX_BTS_AVAILABLE) != 0) {
         //
         // Per IA32 manuals:
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h 
b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h
index 1a82ac05ce..02554a9983 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h
@@ -1,9 +1,9 @@
   /** @file
   SMM profile header file.

-Copyright (c) 2012 - 2019, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2012 - 2024, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent

   **/

   #ifndef _SMM_PROFILE_H_
@@ -81,14 +81,16 @@ PageFaultIdtHandlerSmmProfile (
     );

   /**
     Check if feature is supported by a processor.

+  @param[in] IsBsp   Indicate it's called by BSP or not.
+
   **/
   VOID
   CheckFeatureSupported (
-  VOID
+  IN BOOLEAN  IsBsp
     );

   /**
     Update page table according to protected memory ranges and the 
4KB-page mapped memory ranges.


-- 
Brian J. Johnson
Enterprise X86 Lab
Hewlett Packard Enterprise


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115122): https://edk2.groups.io/g/devel/message/115122
Mute This Topic: https://groups.io/mt/104153488/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-