[edk2-devel] [PATCH v1 0/2] SMM CPU Optimization for SMM Init & SMI Process

[edk2-devel] [PATCH v1 0/2] SMM CPU Optimization for SMM Init & SMI Process

[Wu, Jiaxin]

Optimization 1:
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.
As for MSR_IA32_MISC_ENABLE.BTS, it's core scope according SDM. So, still keep it check on each processor.

Optimization 2:
UefiCpuPkg/PiSmmCpuDxeSmm: Check BspIndex first before lock cmpxchg
This patch is to check BspIndex first before lock cmpxchg operation.
It's the optimization to lower the resource contention caused by the
atomic compare exchange operation, so as to improve the SMI
performance for BSP election.

Jiaxin Wu (2):
  UefiCpuPkg/PiSmmCpuDxeSmm: Execute CET and XD check only on BSP
  UefiCpuPkg/PiSmmCpuDxeSmm: Check BspIndex first before lock cmpxchg

 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c      | 12 +++--
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c |  6 +--
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c     | 78 +++++++++++++++++-------------
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.h     |  6 ++-
 4 files changed, 59 insertions(+), 43 deletions(-)

-- 
2.16.2.windows.1



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

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

[Wu, Jiaxin]

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.

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

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>
---
 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.
 
-- 
2.16.2.windows.1



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

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

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@Intel.com>

I originally thought CheckFeatureSupported() when running in parallel when SMM base relocation is done in PEI
might corrupt the global variables.
But then I realized the function only perform variable modification from TRUE to FALSE.
So even the code runs in parallel, it should be safe.

Thanks,
Ray
> -----Original Message-----
> From: Wu, Jiaxin <jiaxin.wu@intel.com>
> Sent: Thursday, February 1, 2024 7:20 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Dong, Eric
> <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> Subject: [PATCH v1 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.
> 
> As for MSR_IA32_MISC_ENABLE.BTS, it's core scope according SDM.
> So, still keep it check on each processor.
> 
> 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>
> ---
>  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.
> 
> --
> 2.16.2.windows.1



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

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

[Wu, Jiaxin]

Yes, Ray, I also realized that, so, this patch only belongs to the optimization to avoid the unnecessary feature check on each process. 

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Friday, February 2, 2024 2:04 PM
> To: Wu, Jiaxin <jiaxin.wu@intel.com>; devel@edk2.groups.io
> Cc: Laszlo Ersek <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>;
> Zeng, Star <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>;
> Kumar, Rahul R <rahul.r.kumar@intel.com>
> Subject: RE: [PATCH v1 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Execute CET and
> XD check only on BSP
> 
> Reviewed-by: Ray Ni <ray.ni@Intel.com>
> 
> I originally thought CheckFeatureSupported() when running in parallel when
> SMM base relocation is done in PEI
> might corrupt the global variables.
> But then I realized the function only perform variable modification from TRUE
> to FALSE.
> So even the code runs in parallel, it should be safe.
> 
> Thanks,
> Ray
> > -----Original Message-----
> > From: Wu, Jiaxin <jiaxin.wu@intel.com>
> > Sent: Thursday, February 1, 2024 7:20 PM
> > To: devel@edk2.groups.io
> > Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Dong,
> Eric
> > <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Gerd Hoffmann
> > <kraxel@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> > Subject: [PATCH v1 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.
> >
> > As for MSR_IA32_MISC_ENABLE.BTS, it's core scope according SDM.
> > So, still keep it check on each processor.
> >
> > 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>
> > ---
> >  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.
> >
> > --
> > 2.16.2.windows.1



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

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

[Laszlo Ersek]

On 2/2/24 07:03, Ni, Ray wrote:
> Reviewed-by: Ray Ni <ray.ni@Intel.com>
> 
> I originally thought CheckFeatureSupported() when running in parallel when SMM base relocation is done in PEI
> might corrupt the global variables.

That was / is my concern as well. That's why I asked Jiaxin if
CheckFeatureSupported() was executed concurrently by multiple processors.

> But then I realized the function only perform variable modification from TRUE to FALSE.
> So even the code runs in parallel, it should be safe.

I too noticed that those transitions are all TRUE->FALSE, and (IIRC) all
the variables are just BOOLEANs (so 1 byte).

Still, I don't like that (to be clear, I don't even like the pre-patch
state):

- The global variables are not even marked volatile.
- We don't use any locking, like interlocked exchange.
- We do other things than just "mBoolean = FALSE": we patch instructions
in the SMI assembly code, too.

All in all -- as long as this function can indeed run on multiple
processors at the same time --, I think it would be much *cleaner* to:

- split the BSP-only functionality off of CheckFeatureSupported() --
more importantly, off of SmmInitHandler. All that feature testing could
be done in "single-threaded" code, could it not?

- if we need to check some feature on each processor in separation, and
then build a big conjunction of those results in the end, then I find it
better if the implementation actually follows this logic. Let the
processors store their results in separate BOOLEAN elements of an array,
and let the BSP go over the array once the APs have settled.

Minimally, at least document why the current lock-less but concurrent
approach is safe!

Laszlo

> 
> Thanks,
> Ray
>> -----Original Message-----
>> From: Wu, Jiaxin <jiaxin.wu@intel.com>
>> Sent: Thursday, February 1, 2024 7:20 PM
>> To: devel@edk2.groups.io
>> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Dong, Eric
>> <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Gerd Hoffmann
>> <kraxel@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
>> Subject: [PATCH v1 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.
>>
>> As for MSR_IA32_MISC_ENABLE.BTS, it's core scope according SDM.
>> So, still keep it check on each processor.
>>
>> 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>
>> ---
>>  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.
>>
>> --
>> 2.16.2.windows.1
> 



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

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

[Wu, Jiaxin]

> > I originally thought CheckFeatureSupported() when running in parallel when
> SMM base relocation is done in PEI
> > might corrupt the global variables.
> 
> That was / is my concern as well. That's why I asked Jiaxin if
> CheckFeatureSupported() was executed concurrently by multiple processors.
> 

Yes, Laszlo, CheckFeatureSupported is executed in parallel by multiple processors.


> > But then I realized the function only perform variable modification from
> TRUE to FALSE.
> > So even the code runs in parallel, it should be safe.
> 
> I too noticed that those transitions are all TRUE->FALSE, and (IIRC) all
> the variables are just BOOLEANs (so 1 byte).
> 
> Still, I don't like that (to be clear, I don't even like the pre-patch
> state):
> 
> - The global variables are not even marked volatile.
> - We don't use any locking, like interlocked exchange.
> - We do other things than just "mBoolean = FALSE": we patch instructions
> in the SMI assembly code, too.
> 
> All in all -- as long as this function can indeed run on multiple
> processors at the same time --, I think it would be much *cleaner* to:
> 
> - split the BSP-only functionality off of CheckFeatureSupported() --
> more importantly, off of SmmInitHandler. All that feature testing could
> be done in "single-threaded" code, could it not?
> 

I doubt the change to split the functionality check off of the SmmInitHandler. SmmInitHandler is called in SMI. I'm are not 100% sure the CET & XD & BTS features are same in non-smm & smm environment. If we move out of the smm environment, it might be have potential issue. Stay in SMI is more safe change.


> - if we need to check some feature on each processor in separation, and
> then build a big conjunction of those results in the end, then I find it
> better if the implementation actually follows this logic. Let the
> processors store their results in separate BOOLEAN elements of an array,
> and let the BSP go over the array once the APs have settled.
> 

I also thought of that way to handle the BTS feature since it's the only feature need check on each processor. That is the good way to handle the global variable might be corrupted by the multiple modification. but as Ray explained, the function only perform variable modification from TRUE to FALSE. So even the code runs in parallel, it's safe. That's the reason why I keep the original logic. 

> Minimally, at least document why the current lock-less but concurrent
> approach is safe!
> 

I agree. We can document in code comment. I will refine the patch to include this. 

Thanks,
Jiaxin


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

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

[Laszlo Ersek]

On 2/1/24 12:20, Wu, Jiaxin wrote:
> 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.
> 
> As for MSR_IA32_MISC_ENABLE.BTS, it's core scope according SDM.
> So, still keep it check on each processor.
> 
> 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>
> ---
>  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.
>  

Do multiple processors execute CheckFeatureSupported() concurrently?

Laszlo



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

[edk2-devel] [PATCH v1 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Check BspIndex first before lock cmpxchg

[Wu, Jiaxin]

This patch is to check BspIndex first before lock cmpxchg operation.
It's the optimization to lower the resource contention caused by the
atomic compare exchange operation, so as to improve the SMI
performance for BSP election.

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>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
index e988ce0542..479024d294 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
@@ -1652,15 +1652,17 @@ SmiRendezvous (
             }
           } else {
             //
             // Platform hook fails to determine, use default BSP election method
             //
-            InterlockedCompareExchange32 (
-              (UINT32 *)&mSmmMpSyncData->BspIndex,
-              (UINT32)-1,
-              (UINT32)CpuIndex
-              );
+            if (mSmmMpSyncData->BspIndex == (UINT32)-1) {
+              InterlockedCompareExchange32 (
+                (UINT32 *)&mSmmMpSyncData->BspIndex,
+                (UINT32)-1,
+                (UINT32)CpuIndex
+                );
+            }
           }
         }
       }
 
       //
-- 
2.16.2.windows.1



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

Re: [edk2-devel] [PATCH v1 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Check BspIndex first before lock cmpxchg

[Michael D Kinney]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu,
> Jiaxin
> Sent: Thursday, February 1, 2024 3:20 AM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Dong,
> Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Gerd
> Hoffmann <kraxel@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>
> Subject: [edk2-devel] [PATCH v1 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Check
> BspIndex first before lock cmpxchg
> 
> This patch is to check BspIndex first before lock cmpxchg operation.
> It's the optimization to lower the resource contention caused by the
> atomic compare exchange operation, so as to improve the SMI
> performance for BSP election.
> 
> 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>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index e988ce0542..479024d294 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -1652,15 +1652,17 @@ SmiRendezvous (
>              }
>            } else {
>              //
>              // Platform hook fails to determine, use default BSP
> election method
>              //
> -            InterlockedCompareExchange32 (
> -              (UINT32 *)&mSmmMpSyncData->BspIndex,
> -              (UINT32)-1,
> -              (UINT32)CpuIndex
> -              );
> +            if (mSmmMpSyncData->BspIndex == (UINT32)-1) {

If the field is UINT32, prefer to use MAX_UINT32 instead of typecase from signed to unsigned value.

> +              InterlockedCompareExchange32 (
> +                (UINT32 *)&mSmmMpSyncData->BspIndex,
> +                (UINT32)-1,

If the parameter is UINT32, prefer to use MAX_UINT32 instead of typecase from signed to unsigned value.

> +                (UINT32)CpuIndex
> +                );
> +            }
>            }
>          }
>        }
> 
>        //
> --
> 2.16.2.windows.1
> 
> 
> 
> 
> 



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

Re: [edk2-devel] [PATCH v1 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Check BspIndex first before lock cmpxchg

[Wu, Jiaxin]

Yes, thanks Mike, I will update the patch.



> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Friday, February 2, 2024 2:21 AM
> To: devel@edk2.groups.io; Wu, Jiaxin <jiaxin.wu@intel.com>
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Dong, Eric
> <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Gerd Hoffmann
> <kraxel@redhat.com>; Kumar, Rahul R <rahul.r.kumar@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] [PATCH v1 2/2] UefiCpuPkg/PiSmmCpuDxeSmm:
> Check BspIndex first before lock cmpxchg
> 
> 
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu,
> > Jiaxin
> > Sent: Thursday, February 1, 2024 3:20 AM
> > To: devel@edk2.groups.io
> > Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Dong,
> > Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>; Gerd
> > Hoffmann <kraxel@redhat.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>
> > Subject: [edk2-devel] [PATCH v1 2/2] UefiCpuPkg/PiSmmCpuDxeSmm:
> Check
> > BspIndex first before lock cmpxchg
> >
> > This patch is to check BspIndex first before lock cmpxchg operation.
> > It's the optimization to lower the resource contention caused by the
> > atomic compare exchange operation, so as to improve the SMI
> > performance for BSP election.
> >
> > 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>
> > ---
> >  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > index e988ce0542..479024d294 100644
> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> > @@ -1652,15 +1652,17 @@ SmiRendezvous (
> >              }
> >            } else {
> >              //
> >              // Platform hook fails to determine, use default BSP
> > election method
> >              //
> > -            InterlockedCompareExchange32 (
> > -              (UINT32 *)&mSmmMpSyncData->BspIndex,
> > -              (UINT32)-1,
> > -              (UINT32)CpuIndex
> > -              );
> > +            if (mSmmMpSyncData->BspIndex == (UINT32)-1) {
> 
> If the field is UINT32, prefer to use MAX_UINT32 instead of typecase from
> signed to unsigned value.
> 
> > +              InterlockedCompareExchange32 (
> > +                (UINT32 *)&mSmmMpSyncData->BspIndex,
> > +                (UINT32)-1,
> 
> If the parameter is UINT32, prefer to use MAX_UINT32 instead of typecase
> from signed to unsigned value.
> 
> > +                (UINT32)CpuIndex
> > +                );
> > +            }
> >            }
> >          }
> >        }
> >
> >        //
> > --
> > 2.16.2.windows.1
> >
> >
> >
> > 
> >



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

Re: [edk2-devel] [PATCH v1 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Check BspIndex first before lock cmpxchg

[Laszlo Ersek]

On 2/1/24 12:20, Wu, Jiaxin wrote:
> This patch is to check BspIndex first before lock cmpxchg operation.
> It's the optimization to lower the resource contention caused by the
> atomic compare exchange operation, so as to improve the SMI
> performance for BSP election.
> 
> 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>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index e988ce0542..479024d294 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -1652,15 +1652,17 @@ SmiRendezvous (
>              }
>            } else {
>              //
>              // Platform hook fails to determine, use default BSP election method
>              //
> -            InterlockedCompareExchange32 (
> -              (UINT32 *)&mSmmMpSyncData->BspIndex,
> -              (UINT32)-1,
> -              (UINT32)CpuIndex
> -              );
> +            if (mSmmMpSyncData->BspIndex == (UINT32)-1) {
> +              InterlockedCompareExchange32 (
> +                (UINT32 *)&mSmmMpSyncData->BspIndex,
> +                (UINT32)-1,
> +                (UINT32)CpuIndex
> +                );
> +            }
>            }
>          }
>        }
>  
>        //

This patch makes me uncomfortable. I understand what it intends to do,
and the intent is not wrong, but we're again treating "volatile UINT32"
as atomic, and non-reorderable against the
InterlockedCompareExchange32(). This kind of "optimization" is what
people write cautionary tales about, later. It would be nice to see a
semi-formal *proof* that this cannot backfire.

Either way: I'm not trying to block the patch. If Ray is happy with it,
I don't object. OVMF implements PlatformSmmBspElection() in
"OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLibQemu.c",
always returning EFI_SUCCESS [*], so the code that is being modified
here cannot be reached in OVMF.

[*] commit 43df61878d94 ("OvmfPkg: enable SMM Monarch Election in
PiSmmCpuDxeSmm", 2020-03-04)

Laszlo



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

Re: [edk2-devel] [PATCH v1 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Check BspIndex first before lock cmpxchg

[Ni, Ray]

> 
> This patch makes me uncomfortable. I understand what it intends to do,
> and the intent is not wrong, but we're again treating "volatile UINT32"
> as atomic, and non-reorderable against the
> InterlockedCompareExchange32(). This kind of "optimization" is what
> people write cautionary tales about, later. It would be nice to see a
> semi-formal *proof* that this cannot backfire.

Laszlo,
Test-and-set is implemented in x86 through the cmpxchg instruction.

The patch follows https://en.wikipedia.org/wiki/Test_and_test-and-set
and adds a "test" before "test-and-set".

> 
> Either way: I'm not trying to block the patch. If Ray is happy with it,
> I don't object. OVMF implements PlatformSmmBspElection() in
> "OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLi
> bQemu.c",
> always returning EFI_SUCCESS [*], so the code that is being modified
> here cannot be reached in OVMF.
> 
> [*] commit 43df61878d94 ("OvmfPkg: enable SMM Monarch Election in
> PiSmmCpuDxeSmm", 2020-03-04)
> 
> Laszlo
> 
> 
> 
> 
> 



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

Re: [edk2-devel] [PATCH v1 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Check BspIndex first before lock cmpxchg

[Laszlo Ersek]

On 2/6/24 02:40, Ni, Ray wrote:
>>
>> This patch makes me uncomfortable. I understand what it intends to do,
>> and the intent is not wrong, but we're again treating "volatile UINT32"
>> as atomic, and non-reorderable against the
>> InterlockedCompareExchange32(). This kind of "optimization" is what
>> people write cautionary tales about, later. It would be nice to see a
>> semi-formal *proof* that this cannot backfire.
> 
> Laszlo,
> Test-and-set is implemented in x86 through the cmpxchg instruction.
> 
> The patch follows https://en.wikipedia.org/wiki/Test_and_test-and-set
> and adds a "test" before "test-and-set".

That may or may not work without special additional guarantees.

From C11 "5.1.2.4 Multi-threaded executions and data races":

- paragraph 4: "Two expression evaluations conflict if one of them
modifies a memory location and the other one reads or modifies the same
memory location."

- paragraph 25: "The execution of a program contains a data race if it
contains two conflicting actions in different threads, at least one of
which is not atomic, and neither happens before the other. Any such data
race results in undefined behavior."

In this case, the outer condition (which reads the volatile UINT32
object "mSmmMpSyncData->BspIndex") is one access. It is a read access,
and it is not atomic. The other access is the
InterlockedCompareExchange32(). It is a read-write access, and it is
atomic. There is no "happens before" relationship enforced between both
accesses.

Therefore, this is a data race: the pre-check on one CPU conflicts with
the InterlockedCompareExchange32() on another CPU, the pre-check is not
atomic, and there is no happens-before between them.

A data race means undefined behavior. In particular, if there is a data
race, then there are no guarantees of sequential consistency, so the
observable values in the object could be totally inexplicable.

If you consider PiSmmCpuDxeSmm's usage of "volatile" to be "atomic",
then that would remove the data race; but volatile is not necessarily
atomic.

Since you linked a wikipedia article, I'll link three articles that
describe a similar (nearly the same) technique called "double-checked
locking". In the general case, that technique is broken.

http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
https://en.wikipedia.org/wiki/Double-checked_locking
https://preshing.com/20130930/double-checked-locking-is-fixed-in-cpp11/

It can be made work in bare-bones C, but it requires explicit fences /
memory barriers.

Again: I'm not trying to block this patch, I just remain unconvinced
that it is safe. (And if it is safe, then it relies on such properties /
guarantees of Intel processors that should be documented in the code.)

Laszlo


> 
>>
>> Either way: I'm not trying to block the patch. If Ray is happy with it,
>> I don't object. OVMF implements PlatformSmmBspElection() in
>> "OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLi
>> bQemu.c",
>> always returning EFI_SUCCESS [*], so the code that is being modified
>> here cannot be reached in OVMF.
>>
>> [*] commit 43df61878d94 ("OvmfPkg: enable SMM Monarch Election in
>> PiSmmCpuDxeSmm", 2020-03-04)
>>
>> Laszlo
>>
>>
>>
>>
>>
> 
> 
> 
> 
> 
> 



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

Re: [edk2-devel] [PATCH v1 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Check BspIndex first before lock cmpxchg

[Wu, Jiaxin]

> From C11 "5.1.2.4 Multi-threaded executions and data races":
> 
> - paragraph 4: "Two expression evaluations conflict if one of them
> modifies a memory location and the other one reads or modifies the same
> memory location."
> 
> - paragraph 25: "The execution of a program contains a data race if it
> contains two conflicting actions in different threads, at least one of
> which is not atomic, and neither happens before the other. Any such data
> race results in undefined behavior."
> 
> In this case, the outer condition (which reads the volatile UINT32
> object "mSmmMpSyncData->BspIndex") is one access. It is a read access,
> and it is not atomic. The other access is the
> InterlockedCompareExchange32(). It is a read-write access, and it is
> atomic. There is no "happens before" relationship enforced between both
> accesses.
> 
> Therefore, this is a data race: the pre-check on one CPU conflicts with
> the InterlockedCompareExchange32() on another CPU, the pre-check is not
> atomic, and there is no happens-before between them.
> 
> A data race means undefined behavior. In particular, if there is a data
> race, then there are no guarantees of sequential consistency, so the
> observable values in the object could be totally inexplicable.
> 

I think here data race won't cause the undefined behavior:

The read operation in one processor might see the 1) old value (MAX_UINT32) or 2) the new value (CpuIndex), but both behaviors are explicable:

If case 1, that's ok continue do the lock comxchg since BspIndex hasn't been elected.
If case 2, which means another processor has already atomic invalid or write the BspIndex, then existing processor absolutely can skip the lock comxchg.

            if (mSmmMpSyncData->BspIndex == MAX_UINT32) {
              InterlockedCompareExchange32 (
                (UINT32 *)&mSmmMpSyncData->BspIndex,
                MAX_UINT32,
                (UINT32)CpuIndex
                );
            }

> If you consider PiSmmCpuDxeSmm's usage of "volatile" to be "atomic",
> then that would remove the data race; but volatile is not necessarily
> atomic.
> 

I *never* do the assumption: "volatile" is "atomic".
BspIndex is volatile, I think it only can ensure the compiler behavior, not processor itself:
1. Compiler will not optimize this variable. All reads and writes to this variable will be done directly to memory, not using cached values in registers. But it doesn't prevent the CPU from caching that variable in its own internal caches.
2. "volatile" can prevent the compiler from optimizing and reordering instructions, it can't prevent the processor from reordering instructions, and can't guarantee the atomicity of operations. 

For processor reordering, I think Ray explained that reordering won't happen. 

> Since you linked a wikipedia article, I'll link three articles that
> describe a similar (nearly the same) technique called "double-checked
> locking". In the general case, that technique is broken.
> 
> http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.
> html
> https://en.wikipedia.org/wiki/Double-checked_locking
> https://preshing.com/20130930/double-checked-locking-is-fixed-in-cpp11/
> 
> It can be made work in bare-bones C, but it requires explicit fences /
> memory barriers.

Here, the case is different to the above three "Double-checked locking".  Cache coherency can make sure the read value for check is only 2 cases as I explained above.
The only possible impact is the AP behavior: AP won't pending on lock comxchg, while it will continue do the following code logic after if check: for example performs the APHandler. But it won't has the negative-impact since it has the timeout BSP check in APHandler. And even failure of that, we still has the error handling to sync out the existing AP due to don't know the BSP:

      SmmCpuSyncCheckOutCpu (mSmmMpSyncData->SyncContext, CpuIndex);
      return;

Thanks,
Jiaxin


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

Re: [edk2-devel] [PATCH v1 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Check BspIndex first before lock cmpxchg

[Laszlo Ersek]

On 2/20/24 04:41, Wu, Jiaxin wrote:
>> From C11 "5.1.2.4 Multi-threaded executions and data races":
>>
>> - paragraph 4: "Two expression evaluations conflict if one of them
>> modifies a memory location and the other one reads or modifies the same
>> memory location."
>>
>> - paragraph 25: "The execution of a program contains a data race if it
>> contains two conflicting actions in different threads, at least one of
>> which is not atomic, and neither happens before the other. Any such data
>> race results in undefined behavior."
>>
>> In this case, the outer condition (which reads the volatile UINT32
>> object "mSmmMpSyncData->BspIndex") is one access. It is a read access,
>> and it is not atomic. The other access is the
>> InterlockedCompareExchange32(). It is a read-write access, and it is
>> atomic. There is no "happens before" relationship enforced between both
>> accesses.
>>
>> Therefore, this is a data race: the pre-check on one CPU conflicts with
>> the InterlockedCompareExchange32() on another CPU, the pre-check is not
>> atomic, and there is no happens-before between them.
>>
>> A data race means undefined behavior. In particular, if there is a data
>> race, then there are no guarantees of sequential consistency, so the
>> observable values in the object could be totally inexplicable.
>>
> 
> I think here data race won't cause the undefined behavior:
> 
> The read operation in one processor might see the 1) old value (MAX_UINT32) or 2) the new value (CpuIndex), but both behaviors are explicable:
> 
> If case 1, that's ok continue do the lock comxchg since BspIndex hasn't been elected.
> If case 2, which means another processor has already atomic invalid or write the BspIndex, then existing processor absolutely can skip the lock comxchg.
> 
>             if (mSmmMpSyncData->BspIndex == MAX_UINT32) {
>               InterlockedCompareExchange32 (
>                 (UINT32 *)&mSmmMpSyncData->BspIndex,
>                 MAX_UINT32,
>                 (UINT32)CpuIndex
>                 );
>             }
> 
>> If you consider PiSmmCpuDxeSmm's usage of "volatile" to be "atomic",
>> then that would remove the data race; but volatile is not necessarily
>> atomic.
>>
> 
> I *never* do the assumption: "volatile" is "atomic".
> BspIndex is volatile, I think it only can ensure the compiler behavior, not processor itself:
> 1. Compiler will not optimize this variable. All reads and writes to this variable will be done directly to memory, not using cached values in registers. But it doesn't prevent the CPU from caching that variable in its own internal caches.
> 2. "volatile" can prevent the compiler from optimizing and reordering instructions, it can't prevent the processor from reordering instructions, and can't guarantee the atomicity of operations. 
> 
> For processor reordering, I think Ray explained that reordering won't happen. 
> 
>> Since you linked a wikipedia article, I'll link three articles that
>> describe a similar (nearly the same) technique called "double-checked
>> locking". In the general case, that technique is broken.
>>
>> http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.
>> html
>> https://en.wikipedia.org/wiki/Double-checked_locking
>> https://preshing.com/20130930/double-checked-locking-is-fixed-in-cpp11/
>>
>> It can be made work in bare-bones C, but it requires explicit fences /
>> memory barriers.
> 
> Here, the case is different to the above three "Double-checked locking".  Cache coherency can make sure the read value for check is only 2 cases as I explained above.
> The only possible impact is the AP behavior: AP won't pending on lock comxchg, while it will continue do the following code logic after if check: for example performs the APHandler. But it won't has the negative-impact since it has the timeout BSP check in APHandler. And even failure of that, we still has the error handling to sync out the existing AP due to don't know the BSP:
> 
>       SmmCpuSyncCheckOutCpu (mSmmMpSyncData->SyncContext, CpuIndex);
>       return;

I don't have other objections; feel free to proceed.

Thanks
Laszlo



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

Re: [edk2-devel] [PATCH v1 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Check BspIndex first before lock cmpxchg

[Ni, Ray]

> This patch makes me uncomfortable. I understand what it intends to do,
> and the intent is not wrong, but we're again treating "volatile UINT32"
> as atomic, and non-reorderable against the
> InterlockedCompareExchange32(). This kind of "optimization" is what
> people write cautionary tales about, later. It would be nice to see a
> semi-formal *proof* that this cannot backfire.

Laszlo,
Thanks for the questions. I was OOO in the past 10 days for Chinese New Year.

From Chapter 9 "Multiple-Processor Management",
section 9.2.2 "Memory Ordering in P6 and More Recent Processor Families",
I read the following:

  In a single-processor system for memory regions defined as write-back cacheable,
  the memory ordering model respects the following principles...:
    * ...
    * Reads or writes cannot be reordered with I/O instructions, locked instructions, or serializing instructions.

So, it guarantees/proves "read of BspIndex" performs before "InterlockedCompareExchange32()/lock comxchg".

I think that can resolve one of your concern regarding the "ordering".

I don't quite understand your concern regarding the "atomic".


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