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

["Ni, Ray" <ray.ni@intel.com>]

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]
-=-=-=-=-=-=-=-=-=-=-=-