* [PATCH 0/6] Fix issues caused by NX memory protection
@ 2018-01-15 8:54 Jian J Wang
2018-01-15 8:54 ` [PATCH 1/6] UefiCpuPkg/MpInitLib: split wake up buffer into two parts Jian J Wang
` (5 more replies)
0 siblings, 6 replies; 22+ messages in thread
From: Jian J Wang @ 2018-01-15 8:54 UTC (permalink / raw)
To: edk2-devel
NX memory protection feature enabled by PcdDxeNxMemoryProtectionPolicy
was not fully tested, especially if it's enabled for memory with type
of EfiBootServicesCode, EfiConventionalMemory and EfiReservedMemoryType.
This series will fix all issues caused by it.
Jian J Wang (6):
UefiCpuPkg/MpInitLib: split wake up buffer into two parts
UefiCpuPkg/CpuExceptionHandlerLib: alloc code memory for exception
handlers
UefiCpuPkg/CpuDxe: clear NX attr for page directory
UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's available
MdeModulePkg/PiSmmCore: remove NX attr from SMM RAM
MdeModulePkg/BootScriptExecutorDxe: remove NX attr for FfsBuffer
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 18 ++++-
.../BootScriptExecutorDxe.inf | 1 +
.../Acpi/BootScriptExecutorDxe/ScriptExecute.c | 14 ++++
.../Acpi/BootScriptExecutorDxe/ScriptExecute.h | 1 +
UefiCpuPkg/CpuDxe/CpuPageTable.c | 6 +-
.../Library/CpuExceptionHandlerLib/DxeException.c | 18 ++++-
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 34 +++++++++
UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc | 5 ++
UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 32 ++++----
UefiCpuPkg/Library/MpInitLib/MpLib.c | 45 +++++++++++
UefiCpuPkg/Library/MpInitLib/MpLib.h | 22 ++++++
UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 23 ++++++
UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc | 5 +-
UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 87 ++++++++++++++--------
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 14 ++++
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm | 12 ++-
16 files changed, 278 insertions(+), 59 deletions(-)
--
2.15.1.windows.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/6] UefiCpuPkg/MpInitLib: split wake up buffer into two parts
2018-01-15 8:54 [PATCH 0/6] Fix issues caused by NX memory protection Jian J Wang
@ 2018-01-15 8:54 ` Jian J Wang
2018-01-18 6:53 ` Dong, Eric
2018-01-27 16:17 ` Laszlo Ersek
2018-01-15 8:54 ` [PATCH 2/6] UefiCpuPkg/CpuExceptionHandlerLib: alloc code memory for exception handlers Jian J Wang
` (4 subsequent siblings)
5 siblings, 2 replies; 22+ messages in thread
From: Jian J Wang @ 2018-01-15 8:54 UTC (permalink / raw)
To: edk2-devel; +Cc: Jiewen Yao, Ruiyu Ni, Eric Dong, Laszlo Ersek
If PcdDxeNxMemoryProtectionPolicy is set to enable protection for memory
of EfiBootServicesCode, EfiConventionalMemory, the BIOS will hang at a page
fault exception during MP initialization.
The root cause is that the AP wake up buffer, which is below 1MB and used
to hold both AP init code and data, is type of EfiConventionalMemory (not
really allocated because of potential conflict with legacy code), and is
marked as non-executable. During the transition from real address mode
to long mode, the AP init code has to enable paging which will then cause
itself a page fault exception because it's just running in non-executable
memory.
The solution is splitting AP wake up buffer into two part: lower part is
still below 1MB and shared with legacy system, higher part is really
allocated memory of BootServicesCode type. The init code in the memory
below 1MB will not enable paging but just switch to protected mode and
jump to higher memory, in which the init code will enable paging and
switch to long mode.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 34 ++++++++++
UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc | 5 ++
UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 32 +++++-----
UefiCpuPkg/Library/MpInitLib/MpLib.c | 45 +++++++++++++
UefiCpuPkg/Library/MpInitLib/MpLib.h | 22 +++++++
UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 23 +++++++
UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc | 5 +-
UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 87 ++++++++++++++++----------
8 files changed, 204 insertions(+), 49 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index d2bcef53d6..fd2317924f 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -113,6 +113,40 @@ GetWakeupBuffer (
return (UINTN) StartAddress;
}
+/**
+ Get available EfiBootServicesCode memory below 4GB by specified size.
+
+ This buffer is required to safely transfer AP from real address mode to
+ protected mode or long mode, due to the fact that the buffer returned by
+ GetWakeupBuffer() may be marked as non-executable.
+
+ @param[in] BufferSize Wakeup transition buffer size.
+
+ @retval other Return wakeup transition buffer address below 4GB.
+ @retval 0 Cannot find free memory below 4GB.
+**/
+UINTN
+GetModeTransitionBuffer (
+ IN UINTN BufferSize
+ )
+{
+ EFI_STATUS Status;
+ EFI_PHYSICAL_ADDRESS StartAddress;
+
+ StartAddress = BASE_4GB - 1;
+ Status = gBS->AllocatePages (
+ AllocateMaxAddress,
+ EfiBootServicesCode,
+ EFI_SIZE_TO_PAGES (BufferSize),
+ &StartAddress
+ );
+ if (EFI_ERROR (Status)) {
+ StartAddress = 0;
+ }
+
+ return (UINTN)StartAddress;
+}
+
/**
Checks APs status and updates APs status if needed.
diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
index bdfe0d33cc..1648f2c4b0 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
@@ -41,4 +41,9 @@ Cr3Location equ LockLocation + 34h
InitFlagLocation equ LockLocation + 38h
CpuInfoLocation equ LockLocation + 3Ch
NumApsExecutingLocation equ LockLocation + 40h
+InitializeFloatingPointUnitsAddress equ LockLocation + 48h
+ModeTransitionMemoryLocation equ LockLocation + 4Ch
+ModeTransitionSegmentLocation equ LockLocation + 50h
+ModeHighMemoryLocation equ LockLocation + 52h
+ModeHighSegmentLocation equ LockLocation + 56h
diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
index 2b6c27d4ec..bd79be0f5e 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
@@ -48,34 +48,35 @@ BITS 16
mov si, BufferStartLocation
mov ebx, [si]
- mov si, ModeOffsetLocation
- mov eax, [si]
- mov si, CodeSegmentLocation
- mov edx, [si]
- mov di, ax
- sub di, 02h
- mov [di], dx
- sub di, 04h
- add eax, ebx
- mov [di],eax
-
mov si, DataSegmentLocation
mov edx, [si]
+ ;
+ ; Get start address of 32-bit code in low memory (<1MB)
+ ;
+ mov edi, ModeTransitionMemoryLocation
+
mov si, GdtrLocation
o32 lgdt [cs:si]
mov si, IdtrLocation
o32 lidt [cs:si]
- xor ax, ax
- mov ds, ax
-
+ ;
+ ; Switch to protected mode
+ ;
mov eax, cr0 ; Get control register 0
or eax, 000000003h ; Set PE bit (bit #0) & MP
mov cr0, eax
- jmp 0:strict dword 0 ; far jump to protected mode
+ ; Switch to 32-bit code in executable memory (>1MB)
+o32 jmp far [cs:di]
+
+;
+; Following code may be copied to memory with type of EfiBootServicesCode.
+; This is required at DXE phase if NX is enabled for EfiBootServicesCode of
+; memory.
+;
BITS 32
Flat32Start: ; protected mode entry point
mov ds, dx
@@ -266,6 +267,7 @@ ASM_PFX(AsmGetAddressMap):
mov dword [ebx + 8h], RendezvousFunnelProcEnd - RendezvousFunnelProcStart
mov dword [ebx + 0Ch], AsmRelocateApLoopStart
mov dword [ebx + 10h], AsmRelocateApLoopEnd - AsmRelocateApLoopStart
+ mov dword [ebx + 14h], Flat32Start - RendezvousFunnelProcStart
popad
ret
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index cdc03113e5..fbcbcc6cc9 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -772,6 +772,8 @@ FillExchangeInfoData (
)
{
volatile MP_CPU_EXCHANGE_INFO *ExchangeInfo;
+ UINTN Size;
+ IA32_SEGMENT_DESCRIPTOR *Selector;
ExchangeInfo = CpuMpData->MpCpuExchangeInfo;
ExchangeInfo->Lock = 0;
@@ -801,6 +803,44 @@ FillExchangeInfoData (
//
AsmReadGdtr ((IA32_DESCRIPTOR *) &ExchangeInfo->GdtrProfile);
AsmReadIdtr ((IA32_DESCRIPTOR *) &ExchangeInfo->IdtrProfile);
+
+ //
+ // Find a 32-bit code segment
+ //
+ Selector = (IA32_SEGMENT_DESCRIPTOR *)ExchangeInfo->GdtrProfile.Base;
+ Size = ExchangeInfo->GdtrProfile.Limit + 1;
+ while (Size > 0) {
+ if (Selector->Bits.L == 0 && Selector->Bits.Type >= 8) {
+ ExchangeInfo->ModeTransitionSegment =
+ (UINT16)((UINTN)Selector - ExchangeInfo->GdtrProfile.Base);
+ break;
+ }
+ Selector += 1;
+ Size -= sizeof (IA32_SEGMENT_DESCRIPTOR);
+ }
+
+ //
+ // Copy all 32-bit code and 64-bit code into memory with type of
+ // EfiBootServicesCode to avoid page fault if NX memory protection is enabled.
+ //
+ if (ExchangeInfo->ModeTransitionMemory != 0) {
+ Size = CpuMpData->AddressMap.RendezvousFunnelSize -
+ CpuMpData->AddressMap.ModeTransitionOffset;
+ CopyMem (
+ (VOID *)(UINTN)ExchangeInfo->ModeTransitionMemory,
+ CpuMpData->AddressMap.RendezvousFunnelAddress +
+ CpuMpData->AddressMap.ModeTransitionOffset,
+ Size
+ );
+
+ ExchangeInfo->ModeHighMemory = ExchangeInfo->ModeTransitionMemory;
+ ExchangeInfo->ModeHighMemory += (UINT32)ExchangeInfo->ModeOffset -
+ (UINT32)CpuMpData->AddressMap.ModeTransitionOffset;
+ ExchangeInfo->ModeHighSegment = (UINT16)ExchangeInfo->CodeSegment;
+ } else {
+ ExchangeInfo->ModeTransitionMemory = (UINT32)
+ (ExchangeInfo->BufferStart + CpuMpData->AddressMap.ModeTransitionOffset);
+ }
}
/**
@@ -876,6 +916,11 @@ AllocateResetVector (
CpuMpData->WakeupBuffer = GetWakeupBuffer (ApResetVectorSize);
CpuMpData->MpCpuExchangeInfo = (MP_CPU_EXCHANGE_INFO *) (UINTN)
(CpuMpData->WakeupBuffer + CpuMpData->AddressMap.RendezvousFunnelSize);
+ CpuMpData->MpCpuExchangeInfo->ModeTransitionMemory = (UINT32)
+ GetModeTransitionBuffer (
+ CpuMpData->AddressMap.RendezvousFunnelSize -
+ CpuMpData->AddressMap.ModeTransitionOffset
+ );
}
BackupAndPrepareWakeupBuffer (CpuMpData);
}
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 685e96cbac..0232fe896a 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -152,6 +152,7 @@ typedef struct {
UINTN RendezvousFunnelSize;
UINT8 *RelocateApLoopFuncAddress;
UINTN RelocateApLoopFuncSize;
+ UINTN ModeTransitionOffset;
} MP_ASSEMBLY_ADDRESS_MAP;
typedef struct _CPU_MP_DATA CPU_MP_DATA;
@@ -182,6 +183,10 @@ typedef struct {
UINTN NumApsExecuting;
CPU_MP_DATA *CpuMpData;
UINTN InitializeFloatingPointUnitsAddress;
+ UINT32 ModeTransitionMemory;
+ UINT16 ModeTransitionSegment;
+ UINT32 ModeHighMemory;
+ UINT16 ModeHighSegment;
} MP_CPU_EXCHANGE_INFO;
#pragma pack()
@@ -329,6 +334,23 @@ GetWakeupBuffer (
IN UINTN WakeupBufferSize
);
+/**
+ Get available EfiBootServicesCode memory below 4GB by specified size.
+
+ This buffer is required to safely transfer AP from real address mode to
+ protected mode or long mode, due to the fact that the buffer returned by
+ GetWakeupBuffer() may be marked as non-executable.
+
+ @param[in] BufferSize Wakeup transition buffer size.
+
+ @retval other Return wakeup transition buffer address below 4GB.
+ @retval 0 Cannot find free memory below 4GB.
+**/
+UINTN
+GetModeTransitionBuffer (
+ IN UINTN BufferSize
+ );
+
/**
This function will be called by BSP to wakeup AP.
diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
index 70c2bc7323..ad43bd33f5 100644
--- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
@@ -187,6 +187,29 @@ GetWakeupBuffer (
return (UINTN) -1;
}
+/**
+ Get available EfiBootServicesCode memory below 4GB by specified size.
+
+ This buffer is required to safely transfer AP from real address mode to
+ protected mode or long mode, due to the fact that the buffer returned by
+ GetWakeupBuffer() may be marked as non-executable.
+
+ @param[in] BufferSize Wakeup transition buffer size.
+
+ @retval other Return wakeup transition buffer address below 4GB.
+ @retval 0 Cannot find free memory below 4GB.
+**/
+UINTN
+GetModeTransitionBuffer (
+ IN UINTN BufferSize
+ )
+{
+ //
+ // PEI phase doesn't need to do such transition. So simply return 0.
+ //
+ return 0;
+}
+
/**
Checks APs status and updates APs status if needed.
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
index d255ca5e1b..b5e09c6bc1 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
@@ -42,4 +42,7 @@ InitFlagLocation equ LockLocation + 6Ch
CpuInfoLocation equ LockLocation + 74h
NumApsExecutingLocation equ LockLocation + 7Ch
InitializeFloatingPointUnitsAddress equ LockLocation + 8Ch
-
+ModeTransitionMemoryLocation equ LockLocation + 94h
+ModeTransitionSegmentLocation equ LockLocation + 98h
+ModeHighMemoryLocation equ LockLocation + 9Ah
+ModeHighSegmentLocation equ LockLocation + 9Eh
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 21d278600d..7595988884 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -52,16 +52,13 @@ BITS 16
mov si, BufferStartLocation
mov ebx, [si]
- mov di, ModeOffsetLocation
- mov eax, [di]
- mov di, CodeSegmentLocation
- mov edx, [di]
- mov di, ax
- sub di, 02h
- mov [di],dx ; Patch long mode CS
- sub di, 04h
- add eax, ebx
- mov [di],eax ; Patch address
+ mov si, DataSegmentLocation
+ mov edx, [si]
+
+ ;
+ ; Get start address of 32-bit code in low memory (<1MB)
+ ;
+ mov edi, ModeTransitionMemoryLocation
mov si, GdtrLocation
o32 lgdt [cs:si]
@@ -69,56 +66,79 @@ o32 lgdt [cs:si]
mov si, IdtrLocation
o32 lidt [cs:si]
- mov si, EnableExecuteDisableLocation
- cmp byte [si], 0
- jz SkipEnableExecuteDisableBit
+ ;
+ ; Switch to protected mode
+ ;
+ mov eax, cr0 ; Get control register 0
+ or eax, 000000003h ; Set PE bit (bit #0) & MP
+ mov cr0, eax
+
+ ; Switch to 32-bit code (>1MB)
+o32 jmp far [cs:di]
+
+;
+; Following code must be copied to memory with type of EfiBootServicesCode.
+; This is required if NX is enabled for EfiBootServicesCode of memory.
+;
+BITS 32
+Flat32Start: ; protected mode entry point
+ mov ds, dx
+ mov es, dx
+ mov fs, dx
+ mov gs, dx
+ mov ss, dx
;
; Enable execute disable bit
;
+ mov esi, EnableExecuteDisableLocation
+ cmp byte [ebx + esi], 0
+ jz SkipEnableExecuteDisableBit
+
mov ecx, 0c0000080h ; EFER MSR number
rdmsr ; Read EFER
bts eax, 11 ; Enable Execute Disable Bit
wrmsr ; Write EFER
SkipEnableExecuteDisableBit:
-
- mov di, DataSegmentLocation
- mov edi, [di] ; Save long mode DS in edi
-
- mov si, Cr3Location ; Save CR3 in ecx
- mov ecx, [si]
-
- xor ax, ax
- mov ds, ax ; Clear data segment
-
- mov eax, cr0 ; Get control register 0
- or eax, 000000003h ; Set PE bit (bit #0) & MP
- mov cr0, eax
-
+ ;
+ ; Enable PAE
+ ;
mov eax, cr4
bts eax, 5
mov cr4, eax
+ ;
+ ; Load page table
+ ;
+ mov esi, Cr3Location ; Save CR3 in ecx
+ mov ecx, [ebx + esi]
mov cr3, ecx ; Load CR3
+ ;
+ ; Enable long mode
+ ;
mov ecx, 0c0000080h ; EFER MSR number
rdmsr ; Read EFER
bts eax, 8 ; Set LME=1
wrmsr ; Write EFER
+ ;
+ ; Enable paging
+ ;
mov eax, cr0 ; Read CR0
bts eax, 31 ; Set PG=1
mov cr0, eax ; Write CR0
- jmp 0:strict dword 0 ; far jump to long mode
+ ;
+ ; Far jump to 64-bit code
+ ;
+ mov edi, ModeHighMemoryLocation
+ add edi, ebx
+ jmp far [edi]
+
BITS 64
LongModeStart:
- mov eax, edi
- mov ds, ax
- mov es, ax
- mov ss, ax
-
mov esi, ebx
lea edi, [esi + InitFlagLocation]
cmp qword [edi], 1 ; ApInitConfig
@@ -295,6 +315,7 @@ ASM_PFX(AsmGetAddressMap):
lea rax, [ASM_PFX(AsmRelocateApLoop)]
mov qword [rcx + 18h], rax
mov qword [rcx + 20h], AsmRelocateApLoopEnd - AsmRelocateApLoopStart
+ mov qword [rcx + 28h], Flat32Start - RendezvousFunnelProcStart
ret
;-------------------------------------------------------------------------------------
--
2.15.1.windows.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/6] UefiCpuPkg/CpuExceptionHandlerLib: alloc code memory for exception handlers
2018-01-15 8:54 [PATCH 0/6] Fix issues caused by NX memory protection Jian J Wang
2018-01-15 8:54 ` [PATCH 1/6] UefiCpuPkg/MpInitLib: split wake up buffer into two parts Jian J Wang
@ 2018-01-15 8:54 ` Jian J Wang
2018-01-16 14:02 ` Dong, Eric
2018-01-15 8:54 ` [PATCH 3/6] UefiCpuPkg/CpuDxe: clear NX attr for page directory Jian J Wang
` (3 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Jian J Wang @ 2018-01-15 8:54 UTC (permalink / raw)
To: edk2-devel; +Cc: Jiewen Yao, Ruiyu Ni, Eric Dong, Laszlo Ersek
If PcdDxeNxMemoryProtectionPolicy is set to enable protection for memory
of EfiBootServicesData, EfiConventionalMemory, the BIOS will reset after
timer initialized and started.
The root cause is that the memory used to hold the exception and interrupt
handler is allocated with type of EfiBootServicesData and marked as
non-executable due to NX feature enabled. This patch fixes it by allocating
EfiBootServicesCode type of memory for those handlers instead.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
.../Library/CpuExceptionHandlerLib/DxeException.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
index 9a72b37e77..6d1b54d31d 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
@@ -16,6 +16,7 @@
#include "CpuExceptionCommon.h"
#include <Library/DebugLib.h>
#include <Library/MemoryAllocationLib.h>
+#include <Library/UefiBootServicesTableLib.h>
CONST UINTN mDoFarReturnFlag = 0;
@@ -106,8 +107,12 @@ InitializeCpuInterruptHandlers (
RESERVED_VECTORS_DATA *ReservedVectors;
EFI_CPU_INTERRUPT_HANDLER *ExternalInterruptHandler;
- ReservedVectors = AllocatePool (sizeof (RESERVED_VECTORS_DATA) * CPU_INTERRUPT_NUM);
- ASSERT (ReservedVectors != NULL);
+ Status = gBS->AllocatePool (
+ EfiBootServicesCode,
+ sizeof (RESERVED_VECTORS_DATA) * CPU_INTERRUPT_NUM,
+ (VOID **)&ReservedVectors
+ );
+ ASSERT (!EFI_ERROR (Status) && ReservedVectors != NULL);
SetMem ((VOID *) ReservedVectors, sizeof (RESERVED_VECTORS_DATA) * CPU_INTERRUPT_NUM, 0xff);
if (VectorInfo != NULL) {
Status = ReadAndVerifyVectorInfo (VectorInfo, ReservedVectors, CPU_INTERRUPT_NUM);
@@ -137,8 +142,13 @@ InitializeCpuInterruptHandlers (
AsmGetTemplateAddressMap (&TemplateMap);
ASSERT (TemplateMap.ExceptionStubHeaderSize <= HOOKAFTER_STUB_SIZE);
- InterruptEntryCode = AllocatePool (TemplateMap.ExceptionStubHeaderSize * CPU_INTERRUPT_NUM);
- ASSERT (InterruptEntryCode != NULL);
+
+ Status = gBS->AllocatePool (
+ EfiBootServicesCode,
+ TemplateMap.ExceptionStubHeaderSize * CPU_INTERRUPT_NUM,
+ (VOID **)&InterruptEntryCode
+ );
+ ASSERT (!EFI_ERROR (Status) && InterruptEntryCode != NULL);
InterruptEntry = (UINTN) InterruptEntryCode;
for (Index = 0; Index < CPU_INTERRUPT_NUM; Index ++) {
--
2.15.1.windows.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 3/6] UefiCpuPkg/CpuDxe: clear NX attr for page directory
2018-01-15 8:54 [PATCH 0/6] Fix issues caused by NX memory protection Jian J Wang
2018-01-15 8:54 ` [PATCH 1/6] UefiCpuPkg/MpInitLib: split wake up buffer into two parts Jian J Wang
2018-01-15 8:54 ` [PATCH 2/6] UefiCpuPkg/CpuExceptionHandlerLib: alloc code memory for exception handlers Jian J Wang
@ 2018-01-15 8:54 ` Jian J Wang
2018-01-16 14:02 ` Dong, Eric
2018-01-15 8:54 ` [PATCH 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's supported Jian J Wang
` (2 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: Jian J Wang @ 2018-01-15 8:54 UTC (permalink / raw)
To: edk2-devel; +Cc: Jiewen Yao, Ruiyu Ni, Eric Dong, Laszlo Ersek
If PcdDxeNxMemoryProtectionPolicy is set to enable protection for memory
of EfiBootServicesCode, EfiConventionalMemory and EfiReservedMemoryType,
the BIOS will hang at a page fault exception randomly.
The root cause is that the memory allocation for driver images (actually
a memory type conversion from free memory, type of EfiConventionalMemory,
to code memory, type of EfiBootServicesCode/EfiRuntimeServicesCode)
will get memory with NX set, because the CpuDxe driver will keep the NX
attribute (with free memory) in page directory during page table splitting
and then override the NX attribute of all its entries.
This patch fixes this issue by not inheriting NX attribute when turning
a page entry into a page directory during page granularity split.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
UefiCpuPkg/CpuDxe/CpuPageTable.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index a9c9bc9d5e..1654e71103 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -528,7 +528,7 @@ SplitPage (
for (Index = 0; Index < SIZE_4KB / sizeof(UINT64); Index++) {
NewPageEntry[Index] = (BaseAddress + SIZE_4KB * Index) | AddressEncMask | ((*PageEntry) & PAGE_PROGATE_BITS);
}
- (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask | ((*PageEntry) & PAGE_PROGATE_BITS);
+ (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask | ((*PageEntry) & PAGE_ATTRIBUTE_BITS);
return RETURN_SUCCESS;
} else {
return RETURN_UNSUPPORTED;
@@ -549,7 +549,7 @@ SplitPage (
for (Index = 0; Index < SIZE_4KB / sizeof(UINT64); Index++) {
NewPageEntry[Index] = (BaseAddress + SIZE_2MB * Index) | AddressEncMask | IA32_PG_PS | ((*PageEntry) & PAGE_PROGATE_BITS);
}
- (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask | ((*PageEntry) & PAGE_PROGATE_BITS);
+ (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask | ((*PageEntry) & PAGE_ATTRIBUTE_BITS);
return RETURN_SUCCESS;
} else {
return RETURN_UNSUPPORTED;
@@ -979,7 +979,7 @@ RefreshGcdMemoryAttributesFromPaging (
);
ASSERT_EFI_ERROR (Status);
DEBUG ((
- DEBUG_INFO,
+ DEBUG_VERBOSE,
"Updated memory space attribute: [%lu] %016lx - %016lx (%016lx -> %016lx)\r\n",
(UINT64)Index, BaseAddress, BaseAddress + Length - 1,
MemorySpaceMap[Index].Attributes,
--
2.15.1.windows.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's supported
2018-01-15 8:54 [PATCH 0/6] Fix issues caused by NX memory protection Jian J Wang
` (2 preceding siblings ...)
2018-01-15 8:54 ` [PATCH 3/6] UefiCpuPkg/CpuDxe: clear NX attr for page directory Jian J Wang
@ 2018-01-15 8:54 ` Jian J Wang
2018-01-16 14:02 ` Dong, Eric
2018-01-28 22:46 ` Laszlo Ersek
2018-01-15 8:54 ` [PATCH 5/6] MdeModulePkg/PiSmmCore: remove NX attr for SMM RAM Jian J Wang
2018-01-15 8:54 ` [PATCH 6/6] MdeModulePkg/BootScriptExecutorDxe: remove NX attr for FfsBuffer Jian J Wang
5 siblings, 2 replies; 22+ messages in thread
From: Jian J Wang @ 2018-01-15 8:54 UTC (permalink / raw)
To: edk2-devel; +Cc: Jiewen Yao, Ruiyu Ni, Eric Dong, Laszlo Ersek
If PcdDxeNxMemoryProtectionPolicy is set to enable protection for memory
of EfiBootServicesCode, EfiConventionalMemory, the BIOS will hang at a page
fault exception triggered by PiSmmCpuDxeSmm.
The root cause is that PiSmmCpuDxeSmm will access default SMM RAM starting
at 0x30000 which is marked as non-executable, but NX feature was not
enabled during SMM initialization. Accessing memory which has invalid
attributes set will cause page fault exception. This patch fixes it by
checking NX capability in cpuid and enable NXE in EFER MSR if it's
available.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 14 ++++++++++++++
UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm | 12 +++++++++++-
2 files changed, 25 insertions(+), 1 deletion(-)
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
index d9df3626c7..db172f108a 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
@@ -42,6 +42,11 @@ ASM_PFX(gcSmiInitGdtr):
global ASM_PFX(SmmStartup)
ASM_PFX(SmmStartup):
+ DB 0x66
+ mov eax, 0x80000001 ; read capability
+ cpuid
+ DB 0x66
+ mov ebx, edx ; rdmsr will change edx. keep it in ebx.
DB 0x66, 0xb8
ASM_PFX(gSmmCr3): DD 0
mov cr3, eax
@@ -50,6 +55,15 @@ ASM_PFX(gSmmCr3): DD 0
DB 0x66, 0xb8
ASM_PFX(gSmmCr4): DD 0
mov cr4, eax
+ DB 0x66
+ mov ecx, 0xc0000080 ; IA32_EFER MSR
+ rdmsr
+ DB 0x66
+ test ebx, BIT20 ; check NXE capability
+ jz .1
+ or ah, BIT3 ; set NXE bit
+ wrmsr
+.1:
DB 0x66, 0xb8
ASM_PFX(gSmmCr0): DD 0
DB 0xbf, PROTECT_MODE_DS, 0 ; mov di, PROTECT_MODE_DS
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm
index 9d05e2cb05..2a3a1141c3 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm
@@ -42,6 +42,11 @@ ASM_PFX(gcSmiInitGdtr):
global ASM_PFX(SmmStartup)
ASM_PFX(SmmStartup):
+ DB 0x66
+ mov eax, 0x80000001 ; read capability
+ cpuid
+ DB 0x66
+ mov ebx, edx ; rdmsr will change edx. keep it in ebx.
DB 0x66, 0xb8 ; mov eax, imm32
ASM_PFX(gSmmCr3): DD 0
mov cr3, rax
@@ -54,7 +59,12 @@ ASM_PFX(gSmmCr4): DD 0
DB 0x66
mov ecx, 0xc0000080 ; IA32_EFER MSR
rdmsr
- or ah, 1 ; set LME bit
+ or ah, BIT0 ; set LME bit
+ DB 0x66
+ test ebx, BIT20 ; check NXE capability
+ jz .1
+ or ah, BIT3 ; set NXE bit
+.1:
wrmsr
DB 0x66, 0xb8 ; mov eax, imm32
ASM_PFX(gSmmCr0): DD 0
--
2.15.1.windows.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 5/6] MdeModulePkg/PiSmmCore: remove NX attr for SMM RAM
2018-01-15 8:54 [PATCH 0/6] Fix issues caused by NX memory protection Jian J Wang
` (3 preceding siblings ...)
2018-01-15 8:54 ` [PATCH 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's supported Jian J Wang
@ 2018-01-15 8:54 ` Jian J Wang
2018-01-15 10:18 ` Zeng, Star
2018-01-15 8:54 ` [PATCH 6/6] MdeModulePkg/BootScriptExecutorDxe: remove NX attr for FfsBuffer Jian J Wang
5 siblings, 1 reply; 22+ messages in thread
From: Jian J Wang @ 2018-01-15 8:54 UTC (permalink / raw)
To: edk2-devel; +Cc: Jiewen Yao, Ruiyu Ni, Eric Dong, Star Zeng
If PcdDxeNxMemoryProtectionPolicy is set to enable protection for memory
of EfiReservedMemoryType, the BIOS will hang at a page fault exception
during starting SMM driver.
The root cause is that SMM RAM is type of EfiReservedMemoryType and
marked as non-executable. The fix is simply removing NX attribute for
those memory.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index a7663ca291..94d671bd74 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -1550,6 +1550,7 @@ SmmIplEntry (
EFI_CPU_ARCH_PROTOCOL *CpuArch;
EFI_STATUS SetAttrStatus;
EFI_SMRAM_DESCRIPTOR *SmramRangeSmmDriver;
+ EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc;
//
// Fill in the image handle of the SMM IPL so the SMM Core can use this as the
@@ -1616,7 +1617,8 @@ SmmIplEntry (
GetSmramCacheRange (mCurrentSmramRange, &mSmramCacheBase, &mSmramCacheSize);
//
- // If CPU AP is present, attempt to set SMRAM cacheability to WB
+ // If CPU AP is present, attempt to set SMRAM cacheability to WB and clear
+ // XP if it's set.
// Note that it is expected that cacheability of SMRAM has been set to WB if CPU AP
// is not available here.
//
@@ -1630,7 +1632,19 @@ SmmIplEntry (
);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_WARN, "SMM IPL failed to set SMRAM window to EFI_MEMORY_WB\n"));
- }
+ }
+
+ Status = gDS->GetMemorySpaceDescriptor(
+ mCurrentSmramRange->PhysicalStart,
+ &MemDesc
+ );
+ if (!EFI_ERROR (Status) && (MemDesc.Attributes & EFI_MEMORY_XP) != 0) {
+ gDS->SetMemorySpaceAttributes (
+ mCurrentSmramRange->PhysicalStart,
+ mCurrentSmramRange->PhysicalSize,
+ MemDesc.Attributes & (~EFI_MEMORY_XP)
+ );
+ }
}
//
// if Loading module at Fixed Address feature is enabled, save the SMRAM base to Load
--
2.15.1.windows.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 6/6] MdeModulePkg/BootScriptExecutorDxe: remove NX attr for FfsBuffer
2018-01-15 8:54 [PATCH 0/6] Fix issues caused by NX memory protection Jian J Wang
` (4 preceding siblings ...)
2018-01-15 8:54 ` [PATCH 5/6] MdeModulePkg/PiSmmCore: remove NX attr for SMM RAM Jian J Wang
@ 2018-01-15 8:54 ` Jian J Wang
2018-01-15 10:18 ` Zeng, Star
5 siblings, 1 reply; 22+ messages in thread
From: Jian J Wang @ 2018-01-15 8:54 UTC (permalink / raw)
To: edk2-devel; +Cc: Jiewen Yao, Ruiyu Ni, Eric Dong, Star Zeng
If PcdDxeNxMemoryProtectionPolicy is set to enable protection for memory
of EfiReservedMemoryType, the BIOS will hang at a page fault exception
triggered by BootScriptExecutorDxe.
The root cause is that this driver will allocate memory of
EfiReservedMemoryType and relocate itself into this new memory. Since
EfiReservedMemoryType of memory is marked non-executable, re-start this
driver after relocation will cause exception. The fix is removing the NX
attribute after memory allocation.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
.../Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf | 1 +
.../Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.c | 14 ++++++++++++++
.../Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.h | 1 +
3 files changed, 16 insertions(+)
diff --git a/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf b/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf
index 29af7f55ec..aac132122c 100644
--- a/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf
+++ b/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf
@@ -68,6 +68,7 @@
LockBoxLib
CpuExceptionHandlerLib
DevicePathLib
+ DxeServicesTableLib
[Guids]
gEfiBootScriptExecutorVariableGuid ## PRODUCES ## UNDEFINED # SaveLockBox
diff --git a/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.c b/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.c
index 4545d6e581..263a282188 100644
--- a/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.c
+++ b/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.c
@@ -273,6 +273,7 @@ ReadyToLockEventNotify (
UINTN Pages;
EFI_PHYSICAL_ADDRESS FfsBuffer;
PE_COFF_LOADER_IMAGE_CONTEXT ImageContext;
+ EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc;
Status = gBS->LocateProtocol (&gEfiDxeSmmReadyToLockProtocolGuid, NULL, &Interface);
if (EFI_ERROR (Status)) {
@@ -322,6 +323,19 @@ ReadyToLockEventNotify (
&FfsBuffer
);
ASSERT_EFI_ERROR (Status);
+
+ //
+ // Make sure that the buffer can be used to store code.
+ //
+ Status = gDS->GetMemorySpaceDescriptor (FfsBuffer, &MemDesc);
+ if (!EFI_ERROR (Status) && (MemDesc.Attributes & EFI_MEMORY_XP) != 0) {
+ gDS->SetMemorySpaceAttributes (
+ FfsBuffer,
+ EFI_PAGES_TO_SIZE (Pages),
+ MemDesc.Attributes & (~EFI_MEMORY_XP)
+ );
+ }
+
ImageContext.ImageAddress = (PHYSICAL_ADDRESS)(UINTN)FfsBuffer;
//
// Align buffer on section boundary
diff --git a/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.h b/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.h
index 75327569d7..94deae87e6 100644
--- a/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.h
+++ b/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.h
@@ -38,6 +38,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include <Library/LockBoxLib.h>
#include <Library/CpuExceptionHandlerLib.h>
#include <Library/DevicePathLib.h>
+#include <Library/DxeServicesTableLib.h>
#include <Guid/AcpiS3Context.h>
#include <Guid/BootScriptExecutorVariable.h>
--
2.15.1.windows.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 5/6] MdeModulePkg/PiSmmCore: remove NX attr for SMM RAM
2018-01-15 8:54 ` [PATCH 5/6] MdeModulePkg/PiSmmCore: remove NX attr for SMM RAM Jian J Wang
@ 2018-01-15 10:18 ` Zeng, Star
0 siblings, 0 replies; 22+ messages in thread
From: Zeng, Star @ 2018-01-15 10:18 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Yao, Jiewen, Ni, Ruiyu, Dong, Eric, Zeng, Star
Reviewed-by: Star Zeng <star.zeng@intel.com>
-----Original Message-----
From: Wang, Jian J
Sent: Monday, January 15, 2018 4:55 PM
To: edk2-devel@lists.01.org
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH 5/6] MdeModulePkg/PiSmmCore: remove NX attr for SMM RAM
If PcdDxeNxMemoryProtectionPolicy is set to enable protection for memory of EfiReservedMemoryType, the BIOS will hang at a page fault exception during starting SMM driver.
The root cause is that SMM RAM is type of EfiReservedMemoryType and marked as non-executable. The fix is simply removing NX attribute for those memory.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
index a7663ca291..94d671bd74 100644
--- a/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
+++ b/MdeModulePkg/Core/PiSmmCore/PiSmmIpl.c
@@ -1550,6 +1550,7 @@ SmmIplEntry (
EFI_CPU_ARCH_PROTOCOL *CpuArch;
EFI_STATUS SetAttrStatus;
EFI_SMRAM_DESCRIPTOR *SmramRangeSmmDriver;
+ EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc;
//
// Fill in the image handle of the SMM IPL so the SMM Core can use this as the @@ -1616,7 +1617,8 @@ SmmIplEntry (
GetSmramCacheRange (mCurrentSmramRange, &mSmramCacheBase, &mSmramCacheSize);
//
- // If CPU AP is present, attempt to set SMRAM cacheability to WB
+ // If CPU AP is present, attempt to set SMRAM cacheability to WB and clear
+ // XP if it's set.
// Note that it is expected that cacheability of SMRAM has been set to WB if CPU AP
// is not available here.
//
@@ -1630,7 +1632,19 @@ SmmIplEntry (
);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_WARN, "SMM IPL failed to set SMRAM window to EFI_MEMORY_WB\n"));
- }
+ }
+
+ Status = gDS->GetMemorySpaceDescriptor(
+ mCurrentSmramRange->PhysicalStart,
+ &MemDesc
+ );
+ if (!EFI_ERROR (Status) && (MemDesc.Attributes & EFI_MEMORY_XP) != 0) {
+ gDS->SetMemorySpaceAttributes (
+ mCurrentSmramRange->PhysicalStart,
+ mCurrentSmramRange->PhysicalSize,
+ MemDesc.Attributes & (~EFI_MEMORY_XP)
+ );
+ }
}
//
// if Loading module at Fixed Address feature is enabled, save the SMRAM base to Load
--
2.15.1.windows.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] MdeModulePkg/BootScriptExecutorDxe: remove NX attr for FfsBuffer
2018-01-15 8:54 ` [PATCH 6/6] MdeModulePkg/BootScriptExecutorDxe: remove NX attr for FfsBuffer Jian J Wang
@ 2018-01-15 10:18 ` Zeng, Star
0 siblings, 0 replies; 22+ messages in thread
From: Zeng, Star @ 2018-01-15 10:18 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Yao, Jiewen, Ni, Ruiyu, Dong, Eric, Zeng, Star
Reviewed-by: Star Zeng <star.zeng@intel.com>
-----Original Message-----
From: Wang, Jian J
Sent: Monday, January 15, 2018 4:55 PM
To: edk2-devel@lists.01.org
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH 6/6] MdeModulePkg/BootScriptExecutorDxe: remove NX attr for FfsBuffer
If PcdDxeNxMemoryProtectionPolicy is set to enable protection for memory of EfiReservedMemoryType, the BIOS will hang at a page fault exception triggered by BootScriptExecutorDxe.
The root cause is that this driver will allocate memory of EfiReservedMemoryType and relocate itself into this new memory. Since EfiReservedMemoryType of memory is marked non-executable, re-start this driver after relocation will cause exception. The fix is removing the NX attribute after memory allocation.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
.../Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf | 1 +
.../Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.c | 14 ++++++++++++++
.../Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.h | 1 +
3 files changed, 16 insertions(+)
diff --git a/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf b/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf
index 29af7f55ec..aac132122c 100644
--- a/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.inf
+++ b/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecut
+++ orDxe.inf
@@ -68,6 +68,7 @@
LockBoxLib
CpuExceptionHandlerLib
DevicePathLib
+ DxeServicesTableLib
[Guids]
gEfiBootScriptExecutorVariableGuid ## PRODUCES ## UNDEFINED # SaveLockBox
diff --git a/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.c b/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.c
index 4545d6e581..263a282188 100644
--- a/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.c
+++ b/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.c
@@ -273,6 +273,7 @@ ReadyToLockEventNotify (
UINTN Pages;
EFI_PHYSICAL_ADDRESS FfsBuffer;
PE_COFF_LOADER_IMAGE_CONTEXT ImageContext;
+ EFI_GCD_MEMORY_SPACE_DESCRIPTOR MemDesc;
Status = gBS->LocateProtocol (&gEfiDxeSmmReadyToLockProtocolGuid, NULL, &Interface);
if (EFI_ERROR (Status)) {
@@ -322,6 +323,19 @@ ReadyToLockEventNotify (
&FfsBuffer
);
ASSERT_EFI_ERROR (Status);
+
+ //
+ // Make sure that the buffer can be used to store code.
+ //
+ Status = gDS->GetMemorySpaceDescriptor (FfsBuffer, &MemDesc); if
+ (!EFI_ERROR (Status) && (MemDesc.Attributes & EFI_MEMORY_XP) != 0) {
+ gDS->SetMemorySpaceAttributes (
+ FfsBuffer,
+ EFI_PAGES_TO_SIZE (Pages),
+ MemDesc.Attributes & (~EFI_MEMORY_XP)
+ );
+ }
+
ImageContext.ImageAddress = (PHYSICAL_ADDRESS)(UINTN)FfsBuffer;
//
// Align buffer on section boundary
diff --git a/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.h b/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.h
index 75327569d7..94deae87e6 100644
--- a/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.h
+++ b/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.h
@@ -38,6 +38,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
#include <Library/LockBoxLib.h>
#include <Library/CpuExceptionHandlerLib.h>
#include <Library/DevicePathLib.h>
+#include <Library/DxeServicesTableLib.h>
#include <Guid/AcpiS3Context.h>
#include <Guid/BootScriptExecutorVariable.h>
--
2.15.1.windows.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 2/6] UefiCpuPkg/CpuExceptionHandlerLib: alloc code memory for exception handlers
2018-01-15 8:54 ` [PATCH 2/6] UefiCpuPkg/CpuExceptionHandlerLib: alloc code memory for exception handlers Jian J Wang
@ 2018-01-16 14:02 ` Dong, Eric
0 siblings, 0 replies; 22+ messages in thread
From: Dong, Eric @ 2018-01-16 14:02 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Yao, Jiewen, Ni, Ruiyu, Laszlo Ersek
Reviewed-by: Eric Dong <eric.dong@intel.com>
> -----Original Message-----
> From: Wang, Jian J
> Sent: Monday, January 15, 2018 4:54 PM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>;
> Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH 2/6] UefiCpuPkg/CpuExceptionHandlerLib: alloc code
> memory for exception handlers
>
> If PcdDxeNxMemoryProtectionPolicy is set to enable protection for memory
> of EfiBootServicesData, EfiConventionalMemory, the BIOS will reset after
> timer initialized and started.
>
> The root cause is that the memory used to hold the exception and interrupt
> handler is allocated with type of EfiBootServicesData and marked as non-
> executable due to NX feature enabled. This patch fixes it by allocating
> EfiBootServicesCode type of memory for those handlers instead.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
> .../Library/CpuExceptionHandlerLib/DxeException.c | 18
> ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
> index 9a72b37e77..6d1b54d31d 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
> @@ -16,6 +16,7 @@
> #include "CpuExceptionCommon.h"
> #include <Library/DebugLib.h>
> #include <Library/MemoryAllocationLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
>
> CONST UINTN mDoFarReturnFlag = 0;
>
> @@ -106,8 +107,12 @@ InitializeCpuInterruptHandlers (
> RESERVED_VECTORS_DATA *ReservedVectors;
> EFI_CPU_INTERRUPT_HANDLER *ExternalInterruptHandler;
>
> - ReservedVectors = AllocatePool (sizeof (RESERVED_VECTORS_DATA) *
> CPU_INTERRUPT_NUM);
> - ASSERT (ReservedVectors != NULL);
> + Status = gBS->AllocatePool (
> + EfiBootServicesCode,
> + sizeof (RESERVED_VECTORS_DATA) * CPU_INTERRUPT_NUM,
> + (VOID **)&ReservedVectors
> + );
> + ASSERT (!EFI_ERROR (Status) && ReservedVectors != NULL);
> SetMem ((VOID *) ReservedVectors, sizeof (RESERVED_VECTORS_DATA) *
> CPU_INTERRUPT_NUM, 0xff);
> if (VectorInfo != NULL) {
> Status = ReadAndVerifyVectorInfo (VectorInfo, ReservedVectors,
> CPU_INTERRUPT_NUM); @@ -137,8 +142,13 @@
> InitializeCpuInterruptHandlers (
>
> AsmGetTemplateAddressMap (&TemplateMap);
> ASSERT (TemplateMap.ExceptionStubHeaderSize <=
> HOOKAFTER_STUB_SIZE);
> - InterruptEntryCode = AllocatePool
> (TemplateMap.ExceptionStubHeaderSize * CPU_INTERRUPT_NUM);
> - ASSERT (InterruptEntryCode != NULL);
> +
> + Status = gBS->AllocatePool (
> + EfiBootServicesCode,
> + TemplateMap.ExceptionStubHeaderSize * CPU_INTERRUPT_NUM,
> + (VOID **)&InterruptEntryCode
> + );
> + ASSERT (!EFI_ERROR (Status) && InterruptEntryCode != NULL);
>
> InterruptEntry = (UINTN) InterruptEntryCode;
> for (Index = 0; Index < CPU_INTERRUPT_NUM; Index ++) {
> --
> 2.15.1.windows.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/6] UefiCpuPkg/CpuDxe: clear NX attr for page directory
2018-01-15 8:54 ` [PATCH 3/6] UefiCpuPkg/CpuDxe: clear NX attr for page directory Jian J Wang
@ 2018-01-16 14:02 ` Dong, Eric
0 siblings, 0 replies; 22+ messages in thread
From: Dong, Eric @ 2018-01-16 14:02 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Ni, Ruiyu, Laszlo Ersek, Yao, Jiewen
Reviewed-by: Eric Dong <eric.dong@intel.com>
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Jian J Wang
> Sent: Monday, January 15, 2018 4:55 PM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: [edk2] [PATCH 3/6] UefiCpuPkg/CpuDxe: clear NX attr for page
> directory
>
> If PcdDxeNxMemoryProtectionPolicy is set to enable protection for memory
> of EfiBootServicesCode, EfiConventionalMemory and
> EfiReservedMemoryType, the BIOS will hang at a page fault exception
> randomly.
>
> The root cause is that the memory allocation for driver images (actually a
> memory type conversion from free memory, type of EfiConventionalMemory,
> to code memory, type of EfiBootServicesCode/EfiRuntimeServicesCode)
> will get memory with NX set, because the CpuDxe driver will keep the NX
> attribute (with free memory) in page directory during page table splitting and
> then override the NX attribute of all its entries.
>
> This patch fixes this issue by not inheriting NX attribute when turning a page
> entry into a page directory during page granularity split.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
> UefiCpuPkg/CpuDxe/CpuPageTable.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index a9c9bc9d5e..1654e71103 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -528,7 +528,7 @@ SplitPage (
> for (Index = 0; Index < SIZE_4KB / sizeof(UINT64); Index++) {
> NewPageEntry[Index] = (BaseAddress + SIZE_4KB * Index) |
> AddressEncMask | ((*PageEntry) & PAGE_PROGATE_BITS);
> }
> - (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask |
> ((*PageEntry) & PAGE_PROGATE_BITS);
> + (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask |
> + ((*PageEntry) & PAGE_ATTRIBUTE_BITS);
> return RETURN_SUCCESS;
> } else {
> return RETURN_UNSUPPORTED;
> @@ -549,7 +549,7 @@ SplitPage (
> for (Index = 0; Index < SIZE_4KB / sizeof(UINT64); Index++) {
> NewPageEntry[Index] = (BaseAddress + SIZE_2MB * Index) |
> AddressEncMask | IA32_PG_PS | ((*PageEntry) & PAGE_PROGATE_BITS);
> }
> - (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask |
> ((*PageEntry) & PAGE_PROGATE_BITS);
> + (*PageEntry) = (UINT64)(UINTN)NewPageEntry | AddressEncMask |
> + ((*PageEntry) & PAGE_ATTRIBUTE_BITS);
> return RETURN_SUCCESS;
> } else {
> return RETURN_UNSUPPORTED;
> @@ -979,7 +979,7 @@ RefreshGcdMemoryAttributesFromPaging (
> );
> ASSERT_EFI_ERROR (Status);
> DEBUG ((
> - DEBUG_INFO,
> + DEBUG_VERBOSE,
> "Updated memory space attribute: [%lu] %016lx - %016lx (%016lx -
> > %016lx)\r\n",
> (UINT64)Index, BaseAddress, BaseAddress + Length - 1,
> MemorySpaceMap[Index].Attributes,
> --
> 2.15.1.windows.2
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's supported
2018-01-15 8:54 ` [PATCH 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's supported Jian J Wang
@ 2018-01-16 14:02 ` Dong, Eric
2018-01-28 22:46 ` Laszlo Ersek
1 sibling, 0 replies; 22+ messages in thread
From: Dong, Eric @ 2018-01-16 14:02 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Yao, Jiewen, Ni, Ruiyu, Laszlo Ersek
Reviewed-by: Eric Dong <eric.dong@intel.com>
> -----Original Message-----
> From: Wang, Jian J
> Sent: Monday, January 15, 2018 4:55 PM
> To: edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>;
> Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's
> supported
>
> If PcdDxeNxMemoryProtectionPolicy is set to enable protection for memory
> of EfiBootServicesCode, EfiConventionalMemory, the BIOS will hang at a
> page fault exception triggered by PiSmmCpuDxeSmm.
>
> The root cause is that PiSmmCpuDxeSmm will access default SMM RAM
> starting at 0x30000 which is marked as non-executable, but NX feature was
> not enabled during SMM initialization. Accessing memory which has invalid
> attributes set will cause page fault exception. This patch fixes it by checking
> NX capability in cpuid and enable NXE in EFER MSR if it's available.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 14 ++++++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm | 12 +++++++++++-
> 2 files changed, 25 insertions(+), 1 deletion(-)
>
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> index d9df3626c7..db172f108a 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm
> @@ -42,6 +42,11 @@ ASM_PFX(gcSmiInitGdtr):
>
> global ASM_PFX(SmmStartup)
> ASM_PFX(SmmStartup):
> + DB 0x66
> + mov eax, 0x80000001 ; read capability
> + cpuid
> + DB 0x66
> + mov ebx, edx ; rdmsr will change edx. keep it in ebx.
> DB 0x66, 0xb8
> ASM_PFX(gSmmCr3): DD 0
> mov cr3, eax
> @@ -50,6 +55,15 @@ ASM_PFX(gSmmCr3): DD 0
> DB 0x66, 0xb8
> ASM_PFX(gSmmCr4): DD 0
> mov cr4, eax
> + DB 0x66
> + mov ecx, 0xc0000080 ; IA32_EFER MSR
> + rdmsr
> + DB 0x66
> + test ebx, BIT20 ; check NXE capability
> + jz .1
> + or ah, BIT3 ; set NXE bit
> + wrmsr
> +.1:
> DB 0x66, 0xb8
> ASM_PFX(gSmmCr0): DD 0
> DB 0xbf, PROTECT_MODE_DS, 0 ; mov di, PROTECT_MODE_DS
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm
> index 9d05e2cb05..2a3a1141c3 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm
> @@ -42,6 +42,11 @@ ASM_PFX(gcSmiInitGdtr):
>
> global ASM_PFX(SmmStartup)
> ASM_PFX(SmmStartup):
> + DB 0x66
> + mov eax, 0x80000001 ; read capability
> + cpuid
> + DB 0x66
> + mov ebx, edx ; rdmsr will change edx. keep it in ebx.
> DB 0x66, 0xb8 ; mov eax, imm32
> ASM_PFX(gSmmCr3): DD 0
> mov cr3, rax
> @@ -54,7 +59,12 @@ ASM_PFX(gSmmCr4): DD 0
> DB 0x66
> mov ecx, 0xc0000080 ; IA32_EFER MSR
> rdmsr
> - or ah, 1 ; set LME bit
> + or ah, BIT0 ; set LME bit
> + DB 0x66
> + test ebx, BIT20 ; check NXE capability
> + jz .1
> + or ah, BIT3 ; set NXE bit
> +.1:
> wrmsr
> DB 0x66, 0xb8 ; mov eax, imm32
> ASM_PFX(gSmmCr0): DD 0
> --
> 2.15.1.windows.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/6] UefiCpuPkg/MpInitLib: split wake up buffer into two parts
2018-01-15 8:54 ` [PATCH 1/6] UefiCpuPkg/MpInitLib: split wake up buffer into two parts Jian J Wang
@ 2018-01-18 6:53 ` Dong, Eric
2018-01-27 16:17 ` Laszlo Ersek
1 sibling, 0 replies; 22+ messages in thread
From: Dong, Eric @ 2018-01-18 6:53 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Yao, Jiewen, Ni, Ruiyu, Laszlo Ersek
Reviewed-by: Eric Dong <eric.dong@intel.com>
-----Original Message-----
From: Wang, Jian J
Sent: Monday, January 15, 2018 4:54 PM
To: edk2-devel@lists.01.org
Cc: Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
Subject: [PATCH 1/6] UefiCpuPkg/MpInitLib: split wake up buffer into two parts
If PcdDxeNxMemoryProtectionPolicy is set to enable protection for memory of EfiBootServicesCode, EfiConventionalMemory, the BIOS will hang at a page fault exception during MP initialization.
The root cause is that the AP wake up buffer, which is below 1MB and used to hold both AP init code and data, is type of EfiConventionalMemory (not really allocated because of potential conflict with legacy code), and is marked as non-executable. During the transition from real address mode to long mode, the AP init code has to enable paging which will then cause itself a page fault exception because it's just running in non-executable memory.
The solution is splitting AP wake up buffer into two part: lower part is still below 1MB and shared with legacy system, higher part is really allocated memory of BootServicesCode type. The init code in the memory below 1MB will not enable paging but just switch to protected mode and jump to higher memory, in which the init code will enable paging and switch to long mode.
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 34 ++++++++++
UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc | 5 ++
UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 32 +++++-----
UefiCpuPkg/Library/MpInitLib/MpLib.c | 45 +++++++++++++
UefiCpuPkg/Library/MpInitLib/MpLib.h | 22 +++++++
UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 23 +++++++
UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc | 5 +-
UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 87 ++++++++++++++++----------
8 files changed, 204 insertions(+), 49 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index d2bcef53d6..fd2317924f 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -113,6 +113,40 @@ GetWakeupBuffer (
return (UINTN) StartAddress;
}
+/**
+ Get available EfiBootServicesCode memory below 4GB by specified size.
+
+ This buffer is required to safely transfer AP from real address mode
+ to protected mode or long mode, due to the fact that the buffer
+ returned by
+ GetWakeupBuffer() may be marked as non-executable.
+
+ @param[in] BufferSize Wakeup transition buffer size.
+
+ @retval other Return wakeup transition buffer address below 4GB.
+ @retval 0 Cannot find free memory below 4GB.
+**/
+UINTN
+GetModeTransitionBuffer (
+ IN UINTN BufferSize
+ )
+{
+ EFI_STATUS Status;
+ EFI_PHYSICAL_ADDRESS StartAddress;
+
+ StartAddress = BASE_4GB - 1;
+ Status = gBS->AllocatePages (
+ AllocateMaxAddress,
+ EfiBootServicesCode,
+ EFI_SIZE_TO_PAGES (BufferSize),
+ &StartAddress
+ );
+ if (EFI_ERROR (Status)) {
+ StartAddress = 0;
+ }
+
+ return (UINTN)StartAddress;
+}
+
/**
Checks APs status and updates APs status if needed.
diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
index bdfe0d33cc..1648f2c4b0 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc
@@ -41,4 +41,9 @@ Cr3Location equ LockLocation + 34h
InitFlagLocation equ LockLocation + 38h
CpuInfoLocation equ LockLocation + 3Ch
NumApsExecutingLocation equ LockLocation + 40h
+InitializeFloatingPointUnitsAddress equ LockLocation + 48h
+ModeTransitionMemoryLocation equ LockLocation + 4Ch
+ModeTransitionSegmentLocation equ LockLocation + 50h
+ModeHighMemoryLocation equ LockLocation + 52h
+ModeHighSegmentLocation equ LockLocation + 56h
diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
index 2b6c27d4ec..bd79be0f5e 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
@@ -48,34 +48,35 @@ BITS 16
mov si, BufferStartLocation
mov ebx, [si]
- mov si, ModeOffsetLocation
- mov eax, [si]
- mov si, CodeSegmentLocation
- mov edx, [si]
- mov di, ax
- sub di, 02h
- mov [di], dx
- sub di, 04h
- add eax, ebx
- mov [di],eax
-
mov si, DataSegmentLocation
mov edx, [si]
+ ;
+ ; Get start address of 32-bit code in low memory (<1MB)
+ ;
+ mov edi, ModeTransitionMemoryLocation
+
mov si, GdtrLocation
o32 lgdt [cs:si]
mov si, IdtrLocation
o32 lidt [cs:si]
- xor ax, ax
- mov ds, ax
-
+ ;
+ ; Switch to protected mode
+ ;
mov eax, cr0 ; Get control register 0
or eax, 000000003h ; Set PE bit (bit #0) & MP
mov cr0, eax
- jmp 0:strict dword 0 ; far jump to protected mode
+ ; Switch to 32-bit code in executable memory (>1MB)
+o32 jmp far [cs:di]
+
+;
+; Following code may be copied to memory with type of EfiBootServicesCode.
+; This is required at DXE phase if NX is enabled for
+EfiBootServicesCode of ; memory.
+;
BITS 32
Flat32Start: ; protected mode entry point
mov ds, dx
@@ -266,6 +267,7 @@ ASM_PFX(AsmGetAddressMap):
mov dword [ebx + 8h], RendezvousFunnelProcEnd - RendezvousFunnelProcStart
mov dword [ebx + 0Ch], AsmRelocateApLoopStart
mov dword [ebx + 10h], AsmRelocateApLoopEnd - AsmRelocateApLoopStart
+ mov dword [ebx + 14h], Flat32Start - RendezvousFunnelProcStart
popad
ret
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index cdc03113e5..fbcbcc6cc9 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -772,6 +772,8 @@ FillExchangeInfoData (
)
{
volatile MP_CPU_EXCHANGE_INFO *ExchangeInfo;
+ UINTN Size;
+ IA32_SEGMENT_DESCRIPTOR *Selector;
ExchangeInfo = CpuMpData->MpCpuExchangeInfo;
ExchangeInfo->Lock = 0;
@@ -801,6 +803,44 @@ FillExchangeInfoData (
//
AsmReadGdtr ((IA32_DESCRIPTOR *) &ExchangeInfo->GdtrProfile);
AsmReadIdtr ((IA32_DESCRIPTOR *) &ExchangeInfo->IdtrProfile);
+
+ //
+ // Find a 32-bit code segment
+ //
+ Selector = (IA32_SEGMENT_DESCRIPTOR *)ExchangeInfo->GdtrProfile.Base;
+ Size = ExchangeInfo->GdtrProfile.Limit + 1; while (Size > 0) {
+ if (Selector->Bits.L == 0 && Selector->Bits.Type >= 8) {
+ ExchangeInfo->ModeTransitionSegment =
+ (UINT16)((UINTN)Selector - ExchangeInfo->GdtrProfile.Base);
+ break;
+ }
+ Selector += 1;
+ Size -= sizeof (IA32_SEGMENT_DESCRIPTOR); }
+
+ //
+ // Copy all 32-bit code and 64-bit code into memory with type of //
+ EfiBootServicesCode to avoid page fault if NX memory protection is enabled.
+ //
+ if (ExchangeInfo->ModeTransitionMemory != 0) {
+ Size = CpuMpData->AddressMap.RendezvousFunnelSize -
+ CpuMpData->AddressMap.ModeTransitionOffset;
+ CopyMem (
+ (VOID *)(UINTN)ExchangeInfo->ModeTransitionMemory,
+ CpuMpData->AddressMap.RendezvousFunnelAddress +
+ CpuMpData->AddressMap.ModeTransitionOffset,
+ Size
+ );
+
+ ExchangeInfo->ModeHighMemory = ExchangeInfo->ModeTransitionMemory;
+ ExchangeInfo->ModeHighMemory += (UINT32)ExchangeInfo->ModeOffset -
+ (UINT32)CpuMpData->AddressMap.ModeTransitionOffset;
+ ExchangeInfo->ModeHighSegment = (UINT16)ExchangeInfo->CodeSegment;
+ } else {
+ ExchangeInfo->ModeTransitionMemory = (UINT32)
+ (ExchangeInfo->BufferStart +
+ CpuMpData->AddressMap.ModeTransitionOffset);
+ }
}
/**
@@ -876,6 +916,11 @@ AllocateResetVector (
CpuMpData->WakeupBuffer = GetWakeupBuffer (ApResetVectorSize);
CpuMpData->MpCpuExchangeInfo = (MP_CPU_EXCHANGE_INFO *) (UINTN)
(CpuMpData->WakeupBuffer + CpuMpData->AddressMap.RendezvousFunnelSize);
+ CpuMpData->MpCpuExchangeInfo->ModeTransitionMemory = (UINT32)
+ GetModeTransitionBuffer (
+ CpuMpData->AddressMap.RendezvousFunnelSize -
+ CpuMpData->AddressMap.ModeTransitionOffset
+ );
}
BackupAndPrepareWakeupBuffer (CpuMpData); } diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 685e96cbac..0232fe896a 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -152,6 +152,7 @@ typedef struct {
UINTN RendezvousFunnelSize;
UINT8 *RelocateApLoopFuncAddress;
UINTN RelocateApLoopFuncSize;
+ UINTN ModeTransitionOffset;
} MP_ASSEMBLY_ADDRESS_MAP;
typedef struct _CPU_MP_DATA CPU_MP_DATA; @@ -182,6 +183,10 @@ typedef struct {
UINTN NumApsExecuting;
CPU_MP_DATA *CpuMpData;
UINTN InitializeFloatingPointUnitsAddress;
+ UINT32 ModeTransitionMemory;
+ UINT16 ModeTransitionSegment;
+ UINT32 ModeHighMemory;
+ UINT16 ModeHighSegment;
} MP_CPU_EXCHANGE_INFO;
#pragma pack()
@@ -329,6 +334,23 @@ GetWakeupBuffer (
IN UINTN WakeupBufferSize
);
+/**
+ Get available EfiBootServicesCode memory below 4GB by specified size.
+
+ This buffer is required to safely transfer AP from real address mode
+ to protected mode or long mode, due to the fact that the buffer
+ returned by
+ GetWakeupBuffer() may be marked as non-executable.
+
+ @param[in] BufferSize Wakeup transition buffer size.
+
+ @retval other Return wakeup transition buffer address below 4GB.
+ @retval 0 Cannot find free memory below 4GB.
+**/
+UINTN
+GetModeTransitionBuffer (
+ IN UINTN BufferSize
+ );
+
/**
This function will be called by BSP to wakeup AP.
diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
index 70c2bc7323..ad43bd33f5 100644
--- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
@@ -187,6 +187,29 @@ GetWakeupBuffer (
return (UINTN) -1;
}
+/**
+ Get available EfiBootServicesCode memory below 4GB by specified size.
+
+ This buffer is required to safely transfer AP from real address mode
+ to protected mode or long mode, due to the fact that the buffer
+ returned by
+ GetWakeupBuffer() may be marked as non-executable.
+
+ @param[in] BufferSize Wakeup transition buffer size.
+
+ @retval other Return wakeup transition buffer address below 4GB.
+ @retval 0 Cannot find free memory below 4GB.
+**/
+UINTN
+GetModeTransitionBuffer (
+ IN UINTN BufferSize
+ )
+{
+ //
+ // PEI phase doesn't need to do such transition. So simply return 0.
+ //
+ return 0;
+}
+
/**
Checks APs status and updates APs status if needed.
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
index d255ca5e1b..b5e09c6bc1 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc
@@ -42,4 +42,7 @@ InitFlagLocation equ LockLocation + 6Ch
CpuInfoLocation equ LockLocation + 74h
NumApsExecutingLocation equ LockLocation + 7Ch
InitializeFloatingPointUnitsAddress equ LockLocation + 8Ch
-
+ModeTransitionMemoryLocation equ LockLocation + 94h
+ModeTransitionSegmentLocation equ LockLocation + 98h
+ModeHighMemoryLocation equ LockLocation + 9Ah
+ModeHighSegmentLocation equ LockLocation + 9Eh
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 21d278600d..7595988884 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -52,16 +52,13 @@ BITS 16
mov si, BufferStartLocation
mov ebx, [si]
- mov di, ModeOffsetLocation
- mov eax, [di]
- mov di, CodeSegmentLocation
- mov edx, [di]
- mov di, ax
- sub di, 02h
- mov [di],dx ; Patch long mode CS
- sub di, 04h
- add eax, ebx
- mov [di],eax ; Patch address
+ mov si, DataSegmentLocation
+ mov edx, [si]
+
+ ;
+ ; Get start address of 32-bit code in low memory (<1MB)
+ ;
+ mov edi, ModeTransitionMemoryLocation
mov si, GdtrLocation
o32 lgdt [cs:si]
@@ -69,56 +66,79 @@ o32 lgdt [cs:si]
mov si, IdtrLocation
o32 lidt [cs:si]
- mov si, EnableExecuteDisableLocation
- cmp byte [si], 0
- jz SkipEnableExecuteDisableBit
+ ;
+ ; Switch to protected mode
+ ;
+ mov eax, cr0 ; Get control register 0
+ or eax, 000000003h ; Set PE bit (bit #0) & MP
+ mov cr0, eax
+
+ ; Switch to 32-bit code (>1MB)
+o32 jmp far [cs:di]
+
+;
+; Following code must be copied to memory with type of EfiBootServicesCode.
+; This is required if NX is enabled for EfiBootServicesCode of memory.
+;
+BITS 32
+Flat32Start: ; protected mode entry point
+ mov ds, dx
+ mov es, dx
+ mov fs, dx
+ mov gs, dx
+ mov ss, dx
;
; Enable execute disable bit
;
+ mov esi, EnableExecuteDisableLocation
+ cmp byte [ebx + esi], 0
+ jz SkipEnableExecuteDisableBit
+
mov ecx, 0c0000080h ; EFER MSR number
rdmsr ; Read EFER
bts eax, 11 ; Enable Execute Disable Bit
wrmsr ; Write EFER
SkipEnableExecuteDisableBit:
-
- mov di, DataSegmentLocation
- mov edi, [di] ; Save long mode DS in edi
-
- mov si, Cr3Location ; Save CR3 in ecx
- mov ecx, [si]
-
- xor ax, ax
- mov ds, ax ; Clear data segment
-
- mov eax, cr0 ; Get control register 0
- or eax, 000000003h ; Set PE bit (bit #0) & MP
- mov cr0, eax
-
+ ;
+ ; Enable PAE
+ ;
mov eax, cr4
bts eax, 5
mov cr4, eax
+ ;
+ ; Load page table
+ ;
+ mov esi, Cr3Location ; Save CR3 in ecx
+ mov ecx, [ebx + esi]
mov cr3, ecx ; Load CR3
+ ;
+ ; Enable long mode
+ ;
mov ecx, 0c0000080h ; EFER MSR number
rdmsr ; Read EFER
bts eax, 8 ; Set LME=1
wrmsr ; Write EFER
+ ;
+ ; Enable paging
+ ;
mov eax, cr0 ; Read CR0
bts eax, 31 ; Set PG=1
mov cr0, eax ; Write CR0
- jmp 0:strict dword 0 ; far jump to long mode
+ ;
+ ; Far jump to 64-bit code
+ ;
+ mov edi, ModeHighMemoryLocation
+ add edi, ebx
+ jmp far [edi]
+
BITS 64
LongModeStart:
- mov eax, edi
- mov ds, ax
- mov es, ax
- mov ss, ax
-
mov esi, ebx
lea edi, [esi + InitFlagLocation]
cmp qword [edi], 1 ; ApInitConfig
@@ -295,6 +315,7 @@ ASM_PFX(AsmGetAddressMap):
lea rax, [ASM_PFX(AsmRelocateApLoop)]
mov qword [rcx + 18h], rax
mov qword [rcx + 20h], AsmRelocateApLoopEnd - AsmRelocateApLoopStart
+ mov qword [rcx + 28h], Flat32Start - RendezvousFunnelProcStart
ret
;-------------------------------------------------------------------------------------
--
2.15.1.windows.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/6] UefiCpuPkg/MpInitLib: split wake up buffer into two parts
2018-01-15 8:54 ` [PATCH 1/6] UefiCpuPkg/MpInitLib: split wake up buffer into two parts Jian J Wang
2018-01-18 6:53 ` Dong, Eric
@ 2018-01-27 16:17 ` Laszlo Ersek
2018-01-28 21:43 ` Laszlo Ersek
1 sibling, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2018-01-27 16:17 UTC (permalink / raw)
To: Jian J Wang, edk2-devel; +Cc: Ruiyu Ni, Jiewen Yao, Eric Dong
Hello Jian,
On 01/15/18 09:54, Jian J Wang wrote:
> If PcdDxeNxMemoryProtectionPolicy is set to enable protection for memory
> of EfiBootServicesCode, EfiConventionalMemory, the BIOS will hang at a page
> fault exception during MP initialization.
>
> The root cause is that the AP wake up buffer, which is below 1MB and used
> to hold both AP init code and data, is type of EfiConventionalMemory (not
> really allocated because of potential conflict with legacy code), and is
> marked as non-executable. During the transition from real address mode
> to long mode, the AP init code has to enable paging which will then cause
> itself a page fault exception because it's just running in non-executable
> memory.
>
> The solution is splitting AP wake up buffer into two part: lower part is
> still below 1MB and shared with legacy system, higher part is really
> allocated memory of BootServicesCode type. The init code in the memory
> below 1MB will not enable paging but just switch to protected mode and
> jump to higher memory, in which the init code will enable paging and
> switch to long mode.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 34 ++++++++++
> UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc | 5 ++
> UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 32 +++++-----
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 45 +++++++++++++
> UefiCpuPkg/Library/MpInitLib/MpLib.h | 22 +++++++
> UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 23 +++++++
> UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc | 5 +-
> UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 87 ++++++++++++++++----------
> 8 files changed, 204 insertions(+), 49 deletions(-)
This patch breaks OVMF on KVM. The symptom is that the guest crashes and reboots as follows (infinite reboot loop):
> Loading PEIM at 0x0007FEB0000 EntryPoint=0x0007FEB5C96 CpuMpPei.efi
> AP Loop Mode is 1
> WakeupBufferStart = 9F000, WakeupBufferSize = 1000
> -- crash & reboot here --
> SecCoreStartupWithStack(0xFFFCC000, 0x820000)
Here's the bisection log:
git bisect start
# bad: [06c1f423e17fe5ddef824d688d21c83730238ba6] BeagleBoardPkg: reroute Firmware Vendor Pcd to MdeModulePkg
git bisect bad 06c1f423e17fe5ddef824d688d21c83730238ba6
# good: [018432f0ce1b42541977f61f9c7607257a4bf43a] MdeModulePkg/Ip4Dxe: Add an independent timer for reconfig checking
git bisect good 018432f0ce1b42541977f61f9c7607257a4bf43a
# bad: [8ab0bd2397c9d3922e0c7dbb1aa6f7e08799079f] MdePkg/DMAR: Add the definition for DMA_CTRL_PLATFORM_OPT_IN_FLAG bit
git bisect bad 8ab0bd2397c9d3922e0c7dbb1aa6f7e08799079f
# good: [24a105a7d8b4b8312743cf265f869dc049b7ff92] BaseTools: Disable warning varargs in XCODE5 align to CLANG38
git bisect good 24a105a7d8b4b8312743cf265f869dc049b7ff92
# good: [b2725f57c7a1e6feeb176f1563a4f1a8c2eb6c6f] IntelSiliconPkg IntelVTdPmrPei: Get high top by host address width
git bisect good b2725f57c7a1e6feeb176f1563a4f1a8c2eb6c6f
# good: [4f10654e04601fe67a750c9b5a4242efd4141569] UefiCpuPkg/CpuDxe: fix SetMemoryAttributes issue in 32-bit mode
git bisect good 4f10654e04601fe67a750c9b5a4242efd4141569
# bad: [fbe2c4b9be98a5c2b9c1f6976f51e2456467e752] UefiCpuPkg/CpuDxe: clear NX attr for page directory
git bisect bad fbe2c4b9be98a5c2b9c1f6976f51e2456467e752
# bad: [fceafda5185af0445d83f8c819b65417b981c485] UefiCpuPkg/CpuExceptionHandlerLib: alloc code memory for exception handlers
git bisect bad fceafda5185af0445d83f8c819b65417b981c485
# bad: [f32bfe6d061420a15bac6083063d227c567e6388] UefiCpuPkg/MpInitLib: split wake up buffer into two parts
git bisect bad f32bfe6d061420a15bac6083063d227c567e6388
# first bad commit: [f32bfe6d061420a15bac6083063d227c567e6388] UefiCpuPkg/MpInitLib: split wake up buffer into two parts
Thanks
Laszlo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/6] UefiCpuPkg/MpInitLib: split wake up buffer into two parts
2018-01-27 16:17 ` Laszlo Ersek
@ 2018-01-28 21:43 ` Laszlo Ersek
2018-01-29 1:06 ` Wang, Jian J
0 siblings, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2018-01-28 21:43 UTC (permalink / raw)
To: Jian J Wang, edk2-devel; +Cc: Ruiyu Ni, Jiewen Yao, Eric Dong
On 01/27/18 17:17, Laszlo Ersek wrote:
> Hello Jian,
>
> On 01/15/18 09:54, Jian J Wang wrote:
>> If PcdDxeNxMemoryProtectionPolicy is set to enable protection for memory
>> of EfiBootServicesCode, EfiConventionalMemory, the BIOS will hang at a page
>> fault exception during MP initialization.
>>
>> The root cause is that the AP wake up buffer, which is below 1MB and used
>> to hold both AP init code and data, is type of EfiConventionalMemory (not
>> really allocated because of potential conflict with legacy code), and is
>> marked as non-executable. During the transition from real address mode
>> to long mode, the AP init code has to enable paging which will then cause
>> itself a page fault exception because it's just running in non-executable
>> memory.
>>
>> The solution is splitting AP wake up buffer into two part: lower part is
>> still below 1MB and shared with legacy system, higher part is really
>> allocated memory of BootServicesCode type. The init code in the memory
>> below 1MB will not enable paging but just switch to protected mode and
>> jump to higher memory, in which the init code will enable paging and
>> switch to long mode.
>>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
>> ---
>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 34 ++++++++++
>> UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc | 5 ++
>> UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 32 +++++-----
>> UefiCpuPkg/Library/MpInitLib/MpLib.c | 45 +++++++++++++
>> UefiCpuPkg/Library/MpInitLib/MpLib.h | 22 +++++++
>> UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 23 +++++++
>> UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc | 5 +-
>> UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 87 ++++++++++++++++----------
>> 8 files changed, 204 insertions(+), 49 deletions(-)
>
> This patch breaks OVMF on KVM. The symptom is that the guest crashes
> and reboots as follows (infinite reboot loop):
>
>> Loading PEIM at 0x0007FEB0000 EntryPoint=0x0007FEB5C96 CpuMpPei.efi
>> AP Loop Mode is 1
>> WakeupBufferStart = 9F000, WakeupBufferSize = 1000
>> -- crash & reboot here --
>> SecCoreStartupWithStack(0xFFFCC000, 0x820000)
The following build options were used for this build:
$ build -a X64 -p OvmfPkg/OvmfPkgX64.dsc -D SECURE_BOOT_ENABLE \
-t GCC48 -b NOOPT -D HTTP_BOOT_ENABLE
The tree was built at 06c1f423e17f ("BeagleBoardPkg: reroute Firmware
Vendor Pcd to MdeModulePkg", 2018-01-26). (This commit is listed at the
top of the bisection log in my previous email.)
Here's the KVM log up to the triple fault:
> CPU-32283 [004] 13652.374591: kvm_entry: vcpu 2
> CPU-32283 [004] 13652.374594: kvm_exit: reason CR_ACCESS rip 0x3a info 0 0
> CPU-32283 [004] 13652.374595: kvm_cr: cr_write 0 = 0x60000013
> CPU-32283 [004] 13652.374596: kvm_entry: vcpu 2
> CPU-32283 [004] 13652.374597: kvm_exit: reason CR_ACCESS rip 0x9f06a info 4 0
> CPU-32283 [004] 13652.374598: kvm_cr: cr_write 4 = 0x20
> CPU-32283 [004] 13652.374603: kvm_entry: vcpu 2
> CPU-32283 [004] 13652.374604: kvm_exit: reason CR_ACCESS rip 0x9f075 info 103 0
> CPU-32283 [004] 13652.374605: kvm_cr: cr_write 3 = 0x800000
> CPU-32283 [004] 13652.374606: kvm_entry: vcpu 2
> CPU-32283 [004] 13652.374607: kvm_exit: reason MSR_READ rip 0x9f07d info 0 0
> CPU-32283 [004] 13652.374608: kvm_msr: msr_read c0000080 = 0x0
> CPU-32283 [004] 13652.374608: kvm_entry: vcpu 2
> CPU-32283 [004] 13652.374609: kvm_exit: reason MSR_WRITE rip 0x9f083 info 0 0
> CPU-32283 [004] 13652.374611: kvm_msr: msr_write c0000080 = 0x100
> CPU-32283 [004] 13652.374612: kvm_entry: vcpu 2
> CPU-32283 [004] 13652.374613: kvm_exit: reason CR_ACCESS rip 0x9f08c info 0 0
> CPU-32283 [004] 13652.374613: kvm_cr: cr_write 0 = 0xe0000013
> CPU-32283 [004] 13652.374620: kvm_entry: vcpu 2
> CPU-32283 [004] 13652.374622: kvm_exit: reason TRIPLE_FAULT rip 0x9f096 info 0 0
Offset 0x96 (relative to 0x9F000) is from
"UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm":
> 5c66d125eaae5 (Jeff Fan 2016-07-29 21:13:34 +0800 103) SkipEnableExecuteDisableBit:
> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 104) ;
> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 105) ; Enable PAE
> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 106) ;
> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 107) mov eax, cr4
> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 108) bts eax, 5
> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 109) mov cr4, eax
> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 110)
> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 111) ;
> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 112) ; Load page table
> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 113) ;
> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 114) mov esi, Cr3Location ; Save CR3 in ecx
> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 115) mov ecx, [ebx + esi]
> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 116) mov cr3, ecx ; Load CR3
> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 117)
> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 118) ;
> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 119) ; Enable long mode
> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 120) ;
> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 121) mov ecx, 0c0000080h ; EFER MSR number
> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 122) rdmsr ; Read EFER
> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 123) bts eax, 8 ; Set LME=1
> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 124) wrmsr ; Write EFER
> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 125)
> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 126) ;
> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 127) ; Enable paging
> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 128) ;
> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 129) mov eax, cr0 ; Read CR0
> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 130) bts eax, 31 ; Set PG=1
> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 131) mov cr0, eax ; Write CR0
> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 132)
> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 133) ;
> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 134) ; Far jump to 64-bit code
> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 135) ;
> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 136) mov edi, ModeHighMemoryLocation
> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 137) add edi, ebx
> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 138) jmp far [edi] <- here
Thanks
Laszlo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's supported
2018-01-15 8:54 ` [PATCH 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's supported Jian J Wang
2018-01-16 14:02 ` Dong, Eric
@ 2018-01-28 22:46 ` Laszlo Ersek
2018-01-29 9:02 ` Wang, Jian J
1 sibling, 1 reply; 22+ messages in thread
From: Laszlo Ersek @ 2018-01-28 22:46 UTC (permalink / raw)
To: Jian J Wang, edk2-devel
Cc: Ruiyu Ni, Jiewen Yao, Eric Dong, Paolo Bonzini,
Radim Krčmář
Hello Jian,
On 01/15/18 09:54, Jian J Wang wrote:
> If PcdDxeNxMemoryProtectionPolicy is set to enable protection for memory
> of EfiBootServicesCode, EfiConventionalMemory, the BIOS will hang at a page
> fault exception triggered by PiSmmCpuDxeSmm.
>
> The root cause is that PiSmmCpuDxeSmm will access default SMM RAM starting
> at 0x30000 which is marked as non-executable, but NX feature was not
> enabled during SMM initialization. Accessing memory which has invalid
> attributes set will cause page fault exception. This patch fixes it by
> checking NX capability in cpuid and enable NXE in EFER MSR if it's
> available.
>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 14 ++++++++++++++
> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm | 12 +++++++++++-
> 2 files changed, 25 insertions(+), 1 deletion(-)
This patch also breaks OVMF on KVM. However, the circumstances and the
symptom differ from those of the other regression that I reported under
patch 1/6 in this series [1] [2].
Namely, this affects the IA32 build, with SMM:
$ build -a IA32 -p OvmfPkg/OvmfPkgIa32.dsc -D SECURE_BOOT_ENABLE \
-D SMM_REQUIRE -t GCC48 -b NOOPT -D HTTP_BOOT_ENABLE
The value of PcdDxeNxMemoryProtectionPolicy is zero in this build too.
The virtual CPU model that this OVMF build runs on does *not* support
NX. (Please refer to "OvmfPkg/README":
> === SMM support ===
>
> [...]
>
> * QEMU binary and options specific to 32-bit guests:
>
> $ qemu-system-i386 -cpu coreduo,-nx \
)
The boot hangs at the following location:
> Loading SMM driver at 0x0007FFA2000 EntryPoint=0x0007FFA844D PiSmmCpuDxeSmm.efi
> SMRR Base: 0x7F000000, SMRR Size: 0x1000000
> PcdCpuSmmCodeAccessCheckEnable = 1
> mAddressEncMask = 0x0
> SMRAM TileSize = 0x00002000 (0x00001000, 0x00001000)
> SMRAM SaveState Buffer (0x7FF94000, 0x0000E000)
> CPU[000] APIC ID=0000 SMBASE=7FF8C000 SaveState=7FF9BC00 Size=00000400
> CPU[001] APIC ID=0001 SMBASE=7FF8E000 SaveState=7FF9DC00 Size=00000400
> CPU[002] APIC ID=0002 SMBASE=7FF90000 SaveState=7FF9FC00 Size=00000400
> CPU[003] APIC ID=0003 SMBASE=7FF92000 SaveState=7FFA1C00 Size=00000400
> <--HANG-->
KVM trace (excerpt):
> 1 CPU-4989 [002] 16163.874223: kvm_enter_smm: vcpu 1: entering SMM, smbase 0x30000
> 2 CPU-4989 [002] 16163.874244: kvm_fpu: load
> 3 CPU-4989 [002] 16163.874245: kvm_entry: vcpu 1
> 4 CPU-4989 [002] 16163.874247: kvm_exit: reason EPT_VIOLATION rip 0x8000 info 184 0
> 5 CPU-4989 [002] 16163.874247: kvm_page_fault: address 38000 error_code 184
> 6 CPU-4989 [002] 16163.874251: kvm_entry: vcpu 1
> 7 CPU-4989 [002] 16163.874253: kvm_exit: reason EPT_VIOLATION rip 0x7ff864ba info 184 0
> 8 CPU-4989 [002] 16163.874253: kvm_page_fault: address 7ffb64ba error_code 184
> 9 CPU-4989 [002] 16163.874256: kvm_entry: vcpu 1
> 10 CPU-4989 [002] 16163.874257: kvm_exit: reason CPUID rip 0x7ff864c0 info 0 0
> 11 CPU-4989 [002] 16163.874258: kvm_cpuid: func 80000001 rax 6e8 rbx 0 rcx 0 rdx 0
> 12 CPU-4989 [002] 16163.874258: kvm_entry: vcpu 1
> 13 CPU-4989 [002] 16163.874259: kvm_exit: reason CR_ACCESS rip 0x7ff864cb info 3 0
> 14 CPU-4989 [002] 16163.874260: kvm_cr: cr_write 3 = 0x0
> 15 CPU-4989 [002] 16163.874261: kvm_entry: vcpu 1
> 16 CPU-4989 [002] 16163.874262: kvm_exit: reason CR_ACCESS rip 0x7ff864db info 4 0
> 17 CPU-4989 [002] 16163.874263: kvm_cr: cr_write 4 = 0x640
> 18 CPU-4989 [002] 16163.874273: kvm_entry: vcpu 1
> 19 CPU-4989 [002] 16163.874274: kvm_exit: reason MSR_READ rip 0x7ff864e4 info 0 0
> 20 CPU-4989 [002] 16163.874275: kvm_msr: msr_read c0000080 = 0x0
> 21 CPU-4989 [002] 16163.874276: kvm_entry: vcpu 1
> 22 CPU-4989 [002] 16163.874277: kvm_exit: reason EPT_VIOLATION rip 0x64f4 info 184 0
> 23 CPU-4989 [002] 16163.874277: kvm_page_fault: address 364f4 error_code 184
> 24 CPU-4989 [002] 16163.874282: kvm_entry: vcpu 1
> 25 CPU-4989 [002] 16163.874283: kvm_exit: reason EPT_VIOLATION rip 0x64f4 info 183 0
> 26 CPU-4989 [002] 16163.874283: kvm_page_fault: address f000 error_code 183
> 27 CPU-4989 [002] 16163.874288: kvm_entry: vcpu 1
> 28 CPU-4989 [002] 16163.874292: kvm_exit: reason EPT_VIOLATION rip 0x7000 info 184 0
> 29 CPU-4989 [002] 16163.874293: kvm_page_fault: address 37000 error_code 184
> 30 CPU-4989 [002] 16163.874295: kvm_entry: vcpu 1
> 31 CPU-4989 [002] 16163.874300: kvm_exit: reason CPUID rip 0x7ff864c0 info 0 0
> 32 CPU-4989 [002] 16163.874301: kvm_cpuid: func 80000001 rax 6e8 rbx 0 rcx 0 rdx 0
> 33 CPU-4989 [002] 16163.874301: kvm_entry: vcpu 1
> 34 CPU-4989 [002] 16163.874302: kvm_exit: reason CR_ACCESS rip 0x7ff864cb info 3 0
> 35 CPU-4989 [002] 16163.874303: kvm_cr: cr_write 3 = 0x0
> 36 CPU-4989 [002] 16163.874304: kvm_entry: vcpu 1
After this point, lines 19-36 repeat infinitely.
The above trace corresponds to the following, from
"UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm":
> 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 43) global ASM_PFX(SmmStartup)
> 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 44) ASM_PFX(SmmStartup):
> d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 45) DB 0x66
> d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 46) mov eax, 0x80000001 ; read capability
> d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 47) cpuid
> d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 48) DB 0x66
> d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 49) mov ebx, edx ; rdmsr will change edx. keep it in ebx.
> 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 50) DB 0x66, 0xb8
> 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 51) ASM_PFX(gSmmCr3): DD 0
> 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 52) mov cr3, eax
> 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 53) DB 0x67, 0x66
> 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 54) lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) - ASM_PFX(SmmStartup))]
> 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 55) DB 0x66, 0xb8
> 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 56) ASM_PFX(gSmmCr4): DD 0
> 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 57) mov cr4, eax
> d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 58) DB 0x66
> d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 59) mov ecx, 0xc0000080 ; IA32_EFER MSR
> d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 60) rdmsr
> d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 61) DB 0x66
> d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 62) test ebx, BIT20 ; check NXE capability
> d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 63) jz .1
> d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 64) or ah, BIT3 ; set NXE bit
> d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 65) wrmsr
> d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 66) .1:
> 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 67) DB 0x66, 0xb8
> 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 68) ASM_PFX(gSmmCr0): DD 0
> 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 69) DB 0xbf, PROTECT_MODE_DS, 0 ; mov di, PROTECT_MODE_DS
> 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 70) mov cr0, eax
> 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 71) DB 0x66, 0xea ; jmp far [ptr48]
The WRMSR is never reached (which is fine) but the CR0 write is also not
reached, ever. Instead, we seem to be stuck in SMM forever, looping back
to SmmStartup.
Here's the bisection log:
> git bisect start
> # good: [018432f0ce1b42541977f61f9c7607257a4bf43a] MdeModulePkg/Ip4Dxe: Add an independent timer for reconfig checking
> git bisect good 018432f0ce1b42541977f61f9c7607257a4bf43a
> # bad: [06c1f423e17fe5ddef824d688d21c83730238ba6] BeagleBoardPkg: reroute Firmware Vendor Pcd to MdeModulePkg
> git bisect bad 06c1f423e17fe5ddef824d688d21c83730238ba6
> # bad: [8ab0bd2397c9d3922e0c7dbb1aa6f7e08799079f] MdePkg/DMAR: Add the definition for DMA_CTRL_PLATFORM_OPT_IN_FLAG bit
> git bisect bad 8ab0bd2397c9d3922e0c7dbb1aa6f7e08799079f
> # good: [24a105a7d8b4b8312743cf265f869dc049b7ff92] BaseTools: Disable warning varargs in XCODE5 align to CLANG38
> git bisect good 24a105a7d8b4b8312743cf265f869dc049b7ff92
> # good: [b2725f57c7a1e6feeb176f1563a4f1a8c2eb6c6f] IntelSiliconPkg IntelVTdPmrPei: Get high top by host address width
> git bisect good b2725f57c7a1e6feeb176f1563a4f1a8c2eb6c6f
> # good: [4f10654e04601fe67a750c9b5a4242efd4141569] UefiCpuPkg/CpuDxe: fix SetMemoryAttributes issue in 32-bit mode
> git bisect good 4f10654e04601fe67a750c9b5a4242efd4141569
> # good: [fbe2c4b9be98a5c2b9c1f6976f51e2456467e752] UefiCpuPkg/CpuDxe: clear NX attr for page directory
> git bisect good fbe2c4b9be98a5c2b9c1f6976f51e2456467e752
> # bad: [94c0129d244f91fa0a7b122414872da49a35f853] MdeModulePkg/PiSmmCore: remove NX attr for SMM RAM
> git bisect bad 94c0129d244f91fa0a7b122414872da49a35f853
> # bad: [d4d87596c11d6e3f8220b6d9677797c802af3a33] UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's supported
> git bisect bad d4d87596c11d6e3f8220b6d9677797c802af3a33
> # first bad commit: [d4d87596c11d6e3f8220b6d9677797c802af3a33] UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's supported
Finally, I have an independent question: why are we still adding *new*
0x66 size prefixes with "DB"? NASM supports 16-bit, 32-bit and 64-bit
assembly code in the same source file, and such prefixes can be encoded
symbolically [3]:
> Explicit address-size and operand-size prefixes A16, A32, A64, O16 and
> O32, O64 are provided -- one example of their use is given in chapter
> 10.
Thanks
Laszlo
[1] https://lists.01.org/pipermail/edk2-devel/2018-January/020582.html
[2] https://lists.01.org/pipermail/edk2-devel/2018-January/020586.html
[3] http://www.nasm.us/doc/nasmdoc3.html
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/6] UefiCpuPkg/MpInitLib: split wake up buffer into two parts
2018-01-28 21:43 ` Laszlo Ersek
@ 2018-01-29 1:06 ` Wang, Jian J
2018-01-29 15:50 ` Laszlo Ersek
0 siblings, 1 reply; 22+ messages in thread
From: Wang, Jian J @ 2018-01-29 1:06 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Yao, Jiewen, Dong, Eric
Hi Laszlo,
We've found this issue. The patch for it has sent out at
https://lists.01.org/pipermail/edk2-devel/2018-January/020467.html
It will be checked in soon.
Regards,
Jian
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, January 29, 2018 5:44 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Dong,
> Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH 1/6] UefiCpuPkg/MpInitLib: split wake up buffer into
> two parts
>
> On 01/27/18 17:17, Laszlo Ersek wrote:
> > Hello Jian,
> >
> > On 01/15/18 09:54, Jian J Wang wrote:
> >> If PcdDxeNxMemoryProtectionPolicy is set to enable protection for memory
> >> of EfiBootServicesCode, EfiConventionalMemory, the BIOS will hang at a
> page
> >> fault exception during MP initialization.
> >>
> >> The root cause is that the AP wake up buffer, which is below 1MB and used
> >> to hold both AP init code and data, is type of EfiConventionalMemory (not
> >> really allocated because of potential conflict with legacy code), and is
> >> marked as non-executable. During the transition from real address mode
> >> to long mode, the AP init code has to enable paging which will then cause
> >> itself a page fault exception because it's just running in non-executable
> >> memory.
> >>
> >> The solution is splitting AP wake up buffer into two part: lower part is
> >> still below 1MB and shared with legacy system, higher part is really
> >> allocated memory of BootServicesCode type. The init code in the memory
> >> below 1MB will not enable paging but just switch to protected mode and
> >> jump to higher memory, in which the init code will enable paging and
> >> switch to long mode.
> >>
> >> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >> Cc: Eric Dong <eric.dong@intel.com>
> >> Cc: Laszlo Ersek <lersek@redhat.com>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> >> ---
> >> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 34 ++++++++++
> >> UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc | 5 ++
> >> UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 32 +++++-----
> >> UefiCpuPkg/Library/MpInitLib/MpLib.c | 45 +++++++++++++
> >> UefiCpuPkg/Library/MpInitLib/MpLib.h | 22 +++++++
> >> UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 23 +++++++
> >> UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc | 5 +-
> >> UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 87 ++++++++++++++++-
> ---------
> >> 8 files changed, 204 insertions(+), 49 deletions(-)
> >
> > This patch breaks OVMF on KVM. The symptom is that the guest crashes
> > and reboots as follows (infinite reboot loop):
> >
> >> Loading PEIM at 0x0007FEB0000 EntryPoint=0x0007FEB5C96 CpuMpPei.efi
> >> AP Loop Mode is 1
> >> WakeupBufferStart = 9F000, WakeupBufferSize = 1000
> >> -- crash & reboot here --
> >> SecCoreStartupWithStack(0xFFFCC000, 0x820000)
>
> The following build options were used for this build:
>
> $ build -a X64 -p OvmfPkg/OvmfPkgX64.dsc -D SECURE_BOOT_ENABLE \
> -t GCC48 -b NOOPT -D HTTP_BOOT_ENABLE
>
> The tree was built at 06c1f423e17f ("BeagleBoardPkg: reroute Firmware
> Vendor Pcd to MdeModulePkg", 2018-01-26). (This commit is listed at the
> top of the bisection log in my previous email.)
>
> Here's the KVM log up to the triple fault:
>
> > CPU-32283 [004] 13652.374591: kvm_entry: vcpu 2
> > CPU-32283 [004] 13652.374594: kvm_exit: reason CR_ACCESS
> rip 0x3a info 0 0
> > CPU-32283 [004] 13652.374595: kvm_cr: cr_write 0 =
> 0x60000013
> > CPU-32283 [004] 13652.374596: kvm_entry: vcpu 2
> > CPU-32283 [004] 13652.374597: kvm_exit: reason CR_ACCESS
> rip 0x9f06a info 4 0
> > CPU-32283 [004] 13652.374598: kvm_cr: cr_write 4 = 0x20
> > CPU-32283 [004] 13652.374603: kvm_entry: vcpu 2
> > CPU-32283 [004] 13652.374604: kvm_exit: reason CR_ACCESS
> rip 0x9f075 info 103 0
> > CPU-32283 [004] 13652.374605: kvm_cr: cr_write 3 = 0x800000
> > CPU-32283 [004] 13652.374606: kvm_entry: vcpu 2
> > CPU-32283 [004] 13652.374607: kvm_exit: reason MSR_READ
> rip 0x9f07d info 0 0
> > CPU-32283 [004] 13652.374608: kvm_msr: msr_read c0000080
> = 0x0
> > CPU-32283 [004] 13652.374608: kvm_entry: vcpu 2
> > CPU-32283 [004] 13652.374609: kvm_exit: reason MSR_WRITE
> rip 0x9f083 info 0 0
> > CPU-32283 [004] 13652.374611: kvm_msr: msr_write c0000080
> = 0x100
> > CPU-32283 [004] 13652.374612: kvm_entry: vcpu 2
> > CPU-32283 [004] 13652.374613: kvm_exit: reason CR_ACCESS
> rip 0x9f08c info 0 0
> > CPU-32283 [004] 13652.374613: kvm_cr: cr_write 0 =
> 0xe0000013
> > CPU-32283 [004] 13652.374620: kvm_entry: vcpu 2
> > CPU-32283 [004] 13652.374622: kvm_exit: reason
> TRIPLE_FAULT rip 0x9f096 info 0 0
>
> Offset 0x96 (relative to 0x9F000) is from
> "UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm":
>
> > 5c66d125eaae5 (Jeff Fan 2016-07-29 21:13:34 +0800 103)
> SkipEnableExecuteDisableBit:
> > f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 104) ;
> > f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 105) ; Enable PAE
> > f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 106) ;
> > d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 107) mov eax,
> cr4
> > d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 108) bts eax, 5
> > d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 109) mov cr4,
> eax
> > d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 110)
> > f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 111) ;
> > f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 112) ; Load page
> table
> > f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 113) ;
> > f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 114) mov esi,
> Cr3Location ; Save CR3 in ecx
> > f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 115) mov ecx,
> [ebx + esi]
> > d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 116) mov cr3,
> ecx ; Load CR3
> > d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 117)
> > f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 118) ;
> > f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 119) ; Enable long
> mode
> > f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 120) ;
> > d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 121) mov ecx,
> 0c0000080h ; EFER MSR number
> > d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 122)
> rdmsr ; Read EFER
> > d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 123) bts eax,
> 8 ; Set LME=1
> > d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 124)
> wrmsr ; Write EFER
> > d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 125)
> > f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 126) ;
> > f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 127) ; Enable
> paging
> > f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 128) ;
> > d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 129) mov eax,
> cr0 ; Read CR0
> > d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 130) bts eax,
> 31 ; Set PG=1
> > d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 131) mov cr0,
> eax ; Write CR0
> > d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 132)
> > f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 133) ;
> > f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 134) ; Far jump to
> 64-bit code
> > f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 135) ;
> > f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 136) mov edi,
> ModeHighMemoryLocation
> > f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 137) add edi,
> ebx
> > f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 138) jmp far [edi]
> <- here
>
> Thanks
> Laszlo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's supported
2018-01-28 22:46 ` Laszlo Ersek
@ 2018-01-29 9:02 ` Wang, Jian J
2018-01-29 19:48 ` Laszlo Ersek
0 siblings, 1 reply; 22+ messages in thread
From: Wang, Jian J @ 2018-01-29 9:02 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Cc: Ni, Ruiyu, Yao, Jiewen, Dong, Eric, Paolo Bonzini,
Radim Kr?má?
Hi Laszlo,
I don't know the history of these code but I guess they're converted from .asm file.
That may be why there's "DB 66h" prefix. I think you're right these tricks should be
replaced with more formal ways. Please submit a bz tracker for it.
As to the issue, I don't have clue right now. The code seems no problem. Since msr
write didn't happen, code flow is correct. And those code has executed on real
32-bit platform without problem. I need more time to investigate it.
Regards,
Jian
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, January 29, 2018 6:47 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Dong,
> Eric <eric.dong@intel.com>; Paolo Bonzini <pbonzini@redhat.com>; Radim
> Kr?má? <rkrcmar@redhat.com>
> Subject: Re: [edk2] [PATCH 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if
> it's supported
>
> Hello Jian,
>
> On 01/15/18 09:54, Jian J Wang wrote:
> > If PcdDxeNxMemoryProtectionPolicy is set to enable protection for memory
> > of EfiBootServicesCode, EfiConventionalMemory, the BIOS will hang at a page
> > fault exception triggered by PiSmmCpuDxeSmm.
> >
> > The root cause is that PiSmmCpuDxeSmm will access default SMM RAM
> starting
> > at 0x30000 which is marked as non-executable, but NX feature was not
> > enabled during SMM initialization. Accessing memory which has invalid
> > attributes set will cause page fault exception. This patch fixes it by
> > checking NX capability in cpuid and enable NXE in EFER MSR if it's
> > available.
> >
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 14 ++++++++++++++
> > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmInit.nasm | 12 +++++++++++-
> > 2 files changed, 25 insertions(+), 1 deletion(-)
>
> This patch also breaks OVMF on KVM. However, the circumstances and the
> symptom differ from those of the other regression that I reported under
> patch 1/6 in this series [1] [2].
>
> Namely, this affects the IA32 build, with SMM:
>
> $ build -a IA32 -p OvmfPkg/OvmfPkgIa32.dsc -D SECURE_BOOT_ENABLE \
> -D SMM_REQUIRE -t GCC48 -b NOOPT -D HTTP_BOOT_ENABLE
>
> The value of PcdDxeNxMemoryProtectionPolicy is zero in this build too.
>
> The virtual CPU model that this OVMF build runs on does *not* support
> NX. (Please refer to "OvmfPkg/README":
>
> > === SMM support ===
> >
> > [...]
> >
> > * QEMU binary and options specific to 32-bit guests:
> >
> > $ qemu-system-i386 -cpu coreduo,-nx \
>
> )
>
> The boot hangs at the following location:
>
> > Loading SMM driver at 0x0007FFA2000 EntryPoint=0x0007FFA844D
> PiSmmCpuDxeSmm.efi
> > SMRR Base: 0x7F000000, SMRR Size: 0x1000000
> > PcdCpuSmmCodeAccessCheckEnable = 1
> > mAddressEncMask = 0x0
> > SMRAM TileSize = 0x00002000 (0x00001000, 0x00001000)
> > SMRAM SaveState Buffer (0x7FF94000, 0x0000E000)
> > CPU[000] APIC ID=0000 SMBASE=7FF8C000 SaveState=7FF9BC00
> Size=00000400
> > CPU[001] APIC ID=0001 SMBASE=7FF8E000 SaveState=7FF9DC00
> Size=00000400
> > CPU[002] APIC ID=0002 SMBASE=7FF90000 SaveState=7FF9FC00
> Size=00000400
> > CPU[003] APIC ID=0003 SMBASE=7FF92000 SaveState=7FFA1C00
> Size=00000400
> > <--HANG-->
>
> KVM trace (excerpt):
>
> > 1 CPU-4989 [002] 16163.874223: kvm_enter_smm: vcpu 1: entering SMM,
> smbase 0x30000
> > 2 CPU-4989 [002] 16163.874244: kvm_fpu: load
> > 3 CPU-4989 [002] 16163.874245: kvm_entry: vcpu 1
> > 4 CPU-4989 [002] 16163.874247: kvm_exit: reason EPT_VIOLATION rip
> 0x8000 info 184 0
> > 5 CPU-4989 [002] 16163.874247: kvm_page_fault: address 38000
> error_code 184
> > 6 CPU-4989 [002] 16163.874251: kvm_entry: vcpu 1
> > 7 CPU-4989 [002] 16163.874253: kvm_exit: reason EPT_VIOLATION rip
> 0x7ff864ba info 184 0
> > 8 CPU-4989 [002] 16163.874253: kvm_page_fault: address 7ffb64ba
> error_code 184
> > 9 CPU-4989 [002] 16163.874256: kvm_entry: vcpu 1
> > 10 CPU-4989 [002] 16163.874257: kvm_exit: reason CPUID rip
> 0x7ff864c0 info 0 0
> > 11 CPU-4989 [002] 16163.874258: kvm_cpuid: func 80000001 rax 6e8
> rbx 0 rcx 0 rdx 0
> > 12 CPU-4989 [002] 16163.874258: kvm_entry: vcpu 1
> > 13 CPU-4989 [002] 16163.874259: kvm_exit: reason CR_ACCESS rip
> 0x7ff864cb info 3 0
> > 14 CPU-4989 [002] 16163.874260: kvm_cr: cr_write 3 = 0x0
> > 15 CPU-4989 [002] 16163.874261: kvm_entry: vcpu 1
> > 16 CPU-4989 [002] 16163.874262: kvm_exit: reason CR_ACCESS rip
> 0x7ff864db info 4 0
> > 17 CPU-4989 [002] 16163.874263: kvm_cr: cr_write 4 = 0x640
> > 18 CPU-4989 [002] 16163.874273: kvm_entry: vcpu 1
> > 19 CPU-4989 [002] 16163.874274: kvm_exit: reason MSR_READ rip
> 0x7ff864e4 info 0 0
> > 20 CPU-4989 [002] 16163.874275: kvm_msr: msr_read c0000080 = 0x0
> > 21 CPU-4989 [002] 16163.874276: kvm_entry: vcpu 1
> > 22 CPU-4989 [002] 16163.874277: kvm_exit: reason EPT_VIOLATION
> rip 0x64f4 info 184 0
> > 23 CPU-4989 [002] 16163.874277: kvm_page_fault: address 364f4
> error_code 184
> > 24 CPU-4989 [002] 16163.874282: kvm_entry: vcpu 1
> > 25 CPU-4989 [002] 16163.874283: kvm_exit: reason EPT_VIOLATION
> rip 0x64f4 info 183 0
> > 26 CPU-4989 [002] 16163.874283: kvm_page_fault: address f000
> error_code 183
> > 27 CPU-4989 [002] 16163.874288: kvm_entry: vcpu 1
> > 28 CPU-4989 [002] 16163.874292: kvm_exit: reason EPT_VIOLATION
> rip 0x7000 info 184 0
> > 29 CPU-4989 [002] 16163.874293: kvm_page_fault: address 37000
> error_code 184
> > 30 CPU-4989 [002] 16163.874295: kvm_entry: vcpu 1
> > 31 CPU-4989 [002] 16163.874300: kvm_exit: reason CPUID rip
> 0x7ff864c0 info 0 0
> > 32 CPU-4989 [002] 16163.874301: kvm_cpuid: func 80000001 rax 6e8
> rbx 0 rcx 0 rdx 0
> > 33 CPU-4989 [002] 16163.874301: kvm_entry: vcpu 1
> > 34 CPU-4989 [002] 16163.874302: kvm_exit: reason CR_ACCESS rip
> 0x7ff864cb info 3 0
> > 35 CPU-4989 [002] 16163.874303: kvm_cr: cr_write 3 = 0x0
> > 36 CPU-4989 [002] 16163.874304: kvm_entry: vcpu 1
>
> After this point, lines 19-36 repeat infinitely.
>
> The above trace corresponds to the following, from
> "UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm":
>
> > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 43) global
> ASM_PFX(SmmStartup)
> > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 44)
> ASM_PFX(SmmStartup):
> > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 45) DB 0x66
> > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 46) mov eax,
> 0x80000001 ; read capability
> > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 47) cpuid
> > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 48) DB 0x66
> > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 49) mov ebx,
> edx ; rdmsr will change edx. keep it in ebx.
> > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 50) DB 0x66,
> 0xb8
> > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 51)
> ASM_PFX(gSmmCr3): DD 0
> > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 52) mov cr3, eax
> > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 53) DB 0x67,
> 0x66
> > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 54) lgdt [cs:ebp +
> (ASM_PFX(gcSmiInitGdtr) - ASM_PFX(SmmStartup))]
> > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 55) DB 0x66,
> 0xb8
> > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 56)
> ASM_PFX(gSmmCr4): DD 0
> > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 57) mov cr4, eax
> > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 58) DB 0x66
> > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 59) mov ecx,
> 0xc0000080 ; IA32_EFER MSR
> > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 60) rdmsr
> > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 61) DB 0x66
> > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 62) test ebx,
> BIT20 ; check NXE capability
> > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 63) jz .1
> > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 64) or ah,
> BIT3 ; set NXE bit
> > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 65) wrmsr
> > d4d87596c11d6 (Jian J Wang 2018-01-15 10:16:26 +0800 66) .1:
> > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 67) DB 0x66,
> 0xb8
> > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 68)
> ASM_PFX(gSmmCr0): DD 0
> > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 69) DB 0xbf,
> PROTECT_MODE_DS, 0 ; mov di, PROTECT_MODE_DS
> > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 70) mov cr0, eax
> > 246cd9085f806 (Liming Gao 2016-06-14 16:29:40 +0800 71) DB 0x66,
> 0xea ; jmp far [ptr48]
>
> The WRMSR is never reached (which is fine) but the CR0 write is also not
> reached, ever. Instead, we seem to be stuck in SMM forever, looping back
> to SmmStartup.
>
> Here's the bisection log:
>
> > git bisect start
> > # good: [018432f0ce1b42541977f61f9c7607257a4bf43a]
> MdeModulePkg/Ip4Dxe: Add an independent timer for reconfig checking
> > git bisect good 018432f0ce1b42541977f61f9c7607257a4bf43a
> > # bad: [06c1f423e17fe5ddef824d688d21c83730238ba6] BeagleBoardPkg:
> reroute Firmware Vendor Pcd to MdeModulePkg
> > git bisect bad 06c1f423e17fe5ddef824d688d21c83730238ba6
> > # bad: [8ab0bd2397c9d3922e0c7dbb1aa6f7e08799079f] MdePkg/DMAR: Add
> the definition for DMA_CTRL_PLATFORM_OPT_IN_FLAG bit
> > git bisect bad 8ab0bd2397c9d3922e0c7dbb1aa6f7e08799079f
> > # good: [24a105a7d8b4b8312743cf265f869dc049b7ff92] BaseTools: Disable
> warning varargs in XCODE5 align to CLANG38
> > git bisect good 24a105a7d8b4b8312743cf265f869dc049b7ff92
> > # good: [b2725f57c7a1e6feeb176f1563a4f1a8c2eb6c6f] IntelSiliconPkg
> IntelVTdPmrPei: Get high top by host address width
> > git bisect good b2725f57c7a1e6feeb176f1563a4f1a8c2eb6c6f
> > # good: [4f10654e04601fe67a750c9b5a4242efd4141569] UefiCpuPkg/CpuDxe:
> fix SetMemoryAttributes issue in 32-bit mode
> > git bisect good 4f10654e04601fe67a750c9b5a4242efd4141569
> > # good: [fbe2c4b9be98a5c2b9c1f6976f51e2456467e752] UefiCpuPkg/CpuDxe:
> clear NX attr for page directory
> > git bisect good fbe2c4b9be98a5c2b9c1f6976f51e2456467e752
> > # bad: [94c0129d244f91fa0a7b122414872da49a35f853]
> MdeModulePkg/PiSmmCore: remove NX attr for SMM RAM
> > git bisect bad 94c0129d244f91fa0a7b122414872da49a35f853
> > # bad: [d4d87596c11d6e3f8220b6d9677797c802af3a33]
> UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's supported
> > git bisect bad d4d87596c11d6e3f8220b6d9677797c802af3a33
> > # first bad commit: [d4d87596c11d6e3f8220b6d9677797c802af3a33]
> UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's supported
>
>
> Finally, I have an independent question: why are we still adding *new*
> 0x66 size prefixes with "DB"? NASM supports 16-bit, 32-bit and 64-bit
> assembly code in the same source file, and such prefixes can be encoded
> symbolically [3]:
>
> > Explicit address-size and operand-size prefixes A16, A32, A64, O16 and
> > O32, O64 are provided -- one example of their use is given in chapter
> > 10.
>
> Thanks
> Laszlo
>
> [1] https://lists.01.org/pipermail/edk2-devel/2018-January/020582.html
> [2] https://lists.01.org/pipermail/edk2-devel/2018-January/020586.html
> [3] http://www.nasm.us/doc/nasmdoc3.html
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/6] UefiCpuPkg/MpInitLib: split wake up buffer into two parts
2018-01-29 1:06 ` Wang, Jian J
@ 2018-01-29 15:50 ` Laszlo Ersek
0 siblings, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2018-01-29 15:50 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Yao, Jiewen, Dong, Eric
On 01/29/18 02:06, Wang, Jian J wrote:
> Hi Laszlo,
>
> We've found this issue. The patch for it has sent out at
>
> https://lists.01.org/pipermail/edk2-devel/2018-January/020467.html
>
> It will be checked in soon.
I've rebuilt OVMF @ c4122dcaadb9 ("SecurityPkg: Tcg2Smm: Enable TPM2.0
interrupt support", 2018-01-29) and indeed it works fine. Thank you!
Laszlo
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, January 29, 2018 5:44 AM
>> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Dong,
>> Eric <eric.dong@intel.com>
>> Subject: Re: [edk2] [PATCH 1/6] UefiCpuPkg/MpInitLib: split wake up buffer into
>> two parts
>>
>> On 01/27/18 17:17, Laszlo Ersek wrote:
>>> Hello Jian,
>>>
>>> On 01/15/18 09:54, Jian J Wang wrote:
>>>> If PcdDxeNxMemoryProtectionPolicy is set to enable protection for memory
>>>> of EfiBootServicesCode, EfiConventionalMemory, the BIOS will hang at a
>> page
>>>> fault exception during MP initialization.
>>>>
>>>> The root cause is that the AP wake up buffer, which is below 1MB and used
>>>> to hold both AP init code and data, is type of EfiConventionalMemory (not
>>>> really allocated because of potential conflict with legacy code), and is
>>>> marked as non-executable. During the transition from real address mode
>>>> to long mode, the AP init code has to enable paging which will then cause
>>>> itself a page fault exception because it's just running in non-executable
>>>> memory.
>>>>
>>>> The solution is splitting AP wake up buffer into two part: lower part is
>>>> still below 1MB and shared with legacy system, higher part is really
>>>> allocated memory of BootServicesCode type. The init code in the memory
>>>> below 1MB will not enable paging but just switch to protected mode and
>>>> jump to higher memory, in which the init code will enable paging and
>>>> switch to long mode.
>>>>
>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
>>>> ---
>>>> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 34 ++++++++++
>>>> UefiCpuPkg/Library/MpInitLib/Ia32/MpEqu.inc | 5 ++
>>>> UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 32 +++++-----
>>>> UefiCpuPkg/Library/MpInitLib/MpLib.c | 45 +++++++++++++
>>>> UefiCpuPkg/Library/MpInitLib/MpLib.h | 22 +++++++
>>>> UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 23 +++++++
>>>> UefiCpuPkg/Library/MpInitLib/X64/MpEqu.inc | 5 +-
>>>> UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 87 ++++++++++++++++-
>> ---------
>>>> 8 files changed, 204 insertions(+), 49 deletions(-)
>>>
>>> This patch breaks OVMF on KVM. The symptom is that the guest crashes
>>> and reboots as follows (infinite reboot loop):
>>>
>>>> Loading PEIM at 0x0007FEB0000 EntryPoint=0x0007FEB5C96 CpuMpPei.efi
>>>> AP Loop Mode is 1
>>>> WakeupBufferStart = 9F000, WakeupBufferSize = 1000
>>>> -- crash & reboot here --
>>>> SecCoreStartupWithStack(0xFFFCC000, 0x820000)
>>
>> The following build options were used for this build:
>>
>> $ build -a X64 -p OvmfPkg/OvmfPkgX64.dsc -D SECURE_BOOT_ENABLE \
>> -t GCC48 -b NOOPT -D HTTP_BOOT_ENABLE
>>
>> The tree was built at 06c1f423e17f ("BeagleBoardPkg: reroute Firmware
>> Vendor Pcd to MdeModulePkg", 2018-01-26). (This commit is listed at the
>> top of the bisection log in my previous email.)
>>
>> Here's the KVM log up to the triple fault:
>>
>>> CPU-32283 [004] 13652.374591: kvm_entry: vcpu 2
>>> CPU-32283 [004] 13652.374594: kvm_exit: reason CR_ACCESS
>> rip 0x3a info 0 0
>>> CPU-32283 [004] 13652.374595: kvm_cr: cr_write 0 =
>> 0x60000013
>>> CPU-32283 [004] 13652.374596: kvm_entry: vcpu 2
>>> CPU-32283 [004] 13652.374597: kvm_exit: reason CR_ACCESS
>> rip 0x9f06a info 4 0
>>> CPU-32283 [004] 13652.374598: kvm_cr: cr_write 4 = 0x20
>>> CPU-32283 [004] 13652.374603: kvm_entry: vcpu 2
>>> CPU-32283 [004] 13652.374604: kvm_exit: reason CR_ACCESS
>> rip 0x9f075 info 103 0
>>> CPU-32283 [004] 13652.374605: kvm_cr: cr_write 3 = 0x800000
>>> CPU-32283 [004] 13652.374606: kvm_entry: vcpu 2
>>> CPU-32283 [004] 13652.374607: kvm_exit: reason MSR_READ
>> rip 0x9f07d info 0 0
>>> CPU-32283 [004] 13652.374608: kvm_msr: msr_read c0000080
>> = 0x0
>>> CPU-32283 [004] 13652.374608: kvm_entry: vcpu 2
>>> CPU-32283 [004] 13652.374609: kvm_exit: reason MSR_WRITE
>> rip 0x9f083 info 0 0
>>> CPU-32283 [004] 13652.374611: kvm_msr: msr_write c0000080
>> = 0x100
>>> CPU-32283 [004] 13652.374612: kvm_entry: vcpu 2
>>> CPU-32283 [004] 13652.374613: kvm_exit: reason CR_ACCESS
>> rip 0x9f08c info 0 0
>>> CPU-32283 [004] 13652.374613: kvm_cr: cr_write 0 =
>> 0xe0000013
>>> CPU-32283 [004] 13652.374620: kvm_entry: vcpu 2
>>> CPU-32283 [004] 13652.374622: kvm_exit: reason
>> TRIPLE_FAULT rip 0x9f096 info 0 0
>>
>> Offset 0x96 (relative to 0x9F000) is from
>> "UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm":
>>
>>> 5c66d125eaae5 (Jeff Fan 2016-07-29 21:13:34 +0800 103)
>> SkipEnableExecuteDisableBit:
>>> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 104) ;
>>> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 105) ; Enable PAE
>>> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 106) ;
>>> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 107) mov eax,
>> cr4
>>> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 108) bts eax, 5
>>> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 109) mov cr4,
>> eax
>>> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 110)
>>> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 111) ;
>>> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 112) ; Load page
>> table
>>> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 113) ;
>>> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 114) mov esi,
>> Cr3Location ; Save CR3 in ecx
>>> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 115) mov ecx,
>> [ebx + esi]
>>> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 116) mov cr3,
>> ecx ; Load CR3
>>> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 117)
>>> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 118) ;
>>> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 119) ; Enable long
>> mode
>>> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 120) ;
>>> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 121) mov ecx,
>> 0c0000080h ; EFER MSR number
>>> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 122)
>> rdmsr ; Read EFER
>>> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 123) bts eax,
>> 8 ; Set LME=1
>>> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 124)
>> wrmsr ; Write EFER
>>> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 125)
>>> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 126) ;
>>> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 127) ; Enable
>> paging
>>> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 128) ;
>>> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 129) mov eax,
>> cr0 ; Read CR0
>>> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 130) bts eax,
>> 31 ; Set PG=1
>>> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 131) mov cr0,
>> eax ; Write CR0
>>> d94e5f672994f (Jeff Fan 2016-07-20 22:44:39 +0800 132)
>>> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 133) ;
>>> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 134) ; Far jump to
>> 64-bit code
>>> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 135) ;
>>> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 136) mov edi,
>> ModeHighMemoryLocation
>>> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 137) add edi,
>> ebx
>>> f32bfe6d06142 (Jian J Wang 2017-12-29 09:12:54 +0800 138) jmp far [edi]
>> <- here
>>
>> Thanks
>> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's supported
2018-01-29 9:02 ` Wang, Jian J
@ 2018-01-29 19:48 ` Laszlo Ersek
2018-01-30 13:09 ` Laszlo Ersek
2018-02-01 1:08 ` Wang, Jian J
0 siblings, 2 replies; 22+ messages in thread
From: Laszlo Ersek @ 2018-01-29 19:48 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Ni, Ruiyu, Paolo Bonzini, Yao, Jiewen, Dong, Eric,
Radim Krčmář
On 01/29/18 10:02, Wang, Jian J wrote:
> Hi Laszlo,
>
> I don't know the history of these code but I guess they're converted
> from .asm file. That may be why there's "DB 66h" prefix. I think
> you're right these tricks should be replaced with more formal ways.
> Please submit a bz tracker for it.
I submitted <https://bugzilla.tianocore.org/show_bug.cgi?id=866>.
> As to the issue, I don't have clue right now. The code seems no
> problem. Since msr write didn't happen, code flow is correct. And
> those code has executed on real 32-bit platform without problem. I
> need more time to investigate it.
I think I've found the issue; more exactly I narrowed it down a bit. I
remember that the same problem drove me mad a few years ago. :)
The issue is that in the middle of such processor mode switches, no jump
instructions work *at all* on KVM. I don't know why, this is just my
experience. The KVM behavior could even be justified by the Intel SDM.
I'm unsure.
Let's look at the patch with more context:
> global ASM_PFX(SmmStartup)
> ASM_PFX(SmmStartup):
> + DB 0x66
> + mov eax, 0x80000001 ; read capability
> + cpuid
> + DB 0x66
> + mov ebx, edx ; rdmsr will change edx. keep it in ebx.
> DB 0x66, 0xb8
> ASM_PFX(gSmmCr3): DD 0
> mov cr3, eax
> DB 0x67, 0x66
> lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) - ASM_PFX(SmmStartup))]
> DB 0x66, 0xb8
> ASM_PFX(gSmmCr4): DD 0
> mov cr4, eax
> + DB 0x66
> + mov ecx, 0xc0000080 ; IA32_EFER MSR
> + rdmsr
> + DB 0x66
> + test ebx, BIT20 ; check NXE capability
> + jz .1
> + or ah, BIT3 ; set NXE bit
> + wrmsr
> +.1:
This code has exactly one jump, and in practice it is never taken
(because NX support is ubiquitous on physical platforms).
In my testing I added some other conditional jumps -- I reimplemented
IsExecuteDisableBitAvailable() from
"MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c", which has more
conditions. All the conditional jumps that were *not* taken didn't
cause any issues. (This is why the same logic from the patch works for
me in the X64 version, because there the "jz" is never taken, since NX
is always available there.) However, the first jump that *was* taken in
my testing immediately hung or crashed the IA32 guest.
Finally I replaced the entire NX management code with an unconditional
jump forward:
jmp jump_here
jump_here:
and even this hung / crashed the guest.
For some reason this section of the code is unfit for jumping, under KVM
anyway.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's supported
2018-01-29 19:48 ` Laszlo Ersek
@ 2018-01-30 13:09 ` Laszlo Ersek
2018-02-01 1:08 ` Wang, Jian J
1 sibling, 0 replies; 22+ messages in thread
From: Laszlo Ersek @ 2018-01-30 13:09 UTC (permalink / raw)
To: Wang, Jian J, edk2-devel@lists.01.org
Cc: Ni, Ruiyu, Paolo Bonzini, Yao, Jiewen, Dong, Eric,
Radim Krčmář
On 01/29/18 20:48, Laszlo Ersek wrote:
> On 01/29/18 10:02, Wang, Jian J wrote:
>> Hi Laszlo,
>>
>> I don't know the history of these code but I guess they're converted
>> from .asm file. That may be why there's "DB 66h" prefix. I think
>> you're right these tricks should be replaced with more formal ways.
>> Please submit a bz tracker for it.
>
> I submitted <https://bugzilla.tianocore.org/show_bug.cgi?id=866>.
>
>> As to the issue, I don't have clue right now. The code seems no
>> problem. Since msr write didn't happen, code flow is correct. And
>> those code has executed on real 32-bit platform without problem. I
>> need more time to investigate it.
>
> I think I've found the issue; more exactly I narrowed it down a bit. I
> remember that the same problem drove me mad a few years ago. :)
>
> The issue is that in the middle of such processor mode switches, no jump
> instructions work *at all* on KVM. I don't know why, this is just my
> experience. The KVM behavior could even be justified by the Intel SDM.
> I'm unsure.
>
> Let's look at the patch with more context:
>
>> global ASM_PFX(SmmStartup)
>> ASM_PFX(SmmStartup):
>> + DB 0x66
>> + mov eax, 0x80000001 ; read capability
>> + cpuid
>> + DB 0x66
>> + mov ebx, edx ; rdmsr will change edx. keep it in ebx.
>> DB 0x66, 0xb8
>> ASM_PFX(gSmmCr3): DD 0
>> mov cr3, eax
>> DB 0x67, 0x66
>> lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) - ASM_PFX(SmmStartup))]
>> DB 0x66, 0xb8
>> ASM_PFX(gSmmCr4): DD 0
>> mov cr4, eax
>> + DB 0x66
>> + mov ecx, 0xc0000080 ; IA32_EFER MSR
>> + rdmsr
>> + DB 0x66
>> + test ebx, BIT20 ; check NXE capability
>> + jz .1
>> + or ah, BIT3 ; set NXE bit
>> + wrmsr
>> +.1:
>
> This code has exactly one jump, and in practice it is never taken
> (because NX support is ubiquitous on physical platforms).
>
> In my testing I added some other conditional jumps -- I reimplemented
> IsExecuteDisableBitAvailable() from
> "MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c", which has more
> conditions. All the conditional jumps that were *not* taken didn't
> cause any issues. (This is why the same logic from the patch works for
> me in the X64 version, because there the "jz" is never taken, since NX
> is always available there.) However, the first jump that *was* taken in
> my testing immediately hung or crashed the IA32 guest.
>
> Finally I replaced the entire NX management code with an unconditional
> jump forward:
>
> jmp jump_here
> jump_here:
>
> and even this hung / crashed the guest.
>
> For some reason this section of the code is unfit for jumping, under KVM
> anyway.
I found a work-around for this.
While short jumps (= relative to EIP) do not work under KVM, in initial
SMM, near jumps to absolute 32-bit addresses (specified indirectly via
registers) do work [*]. Except, the address calculation has to take into
account the trick that PiSmmCpuDxeSmm applies.
Namely, for initial SMBASE relocation,
- while the *short* gcSmmInitTemplate routine is copied to SMBASE+32KB
(that is, to (0x3_0000 + 0x8000)), and executed from there,
- the SmmStartup routine is actually executed from the *body* of
PiSmmCpuDxeSmm -- that is, from SMRAM, to which place the SMM Core
loaded and *relocated* PiSmmCpuDxeSmm, via normal SMM driver dispatch.
This is why gcSmmInitTemplate jumps to SmmStartup as follows, in
"UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm":
> BITS 16
> ASM_PFX(gcSmmInitTemplate):
> mov ebp, ASM_PFX(SmmStartup)
> sub ebp, 0x30000
> jmp ebp
>
> ASM_PFX(gcSmmInitSize): DW $ - ASM_PFX(gcSmmInitTemplate)
The "mov ebp, ASM_PFX(SmmStartup)" instruction is relocated at
PiSmmCpuDxeSmm dispatch time, to the absolute address of SmmStartup in
the body of PiSmmCpuDxeSmm. This absolute address is relative to zero.
However, when the "jmp ebp" is executed in initial SMM mode, the CS
register is not zero (i.e. EIP:=EBP will not be interpreted relative to
zero). Instead, CS is initially set to 0x3000 in SMM, implying a code
segment base that equals SMBASE (0x3_0000). Hence the "sub ebp, 0x30000"
-- it compensates the original relocation of the "mov" for the suddenly
increased CS base.
The same applies to any near jump (with absolute indirect target) in the
SmmStartup routine. All these jump instructions are relocated within the
body of PiSmmCpuDxeSmm (at driver dispatch time) against a *zero* code
segment base, but when they are actually executed in initial SMM, they
are executed against a code segment base of 0x3_0000.
I will send a patch (or a patch set, not sure yet).
[*] segment limits are raised to 4GB, and near jumps can use such
addresses with the "o32" (32-bit operand-size override) prefix. In fact,
NASM inserts the prefix automatically upon seeing eg. "jmp ebx" in BITS 16.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's supported
2018-01-29 19:48 ` Laszlo Ersek
2018-01-30 13:09 ` Laszlo Ersek
@ 2018-02-01 1:08 ` Wang, Jian J
1 sibling, 0 replies; 22+ messages in thread
From: Wang, Jian J @ 2018-02-01 1:08 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel@lists.01.org
Cc: Ni, Ruiyu, Paolo Bonzini, Yao, Jiewen, Dong, Eric,
Radim Kr?má?
Very good analysis and investigation.
Regards,
Jian
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, January 30, 2018 3:49 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini <pbonzini@redhat.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Radim
> Kr?má? <rkrcmar@redhat.com>
> Subject: Re: [edk2] [PATCH 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if
> it's supported
>
> On 01/29/18 10:02, Wang, Jian J wrote:
> > Hi Laszlo,
> >
> > I don't know the history of these code but I guess they're converted
> > from .asm file. That may be why there's "DB 66h" prefix. I think
> > you're right these tricks should be replaced with more formal ways.
> > Please submit a bz tracker for it.
>
> I submitted <https://bugzilla.tianocore.org/show_bug.cgi?id=866>.
>
> > As to the issue, I don't have clue right now. The code seems no
> > problem. Since msr write didn't happen, code flow is correct. And
> > those code has executed on real 32-bit platform without problem. I
> > need more time to investigate it.
>
> I think I've found the issue; more exactly I narrowed it down a bit. I
> remember that the same problem drove me mad a few years ago. :)
>
> The issue is that in the middle of such processor mode switches, no jump
> instructions work *at all* on KVM. I don't know why, this is just my
> experience. The KVM behavior could even be justified by the Intel SDM.
> I'm unsure.
>
> Let's look at the patch with more context:
>
> > global ASM_PFX(SmmStartup)
> > ASM_PFX(SmmStartup):
> > + DB 0x66
> > + mov eax, 0x80000001 ; read capability
> > + cpuid
> > + DB 0x66
> > + mov ebx, edx ; rdmsr will change edx. keep it in ebx.
> > DB 0x66, 0xb8
> > ASM_PFX(gSmmCr3): DD 0
> > mov cr3, eax
> > DB 0x67, 0x66
> > lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) - ASM_PFX(SmmStartup))]
> > DB 0x66, 0xb8
> > ASM_PFX(gSmmCr4): DD 0
> > mov cr4, eax
> > + DB 0x66
> > + mov ecx, 0xc0000080 ; IA32_EFER MSR
> > + rdmsr
> > + DB 0x66
> > + test ebx, BIT20 ; check NXE capability
> > + jz .1
> > + or ah, BIT3 ; set NXE bit
> > + wrmsr
> > +.1:
>
> This code has exactly one jump, and in practice it is never taken
> (because NX support is ubiquitous on physical platforms).
>
> In my testing I added some other conditional jumps -- I reimplemented
> IsExecuteDisableBitAvailable() from
> "MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c", which has more
> conditions. All the conditional jumps that were *not* taken didn't
> cause any issues. (This is why the same logic from the patch works for
> me in the X64 version, because there the "jz" is never taken, since NX
> is always available there.) However, the first jump that *was* taken in
> my testing immediately hung or crashed the IA32 guest.
>
> Finally I replaced the entire NX management code with an unconditional
> jump forward:
>
> jmp jump_here
> jump_here:
>
> and even this hung / crashed the guest.
>
> For some reason this section of the code is unfit for jumping, under KVM
> anyway.
>
> Thanks
> Laszlo
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2018-02-01 1:02 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-15 8:54 [PATCH 0/6] Fix issues caused by NX memory protection Jian J Wang
2018-01-15 8:54 ` [PATCH 1/6] UefiCpuPkg/MpInitLib: split wake up buffer into two parts Jian J Wang
2018-01-18 6:53 ` Dong, Eric
2018-01-27 16:17 ` Laszlo Ersek
2018-01-28 21:43 ` Laszlo Ersek
2018-01-29 1:06 ` Wang, Jian J
2018-01-29 15:50 ` Laszlo Ersek
2018-01-15 8:54 ` [PATCH 2/6] UefiCpuPkg/CpuExceptionHandlerLib: alloc code memory for exception handlers Jian J Wang
2018-01-16 14:02 ` Dong, Eric
2018-01-15 8:54 ` [PATCH 3/6] UefiCpuPkg/CpuDxe: clear NX attr for page directory Jian J Wang
2018-01-16 14:02 ` Dong, Eric
2018-01-15 8:54 ` [PATCH 4/6] UefiCpuPkg/PiSmmCpuDxeSmm: Enable NXE if it's supported Jian J Wang
2018-01-16 14:02 ` Dong, Eric
2018-01-28 22:46 ` Laszlo Ersek
2018-01-29 9:02 ` Wang, Jian J
2018-01-29 19:48 ` Laszlo Ersek
2018-01-30 13:09 ` Laszlo Ersek
2018-02-01 1:08 ` Wang, Jian J
2018-01-15 8:54 ` [PATCH 5/6] MdeModulePkg/PiSmmCore: remove NX attr for SMM RAM Jian J Wang
2018-01-15 10:18 ` Zeng, Star
2018-01-15 8:54 ` [PATCH 6/6] MdeModulePkg/BootScriptExecutorDxe: remove NX attr for FfsBuffer Jian J Wang
2018-01-15 10:18 ` Zeng, Star
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox