Re: [PATCH v6 4/4] UefiCpuPkg: PiSmmCpuDxeSmm skip MSR_IA32_MISC_ENABLE manipulation on AMD

["Dong, Eric" <eric.dong@intel.com>]

Reviewed-by: Eric Dong <eric.dong@intel.com>

> -----Original Message-----
> From: Garrett Kirkendall <Garrett.Kirkendall@amd.com>
> Sent: Monday, June 22, 2020 9:18 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 v6 4/4] UefiCpuPkg: PiSmmCpuDxeSmm skip
> MSR_IA32_MISC_ENABLE manipulation on AMD
> 
> 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_GCC
> 5/FV/OVMF_CODE.fd \
>         -drive
> if=pflash,format=raw,unit=1,snapshot=on,file=Build/OvmfIa32/DEBUG_GCC
> 5/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_GCC
> 5/FV/OVMF_CODE.fd \
>         -drive
> if=pflash,format=raw,unit=1,snapshot=on,file=Build/Ovmf3264/DEBUG_GCC
> 5/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