From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web10.19575.1592839073971023509 for ; Mon, 22 Jun 2020 08:17:54 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=dLsfyxpY; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1592839073; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=LQLFgMtPiWmLyKhR9F2rIXXhBCm16d+m9ShbBkDrqHs=; b=dLsfyxpYMhTWI6wAfplhNyeexATeAt/82i4lLjrhGaWQZc31V2yse6FsNE+u3VT6N7FSdp UgQZVXCo2MaSTXkFxqN83UFssMaxrYtZfZmXbePGArwCvls5ah1UOatbbMLhaulwNwrss6 tm11lTrmtUyXrf+8C/SKAymdizTAqnA= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-167-gWOLsLIQNWyQEf5jElxf0w-1; Mon, 22 Jun 2020 11:17:44 -0400 X-MC-Unique: gWOLsLIQNWyQEf5jElxf0w-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 4D7B518A076E; Mon, 22 Jun 2020 15:17:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-246.ams2.redhat.com [10.36.115.246]) by smtp.corp.redhat.com (Postfix) with ESMTP id 268131C8; Mon, 22 Jun 2020 15:17:41 +0000 (UTC) Subject: Re: [PATCH v6 4/4] UefiCpuPkg: PiSmmCpuDxeSmm skip MSR_IA32_MISC_ENABLE manipulation on AMD To: Garrett Kirkendall , devel@edk2.groups.io Cc: Eric Dong , Ray Ni References: <20200622131825.1352-1-Garrett.Kirkendall@amd.com> <20200622131825.1352-5-Garrett.Kirkendall@amd.com> From: "Laszlo Ersek" Message-ID: <198fae29-0b58-2f67-c9b2-3def6c142417@redhat.com> Date: Mon, 22 Jun 2020 17:17:41 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200622131825.1352-5-Garrett.Kirkendall@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 06/22/20 15:18, Garrett Kirkendall 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 > Cc: Ray Ni > Cc: Laszlo Ersek > Signed-off-by: Garrett Kirkendall > --- When carrying forward a patch unmodified from the previous version of the series, then please pick up the feedback tags given under the previous version. See e.g.: https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-28 So, from / : Reviewed-by: Laszlo Ersek Tested-by: Laszlo Ersek BTW the series looks well-formatted to me, on the list, this time around. Thanks Laszlo > > 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.
> +Copyright (c) 2020, AMD Incorporated. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -13,6 +14,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include > #include > #include > +#include > #include > > #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.
> -Copyright (c) 2017, AMD Incorporated. All rights reserved.
> +Copyright (c) 2017 - 2020, AMD Incorporated. All rights reserved.
> > 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.
> +; Copyright (c) 2020, AMD Incorporated. All rights reserved.
> ; 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.
> +; Copyright (c) 2020, AMD Incorporated. All rights reserved.
> ; 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 >