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

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

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