Re: [edk2] [PATCH 2/4] OvmfPkg: SmmCpuFeaturesLib: remove unnecessary bits

[Laszlo Ersek]

On 10/13/15 14:57, Paolo Bonzini wrote:
> SMRR and MTRR support is not needed on a virtual platform.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Paolo Bonzini 
> ---
>  .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  | 180 
> +++--
>  .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf|   4 -
>  2 files changed, 18 insertions(+), 166 deletions(-)

Awesome. I love this diffstat. And, I don't think I can plausibly
disagree with anything you say about QEMU/KVM as a platform :)

So,

Acked-by: Laszlo Ersek 

Thanks!
Laszlo

> 
> diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c 
> b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> index 6ab01e8..97e2010 100644
> --- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> +++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
> @@ -15,46 +15,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
> EXPRESS OR IMPLIED.
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  #include 
>  #include 
> -#include 
>  #include 
>  
> -//
> -// Machine Specific Registers (MSRs)
> -//
> -#define  SMM_FEATURES_LIB_IA32_MTRR_CAP0x0FE
> -#define  SMM_FEATURES_LIB_IA32_FEATURE_CONTROL 0x03A
> -#define  SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE   0x1F2
> -#define  SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK   0x1F3
> -#define  SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE  0x0A0
> -#define  SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSMASK  0x0A1
> -#defineEFI_MSR_SMRR_MASK   0xF000
> -#defineEFI_MSR_SMRR_PHYS_MASK_VALIDBIT11
> -
> -//
> -// Set default value to assume SMRR is not supported
> -//
> -BOOLEAN  mSmrrSupported = FALSE;
> -
> -//
> -// Set default value to assume IA-32 Architectural MSRs are used
> -//
> -UINT32  mSmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE;
> -UINT32  mSmrrPhysMaskMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK;
> -
> -//
> -// Set default value to assume MTRRs need to be configured on each SMI
> -//
> -BOOLEAN  mNeedConfigureMtrrs = TRUE;
> -
> -//
> -// Array for state of SMRR enable on all CPUs
> -//
> -BOOLEAN  *mSmrrEnabled;
> -
>  /**
>The constructor function 
>  
> @@ -68,91 +33,9 @@ SmmCpuFeaturesLibConstructor (
>IN EFI_SYSTEM_TABLE  *SystemTable
>)
>  {
> -  UINT32  RegEax;
> -  UINT32  RegEdx;
> -  UINTN   FamilyId;
> -  UINTN   ModelId;
> -
> -  //
> -  // Retrieve CPU Family and Model 
> -  //
> -  AsmCpuid (CPUID_VERSION_INFO, , NULL, NULL, );
> -  FamilyId = (RegEax >> 8) & 0xf;
> -  ModelId  = (RegEax >> 4) & 0xf;
> -  if (FamilyId == 0x06 || FamilyId == 0x0f) {
> -ModelId = ModelId | ((RegEax >> 12) & 0xf0);
> -  }
> -
> -  //
> -  // Check CPUID(CPUID_VERSION_INFO).EDX[12] for MTRR capability
> -  //
> -  if ((RegEdx & BIT12) != 0) {
> -//
> -// Check MTRR_CAP MSR bit 11 for SMRR support
> -//
> -if ((AsmReadMsr64 (SMM_FEATURES_LIB_IA32_MTRR_CAP) & BIT11) != 0) {
> -  mSmrrSupported = TRUE;
> -}
> -  }
> -
> -  //
> -  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> -  // Volume 3C, Section 35.3 MSRs in the Intel(R) Atom(TM) Processor Family
> -  // 
> -  // If CPU Family/Model is 06_1CH, 06_26H, 06_27H, 06_35H or 06_36H, then 
> -  // SMRR Physical Base and SMM Physical Mask MSRs are not available.
> -  //
> -  if (FamilyId == 0x06) {
> -if (ModelId == 0x1C || ModelId == 0x26 || ModelId == 0x27 || ModelId == 
> 0x35 || ModelId == 0x36) {
> -  mSmrrSupported = FALSE;
> -}
> -  }
> -
> -  //
> -  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> -  // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor Family
> -  //
> -  // If CPU Family/Model is 06_0F or 06_17, then use Intel(R) Core(TM) 2 
> -  // Processor Family MSRs 
> -  //
> -  if (FamilyId == 0x06) {
> -if (ModelId == 0x17 || ModelId == 0x0f) {
> -  mSmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE;
> -  mSmrrPhysMaskMsr = SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSMASK;
> -}
> -  }
> -  
>//
> -  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
> -  // Volume 3C, Section 34.4.2 SMRAM Caching
> -  //   An IA-32 processor does not automatically write back and invalidate 
> its 
> -  //   caches before entering SMM or before exiting SMM. Because of this 
> behavior, 
> -  //   care must be taken in the placement of the SMRAM in system memory and 
> in 
> -  //   the caching of the SMRAM to prevent cache incoherence when switching 
> back 
> -  //   and forth between SMM and protected mode operation.
> +  // No need to program SMRRs on our virtual platform.
>//
> -  // An IA-32 processor is a processor that does not support the Intel 64 
> -  // Architecture.  Support for the Intel 64 Architecture can be detected 
> from 
> -  // CPUID(CPUID_EXTENDED_CPU_SIG).EDX[29]
> -  //
> -  // If an IA-32 processor is detected, then set 

[edk2] [PATCH 2/4] OvmfPkg: SmmCpuFeaturesLib: remove unnecessary bits

[Paolo Bonzini]

SMRR and MTRR support is not needed on a virtual platform.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Paolo Bonzini 
---
 .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c  | 180 +++--
 .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf|   4 -
 2 files changed, 18 insertions(+), 166 deletions(-)

diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c 
b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index 6ab01e8..97e2010 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -15,46 +15,11 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include 
 #include 
 #include 
-#include 
 #include 
 #include 
 #include 
-#include 
 #include 
 
-//
-// Machine Specific Registers (MSRs)
-//
-#define  SMM_FEATURES_LIB_IA32_MTRR_CAP0x0FE
-#define  SMM_FEATURES_LIB_IA32_FEATURE_CONTROL 0x03A
-#define  SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE   0x1F2
-#define  SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK   0x1F3
-#define  SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE  0x0A0
-#define  SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSMASK  0x0A1
-#defineEFI_MSR_SMRR_MASK   0xF000
-#defineEFI_MSR_SMRR_PHYS_MASK_VALIDBIT11
-
-//
-// Set default value to assume SMRR is not supported
-//
-BOOLEAN  mSmrrSupported = FALSE;
-
-//
-// Set default value to assume IA-32 Architectural MSRs are used
-//
-UINT32  mSmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSBASE;
-UINT32  mSmrrPhysMaskMsr = SMM_FEATURES_LIB_IA32_SMRR_PHYSMASK;
-
-//
-// Set default value to assume MTRRs need to be configured on each SMI
-//
-BOOLEAN  mNeedConfigureMtrrs = TRUE;
-
-//
-// Array for state of SMRR enable on all CPUs
-//
-BOOLEAN  *mSmrrEnabled;
-
 /**
   The constructor function 
 
@@ -68,91 +33,9 @@ SmmCpuFeaturesLibConstructor (
   IN EFI_SYSTEM_TABLE  *SystemTable
   )
 {
-  UINT32  RegEax;
-  UINT32  RegEdx;
-  UINTN   FamilyId;
-  UINTN   ModelId;
-
-  //
-  // Retrieve CPU Family and Model 
-  //
-  AsmCpuid (CPUID_VERSION_INFO, , NULL, NULL, );
-  FamilyId = (RegEax >> 8) & 0xf;
-  ModelId  = (RegEax >> 4) & 0xf;
-  if (FamilyId == 0x06 || FamilyId == 0x0f) {
-ModelId = ModelId | ((RegEax >> 12) & 0xf0);
-  }
-
-  //
-  // Check CPUID(CPUID_VERSION_INFO).EDX[12] for MTRR capability
-  //
-  if ((RegEdx & BIT12) != 0) {
-//
-// Check MTRR_CAP MSR bit 11 for SMRR support
-//
-if ((AsmReadMsr64 (SMM_FEATURES_LIB_IA32_MTRR_CAP) & BIT11) != 0) {
-  mSmrrSupported = TRUE;
-}
-  }
-
-  //
-  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
-  // Volume 3C, Section 35.3 MSRs in the Intel(R) Atom(TM) Processor Family
-  // 
-  // If CPU Family/Model is 06_1CH, 06_26H, 06_27H, 06_35H or 06_36H, then 
-  // SMRR Physical Base and SMM Physical Mask MSRs are not available.
-  //
-  if (FamilyId == 0x06) {
-if (ModelId == 0x1C || ModelId == 0x26 || ModelId == 0x27 || ModelId == 
0x35 || ModelId == 0x36) {
-  mSmrrSupported = FALSE;
-}
-  }
-
-  //
-  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
-  // Volume 3C, Section 35.2 MSRs in the Intel(R) Core(TM) 2 Processor Family
-  //
-  // If CPU Family/Model is 06_0F or 06_17, then use Intel(R) Core(TM) 2 
-  // Processor Family MSRs 
-  //
-  if (FamilyId == 0x06) {
-if (ModelId == 0x17 || ModelId == 0x0f) {
-  mSmrrPhysBaseMsr = SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSBASE;
-  mSmrrPhysMaskMsr = SMM_FEATURES_LIB_IA32_CORE_SMRR_PHYSMASK;
-}
-  }
-  
   //
-  // Intel(R) 64 and IA-32 Architectures Software Developer's Manual
-  // Volume 3C, Section 34.4.2 SMRAM Caching
-  //   An IA-32 processor does not automatically write back and invalidate its 
-  //   caches before entering SMM or before exiting SMM. Because of this 
behavior, 
-  //   care must be taken in the placement of the SMRAM in system memory and 
in 
-  //   the caching of the SMRAM to prevent cache incoherence when switching 
back 
-  //   and forth between SMM and protected mode operation.
+  // No need to program SMRRs on our virtual platform.
   //
-  // An IA-32 processor is a processor that does not support the Intel 64 
-  // Architecture.  Support for the Intel 64 Architecture can be detected from 
-  // CPUID(CPUID_EXTENDED_CPU_SIG).EDX[29]
-  //
-  // If an IA-32 processor is detected, then set mNeedConfigureMtrrs to TRUE, 
-  // so caches are flushed on SMI entry and SMI exit, the interrupted code 
-  // MTRRs are saved/restored, and MTRRs for SMM are loaded.
-  //
-  AsmCpuid (CPUID_EXTENDED_FUNCTION, , NULL, NULL, NULL);
-  if (RegEax >= CPUID_EXTENDED_CPU_SIG) {
-AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL, );
-if ((RegEdx & BIT29) != 0) {
-  mNeedConfigureMtrrs = FALSE;
-}
-  }
-  
-  //
-  // Allocate array for state of SMRR enable on all CPUs
-  //
-  mSmrrEnabled = (BOOLEAN *)AllocatePool