From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web11.805.1592324898444467115 for ; Tue, 16 Jun 2020 09:28:19 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Nf50oye4; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1592324897; 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=x/8yOA4lNHncTEY11IP6cutWw31DYJ30AhKVlpYjDJs=; b=Nf50oye4tDthqs0nG/0vA6/rpC36Vgn9iY2U+fZryhYgGhMmLji3Zyj70ffnQKKoWW4UPa xkFHzsNzoVOzB/hCr8w1WY6BvLvofSf9yvhlXAiYZDnwPS8JBZR9/uvAa9Qu7fODPc9rir +R5J/EGcxJGJG1A25LCdyf4SvG0/Zmc= 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-39-ERuf1WqBM4yfLPwD0Dwjsw-1; Tue, 16 Jun 2020 12:28:15 -0400 X-MC-Unique: ERuf1WqBM4yfLPwD0Dwjsw-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 5CFD0873426; Tue, 16 Jun 2020 16:28:14 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-248.ams2.redhat.com [10.36.113.248]) by smtp.corp.redhat.com (Postfix) with ESMTP id D6FDA79315; Tue, 16 Jun 2020 16:28:12 +0000 (UTC) Subject: Re: [PATCH v1 2/2] UefiCpuPkg: PiSmmCpuDxeSmm skip MSR_IA32_MISC_ENABLE manipulation on AMD To: Garrett Kirkendall , devel@edk2.groups.io Cc: Eric Dong , Ray Ni References: <20200615183029.4577-1-Garrett.Kirkendall@amd.com> <20200615183029.4577-3-Garrett.Kirkendall@amd.com> From: "Laszlo Ersek" Message-ID: <97527091-8eeb-ce23-c199-d703f22573ea@redhat.com> Date: Tue, 16 Jun 2020 18:28:11 +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: <20200615183029.4577-3-Garrett.Kirkendall@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com 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/15/20 20:30, 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, and if the processor is an AMD processor skip manipulating > MSR_IA32_MISC_ENABLE[34] XD Disable bit. (1a) I suggest replacing "and if" with "but". (1b) I suggest inserting a comman before "skip". (Side comment: someone give a medal to whomever invented the name "Execution Disable Disable".) > > Cc: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > Signed-off-by: Garrett Kirkendall > --- > 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..0f6e0c9c98ad 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; The edk2 coding style subjects acronyms to CamelCasing. For example, "PCI" is CamelCased as "Pci" in identifiers. (2) Therefore, please replace "IA32" with "Ia32" in the above. > 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..146600e6c3b4 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()) { (3) Please insert a space character before the opening parenthesis. > + // > + // 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..e4ef5899f274 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 > + > +; Clear XD Disable bit if supported (4) please expand the comment: "... if MSR_IA32_MISC_ENABLE is supported" > + 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 (5) Please append a comment to the right: "; don't try to restore the XD Disable bit just before RSM" > + jmp EnableNxe > + > ; > ; Check XD disable bit > ; > +MsrIA32MiscEnableSupported: (6) Please CamelCase "IA32" > 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 Please apply changes (2) through (6) to the X64 version too. Other than these cosmetic comments, the patch looks good to me. I'm ready to R-b with the coding style / comments / commit message updated. Thanks! Laszlo > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm > index 8bfba55b5d08..5a4678c7aa63 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 > + > +; Clear XD Disable bit if supported > + 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 > + 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 >