Re: [PATCH 1/2] UefiCpuPkg/CpuCommonFeaturesLib: Remove XD enable/disable logic

["Laszlo Ersek" <lersek@redhat.com>]

On 11/11/19 06:35, Ray Ni wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2329
> 
> XD (ExecutionDisable) feature, when turned on, allows page table
> entry BIT63 set to 1 indicating the memory pointed by the page table
> is disallowed to execute.
> DxeIpl::CreateIdentityMappingPageTables() enables the XD when CPU
> supports it.
> Later DxeCore modifies the page table to set the BIT63 to protect
> the stack/heap to disallow code execution in stack/heap.
> 
> UefiCpuPkg/CpuCommonFeaturesLib enables/disables the XD feature
> according to PcdCpuFeaturesSetting.
> When XD is disabled, GP fault is generated immediately because some
> page entries have BIT63 set.
> 
> To fix this issue, this patch removes the XD feature logic from
> UefiCpuPkg/CpuCommonFeaturesLib so the XD feature is only taken
> care of by DxeIpl.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  .../CpuCommonFeaturesLib.c                    | 11 ---
>  .../CpuCommonFeaturesLib.inf                  |  3 +-
>  .../CpuCommonFeaturesLib/ExecuteDisable.c     | 95 -------------------
>  3 files changed, 1 insertion(+), 108 deletions(-)
>  delete mode 100644 UefiCpuPkg/Library/CpuCommonFeaturesLib/ExecuteDisable.c
> 
> diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c
> index 238632f88a..3ebd9392a9 100644
> --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c
> +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c
> @@ -66,17 +66,6 @@ CpuCommonFeaturesLibConstructor (
>                 );
>      ASSERT_EFI_ERROR (Status);
>    }
> -  if (IsCpuFeatureSupported (CPU_FEATURE_XD)) {
> -    Status = RegisterCpuFeature (
> -               "Execute Disable",
> -               NULL,
> -               ExecuteDisableSupport,
> -               ExecuteDisableInitialize,
> -               CPU_FEATURE_XD,
> -               CPU_FEATURE_END
> -               );
> -    ASSERT_EFI_ERROR (Status);
> -  }
>    if (IsCpuFeatureSupported (CPU_FEATURE_FASTSTRINGS)) {
>      Status = RegisterCpuFeature (
>                 "FastStrings",
> diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
> index 6347c8997d..7fbcd8da0e 100644
> --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
> +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
> @@ -4,7 +4,7 @@
>  #  This library registers CPU features defined in Intel(R) 64 and IA-32
>  #  Architectures Software Developer's Manual.
>  #
> -# Copyright (c) 2017 - 2018, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2017 - 2019, Intel Corporation. All rights reserved.<BR>
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -34,7 +34,6 @@ [Sources]
>    C1e.c
>    ClockModulation.c
>    Eist.c
> -  ExecuteDisable.c
>    FastStrings.c
>    FeatureControl.c
>    LimitCpuIdMaxval.c
> diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ExecuteDisable.c b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ExecuteDisable.c
> deleted file mode 100644
> index 75ea16309d..0000000000
> --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ExecuteDisable.c
> +++ /dev/null
> @@ -1,95 +0,0 @@
> -/** @file
> -  Execute Disable feature.
> -
> -  Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> -  SPDX-License-Identifier: BSD-2-Clause-Patent
> -
> -**/
> -
> -#include "CpuCommonFeatures.h"
> -
> -/**
> -  Detects if Execute Disable feature supported on current processor.
> -
> -  @param[in]  ProcessorNumber  The index of the CPU executing this function.
> -  @param[in]  CpuInfo          A pointer to the REGISTER_CPU_FEATURE_INFORMATION
> -                               structure for the CPU executing this function.
> -  @param[in]  ConfigData       A pointer to the configuration buffer returned
> -                               by CPU_FEATURE_GET_CONFIG_DATA.  NULL if
> -                               CPU_FEATURE_GET_CONFIG_DATA was not provided in
> -                               RegisterCpuFeature().
> -
> -  @retval TRUE     Execute Disable feature is supported.
> -  @retval FALSE    Execute Disable feature is not supported.
> -
> -  @note This service could be called by BSP/APs.
> -**/
> -BOOLEAN
> -EFIAPI
> -ExecuteDisableSupport (
> -  IN UINTN                             ProcessorNumber,
> -  IN REGISTER_CPU_FEATURE_INFORMATION  *CpuInfo,
> -  IN VOID                              *ConfigData  OPTIONAL
> -  )
> -{
> -  UINT32                         Eax;
> -  CPUID_EXTENDED_CPU_SIG_EDX     Edx;
> -
> -  AsmCpuid (CPUID_EXTENDED_FUNCTION, &Eax, NULL, NULL, NULL);
> -  if (Eax <= CPUID_EXTENDED_FUNCTION) {
> -    //
> -    // Extended CPUID functions are not supported on this processor.
> -    //
> -    return FALSE;
> -  }
> -
> -  AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL, &Edx.Uint32);
> -  return (Edx.Bits.NX != 0);
> -}
> -
> -/**
> -  Initializes Execute Disable feature to specific state.
> -
> -  @param[in]  ProcessorNumber  The index of the CPU executing this function.
> -  @param[in]  CpuInfo          A pointer to the REGISTER_CPU_FEATURE_INFORMATION
> -                               structure for the CPU executing this function.
> -  @param[in]  ConfigData       A pointer to the configuration buffer returned
> -                               by CPU_FEATURE_GET_CONFIG_DATA.  NULL if
> -                               CPU_FEATURE_GET_CONFIG_DATA was not provided in
> -                               RegisterCpuFeature().
> -  @param[in]  State            If TRUE, then the Execute Disable feature must be enabled.
> -                               If FALSE, then the Execute Disable feature must be disabled.
> -
> -  @retval RETURN_SUCCESS       Execute Disable feature is initialized.
> -
> -  @note This service could be called by BSP only.
> -**/
> -RETURN_STATUS
> -EFIAPI
> -ExecuteDisableInitialize (
> -  IN UINTN                             ProcessorNumber,
> -  IN REGISTER_CPU_FEATURE_INFORMATION  *CpuInfo,
> -  IN VOID                              *ConfigData,  OPTIONAL
> -  IN BOOLEAN                           State
> -  )
> -{
> -  //
> -  // The scope of the MSR_IA32_EFER is core for below processor type, only program
> -  // MSR_IA32_EFER for thread 0 in each core.
> -  //
> -  if (IS_SILVERMONT_PROCESSOR (CpuInfo->DisplayFamily, CpuInfo->DisplayModel)) {
> -    if (CpuInfo->ProcessorInfo.Location.Thread != 0) {
> -      return RETURN_SUCCESS;
> -    }
> -  }
> -
> -  CPU_REGISTER_TABLE_WRITE_FIELD (
> -    ProcessorNumber,
> -    Msr,
> -    MSR_IA32_EFER,
> -    MSR_IA32_EFER_REGISTER,
> -    Bits.NXE,
> -    (State) ? 1 : 0
> -    );
> -  return RETURN_SUCCESS;
> -}
> 

series
Acked-by: Laszlo Ersek <lersek@redhat.com>