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 <eric.d...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Signed-off-by: Garrett Kirkendall <garrett.kirkend...@amd.com>
> ---
> 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.<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;
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.<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()) {
(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.<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
> +
> +; 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.<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
> +
> +; 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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61343): https://edk2.groups.io/g/devel/message/61343
Mute This Topic: https://groups.io/mt/74901070/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
