[PATCH v2 0/2] AMD procesor MSR_IA32_MISC_ENABLE

[PATCH v2 0/2] AMD procesor MSR_IA32_MISC_ENABLE

[Kirkendall, Garrett]

AMD processor does not support MSR_IA32_MISC_ENABLE register.  Accessing
this register on AMD causes an unhandled exception in SmmEntry.nasm and
a subsequent failure to boot since this is too early in SMM path for the
exception handler to be loaded.

First, to distinguish between AMD and other processors, refactor
StandardSignatureIsAuthenticAMD into BaseUefiCpuLib.  So there is only
one copy in the source. All changed modules already include UefiCpuLib
either directly or indirectly so could not easly split first patch.

Second, Skip manipulation of MSR_IA32_MISC_ENABLE register if running
on an AMD processor.

Tested on AMD X64 hardware.
OvmfIa32 and OvmfIa32X64 on Intel hardware.

Garrett Kirkendall (2):
  UefiCpuPkg: Move StandardSignatureIsAuthenticAMD to BaseUefiCpuLib
  UefiCpuPkg: PiSmmCpuDxeSmm skip MSR_IA32_MISC_ENABLE manipulation on
    AMD

 UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.inf         |  7 ++++
 UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf             |  2 ++
 UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf |  2 ++
 UefiCpuPkg/Include/Library/UefiCpuLib.h                      | 14 ++++++++
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h               |  3 ++
 UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.c           | 38 ++++++++++++++++++++
 UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c               | 25 ++-----------
 UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c   | 25 ++-----------
 UefiCpuPkg/Library/MpInitLib/MpLib.c                         | 23 ------------
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c                       |  9 ++++-
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm                 | 19 ++++++++--
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm                  | 20 +++++++++--
 12 files changed, 113 insertions(+), 74 deletions(-)
 create mode 100644 UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.c

Changes at:
https://github.com/gkirkendall-amd/edk2/tree/smmentry_nasm_skip_msr_xd_bit_on_amd_v2

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Garrett Kirkendall <garrett.kirkendall@amd.com>

-- 
2.27.0

[PATCH v2 1/2] UefiCpuPkg: Move StandardSignatureIsAuthenticAMD to BaseUefiCpuLib

[Kirkendall, Garrett]

Refactor StandardSignatureIsAuthenticAMD into BaseUefiCpuLib from
separate copies in BaseXApicLib, BaseXApicX2ApicLib, and MpInitLib.
This allows for future use of StandarSignatureIsAuthinticAMD without
creating more instances in other modules.

This function allows IA32/X64 code to determine if it is running on an
AMD brand processor.

UefiCpuLib is already included directly or indirectly in all modified
modules.  Complete move is made in this change.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Garrett Kirkendall <garrett.kirkendall@amd.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Eric Dong <eric.dong@intel.com>
---
 UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.inf         |  7 ++++
 UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf             |  2 ++
 UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf |  2 ++
 UefiCpuPkg/Include/Library/UefiCpuLib.h                      | 14 ++++++++
 UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.c           | 38 ++++++++++++++++++++
 UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c               | 25 ++-----------
 UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c   | 25 ++-----------
 UefiCpuPkg/Library/MpInitLib/MpLib.c                         | 23 ------------
 8 files changed, 67 insertions(+), 69 deletions(-)

diff --git a/UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.inf b/UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.inf
index 006b7acbf14e..34d3a7bb4303 100644
--- a/UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.inf
+++ b/UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.inf
@@ -4,6 +4,7 @@
 #  The library routines are UEFI specification compliant.
 #
 #  Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
 ##
@@ -29,6 +30,12 @@ [Sources.IA32]
 [Sources.X64]
   X64/InitializeFpu.nasm
 
+[Sources]
+  BaseUefiCpuLib.c
+
 [Packages]
   MdePkg/MdePkg.dec
   UefiCpuPkg/UefiCpuPkg.dec
+
+[LibraryClasses]
+  BaseLib
diff --git a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
index bdb2ff372677..561baa44b0e6 100644
--- a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
+++ b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
@@ -5,6 +5,7 @@
 #  where local APIC is disabled.
 #
 #  Copyright (c) 2010 - 2018, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
 ##
@@ -37,6 +38,7 @@ [LibraryClasses]
   TimerLib
   IoLib
   PcdLib
+  UefiCpuLib
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuInitIpiDelayInMicroSeconds  ## SOMETIMES_CONSUMES
diff --git a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
index ac1e0a1c9896..1e2a4f8b790f 100644
--- a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
+++ b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
@@ -5,6 +5,7 @@
 #  where local APIC is disabled.
 #
 #  Copyright (c) 2010 - 2018, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
 ##
@@ -37,6 +38,7 @@ [LibraryClasses]
   TimerLib
   IoLib
   PcdLib
+  UefiCpuLib
 
 [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuInitIpiDelayInMicroSeconds  ## SOMETIMES_CONSUMES
diff --git a/UefiCpuPkg/Include/Library/UefiCpuLib.h b/UefiCpuPkg/Include/Library/UefiCpuLib.h
index 82e53bab3a0f..5326e7246301 100644
--- a/UefiCpuPkg/Include/Library/UefiCpuLib.h
+++ b/UefiCpuPkg/Include/Library/UefiCpuLib.h
@@ -5,6 +5,7 @@
   to be UEFI specification compliant.
 
   Copyright (c) 2009, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -29,4 +30,17 @@ InitializeFloatingPointUnits (
   VOID
   );
 
+/**
+  Determine if the standard CPU signature is "AuthenticAMD".
+
+  @retval TRUE  The CPU signature matches.
+  @retval FALSE The CPU signature does not match.
+
+**/
+BOOLEAN
+EFIAPI
+StandardSignatureIsAuthenticAMD (
+  VOID
+  );
+
 #endif
diff --git a/UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.c b/UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.c
new file mode 100644
index 000000000000..c2cc3ff9a709
--- /dev/null
+++ b/UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.c
@@ -0,0 +1,38 @@
+/** @file
+  This library defines some routines that are generic for IA32 family CPU.
+
+  The library routines are UEFI specification compliant.
+
+  Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Register/Intel/Cpuid.h>
+#include <Register/Amd/Cpuid.h>
+
+#include <Library/BaseLib.h>
+#include <Library/UefiCpuLib.h>
+
+/**
+  Determine if the standard CPU signature is "AuthenticAMD".
+
+  @retval TRUE  The CPU signature matches.
+  @retval FALSE The CPU signature does not match.
+
+**/
+BOOLEAN
+EFIAPI
+StandardSignatureIsAuthenticAMD (
+  VOID
+  )
+{
+  UINT32  RegEbx;
+  UINT32  RegEcx;
+  UINT32  RegEdx;
+
+  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
+  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
+          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
+          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
+}
diff --git a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
index 33ea15ca2916..52bd90d33428 100644
--- a/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
+++ b/UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c
@@ -4,7 +4,7 @@
   This local APIC library instance supports xAPIC mode only.
 
   Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR>
-  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
+  Copyright (c) 2017 - 2020, AMD Inc. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -21,33 +21,12 @@
 #include <Library/IoLib.h>
 #include <Library/TimerLib.h>
 #include <Library/PcdLib.h>
+#include <Library/UefiCpuLib.h>
 
 //
 // Library internal functions
 //
 
-/**
-  Determine if the standard CPU signature is "AuthenticAMD".
-
-  @retval TRUE  The CPU signature matches.
-  @retval FALSE The CPU signature does not match.
-
-**/
-BOOLEAN
-StandardSignatureIsAuthenticAMD (
-  VOID
-  )
-{
-  UINT32  RegEbx;
-  UINT32  RegEcx;
-  UINT32  RegEdx;
-
-  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
-  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
-          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
-          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
-}
-
 /**
   Determine if the CPU supports the Local APIC Base Address MSR.
 
diff --git a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
index d0f92b33dc8c..cdcbca046191 100644
--- a/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
+++ b/UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c
@@ -5,7 +5,7 @@
   which have xAPIC and x2APIC modes.
 
   Copyright (c) 2010 - 2019, Intel Corporation. All rights reserved.<BR>
-  Copyright (c) 2017, AMD Inc. All rights reserved.<BR>
+  Copyright (c) 2017 - 2020, AMD Inc. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -22,33 +22,12 @@
 #include <Library/IoLib.h>
 #include <Library/TimerLib.h>
 #include <Library/PcdLib.h>
+#include <Library/UefiCpuLib.h>
 
 //
 // Library internal functions
 //
 
-/**
-  Determine if the standard CPU signature is "AuthenticAMD".
-
-  @retval TRUE  The CPU signature matches.
-  @retval FALSE The CPU signature does not match.
-
-**/
-BOOLEAN
-StandardSignatureIsAuthenticAMD (
-  VOID
-  )
-{
-  UINT32  RegEbx;
-  UINT32  RegEcx;
-  UINT32  RegEdx;
-
-  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
-  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
-          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
-          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
-}
-
 /**
   Determine if the CPU supports the Local APIC Base Address MSR.
 
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index ab7a8ed6633a..9b0660a5d4ea 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -13,29 +13,6 @@
 EFI_GUID mCpuInitMpLibHobGuid = CPU_INIT_MP_LIB_HOB_GUID;
 
 
-/**
-  Determine if the standard CPU signature is "AuthenticAMD".
-
-  @retval TRUE  The CPU signature matches.
-  @retval FALSE The CPU signature does not match.
-
-**/
-STATIC
-BOOLEAN
-StandardSignatureIsAuthenticAMD (
-  VOID
-  )
-{
-  UINT32  RegEbx;
-  UINT32  RegEcx;
-  UINT32  RegEdx;
-
-  AsmCpuid (CPUID_SIGNATURE, NULL, &RegEbx, &RegEcx, &RegEdx);
-  return (RegEbx == CPUID_SIGNATURE_AUTHENTIC_AMD_EBX &&
-          RegEcx == CPUID_SIGNATURE_AUTHENTIC_AMD_ECX &&
-          RegEdx == CPUID_SIGNATURE_AUTHENTIC_AMD_EDX);
-}
-
 /**
   The function will check if BSP Execute Disable is enabled.
 
-- 
2.27.0

[PATCH v2 2/2] UefiCpuPkg: PiSmmCpuDxeSmm skip MSR_IA32_MISC_ENABLE manipulation on AMD

[Kirkendall, Garrett]

AMD does not support MSR_IA32_MISC_ENABLE.  Accessing that register
causes and exception on AMD processors.  If Execution Disable is
supported, but if the processor is an AMD processor, skip manipulating
MSR_IA32_MISC_ENABLE[34] XD Disable bit.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Garrett Kirkendall <garrett.kirkendall@amd.com>
---

Notes:
    Tested on Intel hardware with Laszlo Ersek's help
    
    (1) downloaded two Linux images from provided links.
    (2) Test using a 32-bit guest on an Intel host (standing in your edk2 tree, with the patches applied):
    
    $ build -a IA32 -b DEBUG -p OvmfPkg/OvmfPkgIa32.dsc -t GCC5 -D SMM_REQUIRE
    
    $ qemu-system-i386 \
        -cpu coreduo,-nx \
        -machine q35,smm=on,accel=kvm \
        -m 4096 \
        -smp 4 \
        -global driver=cfi.pflash01,property=secure,value=on \
        -drive if=pflash,format=raw,unit=0,readonly=on,file=Build/OvmfIa32/DEBUG_GCC5/FV/OVMF_CODE.fd \
        -drive if=pflash,format=raw,unit=1,snapshot=on,file=Build/OvmfIa32/DEBUG_GCC5/FV/OVMF_VARS.fd \
        -drive id=hdd,if=none,format=qcow2,snapshot=on,file=fedora-30-efi-systemd-i686.qcow2 \
        -device virtio-scsi-pci,id=scsi0 \
        -device scsi-hd,drive=hdd,bus=scsi0.0,bootindex=1
    
    (Once you get a login prompt, feel free to interrupt QEMU with Ctrl-C.)
    
    (3) Test using a 64-bit guest on an Intel host:
    
    $ build -a IA32 -a X64 -b DEBUG -p OvmfPkg/OvmfPkgIa32X64.dsc -t GCC5 -D SMM_REQUIRE
    
    $ qemu-system-x86_64 \
        -cpu host \
        -machine q35,smm=on,accel=kvm \
        -m 4096 \
        -smp 4 \
        -global driver=cfi.pflash01,property=secure,value=on \
        -drive if=pflash,format=raw,unit=0,readonly=on,file=Build/Ovmf3264/DEBUG_GCC5/FV/OVMF_CODE.fd \
        -drive if=pflash,format=raw,unit=1,snapshot=on,file=Build/Ovmf3264/DEBUG_GCC5/FV/OVMF_VARS.fd \
        -drive id=hdd,if=none,format=qcow2,snapshot=on,file=fedora-31-efi-grub2-x86_64.qcow2 \
        -device virtio-scsi-pci,id=scsi0 \
        -device scsi-hd,drive=hdd,bus=scsi0.0,bootindex=1
    
    Tested on real AMD Hardware

 UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h |  3 +++
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c         |  9 ++++++++-
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm   | 19 +++++++++++++++++--
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm    | 20 ++++++++++++++++++--
 4 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h
index 43f6935cf9dc..993360a8a8c1 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h
@@ -2,6 +2,7 @@
 SMM profile internal header file.
 
 Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2020, AMD Incorporated. All rights reserved.<BR>
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -13,6 +14,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Library/UefiRuntimeServicesTableLib.h>
 #include <Library/DxeServicesTableLib.h>
 #include <Library/CpuLib.h>
+#include <Library/UefiCpuLib.h>
 #include <IndustryStandard/Acpi.h>
 
 #include "SmmProfileArch.h"
@@ -99,6 +101,7 @@ extern SMM_S3_RESUME_STATE       *mSmmS3ResumeState;
 extern UINTN                     gSmiExceptionHandlers[];
 extern BOOLEAN                   mXdSupported;
 X86_ASSEMBLY_PATCH_LABEL         gPatchXdSupported;
+X86_ASSEMBLY_PATCH_LABEL         gPatchMsrIa32MiscEnableSupported;
 extern UINTN                     *mPFEntryCount;
 extern UINT64                    (*mLastPFEntryValue)[MAX_PF_ENTRY_COUNT];
 extern UINT64                    *(*mLastPFEntryPointer)[MAX_PF_ENTRY_COUNT];
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
index c47b5573e366..d7ed9ab7a770 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
@@ -2,7 +2,7 @@
 Enable SMM profile.
 
 Copyright (c) 2012 - 2019, Intel Corporation. All rights reserved.<BR>
-Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
+Copyright (c) 2017 - 2020, AMD Incorporated. All rights reserved.<BR>
 
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -1015,6 +1015,13 @@ CheckFeatureSupported (
       mXdSupported = FALSE;
       PatchInstructionX86 (gPatchXdSupported, mXdSupported, 1);
     }
+
+    if (StandardSignatureIsAuthenticAMD ()) {
+      //
+      // AMD processors do not support MSR_IA32_MISC_ENABLE
+      //
+      PatchInstructionX86 (gPatchMsrIa32MiscEnableSupported, FALSE, 1);
+    }
   }
 
   if (mBtsSupported) {
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
index f96de9bdeb43..167f5e14dbd4 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
@@ -1,5 +1,6 @@
 ;------------------------------------------------------------------------------ ;
 ; Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.<BR>
+; Copyright (c) 2020, AMD Incorporated. All rights reserved.<BR>
 ; SPDX-License-Identifier: BSD-2-Clause-Patent
 ;
 ; Module Name:
@@ -59,6 +60,7 @@ global ASM_PFX(gPatchSmiStack)
 global ASM_PFX(gPatchSmbase)
 extern ASM_PFX(mXdSupported)
 global ASM_PFX(gPatchXdSupported)
+global ASM_PFX(gPatchMsrIa32MiscEnableSupported)
 extern ASM_PFX(gSmiHandlerIdtr)
 
 extern ASM_PFX(mCetSupported)
@@ -153,17 +155,30 @@ ASM_PFX(gPatchSmiCr3):
 ASM_PFX(gPatchXdSupported):
     cmp     al, 0
     jz      @SkipXd
+
+; If MSR_IA32_MISC_ENABLE is supported, clear XD Disable bit
+    mov     al, strict byte 1           ; source operand may be patched
+ASM_PFX(gPatchMsrIa32MiscEnableSupported):
+    cmp     al, 1
+    jz      MsrIa32MiscEnableSupported
+
+; MSR_IA32_MISC_ENABLE not supported
+    xor     edx, edx
+    push    edx                         ; don't try to restore the XD Disable bit just before RSM
+    jmp     EnableNxe
+
 ;
 ; Check XD disable bit
 ;
+MsrIa32MiscEnableSupported:
     mov     ecx, MSR_IA32_MISC_ENABLE
     rdmsr
     push    edx                        ; save MSR_IA32_MISC_ENABLE[63-32]
     test    edx, BIT2                  ; MSR_IA32_MISC_ENABLE[34]
-    jz      .5
+    jz      EnableNxe
     and     dx, 0xFFFB                 ; clear XD Disable bit if it is set
     wrmsr
-.5:
+EnableNxe:
     mov     ecx, MSR_EFER
     rdmsr
     or      ax, MSR_EFER_XD             ; enable NXE
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
index 8bfba55b5d08..0e154e5db949 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
@@ -1,5 +1,6 @@
 ;------------------------------------------------------------------------------ ;
 ; Copyright (c) 2016 - 2019, Intel Corporation. All rights reserved.<BR>
+; Copyright (c) 2020, AMD Incorporated. All rights reserved.<BR>
 ; SPDX-License-Identifier: BSD-2-Clause-Patent
 ;
 ; Module Name:
@@ -67,6 +68,7 @@ extern ASM_PFX(CpuSmmDebugExit)
 global ASM_PFX(gPatchSmbase)
 extern ASM_PFX(mXdSupported)
 global ASM_PFX(gPatchXdSupported)
+global ASM_PFX(gPatchMsrIa32MiscEnableSupported)
 global ASM_PFX(gPatchSmiStack)
 global ASM_PFX(gPatchSmiCr3)
 global ASM_PFX(gPatch5LevelPagingNeeded)
@@ -152,18 +154,32 @@ SkipEnable5LevelPaging:
 ASM_PFX(gPatchXdSupported):
     cmp     al, 0
     jz      @SkipXd
+
+; If MSR_IA32_MISC_ENABLE is supported, clear XD Disable bit
+    mov     al, strict byte 1           ; source operand may be patched
+ASM_PFX(gPatchMsrIa32MiscEnableSupported):
+    cmp     al, 1
+    jz      MsrIa32MiscEnableSupported
+
+; MSR_IA32_MISC_ENABLE not supported
+    sub     esp, 4
+    xor     rdx, rdx
+    push    rdx                         ; don't try to restore the XD Disable bit just before RSM
+    jmp     EnableNxe
+
 ;
 ; Check XD disable bit
 ;
+MsrIa32MiscEnableSupported:
     mov     ecx, MSR_IA32_MISC_ENABLE
     rdmsr
     sub     esp, 4
     push    rdx                        ; save MSR_IA32_MISC_ENABLE[63-32]
     test    edx, BIT2                  ; MSR_IA32_MISC_ENABLE[34]
-    jz      .0
+    jz      EnableNxe
     and     dx, 0xFFFB                 ; clear XD Disable bit if it is set
     wrmsr
-.0:
+EnableNxe:
     mov     ecx, MSR_EFER
     rdmsr
     or      ax, MSR_EFER_XD            ; enable NXE
-- 
2.27.0

Re: [PATCH v2 0/2] AMD procesor MSR_IA32_MISC_ENABLE

[Dong, Eric]

Hi Garrett,

I create a pull request to verify your changes and it reports some errors for your changes. 
https://github.com/tianocore/edk2/pull/710

please help to resolve these errors before sending your new version changes, also you can create your PR to verify your new changes.

Thanks,
Eric

> -----Original Message-----
> From: Garrett Kirkendall <Garrett.Kirkendall@amd.com>
> Sent: Thursday, June 18, 2020 11:23 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo
> Ersek <lersek@redhat.com>
> Subject: [PATCH v2 0/2] AMD procesor MSR_IA32_MISC_ENABLE
> 
> AMD processor does not support MSR_IA32_MISC_ENABLE register.
> Accessing this register on AMD causes an unhandled exception in
> SmmEntry.nasm and a subsequent failure to boot since this is too early in
> SMM path for the exception handler to be loaded.
> 
> First, to distinguish between AMD and other processors, refactor
> StandardSignatureIsAuthenticAMD into BaseUefiCpuLib.  So there is only one
> copy in the source. All changed modules already include UefiCpuLib either
> directly or indirectly so could not easly split first patch.
> 
> Second, Skip manipulation of MSR_IA32_MISC_ENABLE register if running on
> an AMD processor.
> 
> Tested on AMD X64 hardware.
> OvmfIa32 and OvmfIa32X64 on Intel hardware.
> 
> Garrett Kirkendall (2):
>   UefiCpuPkg: Move StandardSignatureIsAuthenticAMD to BaseUefiCpuLib
>   UefiCpuPkg: PiSmmCpuDxeSmm skip MSR_IA32_MISC_ENABLE
> manipulation on
>     AMD
> 
>  UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.inf         |  7 ++++
>  UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf             |  2 ++
>  UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf |  2 ++
>  UefiCpuPkg/Include/Library/UefiCpuLib.h                      | 14 ++++++++
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h               |  3 ++
>  UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.c           | 38
> ++++++++++++++++++++
>  UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.c               | 25 ++-----------
>  UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.c   | 25 ++------
> -----
>  UefiCpuPkg/Library/MpInitLib/MpLib.c                         | 23 ------------
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c                       |  9 ++++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm                 | 19
> ++++++++--
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm                  | 20
> +++++++++--
>  12 files changed, 113 insertions(+), 74 deletions(-)  create mode 100644
> UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.c
> 
> Changes at:
> https://github.com/gkirkendall-
> amd/edk2/tree/smmentry_nasm_skip_msr_xd_bit_on_amd_v2
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Garrett Kirkendall <garrett.kirkendall@amd.com>
> 
> --
> 2.27.0

Re: [edk2-devel] [PATCH v2 0/2] AMD procesor MSR_IA32_MISC_ENABLE

[Laszlo Ersek]

Hi Garrett,

On 06/19/20 03:00, Dong, Eric wrote:
> Hi Garrett,
>
> I create a pull request to verify your changes and it reports some
> errors for your changes.  https://github.com/tianocore/edk2/pull/710
>
> please help to resolve these errors before sending your new version
> changes, also you can create your PR to verify your new changes.

It seems like the standalone "PcAtChipsetPkg.dsc" build failed.

The reason seems to be that PcAtChipsetPkg.dsc contains a lib class
resolution like

  LocalApicLib|UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf

but does not contain a UefiCpuLib class resolution.

Speaking more generally, the following DSC files reference at least one
of "BaseXApicLib.inf" and "BaseXApicX2ApicLib.inf":

- edk2:

  IntelFsp2WrapperPkg/IntelFsp2WrapperPkg.dsc
  OvmfPkg/OvmfPkgIa32.dsc
  OvmfPkg/OvmfPkgIa32X64.dsc
  OvmfPkg/OvmfPkgX64.dsc
  OvmfPkg/OvmfXen.dsc
  PcAtChipsetPkg/PcAtChipsetPkg.dsc
  SourceLevelDebugPkg/SourceLevelDebugPkg.dsc
  UefiCpuPkg/UefiCpuPkg.dsc
  UefiPayloadPkg/UefiPayloadPkgIa32.dsc
  UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc

- edk2-platforms:

  Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
  Platform/Intel/QuarkPlatformPkg/Quark.dsc
  Platform/Intel/QuarkPlatformPkg/QuarkMin.dsc
  Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgIA32.dsc
  Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgX64.dsc

Among these files, the following ones do *not* already have the

  UefiCpuLib|UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.inf

lib class resolution:

- edk2:

  PcAtChipsetPkg/PcAtChipsetPkg.dsc           [found by CI]
  SourceLevelDebugPkg/SourceLevelDebugPkg.dsc

- edk2-platforms:

  [none]

Therefore, this patch set is sound, it is just incomplete. We need two
patches *prepended*: one patch that adds the UefiCpuLib resolution to
"PcAtChipsetPkg/PcAtChipsetPkg.dsc", and another that adds the same
resolution to "SourceLevelDebugPkg/SourceLevelDebugPkg.dsc". Then the
current (v2) patches can be included verbatim as patch v3 #3, and patch
v3 #4.

Garrett: you can also invoke a personal CI run -- just submit a
github.com pull request with your v3 patches, before posting them to the
list. You will not be able to set the "push" label on your PR, so the CI
system will auto-close the PR (without merging it). But, it will tell
you the build result (pass or failure).

Thanks!
Laszlo

Re: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg: PiSmmCpuDxeSmm skip MSR_IA32_MISC_ENABLE manipulation on AMD

[Laszlo Ersek]

On 06/18/20 17:22, Kirkendall, Garrett wrote:
> AMD does not support MSR_IA32_MISC_ENABLE.  Accessing that register
> causes and exception on AMD processors.  If Execution Disable is
> supported, but if the processor is an AMD processor, skip manipulating
> MSR_IA32_MISC_ENABLE[34] XD Disable bit.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Garrett Kirkendall <garrett.kirkendall@amd.com>
> ---
>
> Notes:
>     Tested on Intel hardware with Laszlo Ersek's help
>
>     (1) downloaded two Linux images from provided links.
>     (2) Test using a 32-bit guest on an Intel host (standing in your edk2 tree, with the patches applied):
>
>     $ build -a IA32 -b DEBUG -p OvmfPkg/OvmfPkgIa32.dsc -t GCC5 -D SMM_REQUIRE
>
>     $ qemu-system-i386 \
>         -cpu coreduo,-nx \
>         -machine q35,smm=on,accel=kvm \
>         -m 4096 \
>         -smp 4 \
>         -global driver=cfi.pflash01,property=secure,value=on \
>         -drive if=pflash,format=raw,unit=0,readonly=on,file=Build/OvmfIa32/DEBUG_GCC5/FV/OVMF_CODE.fd \
>         -drive if=pflash,format=raw,unit=1,snapshot=on,file=Build/OvmfIa32/DEBUG_GCC5/FV/OVMF_VARS.fd \
>         -drive id=hdd,if=none,format=qcow2,snapshot=on,file=fedora-30-efi-systemd-i686.qcow2 \
>         -device virtio-scsi-pci,id=scsi0 \
>         -device scsi-hd,drive=hdd,bus=scsi0.0,bootindex=1
>
>     (Once you get a login prompt, feel free to interrupt QEMU with Ctrl-C.)
>
>     (3) Test using a 64-bit guest on an Intel host:
>
>     $ build -a IA32 -a X64 -b DEBUG -p OvmfPkg/OvmfPkgIa32X64.dsc -t GCC5 -D SMM_REQUIRE
>
>     $ qemu-system-x86_64 \
>         -cpu host \
>         -machine q35,smm=on,accel=kvm \
>         -m 4096 \
>         -smp 4 \
>         -global driver=cfi.pflash01,property=secure,value=on \
>         -drive if=pflash,format=raw,unit=0,readonly=on,file=Build/Ovmf3264/DEBUG_GCC5/FV/OVMF_CODE.fd \
>         -drive if=pflash,format=raw,unit=1,snapshot=on,file=Build/Ovmf3264/DEBUG_GCC5/FV/OVMF_VARS.fd \
>         -drive id=hdd,if=none,format=qcow2,snapshot=on,file=fedora-31-efi-grub2-x86_64.qcow2 \
>         -device virtio-scsi-pci,id=scsi0 \
>         -device scsi-hd,drive=hdd,bus=scsi0.0,bootindex=1
>
>     Tested on real AMD Hardware
>
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h |  3 +++
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c         |  9 ++++++++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm   | 19 +++++++++++++++++--
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm    | 20 ++++++++++++++++++--
>  4 files changed, 46 insertions(+), 5 deletions(-)

For this patch:

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

However, can you please clarify one bit, with regard to testing:

I understand the regression tests in a 32-bit guest on Intel, and in a
64-bit guests on Intel. I also understand the test on a physical AMD
machine (in other words, you built a physical platform firmware for an
AMD board, flashed it, and tested it.)

However, your notes do not seem to mention test (4) that I requested:

"(4) Test using a 64-bit guest on an AMD host -- just repeat step (3) on
an AMD host."

https://edk2.groups.io/g/devel/message/61344

(alternative link:
<http://mid.mail-archive.com/5f2fd5a9-2107-503a-406b-de08529dcb56@redhat.com>)

That is also important. Test (3) intentionally uses "-cpu host", so the
guest will see a different CPU model when run on an Intel host (3) vs.
on an AMD host (4). I'd like to know that the patched 64-bit SMI entry
code continues working in both guest environments (i.e. when seeing
either an Intel CPU model or an AMD CPU model).

I did not request the same "duality" with test (2) -- as you see, there
I wrote "-cpu coreduo,-nx"; i.e., Intel only. The reason is that "-cpu
coreduo,-nx" is more or less the only 32-bit SMM environment that I
generally know about, and verify -- I simply do not have a 32-bit AMD
CPU model "baseline" to test against. This is why I didn't request "-cpu
host" in (2), and consequently, why I didn't request (2) to be run on
both AMD and Intel hosts.

So... Can you please run test (4) too -- i.e., 64-bit guest on an AMD
host, with "-cpu host"?

Thanks!
Laszlo