Topics

[PATCH v13 45/46] UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use

Lendacky, Thomas
 

From: Tom Lendacky <thomas.lendacky@...>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198

Before UEFI transfers control to the OS, it must park the AP. This is
done using the AsmRelocateApLoop function to transition into 32-bit
non-paging mode. For an SEV-ES guest, a few additional things must be
done:
- AsmRelocateApLoop must be updated to support SEV-ES. This means
performing a VMGEXIT AP Reset Hold instead of an MWAIT or HLT loop.
- Since the AP must transition to real mode, a small routine is copied
to the WakeupBuffer area. Since the WakeupBuffer will be used by
the AP during OS booting, it must be placed in reserved memory.
Additionally, the AP stack must be located where it can be accessed
in real mode.
- Once the AP is in real mode it will transfer control to the
destination specified by the OS in the SEV-ES AP Jump Table. The
SEV-ES AP Jump Table address is saved by the hypervisor for the OS
using the GHCB VMGEXIT AP Jump Table exit code.

Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Laszlo Ersek <lersek@...>
Reviewed-by: Eric Dong <eric.dong@...>
Signed-off-by: Tom Lendacky <thomas.lendacky@...>
---
UefiCpuPkg/Library/MpInitLib/MpLib.h | 8 +-
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 54 +++++++-
UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 131 ++++++++++++++++--
3 files changed, 175 insertions(+), 18 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index b1a9d99cb3eb..267aa5201c50 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -293,7 +293,8 @@ struct _CPU_MP_DATA {
UINT64 GhcbBase;
};

-#define AP_RESET_STACK_SIZE 64
+#define AP_SAFE_STACK_SIZE 128
+#define AP_RESET_STACK_SIZE AP_SAFE_STACK_SIZE

#pragma pack(1)

@@ -349,8 +350,11 @@ VOID
IN BOOLEAN MwaitSupport,
IN UINTN ApTargetCState,
IN UINTN PmCodeSegment,
+ IN UINTN Pm16CodeSegment,
IN UINTN TopOfApStack,
- IN UINTN NumberToFinish
+ IN UINTN NumberToFinish,
+ IN UINTN SevEsAPJumpTable,
+ IN UINTN WakeupBuffer
);

/**
diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 9115ff9e3e30..7165bcf3124a 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -12,6 +12,7 @@
#include <Library/UefiBootServicesTableLib.h>
#include <Library/DebugAgentLib.h>
#include <Library/DxeServicesTableLib.h>
+#include <Library/VmgExitLib.h>
#include <Register/Amd/Fam17Msr.h>
#include <Register/Amd/Ghcb.h>

@@ -85,6 +86,13 @@ GetWakeupBuffer (
{
EFI_STATUS Status;
EFI_PHYSICAL_ADDRESS StartAddress;
+ EFI_MEMORY_TYPE MemoryType;
+
+ if (PcdGetBool (PcdSevEsIsEnabled)) {
+ MemoryType = EfiReservedMemoryType;
+ } else {
+ MemoryType = EfiBootServicesData;
+ }

//
// Try to allocate buffer below 1M for waking vector.
@@ -97,7 +105,7 @@ GetWakeupBuffer (
StartAddress = 0x88000;
Status = gBS->AllocatePages (
AllocateMaxAddress,
- EfiBootServicesData,
+ MemoryType,
EFI_SIZE_TO_PAGES (WakeupBufferSize),
&StartAddress
);
@@ -159,8 +167,10 @@ GetSevEsAPMemory (
VOID
)
{
- EFI_STATUS Status;
- EFI_PHYSICAL_ADDRESS StartAddress;
+ EFI_STATUS Status;
+ EFI_PHYSICAL_ADDRESS StartAddress;
+ MSR_SEV_ES_GHCB_REGISTER Msr;
+ GHCB *Ghcb;

//
// Allocate 1 page for AP jump table page
@@ -176,6 +186,16 @@ GetSevEsAPMemory (

DEBUG ((DEBUG_INFO, "Dxe: SevEsAPMemory = %lx\n", (UINTN) StartAddress));

+ //
+ // Save the SevEsAPMemory as the AP jump table.
+ //
+ Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
+ Ghcb = Msr.Ghcb;
+
+ VmgInit (Ghcb);
+ VmgExit (Ghcb, SVM_EXIT_AP_JUMP_TABLE, 0, (UINT64) (UINTN) StartAddress);
+ VmgDone (Ghcb);
+
return (UINTN) StartAddress;
}

@@ -330,17 +350,26 @@ RelocateApLoop (
BOOLEAN MwaitSupport;
ASM_RELOCATE_AP_LOOP AsmRelocateApLoopFunc;
UINTN ProcessorNumber;
+ UINTN StackStart;

MpInitLibWhoAmI (&ProcessorNumber);
CpuMpData = GetCpuMpData ();
MwaitSupport = IsMwaitSupport ();
+ if (CpuMpData->SevEsIsEnabled) {
+ StackStart = CpuMpData->SevEsAPResetStackStart;
+ } else {
+ StackStart = mReservedTopOfApStack;
+ }
AsmRelocateApLoopFunc = (ASM_RELOCATE_AP_LOOP) (UINTN) mReservedApLoopFunc;
AsmRelocateApLoopFunc (
MwaitSupport,
CpuMpData->ApTargetCState,
CpuMpData->PmCodeSegment,
- mReservedTopOfApStack - ProcessorNumber * AP_SAFE_STACK_SIZE,
- (UINTN) &mNumberToFinish
+ CpuMpData->Pm16CodeSegment,
+ StackStart - ProcessorNumber * AP_SAFE_STACK_SIZE,
+ (UINTN) &mNumberToFinish,
+ CpuMpData->SevEsAPBuffer,
+ CpuMpData->WakeupBuffer
);
//
// It should never reach here
@@ -374,6 +403,21 @@ MpInitChangeApLoopCallback (
while (mNumberToFinish > 0) {
CpuPause ();
}
+
+ if (CpuMpData->SevEsIsEnabled && (CpuMpData->WakeupBuffer != (UINTN) -1)) {
+ //
+ // There are APs present. Re-use reserved memory area below 1MB from
+ // WakeupBuffer as the area to be used for transitioning to 16-bit mode
+ // in support of booting of the AP by an OS.
+ //
+ CopyMem (
+ (VOID *) CpuMpData->WakeupBuffer,
+ (VOID *) (CpuMpData->AddressMap.RendezvousFunnelAddress +
+ CpuMpData->AddressMap.SwitchToRealPM16ModeOffset),
+ CpuMpData->AddressMap.SwitchToRealPM16ModeSize
+ );
+ }
+
DEBUG ((DEBUG_INFO, "%a() done!\n", __FUNCTION__));
}

diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 6956b408d004..3b8ec477b8b3 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -465,6 +465,10 @@ BITS 16
; - IP for Real Mode (two bytes)
; - CS for Real Mode (two bytes)
;
+ ; This label is also used with AsmRelocateApLoop. During MP finalization,
+ ; the code from PM16Mode to SwitchToRealProcEnd is copied to the start of
+ ; the WakeupBuffer, allowing a parked AP to be booted by an OS.
+ ;
PM16Mode:
mov eax, cr0 ; Read CR0
btr eax, 0 ; Set PE=0
@@ -487,32 +491,95 @@ PM16Mode:
SwitchToRealProcEnd:

;-------------------------------------------------------------------------------------
-; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack, CountTofinish);
+; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, Pm16CodeSegment, TopOfApStack, CountTofinish, SevEsAPJumpTable, WakeupBuffer);
;-------------------------------------------------------------------------------------
global ASM_PFX(AsmRelocateApLoop)
ASM_PFX(AsmRelocateApLoop):
AsmRelocateApLoopStart:
BITS 64
+ cmp qword [rsp + 56], 0
+ je NoSevEs
+
+ ;
+ ; Perform some SEV-ES related setup before leaving 64-bit mode
+ ;
+ push rcx
+ push rdx
+
+ ;
+ ; Get the RDX reset value using CPUID
+ ;
+ mov rax, 1
+ cpuid
+ mov rsi, rax ; Save off the reset value for RDX
+
+ ;
+ ; Prepare the GHCB for the AP_HLT_LOOP VMGEXIT call
+ ; - Must be done while in 64-bit long mode so that writes to
+ ; the GHCB memory will be unencrypted.
+ ; - No NAE events can be generated once this is set otherwise
+ ; the AP_RESET_HOLD SW_EXITCODE will be overwritten.
+ ;
+ mov rcx, 0xc0010130
+ rdmsr ; Retrieve current GHCB address
+ shl rdx, 32
+ or rdx, rax
+
+ mov rdi, rdx
+ xor rax, rax
+ mov rcx, 0x800
+ shr rcx, 3
+ rep stosq ; Clear the GHCB
+
+ mov rax, 0x80000004 ; VMGEXIT AP_RESET_HOLD
+ mov [rdx + 0x390], rax
+
+ pop rdx
+ pop rcx
+
+NoSevEs:
cli ; Disable interrupt before switching to 32-bit mode
- mov rax, [rsp + 40] ; CountTofinish
+ mov rax, [rsp + 48] ; CountTofinish
lock dec dword [rax] ; (*CountTofinish)--
- mov rsp, r9
- push rcx
- push rdx

- lea rsi, [PmEntry] ; rsi <- The start address of transition code
+ mov rax, [rsp + 56] ; SevEsAPJumpTable
+ mov rbx, [rsp + 64] ; WakeupBuffer
+ mov rsp, [rsp + 40] ; TopOfApStack
+
+ push rax ; Save SevEsAPJumpTable
+ push rbx ; Save WakeupBuffer
+ push r9 ; Save Pm16CodeSegment
+ push rcx ; Save MwaitSupport
+ push rdx ; Save ApTargetCState
+
+ lea rax, [PmEntry] ; rax <- The start address of transition code

push r8
- push rsi
- DB 0x48
- retf
+ push rax
+
+ ;
+ ; Clear R8 - R15, for reset, before going into 32-bit mode
+ ;
+ xor r8, r8
+ xor r9, r9
+ xor r10, r10
+ xor r11, r11
+ xor r12, r12
+ xor r13, r13
+ xor r14, r14
+ xor r15, r15
+
+ ;
+ ; Far return into 32-bit mode
+ ;
+o64 retf
+
BITS 32
PmEntry:
mov eax, cr0
btr eax, 31 ; Clear CR0.PG
mov cr0, eax ; Disable paging and caches

- mov ebx, edx ; Save EntryPoint to rbx, for rdmsr will overwrite rdx
mov ecx, 0xc0000080
rdmsr
and ah, ~ 1 ; Clear LME
@@ -525,6 +592,8 @@ PmEntry:
add esp, 4
pop ecx,
add esp, 4
+
+MwaitCheck:
cmp cl, 1 ; Check mwait-monitor support
jnz HltLoop
mov ebx, edx ; Save C-State to ebx
@@ -538,10 +607,50 @@ MwaitLoop:
shl eax, 4
mwait
jmp MwaitLoop
+
HltLoop:
+ pop edx ; PM16CodeSegment
+ add esp, 4
+ pop ebx ; WakeupBuffer
+ add esp, 4
+ pop eax ; SevEsAPJumpTable
+ add esp, 4
+ cmp eax, 0 ; Check for SEV-ES
+ je DoHlt
+
+ cli
+ ;
+ ; SEV-ES is enabled, use VMGEXIT (GHCB information already
+ ; set by caller)
+ ;
+BITS 64
+ rep vmmcall
+BITS 32
+
+ ;
+ ; Back from VMGEXIT AP_HLT_LOOP
+ ; Push the FLAGS/CS/IP values to use
+ ;
+ push word 0x0002 ; EFLAGS
+ xor ecx, ecx
+ mov cx, [eax + 2] ; CS
+ push cx
+ mov cx, [eax] ; IP
+ push cx
+ push word 0x0000 ; For alignment, will be discarded
+
+ push edx
+ push ebx
+
+ mov edx, esi ; Restore RDX reset value
+
+ retf
+
+DoHlt:
cli
hlt
- jmp HltLoop
+ jmp DoHlt
+
BITS 64
AsmRelocateApLoopEnd:

--
2.27.0

Laszlo Ersek
 

Hi Tom,

On 07/30/20 20:43, Tom Lendacky wrote:
From: Tom Lendacky <thomas.lendacky@...>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198

Before UEFI transfers control to the OS, it must park the AP. This is
done using the AsmRelocateApLoop function to transition into 32-bit
non-paging mode. For an SEV-ES guest, a few additional things must be
done:
- AsmRelocateApLoop must be updated to support SEV-ES. This means
performing a VMGEXIT AP Reset Hold instead of an MWAIT or HLT loop.
- Since the AP must transition to real mode, a small routine is copied
to the WakeupBuffer area. Since the WakeupBuffer will be used by
the AP during OS booting, it must be placed in reserved memory.
Additionally, the AP stack must be located where it can be accessed
in real mode.
- Once the AP is in real mode it will transfer control to the
destination specified by the OS in the SEV-ES AP Jump Table. The
SEV-ES AP Jump Table address is saved by the hypervisor for the OS
using the GHCB VMGEXIT AP Jump Table exit code.

Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Laszlo Ersek <lersek@...>
Reviewed-by: Eric Dong <eric.dong@...>
Signed-off-by: Tom Lendacky <thomas.lendacky@...>
---
UefiCpuPkg/Library/MpInitLib/MpLib.h | 8 +-
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 54 +++++++-
UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 131 ++++++++++++++++--
3 files changed, 175 insertions(+), 18 deletions(-)
Now that this series is almost ready to merge, I've done a bit of
regression-testing.

Unfortunately, this patch breaks booting with IA32 OVMF.

More precisely, it breaks the IA32 version of DxeMpInitLib.

The symptom is that just when the OS would be launched, the
multiprocessor guest hangs. This is how the log terminates:

FSOpen: Open '\370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\linux'
Success
[Security] 3rd party image[0] can be loaded after EndOfDxe:
PciRoot(0x0)/Pci(0x2,0x1)/Pci(0x0,0x0)/Scsi(0x0,0x0)/HD(1,GPT,D9F1FBA5-E5D3-440A-B6A7-87B593E4FAB1,0x800,0x100000)/\370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\linux.
InstallProtocolInterface: [EfiLoadedImageProtocol] 853A03A8
Loading driver at 0x00083E72000 EntryPoint=0x00083E76680
InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 853A0510
ProtectUefiImageCommon - 0x853A03A8
- 0x0000000083E72000 - 0x0000000000E75000
FSOpen: Open '370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\initrd'
Success
PixelBlueGreenRedReserved8BitPerColor
ConvertPages: range 400000 - 1274FFF covers multiple entries
SmmInstallProtocolInterface: [EdkiiSmmExitBootServicesProtocol] 0
CpuDxe: 5-Level Paging = 0
[HANG]
Meanwhile some guest CPUs are pegged.

Normally, when this series is not applied, the next log entry is (in
place of [HANG]):

MpInitChangeApLoopCallback() done!
I've identified this patch by bisection, after applying the series on
current master (137c2c6eff67, "Revert "BaseTools/PatchCheck.py: Add
LicenseCheck"", 2020-07-31).

Here's the bisection log:

git bisect start
# good: [137c2c6eff67f4750d77e8e40af6683c412d3ed0] Revert "BaseTools/PatchCheck.py: Add LicenseCheck"
git bisect good 137c2c6eff67f4750d77e8e40af6683c412d3ed0
# bad: [d3f7971f4f70c9f39170b42af837e58e59435ad3] Maintainers.txt: Add reviewers for the OvmfPkg SEV-related files
git bisect bad d3f7971f4f70c9f39170b42af837e58e59435ad3
# good: [9551e3fc61ba0c0ddf8e79b425a22aa7dd61cb8b] OvmfPkg/VmgExitLib: Add support for RDTSCP NAE events
git bisect good 9551e3fc61ba0c0ddf8e79b425a22aa7dd61cb8b
# good: [10acf16b38522d8a1b538b3aa432daaa72c0e97b] OvmfPkg: Reserve a page in memory for the SEV-ES usage
git bisect good 10acf16b38522d8a1b538b3aa432daaa72c0e97b
# good: [ccb4267e76b6474657c41bef7e76a980930c22ea] UefiCpuPkg: Add a 16-bit protected mode code segment descriptor
git bisect good ccb4267e76b6474657c41bef7e76a980930c22ea
# good: [94e238ae37505cfb081f3b9b4632067e4a113cf9] OvmfPkg: Use the SEV-ES work area for the SEV-ES AP reset vector
git bisect good 94e238ae37505cfb081f3b9b4632067e4a113cf9
# bad: [16c21b9d10b032d66d4105dd4693fd9dc6e6ec18] UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use
git bisect bad 16c21b9d10b032d66d4105dd4693fd9dc6e6ec18
# good: [49855596e383ab2aa6410fa060e22d4817d8e64e] OvmfPkg: Move the GHCB allocations into reserved memory
git bisect good 49855596e383ab2aa6410fa060e22d4817d8e64e
# first bad commit: [16c21b9d10b032d66d4105dd4693fd9dc6e6ec18] UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use
So clearly we should be looking for an IA32-specific change, or
IA32-specific *omission*, in this patch, that could cause the problem.

The bug is the following:

On 07/30/20 20:43, Tom Lendacky wrote:

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index b1a9d99cb3eb..267aa5201c50 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -349,8 +350,11 @@ VOID
IN BOOLEAN MwaitSupport,
IN UINTN ApTargetCState,
IN UINTN PmCodeSegment,
+ IN UINTN Pm16CodeSegment,
IN UINTN TopOfApStack,
- IN UINTN NumberToFinish
+ IN UINTN NumberToFinish,
+ IN UINTN SevEsAPJumpTable,
+ IN UINTN WakeupBuffer
);

/**
(1) This hunk modifies the parameter list of functions pointed-to by
ASM_RELOCATE_AP_LOOP.

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 9115ff9e3e30..7165bcf3124a 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -330,17 +350,26 @@ RelocateApLoop (
BOOLEAN MwaitSupport;
ASM_RELOCATE_AP_LOOP AsmRelocateApLoopFunc;
UINTN ProcessorNumber;
+ UINTN StackStart;

MpInitLibWhoAmI (&ProcessorNumber);
CpuMpData = GetCpuMpData ();
MwaitSupport = IsMwaitSupport ();
+ if (CpuMpData->SevEsIsEnabled) {
+ StackStart = CpuMpData->SevEsAPResetStackStart;
+ } else {
+ StackStart = mReservedTopOfApStack;
+ }
AsmRelocateApLoopFunc = (ASM_RELOCATE_AP_LOOP) (UINTN) mReservedApLoopFunc;
AsmRelocateApLoopFunc (
MwaitSupport,
CpuMpData->ApTargetCState,
CpuMpData->PmCodeSegment,
- mReservedTopOfApStack - ProcessorNumber * AP_SAFE_STACK_SIZE,
- (UINTN) &mNumberToFinish
+ CpuMpData->Pm16CodeSegment,
+ StackStart - ProcessorNumber * AP_SAFE_STACK_SIZE,
+ (UINTN) &mNumberToFinish,
+ CpuMpData->SevEsAPBuffer,
+ CpuMpData->WakeupBuffer
);
//
// It should never reach here
(2) This hunk modifies the call site, in accordance with the prototype
change at (1).

diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 6956b408d004..3b8ec477b8b3 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -465,6 +465,10 @@ BITS 16
; - IP for Real Mode (two bytes)
; - CS for Real Mode (two bytes)
;
+ ; This label is also used with AsmRelocateApLoop. During MP finalization,
+ ; the code from PM16Mode to SwitchToRealProcEnd is copied to the start of
+ ; the WakeupBuffer, allowing a parked AP to be booted by an OS.
+ ;
PM16Mode:
mov eax, cr0 ; Read CR0
btr eax, 0 ; Set PE=0
@@ -487,32 +491,95 @@ PM16Mode:
SwitchToRealProcEnd:

;-------------------------------------------------------------------------------------
-; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack, CountTofinish);
+; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, Pm16CodeSegment, TopOfApStack, CountTofinish, SevEsAPJumpTable, WakeupBuffer);
;-------------------------------------------------------------------------------------
global ASM_PFX(AsmRelocateApLoop)
ASM_PFX(AsmRelocateApLoop):
AsmRelocateApLoopStart:
BITS 64
(3) Unfortunately, the patch only adapts the X64 implementation of the
AsmRelocateApLoopStart() function to the new prototype; the IA32
implementation no longer matches the call site.

(I'm not sure if the intent was for the IA32 version to simply ignore
the new parameters, but even in that case, the "Pm16CodeSegment"
parameter is inserted in the middle of the parameter list, likely
offsetting the rest.)

The problem is foreshadowed even by hunk (2). Namely, in hunk (2), the

s/mReservedTopOfApStack/StackStart/

replacement is *more difficult* to verify than necessary -- exactly
because "CpuMpData->Pm16CodeSegment" is inserted *before* it.

Thanks
Laszlo

Lendacky, Thomas
 

On 7/31/20 7:43 AM, Laszlo Ersek wrote:
Hi Tom,
Hi Laszlo,

On 07/30/20 20:43, Tom Lendacky wrote:
From: Tom Lendacky <thomas.lendacky@...>

BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7Cb8c77cf296c949d2bbd808d8354f542b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637317962138028351&amp;sdata=HISHZmLjOc%2FfgVrBm8MlNeDAk453NJ64%2B51bETZj4rk%3D&amp;reserved=0

Before UEFI transfers control to the OS, it must park the AP. This is
done using the AsmRelocateApLoop function to transition into 32-bit
non-paging mode. For an SEV-ES guest, a few additional things must be
done:
- AsmRelocateApLoop must be updated to support SEV-ES. This means
performing a VMGEXIT AP Reset Hold instead of an MWAIT or HLT loop.
- Since the AP must transition to real mode, a small routine is copied
to the WakeupBuffer area. Since the WakeupBuffer will be used by
the AP during OS booting, it must be placed in reserved memory.
Additionally, the AP stack must be located where it can be accessed
in real mode.
- Once the AP is in real mode it will transfer control to the
destination specified by the OS in the SEV-ES AP Jump Table. The
SEV-ES AP Jump Table address is saved by the hypervisor for the OS
using the GHCB VMGEXIT AP Jump Table exit code.

Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Laszlo Ersek <lersek@...>
Reviewed-by: Eric Dong <eric.dong@...>
Signed-off-by: Tom Lendacky <thomas.lendacky@...>
---
UefiCpuPkg/Library/MpInitLib/MpLib.h | 8 +-
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 54 +++++++-
UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 131 ++++++++++++++++--
3 files changed, 175 insertions(+), 18 deletions(-)
Now that this series is almost ready to merge, I've done a bit of
regression-testing.
Unfortunately, this patch breaks booting with IA32 OVMF.
More precisely, it breaks the IA32 version of DxeMpInitLib.
Yeah, that's not good. I will look into this based on your input below. What's strange is that my system doesn't hang and successfully boots all APs (up to 64 is what I've tested with).

But, yes, both call sites should be the same and I will make that change.

The symptom is that just when the OS would be launched, the
multiprocessor guest hangs. This is how the log terminates:

FSOpen: Open '\370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\linux'
Success
[Security] 3rd party image[0] can be loaded after EndOfDxe:
PciRoot(0x0)/Pci(0x2,0x1)/Pci(0x0,0x0)/Scsi(0x0,0x0)/HD(1,GPT,D9F1FBA5-E5D3-440A-B6A7-87B593E4FAB1,0x800,0x100000)/\370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\linux.
InstallProtocolInterface: [EfiLoadedImageProtocol] 853A03A8
Loading driver at 0x00083E72000 EntryPoint=0x00083E76680
InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 853A0510
ProtectUefiImageCommon - 0x853A03A8
- 0x0000000083E72000 - 0x0000000000E75000
FSOpen: Open '370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\initrd'
Success
PixelBlueGreenRedReserved8BitPerColor
ConvertPages: range 400000 - 1274FFF covers multiple entries
SmmInstallProtocolInterface: [EdkiiSmmExitBootServicesProtocol] 0
CpuDxe: 5-Level Paging = 0
[HANG]
Meanwhile some guest CPUs are pegged.
Normally, when this series is not applied, the next log entry is (in
place of [HANG]):

MpInitChangeApLoopCallback() done!
I've identified this patch by bisection, after applying the series on
current master (137c2c6eff67, "Revert "BaseTools/PatchCheck.py: Add
LicenseCheck"", 2020-07-31).
Here's the bisection log:

git bisect start
# good: [137c2c6eff67f4750d77e8e40af6683c412d3ed0] Revert "BaseTools/PatchCheck.py: Add LicenseCheck"
git bisect good 137c2c6eff67f4750d77e8e40af6683c412d3ed0
# bad: [d3f7971f4f70c9f39170b42af837e58e59435ad3] Maintainers.txt: Add reviewers for the OvmfPkg SEV-related files
git bisect bad d3f7971f4f70c9f39170b42af837e58e59435ad3
# good: [9551e3fc61ba0c0ddf8e79b425a22aa7dd61cb8b] OvmfPkg/VmgExitLib: Add support for RDTSCP NAE events
git bisect good 9551e3fc61ba0c0ddf8e79b425a22aa7dd61cb8b
# good: [10acf16b38522d8a1b538b3aa432daaa72c0e97b] OvmfPkg: Reserve a page in memory for the SEV-ES usage
git bisect good 10acf16b38522d8a1b538b3aa432daaa72c0e97b
# good: [ccb4267e76b6474657c41bef7e76a980930c22ea] UefiCpuPkg: Add a 16-bit protected mode code segment descriptor
git bisect good ccb4267e76b6474657c41bef7e76a980930c22ea
# good: [94e238ae37505cfb081f3b9b4632067e4a113cf9] OvmfPkg: Use the SEV-ES work area for the SEV-ES AP reset vector
git bisect good 94e238ae37505cfb081f3b9b4632067e4a113cf9
# bad: [16c21b9d10b032d66d4105dd4693fd9dc6e6ec18] UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use
git bisect bad 16c21b9d10b032d66d4105dd4693fd9dc6e6ec18
# good: [49855596e383ab2aa6410fa060e22d4817d8e64e] OvmfPkg: Move the GHCB allocations into reserved memory
git bisect good 49855596e383ab2aa6410fa060e22d4817d8e64e
# first bad commit: [16c21b9d10b032d66d4105dd4693fd9dc6e6ec18] UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use
So clearly we should be looking for an IA32-specific change, or
IA32-specific *omission*, in this patch, that could cause the problem.
The bug is the following:
On 07/30/20 20:43, Tom Lendacky wrote:

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index b1a9d99cb3eb..267aa5201c50 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -349,8 +350,11 @@ VOID
IN BOOLEAN MwaitSupport,
IN UINTN ApTargetCState,
IN UINTN PmCodeSegment,
+ IN UINTN Pm16CodeSegment,
IN UINTN TopOfApStack,
- IN UINTN NumberToFinish
+ IN UINTN NumberToFinish,
+ IN UINTN SevEsAPJumpTable,
+ IN UINTN WakeupBuffer
);

/**
(1) This hunk modifies the parameter list of functions pointed-to by
ASM_RELOCATE_AP_LOOP.

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 9115ff9e3e30..7165bcf3124a 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -330,17 +350,26 @@ RelocateApLoop (
BOOLEAN MwaitSupport;
ASM_RELOCATE_AP_LOOP AsmRelocateApLoopFunc;
UINTN ProcessorNumber;
+ UINTN StackStart;

MpInitLibWhoAmI (&ProcessorNumber);
CpuMpData = GetCpuMpData ();
MwaitSupport = IsMwaitSupport ();
+ if (CpuMpData->SevEsIsEnabled) {
+ StackStart = CpuMpData->SevEsAPResetStackStart;
+ } else {
+ StackStart = mReservedTopOfApStack;
+ }
AsmRelocateApLoopFunc = (ASM_RELOCATE_AP_LOOP) (UINTN) mReservedApLoopFunc;
AsmRelocateApLoopFunc (
MwaitSupport,
CpuMpData->ApTargetCState,
CpuMpData->PmCodeSegment,
- mReservedTopOfApStack - ProcessorNumber * AP_SAFE_STACK_SIZE,
- (UINTN) &mNumberToFinish
+ CpuMpData->Pm16CodeSegment,
+ StackStart - ProcessorNumber * AP_SAFE_STACK_SIZE,
+ (UINTN) &mNumberToFinish,
+ CpuMpData->SevEsAPBuffer,
+ CpuMpData->WakeupBuffer
);
//
// It should never reach here
(2) This hunk modifies the call site, in accordance with the prototype
change at (1).

diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 6956b408d004..3b8ec477b8b3 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -465,6 +465,10 @@ BITS 16
; - IP for Real Mode (two bytes)
; - CS for Real Mode (two bytes)
;
+ ; This label is also used with AsmRelocateApLoop. During MP finalization,
+ ; the code from PM16Mode to SwitchToRealProcEnd is copied to the start of
+ ; the WakeupBuffer, allowing a parked AP to be booted by an OS.
+ ;
PM16Mode:
mov eax, cr0 ; Read CR0
btr eax, 0 ; Set PE=0
@@ -487,32 +491,95 @@ PM16Mode:
SwitchToRealProcEnd:

;-------------------------------------------------------------------------------------
-; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack, CountTofinish);
+; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, Pm16CodeSegment, TopOfApStack, CountTofinish, SevEsAPJumpTable, WakeupBuffer);
;-------------------------------------------------------------------------------------
global ASM_PFX(AsmRelocateApLoop)
ASM_PFX(AsmRelocateApLoop):
AsmRelocateApLoopStart:
BITS 64
(3) Unfortunately, the patch only adapts the X64 implementation of the
AsmRelocateApLoopStart() function to the new prototype; the IA32
implementation no longer matches the call site.
(I'm not sure if the intent was for the IA32 version to simply ignore
the new parameters, but even in that case, the "Pm16CodeSegment"
parameter is inserted in the middle of the parameter list, likely
offsetting the rest.)
The problem is foreshadowed even by hunk (2). Namely, in hunk (2), the
s/mReservedTopOfApStack/StackStart/
replacement is *more difficult* to verify than necessary -- exactly
because "CpuMpData->Pm16CodeSegment" is inserted *before* it.
I can do one of two things here and just put the 3 new parameters at the end of the function call rather than keeping the code segment parameters together or update the IA32 call site. Let me see which looks best. But I'll likely update the IA32 call site no matter what with at least comments about the parameters that aren't used, either way.

Thanks,
Tom

Thanks
Laszlo

Lendacky, Thomas
 

On 7/31/20 8:36 AM, Tom Lendacky wrote:
On 7/31/20 7:43 AM, Laszlo Ersek wrote:
Hi Tom,
Hi Laszlo,
Hi Laszlo,

Can you try this incremental patch to see if it fixes the issue you're
seeing? If it does, I'll merge it into patch #45 and send out a v14.

Thanks,
Tom


diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 7165bcf3124a..2c00d72ddefe 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -365,9 +365,9 @@ RelocateApLoop (
MwaitSupport,

CpuMpData->ApTargetCState,

CpuMpData->PmCodeSegment,

- CpuMpData->Pm16CodeSegment,

StackStart - ProcessorNumber * AP_SAFE_STACK_SIZE,

(UINTN) &mNumberToFinish,

+ CpuMpData->Pm16CodeSegment,

CpuMpData->SevEsAPBuffer,

CpuMpData->WakeupBuffer

);

diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
index 309d53bf3b37..7e81d24aa60f 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
@@ -226,7 +226,10 @@ SwitchToRealProcStart:
SwitchToRealProcEnd:



;-------------------------------------------------------------------------------------

-; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack, CountTofinish);

+; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack, CountTofinish, Pm16CodeSegment, SevEsAPJumpTable, WakeupBuffer);

+;

+; The last three parameters (Pm16CodeSegment, SevEsAPJumpTable and WakeupBuffer) are

+; specific to SEV-ES support and are not applicable on IA32.

;-------------------------------------------------------------------------------------

global ASM_PFX(AsmRelocateApLoop)

ASM_PFX(AsmRelocateApLoop):

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 267aa5201c50..02652eaae126 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -350,9 +350,9 @@ VOID
IN BOOLEAN MwaitSupport,

IN UINTN ApTargetCState,

IN UINTN PmCodeSegment,

- IN UINTN Pm16CodeSegment,

IN UINTN TopOfApStack,

IN UINTN NumberToFinish,

+ IN UINTN Pm16CodeSegment,

IN UINTN SevEsAPJumpTable,

IN UINTN WakeupBuffer

);

diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 3b8ec477b8b3..5d30f35b201c 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -491,13 +491,13 @@ PM16Mode:
SwitchToRealProcEnd:



;-------------------------------------------------------------------------------------

-; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, Pm16CodeSegment, TopOfApStack, CountTofinish, SevEsAPJumpTable, WakeupBuffer);

+; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack, CountTofinish, Pm16CodeSegment, SevEsAPJumpTable, WakeupBuffer);

;-------------------------------------------------------------------------------------

global ASM_PFX(AsmRelocateApLoop)

ASM_PFX(AsmRelocateApLoop):

AsmRelocateApLoopStart:

BITS 64

- cmp qword [rsp + 56], 0

+ cmp qword [rsp + 56], 0 ; SevEsAPJumpTable

je NoSevEs



;

@@ -539,16 +539,17 @@ BITS 64


NoSevEs:

cli ; Disable interrupt before switching to 32-bit mode

- mov rax, [rsp + 48] ; CountTofinish

+ mov rax, [rsp + 40] ; CountTofinish

lock dec dword [rax] ; (*CountTofinish)--



+ mov r10, [rsp + 48] ; Pm16CodeSegment

mov rax, [rsp + 56] ; SevEsAPJumpTable

mov rbx, [rsp + 64] ; WakeupBuffer

- mov rsp, [rsp + 40] ; TopOfApStack

+ mov rsp, r9 ; TopOfApStack



push rax ; Save SevEsAPJumpTable

push rbx ; Save WakeupBuffer

- push r9 ; Save Pm16CodeSegment

+ push r10 ; Save Pm16CodeSegment

push rcx ; Save MwaitSupport

push rdx ; Save ApTargetCState







On 07/30/20 20:43, Tom Lendacky wrote:
From: Tom Lendacky <thomas.lendacky@...>

BZ:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7Cb8c77cf296c949d2bbd808d8354f542b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637317962138028351&amp;sdata=HISHZmLjOc%2FfgVrBm8MlNeDAk453NJ64%2B51bETZj4rk%3D&amp;reserved=0


Before UEFI transfers control to the OS, it must park the AP. This is
done using the AsmRelocateApLoop function to transition into 32-bit
non-paging mode. For an SEV-ES guest, a few additional things must be
done:
- AsmRelocateApLoop must be updated to support SEV-ES. This means
performing a VMGEXIT AP Reset Hold instead of an MWAIT or HLT loop.
- Since the AP must transition to real mode, a small routine is copied
to the WakeupBuffer area. Since the WakeupBuffer will be used by
the AP during OS booting, it must be placed in reserved memory.
Additionally, the AP stack must be located where it can be accessed
in real mode.
- Once the AP is in real mode it will transfer control to the
destination specified by the OS in the SEV-ES AP Jump Table. The
SEV-ES AP Jump Table address is saved by the hypervisor for the OS
using the GHCB VMGEXIT AP Jump Table exit code.

Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Laszlo Ersek <lersek@...>
Reviewed-by: Eric Dong <eric.dong@...>
Signed-off-by: Tom Lendacky <thomas.lendacky@...>
---
UefiCpuPkg/Library/MpInitLib/MpLib.h | 8 +-
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 54 +++++++-
UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 131 ++++++++++++++++--
3 files changed, 175 insertions(+), 18 deletions(-)
Now that this series is almost ready to merge, I've done a bit of
regression-testing.

Unfortunately, this patch breaks booting with IA32 OVMF.

More precisely, it breaks the IA32 version of DxeMpInitLib.
Yeah, that's not good. I will look into this based on your input below.
What's strange is that my system doesn't hang and successfully boots all
APs (up to 64 is what I've tested with).

But, yes, both call sites should be the same and I will make that change.


The symptom is that just when the OS would be launched, the
multiprocessor guest hangs. This is how the log terminates:

FSOpen: Open
'\370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\linux'
Success
[Security] 3rd party image[0] can be loaded after EndOfDxe:
PciRoot(0x0)/Pci(0x2,0x1)/Pci(0x0,0x0)/Scsi(0x0,0x0)/HD(1,GPT,D9F1FBA5-E5D3-440A-B6A7-87B593E4FAB1,0x800,0x100000)/\370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\linux.

InstallProtocolInterface: [EfiLoadedImageProtocol] 853A03A8
Loading driver at 0x00083E72000 EntryPoint=0x00083E76680
InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 853A0510
ProtectUefiImageCommon - 0x853A03A8
- 0x0000000083E72000 - 0x0000000000E75000
FSOpen: Open
'370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\initrd'
Success
PixelBlueGreenRedReserved8BitPerColor
ConvertPages: range 400000 - 1274FFF covers multiple entries
SmmInstallProtocolInterface: [EdkiiSmmExitBootServicesProtocol] 0
CpuDxe: 5-Level Paging = 0
[HANG]
Meanwhile some guest CPUs are pegged.

Normally, when this series is not applied, the next log entry is (in
place of [HANG]):

MpInitChangeApLoopCallback() done!
I've identified this patch by bisection, after applying the series on
current master (137c2c6eff67, "Revert "BaseTools/PatchCheck.py: Add
LicenseCheck"", 2020-07-31).

Here's the bisection log:

git bisect start
# good: [137c2c6eff67f4750d77e8e40af6683c412d3ed0] Revert
"BaseTools/PatchCheck.py: Add LicenseCheck"
git bisect good 137c2c6eff67f4750d77e8e40af6683c412d3ed0
# bad: [d3f7971f4f70c9f39170b42af837e58e59435ad3] Maintainers.txt: Add
reviewers for the OvmfPkg SEV-related files
git bisect bad d3f7971f4f70c9f39170b42af837e58e59435ad3
# good: [9551e3fc61ba0c0ddf8e79b425a22aa7dd61cb8b] OvmfPkg/VmgExitLib:
Add support for RDTSCP NAE events
git bisect good 9551e3fc61ba0c0ddf8e79b425a22aa7dd61cb8b
# good: [10acf16b38522d8a1b538b3aa432daaa72c0e97b] OvmfPkg: Reserve a
page in memory for the SEV-ES usage
git bisect good 10acf16b38522d8a1b538b3aa432daaa72c0e97b
# good: [ccb4267e76b6474657c41bef7e76a980930c22ea] UefiCpuPkg: Add a
16-bit protected mode code segment descriptor
git bisect good ccb4267e76b6474657c41bef7e76a980930c22ea
# good: [94e238ae37505cfb081f3b9b4632067e4a113cf9] OvmfPkg: Use the
SEV-ES work area for the SEV-ES AP reset vector
git bisect good 94e238ae37505cfb081f3b9b4632067e4a113cf9
# bad: [16c21b9d10b032d66d4105dd4693fd9dc6e6ec18] UefiCpuPkg/MpInitLib:
Prepare SEV-ES guest APs for OS use
git bisect bad 16c21b9d10b032d66d4105dd4693fd9dc6e6ec18
# good: [49855596e383ab2aa6410fa060e22d4817d8e64e] OvmfPkg: Move the
GHCB allocations into reserved memory
git bisect good 49855596e383ab2aa6410fa060e22d4817d8e64e
# first bad commit: [16c21b9d10b032d66d4105dd4693fd9dc6e6ec18]
UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use
So clearly we should be looking for an IA32-specific change, or
IA32-specific *omission*, in this patch, that could cause the problem.

The bug is the following:

On 07/30/20 20:43, Tom Lendacky wrote:

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index b1a9d99cb3eb..267aa5201c50 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -349,8 +350,11 @@ VOID
IN BOOLEAN MwaitSupport,
IN UINTN ApTargetCState,
IN UINTN PmCodeSegment,
+ IN UINTN Pm16CodeSegment,
IN UINTN TopOfApStack,
- IN UINTN NumberToFinish
+ IN UINTN NumberToFinish,
+ IN UINTN SevEsAPJumpTable,
+ IN UINTN WakeupBuffer
);

/**
(1) This hunk modifies the parameter list of functions pointed-to by
ASM_RELOCATE_AP_LOOP.

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 9115ff9e3e30..7165bcf3124a 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -330,17 +350,26 @@ RelocateApLoop (
BOOLEAN MwaitSupport;
ASM_RELOCATE_AP_LOOP AsmRelocateApLoopFunc;
UINTN ProcessorNumber;
+ UINTN StackStart;

MpInitLibWhoAmI (&ProcessorNumber);
CpuMpData = GetCpuMpData ();
MwaitSupport = IsMwaitSupport ();
+ if (CpuMpData->SevEsIsEnabled) {
+ StackStart = CpuMpData->SevEsAPResetStackStart;
+ } else {
+ StackStart = mReservedTopOfApStack;
+ }
AsmRelocateApLoopFunc = (ASM_RELOCATE_AP_LOOP) (UINTN)
mReservedApLoopFunc;
AsmRelocateApLoopFunc (
MwaitSupport,
CpuMpData->ApTargetCState,
CpuMpData->PmCodeSegment,
- mReservedTopOfApStack - ProcessorNumber * AP_SAFE_STACK_SIZE,
- (UINTN) &mNumberToFinish
+ CpuMpData->Pm16CodeSegment,
+ StackStart - ProcessorNumber * AP_SAFE_STACK_SIZE,
+ (UINTN) &mNumberToFinish,
+ CpuMpData->SevEsAPBuffer,
+ CpuMpData->WakeupBuffer
);
//
// It should never reach here
(2) This hunk modifies the call site, in accordance with the prototype
change at (1).

diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 6956b408d004..3b8ec477b8b3 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -465,6 +465,10 @@ BITS 16
; - IP for Real Mode (two bytes)
; - CS for Real Mode (two bytes)
;
+ ; This label is also used with AsmRelocateApLoop. During MP
finalization,
+ ; the code from PM16Mode to SwitchToRealProcEnd is copied to the
start of
+ ; the WakeupBuffer, allowing a parked AP to be booted by an OS.
+ ;
PM16Mode:
mov eax, cr0 ; Read CR0
btr eax, 0 ; Set PE=0
@@ -487,32 +491,95 @@ PM16Mode:
SwitchToRealProcEnd:


;-------------------------------------------------------------------------------------

-; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
TopOfApStack, CountTofinish);
+; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
Pm16CodeSegment, TopOfApStack, CountTofinish, SevEsAPJumpTable,
WakeupBuffer);

;-------------------------------------------------------------------------------------

global ASM_PFX(AsmRelocateApLoop)
ASM_PFX(AsmRelocateApLoop):
AsmRelocateApLoopStart:
BITS 64
(3) Unfortunately, the patch only adapts the X64 implementation of the
AsmRelocateApLoopStart() function to the new prototype; the IA32
implementation no longer matches the call site.

(I'm not sure if the intent was for the IA32 version to simply ignore
the new parameters, but even in that case, the "Pm16CodeSegment"
parameter is inserted in the middle of the parameter list, likely
offsetting the rest.)

The problem is foreshadowed even by hunk (2). Namely, in hunk (2), the

s/mReservedTopOfApStack/StackStart/

replacement is *more difficult* to verify than necessary -- exactly
because "CpuMpData->Pm16CodeSegment" is inserted *before* it.
I can do one of two things here and just put the 3 new parameters at the
end of the function call rather than keeping the code segment parameters
together or update the IA32 call site. Let me see which looks best. But
I'll likely update the IA32 call site no matter what with at least
comments about the parameters that aren't used, either way.

Thanks,
Tom


Thanks
Laszlo

Lendacky, Thomas
 

On 7/31/20 9:44 AM, Tom Lendacky wrote:
On 7/31/20 8:36 AM, Tom Lendacky wrote:
On 7/31/20 7:43 AM, Laszlo Ersek wrote:
Hi Tom,
Hi Laszlo,
Hi Laszlo,
Can you try this incremental patch to see if it fixes the issue you're
seeing? If it does, I'll merge it into patch #45 and send out a v14.
Looking at the formatting, I'm not sure if Thunderbird messed up the diff. I'll send you another copy directly to you using git send-email just in case.

Thanks,
Tom

Thanks,
Tom
diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 7165bcf3124a..2c00d72ddefe 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -365,9 +365,9 @@ RelocateApLoop (
MwaitSupport,
CpuMpData->ApTargetCState,
CpuMpData->PmCodeSegment,
- CpuMpData->Pm16CodeSegment,
StackStart - ProcessorNumber * AP_SAFE_STACK_SIZE,
(UINTN) &mNumberToFinish,
+ CpuMpData->Pm16CodeSegment,
CpuMpData->SevEsAPBuffer,
CpuMpData->WakeupBuffer
);
diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
index 309d53bf3b37..7e81d24aa60f 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
@@ -226,7 +226,10 @@ SwitchToRealProcStart:
SwitchToRealProcEnd:
;-------------------------------------------------------------------------------------
-; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack, CountTofinish);
+; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack, CountTofinish, Pm16CodeSegment, SevEsAPJumpTable, WakeupBuffer);
+;
+; The last three parameters (Pm16CodeSegment, SevEsAPJumpTable and WakeupBuffer) are
+; specific to SEV-ES support and are not applicable on IA32.
;-------------------------------------------------------------------------------------
global ASM_PFX(AsmRelocateApLoop)
ASM_PFX(AsmRelocateApLoop):
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 267aa5201c50..02652eaae126 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -350,9 +350,9 @@ VOID
IN BOOLEAN MwaitSupport,
IN UINTN ApTargetCState,
IN UINTN PmCodeSegment,
- IN UINTN Pm16CodeSegment,
IN UINTN TopOfApStack,
IN UINTN NumberToFinish,
+ IN UINTN Pm16CodeSegment,
IN UINTN SevEsAPJumpTable,
IN UINTN WakeupBuffer
);
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 3b8ec477b8b3..5d30f35b201c 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -491,13 +491,13 @@ PM16Mode:
SwitchToRealProcEnd:
;-------------------------------------------------------------------------------------
-; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, Pm16CodeSegment, TopOfApStack, CountTofinish, SevEsAPJumpTable, WakeupBuffer);
+; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack, CountTofinish, Pm16CodeSegment, SevEsAPJumpTable, WakeupBuffer);
;-------------------------------------------------------------------------------------
global ASM_PFX(AsmRelocateApLoop)
ASM_PFX(AsmRelocateApLoop):
AsmRelocateApLoopStart:
BITS 64
- cmp qword [rsp + 56], 0
+ cmp qword [rsp + 56], 0 ; SevEsAPJumpTable
je NoSevEs
;
@@ -539,16 +539,17 @@ BITS 64
NoSevEs:
cli ; Disable interrupt before switching to 32-bit mode
- mov rax, [rsp + 48] ; CountTofinish
+ mov rax, [rsp + 40] ; CountTofinish
lock dec dword [rax] ; (*CountTofinish)--
+ mov r10, [rsp + 48] ; Pm16CodeSegment
mov rax, [rsp + 56] ; SevEsAPJumpTable
mov rbx, [rsp + 64] ; WakeupBuffer
- mov rsp, [rsp + 40] ; TopOfApStack
+ mov rsp, r9 ; TopOfApStack
push rax ; Save SevEsAPJumpTable
push rbx ; Save WakeupBuffer
- push r9 ; Save Pm16CodeSegment
+ push r10 ; Save Pm16CodeSegment
push rcx ; Save MwaitSupport
push rdx ; Save ApTargetCState



On 07/30/20 20:43, Tom Lendacky wrote:
From: Tom Lendacky <thomas.lendacky@...>

BZ:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7Cb8c77cf296c949d2bbd808d8354f542b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637317962138028351&amp;sdata=HISHZmLjOc%2FfgVrBm8MlNeDAk453NJ64%2B51bETZj4rk%3D&amp;reserved=0


Before UEFI transfers control to the OS, it must park the AP. This is
done using the AsmRelocateApLoop function to transition into 32-bit
non-paging mode. For an SEV-ES guest, a few additional things must be
done:
- AsmRelocateApLoop must be updated to support SEV-ES. This means
performing a VMGEXIT AP Reset Hold instead of an MWAIT or HLT loop.
- Since the AP must transition to real mode, a small routine is copied
to the WakeupBuffer area. Since the WakeupBuffer will be used by
the AP during OS booting, it must be placed in reserved memory.
Additionally, the AP stack must be located where it can be accessed
in real mode.
- Once the AP is in real mode it will transfer control to the
destination specified by the OS in the SEV-ES AP Jump Table. The
SEV-ES AP Jump Table address is saved by the hypervisor for the OS
using the GHCB VMGEXIT AP Jump Table exit code.

Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Laszlo Ersek <lersek@...>
Reviewed-by: Eric Dong <eric.dong@...>
Signed-off-by: Tom Lendacky <thomas.lendacky@...>
---
UefiCpuPkg/Library/MpInitLib/MpLib.h | 8 +-
UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 54 +++++++-
UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 131 ++++++++++++++++--
3 files changed, 175 insertions(+), 18 deletions(-)
Now that this series is almost ready to merge, I've done a bit of
regression-testing.

Unfortunately, this patch breaks booting with IA32 OVMF.

More precisely, it breaks the IA32 version of DxeMpInitLib.
Yeah, that's not good. I will look into this based on your input below.
What's strange is that my system doesn't hang and successfully boots all
APs (up to 64 is what I've tested with).

But, yes, both call sites should be the same and I will make that change.


The symptom is that just when the OS would be launched, the
multiprocessor guest hangs. This is how the log terminates:

FSOpen: Open
'\370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\linux'
Success
[Security] 3rd party image[0] can be loaded after EndOfDxe:
PciRoot(0x0)/Pci(0x2,0x1)/Pci(0x0,0x0)/Scsi(0x0,0x0)/HD(1,GPT,D9F1FBA5-E5D3-440A-B6A7-87B593E4FAB1,0x800,0x100000)/\370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\linux.

InstallProtocolInterface: [EfiLoadedImageProtocol] 853A03A8
Loading driver at 0x00083E72000 EntryPoint=0x00083E76680
InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 853A0510
ProtectUefiImageCommon - 0x853A03A8
- 0x0000000083E72000 - 0x0000000000E75000
FSOpen: Open
'370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\initrd'
Success
PixelBlueGreenRedReserved8BitPerColor
ConvertPages: range 400000 - 1274FFF covers multiple entries
SmmInstallProtocolInterface: [EdkiiSmmExitBootServicesProtocol] 0
CpuDxe: 5-Level Paging = 0
[HANG]
Meanwhile some guest CPUs are pegged.

Normally, when this series is not applied, the next log entry is (in
place of [HANG]):

MpInitChangeApLoopCallback() done!
I've identified this patch by bisection, after applying the series on
current master (137c2c6eff67, "Revert "BaseTools/PatchCheck.py: Add
LicenseCheck"", 2020-07-31).

Here's the bisection log:

git bisect start
# good: [137c2c6eff67f4750d77e8e40af6683c412d3ed0] Revert
"BaseTools/PatchCheck.py: Add LicenseCheck"
git bisect good 137c2c6eff67f4750d77e8e40af6683c412d3ed0
# bad: [d3f7971f4f70c9f39170b42af837e58e59435ad3] Maintainers.txt: Add
reviewers for the OvmfPkg SEV-related files
git bisect bad d3f7971f4f70c9f39170b42af837e58e59435ad3
# good: [9551e3fc61ba0c0ddf8e79b425a22aa7dd61cb8b] OvmfPkg/VmgExitLib:
Add support for RDTSCP NAE events
git bisect good 9551e3fc61ba0c0ddf8e79b425a22aa7dd61cb8b
# good: [10acf16b38522d8a1b538b3aa432daaa72c0e97b] OvmfPkg: Reserve a
page in memory for the SEV-ES usage
git bisect good 10acf16b38522d8a1b538b3aa432daaa72c0e97b
# good: [ccb4267e76b6474657c41bef7e76a980930c22ea] UefiCpuPkg: Add a
16-bit protected mode code segment descriptor
git bisect good ccb4267e76b6474657c41bef7e76a980930c22ea
# good: [94e238ae37505cfb081f3b9b4632067e4a113cf9] OvmfPkg: Use the
SEV-ES work area for the SEV-ES AP reset vector
git bisect good 94e238ae37505cfb081f3b9b4632067e4a113cf9
# bad: [16c21b9d10b032d66d4105dd4693fd9dc6e6ec18] UefiCpuPkg/MpInitLib:
Prepare SEV-ES guest APs for OS use
git bisect bad 16c21b9d10b032d66d4105dd4693fd9dc6e6ec18
# good: [49855596e383ab2aa6410fa060e22d4817d8e64e] OvmfPkg: Move the
GHCB allocations into reserved memory
git bisect good 49855596e383ab2aa6410fa060e22d4817d8e64e
# first bad commit: [16c21b9d10b032d66d4105dd4693fd9dc6e6ec18]
UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use
So clearly we should be looking for an IA32-specific change, or
IA32-specific *omission*, in this patch, that could cause the problem.

The bug is the following:

On 07/30/20 20:43, Tom Lendacky wrote:

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index b1a9d99cb3eb..267aa5201c50 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -349,8 +350,11 @@ VOID
IN BOOLEAN MwaitSupport,
IN UINTN ApTargetCState,
IN UINTN PmCodeSegment,
+ IN UINTN Pm16CodeSegment,
IN UINTN TopOfApStack,
- IN UINTN NumberToFinish
+ IN UINTN NumberToFinish,
+ IN UINTN SevEsAPJumpTable,
+ IN UINTN WakeupBuffer
);

/**
(1) This hunk modifies the parameter list of functions pointed-to by
ASM_RELOCATE_AP_LOOP.

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 9115ff9e3e30..7165bcf3124a 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -330,17 +350,26 @@ RelocateApLoop (
BOOLEAN MwaitSupport;
ASM_RELOCATE_AP_LOOP AsmRelocateApLoopFunc;
UINTN ProcessorNumber;
+ UINTN StackStart;

MpInitLibWhoAmI (&ProcessorNumber);
CpuMpData = GetCpuMpData ();
MwaitSupport = IsMwaitSupport ();
+ if (CpuMpData->SevEsIsEnabled) {
+ StackStart = CpuMpData->SevEsAPResetStackStart;
+ } else {
+ StackStart = mReservedTopOfApStack;
+ }
AsmRelocateApLoopFunc = (ASM_RELOCATE_AP_LOOP) (UINTN)
mReservedApLoopFunc;
AsmRelocateApLoopFunc (
MwaitSupport,
CpuMpData->ApTargetCState,
CpuMpData->PmCodeSegment,
- mReservedTopOfApStack - ProcessorNumber * AP_SAFE_STACK_SIZE,
- (UINTN) &mNumberToFinish
+ CpuMpData->Pm16CodeSegment,
+ StackStart - ProcessorNumber * AP_SAFE_STACK_SIZE,
+ (UINTN) &mNumberToFinish,
+ CpuMpData->SevEsAPBuffer,
+ CpuMpData->WakeupBuffer
);
//
// It should never reach here
(2) This hunk modifies the call site, in accordance with the prototype
change at (1).

diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 6956b408d004..3b8ec477b8b3 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -465,6 +465,10 @@ BITS 16
; - IP for Real Mode (two bytes)
; - CS for Real Mode (two bytes)
;
+ ; This label is also used with AsmRelocateApLoop. During MP
finalization,
+ ; the code from PM16Mode to SwitchToRealProcEnd is copied to the
start of
+ ; the WakeupBuffer, allowing a parked AP to be booted by an OS.
+ ;
PM16Mode:
mov eax, cr0 ; Read CR0
btr eax, 0 ; Set PE=0
@@ -487,32 +491,95 @@ PM16Mode:
SwitchToRealProcEnd:

;-------------------------------------------------------------------------------------

-; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
TopOfApStack, CountTofinish);
+; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
Pm16CodeSegment, TopOfApStack, CountTofinish, SevEsAPJumpTable,
WakeupBuffer);
;-------------------------------------------------------------------------------------

global ASM_PFX(AsmRelocateApLoop)
ASM_PFX(AsmRelocateApLoop):
AsmRelocateApLoopStart:
BITS 64
(3) Unfortunately, the patch only adapts the X64 implementation of the
AsmRelocateApLoopStart() function to the new prototype; the IA32
implementation no longer matches the call site.

(I'm not sure if the intent was for the IA32 version to simply ignore
the new parameters, but even in that case, the "Pm16CodeSegment"
parameter is inserted in the middle of the parameter list, likely
offsetting the rest.)

The problem is foreshadowed even by hunk (2). Namely, in hunk (2), the

s/mReservedTopOfApStack/StackStart/

replacement is *more difficult* to verify than necessary -- exactly
because "CpuMpData->Pm16CodeSegment" is inserted *before* it.
I can do one of two things here and just put the 3 new parameters at the
end of the function call rather than keeping the code segment parameters
together or update the IA32 call site. Let me see which looks best. But
I'll likely update the IA32 call site no matter what with at least
comments about the parameters that aren't used, either way.

Thanks,
Tom


Thanks
Laszlo

Laszlo Ersek
 

On 07/31/20 16:47, Tom Lendacky wrote:
On 7/31/20 9:44 AM, Tom Lendacky wrote:
On 7/31/20 8:36 AM, Tom Lendacky wrote:
On 7/31/20 7:43 AM, Laszlo Ersek wrote:
Hi Tom,
Hi Laszlo,
Hi Laszlo,

Can you try this incremental patch to see if it fixes the issue you're
seeing? If it does, I'll merge it into patch #45 and send out a v14.
Looking at the formatting, I'm not sure if Thunderbird messed up the
diff. I'll send you another copy directly to you using git send-email
just in case.
I got the separate copy; I'll report back sometime next week.
Thanks!
Laszlo

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 7165bcf3124a..2c00d72ddefe 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -365,9 +365,9 @@ RelocateApLoop (
      MwaitSupport,

      CpuMpData->ApTargetCState,

      CpuMpData->PmCodeSegment,

-    CpuMpData->Pm16CodeSegment,

      StackStart - ProcessorNumber * AP_SAFE_STACK_SIZE,

      (UINTN) &mNumberToFinish,

+    CpuMpData->Pm16CodeSegment,

      CpuMpData->SevEsAPBuffer,

      CpuMpData->WakeupBuffer

      );

diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
index 309d53bf3b37..7e81d24aa60f 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
@@ -226,7 +226,10 @@ SwitchToRealProcStart:
  SwitchToRealProcEnd:

 
 
;-------------------------------------------------------------------------------------


-;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
TopOfApStack, CountTofinish);

+;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
TopOfApStack, CountTofinish, Pm16CodeSegment, SevEsAPJumpTable,
WakeupBuffer);

+;

+;  The last three parameters (Pm16CodeSegment, SevEsAPJumpTable and
WakeupBuffer) are

+;  specific to SEV-ES support and are not applicable on IA32.

 
;-------------------------------------------------------------------------------------


  global ASM_PFX(AsmRelocateApLoop)

  ASM_PFX(AsmRelocateApLoop):

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 267aa5201c50..02652eaae126 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -350,9 +350,9 @@ VOID
    IN BOOLEAN                 MwaitSupport,

    IN UINTN                   ApTargetCState,

    IN UINTN                   PmCodeSegment,

-  IN UINTN                   Pm16CodeSegment,

    IN UINTN                   TopOfApStack,

    IN UINTN                   NumberToFinish,

+  IN UINTN                   Pm16CodeSegment,

    IN UINTN                   SevEsAPJumpTable,

    IN UINTN                   WakeupBuffer

    );

diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 3b8ec477b8b3..5d30f35b201c 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -491,13 +491,13 @@ PM16Mode:
  SwitchToRealProcEnd:

 
 
;-------------------------------------------------------------------------------------


-;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
Pm16CodeSegment, TopOfApStack, CountTofinish, SevEsAPJumpTable,
WakeupBuffer);

+;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
TopOfApStack, CountTofinish, Pm16CodeSegment, SevEsAPJumpTable,
WakeupBuffer);

 
;-------------------------------------------------------------------------------------


  global ASM_PFX(AsmRelocateApLoop)

  ASM_PFX(AsmRelocateApLoop):

  AsmRelocateApLoopStart:

  BITS 64

-    cmp        qword [rsp + 56], 0

+    cmp        qword [rsp + 56], 0  ; SevEsAPJumpTable

      je         NoSevEs

 
      ;

@@ -539,16 +539,17 @@ BITS 64
 
  NoSevEs:

      cli                          ; Disable interrupt before
switching to 32-bit mode

-    mov        rax, [rsp + 48]   ; CountTofinish

+    mov        rax, [rsp + 40]   ; CountTofinish

      lock dec   dword [rax]       ; (*CountTofinish)--

 
+    mov        r10, [rsp + 48]   ; Pm16CodeSegment

      mov        rax, [rsp + 56]   ; SevEsAPJumpTable

      mov        rbx, [rsp + 64]   ; WakeupBuffer

-    mov        rsp, [rsp + 40]   ; TopOfApStack

+    mov        rsp, r9           ; TopOfApStack

 
      push       rax               ; Save SevEsAPJumpTable

      push       rbx               ; Save WakeupBuffer

-    push       r9                ; Save Pm16CodeSegment

+    push       r10               ; Save Pm16CodeSegment

      push       rcx               ; Save MwaitSupport

      push       rdx               ; Save ApTargetCState

 




On 07/30/20 20:43, Tom Lendacky wrote:
From: Tom Lendacky <thomas.lendacky@...>

BZ:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7Cb8c77cf296c949d2bbd808d8354f542b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637317962138028351&amp;sdata=HISHZmLjOc%2FfgVrBm8MlNeDAk453NJ64%2B51bETZj4rk%3D&amp;reserved=0



Before UEFI transfers control to the OS, it must park the AP. This is
done using the AsmRelocateApLoop function to transition into 32-bit
non-paging mode. For an SEV-ES guest, a few additional things must be
done:
    - AsmRelocateApLoop must be updated to support SEV-ES. This means
      performing a VMGEXIT AP Reset Hold instead of an MWAIT or HLT
loop.
    - Since the AP must transition to real mode, a small routine is
copied
      to the WakeupBuffer area. Since the WakeupBuffer will be used by
      the AP during OS booting, it must be placed in reserved memory.
      Additionally, the AP stack must be located where it can be
accessed
      in real mode.
    - Once the AP is in real mode it will transfer control to the
      destination specified by the OS in the SEV-ES AP Jump Table. The
      SEV-ES AP Jump Table address is saved by the hypervisor for
the OS
      using the GHCB VMGEXIT AP Jump Table exit code.

Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Laszlo Ersek <lersek@...>
Reviewed-by: Eric Dong <eric.dong@...>
Signed-off-by: Tom Lendacky <thomas.lendacky@...>
---
   UefiCpuPkg/Library/MpInitLib/MpLib.h          |   8 +-
   UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       |  54 +++++++-
   UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 131
++++++++++++++++--
   3 files changed, 175 insertions(+), 18 deletions(-)
Now that this series is almost ready to merge, I've done a bit of
regression-testing.

Unfortunately, this patch breaks booting with IA32 OVMF.

More precisely, it breaks the IA32 version of DxeMpInitLib.
Yeah, that's not good.  I will look into this based on your input below.
What's strange is that my system doesn't hang and successfully boots all
APs (up to 64 is what I've tested with).

But, yes, both call sites should be the same and I will make that
change.


The symptom is that just when the OS would be launched, the
multiprocessor guest hangs. This is how the log terminates:

FSOpen: Open
'\370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\linux'
Success
[Security] 3rd party image[0] can be loaded after EndOfDxe:
PciRoot(0x0)/Pci(0x2,0x1)/Pci(0x0,0x0)/Scsi(0x0,0x0)/HD(1,GPT,D9F1FBA5-E5D3-440A-B6A7-87B593E4FAB1,0x800,0x100000)/\370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\linux.


InstallProtocolInterface: [EfiLoadedImageProtocol] 853A03A8
Loading driver at 0x00083E72000 EntryPoint=0x00083E76680
InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 853A0510
ProtectUefiImageCommon - 0x853A03A8
    - 0x0000000083E72000 - 0x0000000000E75000
FSOpen: Open
'370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\initrd'
Success
PixelBlueGreenRedReserved8BitPerColor
ConvertPages: range 400000 - 1274FFF covers multiple entries
SmmInstallProtocolInterface: [EdkiiSmmExitBootServicesProtocol] 0
CpuDxe: 5-Level Paging = 0
[HANG]
Meanwhile some guest CPUs are pegged.

Normally, when this series is not applied, the next log entry is (in
place of [HANG]):

MpInitChangeApLoopCallback() done!
I've identified this patch by bisection, after applying the series on
current master (137c2c6eff67, "Revert "BaseTools/PatchCheck.py: Add
LicenseCheck"", 2020-07-31).

Here's the bisection log:

git bisect start
# good: [137c2c6eff67f4750d77e8e40af6683c412d3ed0] Revert
"BaseTools/PatchCheck.py: Add LicenseCheck"
git bisect good 137c2c6eff67f4750d77e8e40af6683c412d3ed0
# bad: [d3f7971f4f70c9f39170b42af837e58e59435ad3] Maintainers.txt: Add
reviewers for the OvmfPkg SEV-related files
git bisect bad d3f7971f4f70c9f39170b42af837e58e59435ad3
# good: [9551e3fc61ba0c0ddf8e79b425a22aa7dd61cb8b] OvmfPkg/VmgExitLib:
Add support for RDTSCP NAE events
git bisect good 9551e3fc61ba0c0ddf8e79b425a22aa7dd61cb8b
# good: [10acf16b38522d8a1b538b3aa432daaa72c0e97b] OvmfPkg: Reserve a
page in memory for the SEV-ES usage
git bisect good 10acf16b38522d8a1b538b3aa432daaa72c0e97b
# good: [ccb4267e76b6474657c41bef7e76a980930c22ea] UefiCpuPkg: Add a
16-bit protected mode code segment descriptor
git bisect good ccb4267e76b6474657c41bef7e76a980930c22ea
# good: [94e238ae37505cfb081f3b9b4632067e4a113cf9] OvmfPkg: Use the
SEV-ES work area for the SEV-ES AP reset vector
git bisect good 94e238ae37505cfb081f3b9b4632067e4a113cf9
# bad: [16c21b9d10b032d66d4105dd4693fd9dc6e6ec18]
UefiCpuPkg/MpInitLib:
Prepare SEV-ES guest APs for OS use
git bisect bad 16c21b9d10b032d66d4105dd4693fd9dc6e6ec18
# good: [49855596e383ab2aa6410fa060e22d4817d8e64e] OvmfPkg: Move the
GHCB allocations into reserved memory
git bisect good 49855596e383ab2aa6410fa060e22d4817d8e64e
# first bad commit: [16c21b9d10b032d66d4105dd4693fd9dc6e6ec18]
UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use
So clearly we should be looking for an IA32-specific change, or
IA32-specific *omission*, in this patch, that could cause the problem.

The bug is the following:

On 07/30/20 20:43, Tom Lendacky wrote:

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index b1a9d99cb3eb..267aa5201c50 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -349,8 +350,11 @@ VOID
     IN BOOLEAN                 MwaitSupport,
     IN UINTN                   ApTargetCState,
     IN UINTN                   PmCodeSegment,
+  IN UINTN                   Pm16CodeSegment,
     IN UINTN                   TopOfApStack,
-  IN UINTN                   NumberToFinish
+  IN UINTN                   NumberToFinish,
+  IN UINTN                   SevEsAPJumpTable,
+  IN UINTN                   WakeupBuffer
     );

   /**
(1) This hunk modifies the parameter list of functions pointed-to by
ASM_RELOCATE_AP_LOOP.

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 9115ff9e3e30..7165bcf3124a 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -330,17 +350,26 @@ RelocateApLoop (
     BOOLEAN                MwaitSupport;
     ASM_RELOCATE_AP_LOOP   AsmRelocateApLoopFunc;
     UINTN                  ProcessorNumber;
+  UINTN                  StackStart;

     MpInitLibWhoAmI (&ProcessorNumber);
     CpuMpData    = GetCpuMpData ();
     MwaitSupport = IsMwaitSupport ();
+  if (CpuMpData->SevEsIsEnabled) {
+    StackStart = CpuMpData->SevEsAPResetStackStart;
+  } else {
+    StackStart = mReservedTopOfApStack;
+  }
     AsmRelocateApLoopFunc = (ASM_RELOCATE_AP_LOOP) (UINTN)
mReservedApLoopFunc;
     AsmRelocateApLoopFunc (
       MwaitSupport,
       CpuMpData->ApTargetCState,
       CpuMpData->PmCodeSegment,
-    mReservedTopOfApStack - ProcessorNumber * AP_SAFE_STACK_SIZE,
-    (UINTN) &mNumberToFinish
+    CpuMpData->Pm16CodeSegment,
+    StackStart - ProcessorNumber * AP_SAFE_STACK_SIZE,
+    (UINTN) &mNumberToFinish,
+    CpuMpData->SevEsAPBuffer,
+    CpuMpData->WakeupBuffer
       );
     //
     // It should never reach here
(2) This hunk modifies the call site, in accordance with the prototype
change at (1).

diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 6956b408d004..3b8ec477b8b3 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -465,6 +465,10 @@ BITS 16
       ;     - IP for Real Mode (two bytes)
       ;     - CS for Real Mode (two bytes)
       ;
+    ; This label is also used with AsmRelocateApLoop. During MP
finalization,
+    ; the code from PM16Mode to SwitchToRealProcEnd is copied to the
start of
+    ; the WakeupBuffer, allowing a parked AP to be booted by an OS.
+    ;
   PM16Mode:
       mov        eax, cr0                    ; Read CR0
       btr        eax, 0                      ; Set PE=0
@@ -487,32 +491,95 @@ PM16Mode:
   SwitchToRealProcEnd:

  
;-------------------------------------------------------------------------------------


-;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
TopOfApStack, CountTofinish);
+;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
Pm16CodeSegment, TopOfApStack, CountTofinish, SevEsAPJumpTable,
WakeupBuffer);
  
;-------------------------------------------------------------------------------------


   global ASM_PFX(AsmRelocateApLoop)
   ASM_PFX(AsmRelocateApLoop):
   AsmRelocateApLoopStart:
   BITS 64
(3) Unfortunately, the patch only adapts the X64 implementation of the
AsmRelocateApLoopStart() function to the new prototype; the IA32
implementation no longer matches the call site.

(I'm not sure if the intent was for the IA32 version to simply ignore
the new parameters, but even in that case, the "Pm16CodeSegment"
parameter is inserted in the middle of the parameter list, likely
offsetting the rest.)

The problem is foreshadowed even by hunk (2). Namely, in hunk (2), the

    s/mReservedTopOfApStack/StackStart/

replacement is *more difficult* to verify than necessary -- exactly
because "CpuMpData->Pm16CodeSegment" is inserted *before* it.
I can do one of two things here and just put the 3 new parameters at the
end of the function call rather than keeping the code segment parameters
together or update the IA32 call site. Let me see which looks best. But
I'll likely update the IA32 call site no matter what with at least
comments about the parameters that aren't used, either way.

Thanks,
Tom


Thanks
Laszlo

Laszlo Ersek
 

On 07/31/20 23:38, Laszlo Ersek wrote:
On 07/31/20 16:47, Tom Lendacky wrote:
On 7/31/20 9:44 AM, Tom Lendacky wrote:
On 7/31/20 8:36 AM, Tom Lendacky wrote:
On 7/31/20 7:43 AM, Laszlo Ersek wrote:
Hi Tom,
Hi Laszlo,
Hi Laszlo,

Can you try this incremental patch to see if it fixes the issue you're
seeing? If it does, I'll merge it into patch #45 and send out a v14.
Looking at the formatting, I'm not sure if Thunderbird messed up the
diff. I'll send you another copy directly to you using git send-email
just in case.
I got the separate copy; I'll report back sometime next week.
The update works fine; IA32 OVMF boots OK with it.

I agree with squashing the update into patch #45, but before sending
v14, maybe we should get some feedback for the MdeModulePkg patches too,
at long last. :/

Thanks!
Laszlo



diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 7165bcf3124a..2c00d72ddefe 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -365,9 +365,9 @@ RelocateApLoop (
      MwaitSupport,

      CpuMpData->ApTargetCState,

      CpuMpData->PmCodeSegment,

-    CpuMpData->Pm16CodeSegment,

      StackStart - ProcessorNumber * AP_SAFE_STACK_SIZE,

      (UINTN) &mNumberToFinish,

+    CpuMpData->Pm16CodeSegment,

      CpuMpData->SevEsAPBuffer,

      CpuMpData->WakeupBuffer

      );

diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
index 309d53bf3b37..7e81d24aa60f 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
@@ -226,7 +226,10 @@ SwitchToRealProcStart:
  SwitchToRealProcEnd:

 
 
;-------------------------------------------------------------------------------------


-;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
TopOfApStack, CountTofinish);

+;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
TopOfApStack, CountTofinish, Pm16CodeSegment, SevEsAPJumpTable,
WakeupBuffer);

+;

+;  The last three parameters (Pm16CodeSegment, SevEsAPJumpTable and
WakeupBuffer) are

+;  specific to SEV-ES support and are not applicable on IA32.

 
;-------------------------------------------------------------------------------------


  global ASM_PFX(AsmRelocateApLoop)

  ASM_PFX(AsmRelocateApLoop):

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 267aa5201c50..02652eaae126 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -350,9 +350,9 @@ VOID
    IN BOOLEAN                 MwaitSupport,

    IN UINTN                   ApTargetCState,

    IN UINTN                   PmCodeSegment,

-  IN UINTN                   Pm16CodeSegment,

    IN UINTN                   TopOfApStack,

    IN UINTN                   NumberToFinish,

+  IN UINTN                   Pm16CodeSegment,

    IN UINTN                   SevEsAPJumpTable,

    IN UINTN                   WakeupBuffer

    );

diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 3b8ec477b8b3..5d30f35b201c 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -491,13 +491,13 @@ PM16Mode:
  SwitchToRealProcEnd:

 
 
;-------------------------------------------------------------------------------------


-;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
Pm16CodeSegment, TopOfApStack, CountTofinish, SevEsAPJumpTable,
WakeupBuffer);

+;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
TopOfApStack, CountTofinish, Pm16CodeSegment, SevEsAPJumpTable,
WakeupBuffer);

 
;-------------------------------------------------------------------------------------


  global ASM_PFX(AsmRelocateApLoop)

  ASM_PFX(AsmRelocateApLoop):

  AsmRelocateApLoopStart:

  BITS 64

-    cmp        qword [rsp + 56], 0

+    cmp        qword [rsp + 56], 0  ; SevEsAPJumpTable

      je         NoSevEs

 
      ;

@@ -539,16 +539,17 @@ BITS 64
 
  NoSevEs:

      cli                          ; Disable interrupt before
switching to 32-bit mode

-    mov        rax, [rsp + 48]   ; CountTofinish

+    mov        rax, [rsp + 40]   ; CountTofinish

      lock dec   dword [rax]       ; (*CountTofinish)--

 
+    mov        r10, [rsp + 48]   ; Pm16CodeSegment

      mov        rax, [rsp + 56]   ; SevEsAPJumpTable

      mov        rbx, [rsp + 64]   ; WakeupBuffer

-    mov        rsp, [rsp + 40]   ; TopOfApStack

+    mov        rsp, r9           ; TopOfApStack

 
      push       rax               ; Save SevEsAPJumpTable

      push       rbx               ; Save WakeupBuffer

-    push       r9                ; Save Pm16CodeSegment

+    push       r10               ; Save Pm16CodeSegment

      push       rcx               ; Save MwaitSupport

      push       rdx               ; Save ApTargetCState

 




On 07/30/20 20:43, Tom Lendacky wrote:
From: Tom Lendacky <thomas.lendacky@...>

BZ:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7Cb8c77cf296c949d2bbd808d8354f542b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637317962138028351&amp;sdata=HISHZmLjOc%2FfgVrBm8MlNeDAk453NJ64%2B51bETZj4rk%3D&amp;reserved=0



Before UEFI transfers control to the OS, it must park the AP. This is
done using the AsmRelocateApLoop function to transition into 32-bit
non-paging mode. For an SEV-ES guest, a few additional things must be
done:
    - AsmRelocateApLoop must be updated to support SEV-ES. This means
      performing a VMGEXIT AP Reset Hold instead of an MWAIT or HLT
loop.
    - Since the AP must transition to real mode, a small routine is
copied
      to the WakeupBuffer area. Since the WakeupBuffer will be used by
      the AP during OS booting, it must be placed in reserved memory.
      Additionally, the AP stack must be located where it can be
accessed
      in real mode.
    - Once the AP is in real mode it will transfer control to the
      destination specified by the OS in the SEV-ES AP Jump Table. The
      SEV-ES AP Jump Table address is saved by the hypervisor for
the OS
      using the GHCB VMGEXIT AP Jump Table exit code.

Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Laszlo Ersek <lersek@...>
Reviewed-by: Eric Dong <eric.dong@...>
Signed-off-by: Tom Lendacky <thomas.lendacky@...>
---
   UefiCpuPkg/Library/MpInitLib/MpLib.h          |   8 +-
   UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       |  54 +++++++-
   UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 131
++++++++++++++++--
   3 files changed, 175 insertions(+), 18 deletions(-)
Now that this series is almost ready to merge, I've done a bit of
regression-testing.

Unfortunately, this patch breaks booting with IA32 OVMF.

More precisely, it breaks the IA32 version of DxeMpInitLib.
Yeah, that's not good.  I will look into this based on your input below.
What's strange is that my system doesn't hang and successfully boots all
APs (up to 64 is what I've tested with).

But, yes, both call sites should be the same and I will make that
change.


The symptom is that just when the OS would be launched, the
multiprocessor guest hangs. This is how the log terminates:

FSOpen: Open
'\370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\linux'
Success
[Security] 3rd party image[0] can be loaded after EndOfDxe:
PciRoot(0x0)/Pci(0x2,0x1)/Pci(0x0,0x0)/Scsi(0x0,0x0)/HD(1,GPT,D9F1FBA5-E5D3-440A-B6A7-87B593E4FAB1,0x800,0x100000)/\370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\linux.


InstallProtocolInterface: [EfiLoadedImageProtocol] 853A03A8
Loading driver at 0x00083E72000 EntryPoint=0x00083E76680
InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 853A0510
ProtectUefiImageCommon - 0x853A03A8
    - 0x0000000083E72000 - 0x0000000000E75000
FSOpen: Open
'370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\initrd'
Success
PixelBlueGreenRedReserved8BitPerColor
ConvertPages: range 400000 - 1274FFF covers multiple entries
SmmInstallProtocolInterface: [EdkiiSmmExitBootServicesProtocol] 0
CpuDxe: 5-Level Paging = 0
[HANG]
Meanwhile some guest CPUs are pegged.

Normally, when this series is not applied, the next log entry is (in
place of [HANG]):

MpInitChangeApLoopCallback() done!
I've identified this patch by bisection, after applying the series on
current master (137c2c6eff67, "Revert "BaseTools/PatchCheck.py: Add
LicenseCheck"", 2020-07-31).

Here's the bisection log:

git bisect start
# good: [137c2c6eff67f4750d77e8e40af6683c412d3ed0] Revert
"BaseTools/PatchCheck.py: Add LicenseCheck"
git bisect good 137c2c6eff67f4750d77e8e40af6683c412d3ed0
# bad: [d3f7971f4f70c9f39170b42af837e58e59435ad3] Maintainers.txt: Add
reviewers for the OvmfPkg SEV-related files
git bisect bad d3f7971f4f70c9f39170b42af837e58e59435ad3
# good: [9551e3fc61ba0c0ddf8e79b425a22aa7dd61cb8b] OvmfPkg/VmgExitLib:
Add support for RDTSCP NAE events
git bisect good 9551e3fc61ba0c0ddf8e79b425a22aa7dd61cb8b
# good: [10acf16b38522d8a1b538b3aa432daaa72c0e97b] OvmfPkg: Reserve a
page in memory for the SEV-ES usage
git bisect good 10acf16b38522d8a1b538b3aa432daaa72c0e97b
# good: [ccb4267e76b6474657c41bef7e76a980930c22ea] UefiCpuPkg: Add a
16-bit protected mode code segment descriptor
git bisect good ccb4267e76b6474657c41bef7e76a980930c22ea
# good: [94e238ae37505cfb081f3b9b4632067e4a113cf9] OvmfPkg: Use the
SEV-ES work area for the SEV-ES AP reset vector
git bisect good 94e238ae37505cfb081f3b9b4632067e4a113cf9
# bad: [16c21b9d10b032d66d4105dd4693fd9dc6e6ec18]
UefiCpuPkg/MpInitLib:
Prepare SEV-ES guest APs for OS use
git bisect bad 16c21b9d10b032d66d4105dd4693fd9dc6e6ec18
# good: [49855596e383ab2aa6410fa060e22d4817d8e64e] OvmfPkg: Move the
GHCB allocations into reserved memory
git bisect good 49855596e383ab2aa6410fa060e22d4817d8e64e
# first bad commit: [16c21b9d10b032d66d4105dd4693fd9dc6e6ec18]
UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use
So clearly we should be looking for an IA32-specific change, or
IA32-specific *omission*, in this patch, that could cause the problem.

The bug is the following:

On 07/30/20 20:43, Tom Lendacky wrote:

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index b1a9d99cb3eb..267aa5201c50 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -349,8 +350,11 @@ VOID
     IN BOOLEAN                 MwaitSupport,
     IN UINTN                   ApTargetCState,
     IN UINTN                   PmCodeSegment,
+  IN UINTN                   Pm16CodeSegment,
     IN UINTN                   TopOfApStack,
-  IN UINTN                   NumberToFinish
+  IN UINTN                   NumberToFinish,
+  IN UINTN                   SevEsAPJumpTable,
+  IN UINTN                   WakeupBuffer
     );

   /**
(1) This hunk modifies the parameter list of functions pointed-to by
ASM_RELOCATE_AP_LOOP.

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 9115ff9e3e30..7165bcf3124a 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -330,17 +350,26 @@ RelocateApLoop (
     BOOLEAN                MwaitSupport;
     ASM_RELOCATE_AP_LOOP   AsmRelocateApLoopFunc;
     UINTN                  ProcessorNumber;
+  UINTN                  StackStart;

     MpInitLibWhoAmI (&ProcessorNumber);
     CpuMpData    = GetCpuMpData ();
     MwaitSupport = IsMwaitSupport ();
+  if (CpuMpData->SevEsIsEnabled) {
+    StackStart = CpuMpData->SevEsAPResetStackStart;
+  } else {
+    StackStart = mReservedTopOfApStack;
+  }
     AsmRelocateApLoopFunc = (ASM_RELOCATE_AP_LOOP) (UINTN)
mReservedApLoopFunc;
     AsmRelocateApLoopFunc (
       MwaitSupport,
       CpuMpData->ApTargetCState,
       CpuMpData->PmCodeSegment,
-    mReservedTopOfApStack - ProcessorNumber * AP_SAFE_STACK_SIZE,
-    (UINTN) &mNumberToFinish
+    CpuMpData->Pm16CodeSegment,
+    StackStart - ProcessorNumber * AP_SAFE_STACK_SIZE,
+    (UINTN) &mNumberToFinish,
+    CpuMpData->SevEsAPBuffer,
+    CpuMpData->WakeupBuffer
       );
     //
     // It should never reach here
(2) This hunk modifies the call site, in accordance with the prototype
change at (1).

diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 6956b408d004..3b8ec477b8b3 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -465,6 +465,10 @@ BITS 16
       ;     - IP for Real Mode (two bytes)
       ;     - CS for Real Mode (two bytes)
       ;
+    ; This label is also used with AsmRelocateApLoop. During MP
finalization,
+    ; the code from PM16Mode to SwitchToRealProcEnd is copied to the
start of
+    ; the WakeupBuffer, allowing a parked AP to be booted by an OS.
+    ;
   PM16Mode:
       mov        eax, cr0                    ; Read CR0
       btr        eax, 0                      ; Set PE=0
@@ -487,32 +491,95 @@ PM16Mode:
   SwitchToRealProcEnd:

  
;-------------------------------------------------------------------------------------


-;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
TopOfApStack, CountTofinish);
+;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
Pm16CodeSegment, TopOfApStack, CountTofinish, SevEsAPJumpTable,
WakeupBuffer);
  
;-------------------------------------------------------------------------------------


   global ASM_PFX(AsmRelocateApLoop)
   ASM_PFX(AsmRelocateApLoop):
   AsmRelocateApLoopStart:
   BITS 64
(3) Unfortunately, the patch only adapts the X64 implementation of the
AsmRelocateApLoopStart() function to the new prototype; the IA32
implementation no longer matches the call site.

(I'm not sure if the intent was for the IA32 version to simply ignore
the new parameters, but even in that case, the "Pm16CodeSegment"
parameter is inserted in the middle of the parameter list, likely
offsetting the rest.)

The problem is foreshadowed even by hunk (2). Namely, in hunk (2), the

    s/mReservedTopOfApStack/StackStart/

replacement is *more difficult* to verify than necessary -- exactly
because "CpuMpData->Pm16CodeSegment" is inserted *before* it.
I can do one of two things here and just put the 3 new parameters at the
end of the function call rather than keeping the code segment parameters
together or update the IA32 call site. Let me see which looks best. But
I'll likely update the IA32 call site no matter what with at least
comments about the parameters that aren't used, either way.

Thanks,
Tom


Thanks
Laszlo

Lendacky, Thomas
 

On 8/1/20 12:31 PM, Laszlo Ersek wrote:
On 07/31/20 23:38, Laszlo Ersek wrote:
On 07/31/20 16:47, Tom Lendacky wrote:
On 7/31/20 9:44 AM, Tom Lendacky wrote:
On 7/31/20 8:36 AM, Tom Lendacky wrote:
On 7/31/20 7:43 AM, Laszlo Ersek wrote:
Hi Tom,
Hi Laszlo,
Hi Laszlo,

Can you try this incremental patch to see if it fixes the issue you're
seeing? If it does, I'll merge it into patch #45 and send out a v14.
Looking at the formatting, I'm not sure if Thunderbird messed up the
diff. I'll send you another copy directly to you using git send-email
just in case.
I got the separate copy; I'll report back sometime next week.
The update works fine; IA32 OVMF boots OK with it.
Thanks for testing so quickly, Laszlo!

I agree with squashing the update into patch #45, but before sending
v14, maybe we should get some feedback for the MdeModulePkg patches too,
at long last. :/
Yup, I'll hold off on sending v14.

Thanks,
Tom

Thanks!
Laszlo


diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 7165bcf3124a..2c00d72ddefe 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -365,9 +365,9 @@ RelocateApLoop (
      MwaitSupport,

      CpuMpData->ApTargetCState,

      CpuMpData->PmCodeSegment,

-    CpuMpData->Pm16CodeSegment,

      StackStart - ProcessorNumber * AP_SAFE_STACK_SIZE,

      (UINTN) &mNumberToFinish,

+    CpuMpData->Pm16CodeSegment,

      CpuMpData->SevEsAPBuffer,

      CpuMpData->WakeupBuffer

      );

diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
index 309d53bf3b37..7e81d24aa60f 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
@@ -226,7 +226,10 @@ SwitchToRealProcStart:
  SwitchToRealProcEnd:

;-------------------------------------------------------------------------------------


-;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
TopOfApStack, CountTofinish);

+;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
TopOfApStack, CountTofinish, Pm16CodeSegment, SevEsAPJumpTable,
WakeupBuffer);

+;

+;  The last three parameters (Pm16CodeSegment, SevEsAPJumpTable and
WakeupBuffer) are

+;  specific to SEV-ES support and are not applicable on IA32.

;-------------------------------------------------------------------------------------


  global ASM_PFX(AsmRelocateApLoop)

  ASM_PFX(AsmRelocateApLoop):

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 267aa5201c50..02652eaae126 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -350,9 +350,9 @@ VOID
    IN BOOLEAN                 MwaitSupport,

    IN UINTN                   ApTargetCState,

    IN UINTN                   PmCodeSegment,

-  IN UINTN                   Pm16CodeSegment,

    IN UINTN                   TopOfApStack,

    IN UINTN                   NumberToFinish,

+  IN UINTN                   Pm16CodeSegment,

    IN UINTN                   SevEsAPJumpTable,

    IN UINTN                   WakeupBuffer

    );

diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 3b8ec477b8b3..5d30f35b201c 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -491,13 +491,13 @@ PM16Mode:
  SwitchToRealProcEnd:

;-------------------------------------------------------------------------------------


-;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
Pm16CodeSegment, TopOfApStack, CountTofinish, SevEsAPJumpTable,
WakeupBuffer);

+;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
TopOfApStack, CountTofinish, Pm16CodeSegment, SevEsAPJumpTable,
WakeupBuffer);

;-------------------------------------------------------------------------------------


  global ASM_PFX(AsmRelocateApLoop)

  ASM_PFX(AsmRelocateApLoop):

  AsmRelocateApLoopStart:

  BITS 64

-    cmp        qword [rsp + 56], 0

+    cmp        qword [rsp + 56], 0  ; SevEsAPJumpTable

      je         NoSevEs

      ;

@@ -539,16 +539,17 @@ BITS 64
  NoSevEs:

      cli                          ; Disable interrupt before
switching to 32-bit mode

-    mov        rax, [rsp + 48]   ; CountTofinish

+    mov        rax, [rsp + 40]   ; CountTofinish

      lock dec   dword [rax]       ; (*CountTofinish)--

+    mov        r10, [rsp + 48]   ; Pm16CodeSegment

      mov        rax, [rsp + 56]   ; SevEsAPJumpTable

      mov        rbx, [rsp + 64]   ; WakeupBuffer

-    mov        rsp, [rsp + 40]   ; TopOfApStack

+    mov        rsp, r9           ; TopOfApStack

      push       rax               ; Save SevEsAPJumpTable

      push       rbx               ; Save WakeupBuffer

-    push       r9                ; Save Pm16CodeSegment

+    push       r10               ; Save Pm16CodeSegment

      push       rcx               ; Save MwaitSupport

      push       rdx               ; Save ApTargetCState





On 07/30/20 20:43, Tom Lendacky wrote:
From: Tom Lendacky <thomas.lendacky@...>

BZ:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7Cb7e0f534fe77439befe908d83640c55f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637318999104802062&amp;sdata=32%2F36d1MHm4JorllRKyMz%2BmZaMfWceFsHK5PQA%2Fojqs%3D&amp;reserved=0



Before UEFI transfers control to the OS, it must park the AP. This is
done using the AsmRelocateApLoop function to transition into 32-bit
non-paging mode. For an SEV-ES guest, a few additional things must be
done:
    - AsmRelocateApLoop must be updated to support SEV-ES. This means
      performing a VMGEXIT AP Reset Hold instead of an MWAIT or HLT
loop.
    - Since the AP must transition to real mode, a small routine is
copied
      to the WakeupBuffer area. Since the WakeupBuffer will be used by
      the AP during OS booting, it must be placed in reserved memory.
      Additionally, the AP stack must be located where it can be
accessed
      in real mode.
    - Once the AP is in real mode it will transfer control to the
      destination specified by the OS in the SEV-ES AP Jump Table. The
      SEV-ES AP Jump Table address is saved by the hypervisor for
the OS
      using the GHCB VMGEXIT AP Jump Table exit code.

Cc: Eric Dong <eric.dong@...>
Cc: Ray Ni <ray.ni@...>
Cc: Laszlo Ersek <lersek@...>
Reviewed-by: Eric Dong <eric.dong@...>
Signed-off-by: Tom Lendacky <thomas.lendacky@...>
---
   UefiCpuPkg/Library/MpInitLib/MpLib.h          |   8 +-
   UefiCpuPkg/Library/MpInitLib/DxeMpLib.c       |  54 +++++++-
   UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 131
++++++++++++++++--
   3 files changed, 175 insertions(+), 18 deletions(-)
Now that this series is almost ready to merge, I've done a bit of
regression-testing.

Unfortunately, this patch breaks booting with IA32 OVMF.

More precisely, it breaks the IA32 version of DxeMpInitLib.
Yeah, that's not good.  I will look into this based on your input below.
What's strange is that my system doesn't hang and successfully boots all
APs (up to 64 is what I've tested with).

But, yes, both call sites should be the same and I will make that
change.


The symptom is that just when the OS would be launched, the
multiprocessor guest hangs. This is how the log terminates:

FSOpen: Open
'\370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\linux'
Success
[Security] 3rd party image[0] can be loaded after EndOfDxe:
PciRoot(0x0)/Pci(0x2,0x1)/Pci(0x0,0x0)/Scsi(0x0,0x0)/HD(1,GPT,D9F1FBA5-E5D3-440A-B6A7-87B593E4FAB1,0x800,0x100000)/\370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\linux.


InstallProtocolInterface: [EfiLoadedImageProtocol] 853A03A8
Loading driver at 0x00083E72000 EntryPoint=0x00083E76680
InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 853A0510
ProtectUefiImageCommon - 0x853A03A8
    - 0x0000000083E72000 - 0x0000000000E75000
FSOpen: Open
'370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\initrd'
Success
PixelBlueGreenRedReserved8BitPerColor
ConvertPages: range 400000 - 1274FFF covers multiple entries
SmmInstallProtocolInterface: [EdkiiSmmExitBootServicesProtocol] 0
CpuDxe: 5-Level Paging = 0
[HANG]
Meanwhile some guest CPUs are pegged.

Normally, when this series is not applied, the next log entry is (in
place of [HANG]):

MpInitChangeApLoopCallback() done!
I've identified this patch by bisection, after applying the series on
current master (137c2c6eff67, "Revert "BaseTools/PatchCheck.py: Add
LicenseCheck"", 2020-07-31).

Here's the bisection log:

git bisect start
# good: [137c2c6eff67f4750d77e8e40af6683c412d3ed0] Revert
"BaseTools/PatchCheck.py: Add LicenseCheck"
git bisect good 137c2c6eff67f4750d77e8e40af6683c412d3ed0
# bad: [d3f7971f4f70c9f39170b42af837e58e59435ad3] Maintainers.txt: Add
reviewers for the OvmfPkg SEV-related files
git bisect bad d3f7971f4f70c9f39170b42af837e58e59435ad3
# good: [9551e3fc61ba0c0ddf8e79b425a22aa7dd61cb8b] OvmfPkg/VmgExitLib:
Add support for RDTSCP NAE events
git bisect good 9551e3fc61ba0c0ddf8e79b425a22aa7dd61cb8b
# good: [10acf16b38522d8a1b538b3aa432daaa72c0e97b] OvmfPkg: Reserve a
page in memory for the SEV-ES usage
git bisect good 10acf16b38522d8a1b538b3aa432daaa72c0e97b
# good: [ccb4267e76b6474657c41bef7e76a980930c22ea] UefiCpuPkg: Add a
16-bit protected mode code segment descriptor
git bisect good ccb4267e76b6474657c41bef7e76a980930c22ea
# good: [94e238ae37505cfb081f3b9b4632067e4a113cf9] OvmfPkg: Use the
SEV-ES work area for the SEV-ES AP reset vector
git bisect good 94e238ae37505cfb081f3b9b4632067e4a113cf9
# bad: [16c21b9d10b032d66d4105dd4693fd9dc6e6ec18]
UefiCpuPkg/MpInitLib:
Prepare SEV-ES guest APs for OS use
git bisect bad 16c21b9d10b032d66d4105dd4693fd9dc6e6ec18
# good: [49855596e383ab2aa6410fa060e22d4817d8e64e] OvmfPkg: Move the
GHCB allocations into reserved memory
git bisect good 49855596e383ab2aa6410fa060e22d4817d8e64e
# first bad commit: [16c21b9d10b032d66d4105dd4693fd9dc6e6ec18]
UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use
So clearly we should be looking for an IA32-specific change, or
IA32-specific *omission*, in this patch, that could cause the problem.

The bug is the following:

On 07/30/20 20:43, Tom Lendacky wrote:

diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h
b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index b1a9d99cb3eb..267aa5201c50 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -349,8 +350,11 @@ VOID
     IN BOOLEAN                 MwaitSupport,
     IN UINTN                   ApTargetCState,
     IN UINTN                   PmCodeSegment,
+  IN UINTN                   Pm16CodeSegment,
     IN UINTN                   TopOfApStack,
-  IN UINTN                   NumberToFinish
+  IN UINTN                   NumberToFinish,
+  IN UINTN                   SevEsAPJumpTable,
+  IN UINTN                   WakeupBuffer
     );

   /**
(1) This hunk modifies the parameter list of functions pointed-to by
ASM_RELOCATE_AP_LOOP.

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index 9115ff9e3e30..7165bcf3124a 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -330,17 +350,26 @@ RelocateApLoop (
     BOOLEAN                MwaitSupport;
     ASM_RELOCATE_AP_LOOP   AsmRelocateApLoopFunc;
     UINTN                  ProcessorNumber;
+  UINTN                  StackStart;

     MpInitLibWhoAmI (&ProcessorNumber);
     CpuMpData    = GetCpuMpData ();
     MwaitSupport = IsMwaitSupport ();
+  if (CpuMpData->SevEsIsEnabled) {
+    StackStart = CpuMpData->SevEsAPResetStackStart;
+  } else {
+    StackStart = mReservedTopOfApStack;
+  }
     AsmRelocateApLoopFunc = (ASM_RELOCATE_AP_LOOP) (UINTN)
mReservedApLoopFunc;
     AsmRelocateApLoopFunc (
       MwaitSupport,
       CpuMpData->ApTargetCState,
       CpuMpData->PmCodeSegment,
-    mReservedTopOfApStack - ProcessorNumber * AP_SAFE_STACK_SIZE,
-    (UINTN) &mNumberToFinish
+    CpuMpData->Pm16CodeSegment,
+    StackStart - ProcessorNumber * AP_SAFE_STACK_SIZE,
+    (UINTN) &mNumberToFinish,
+    CpuMpData->SevEsAPBuffer,
+    CpuMpData->WakeupBuffer
       );
     //
     // It should never reach here
(2) This hunk modifies the call site, in accordance with the prototype
change at (1).

diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 6956b408d004..3b8ec477b8b3 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -465,6 +465,10 @@ BITS 16
       ;     - IP for Real Mode (two bytes)
       ;     - CS for Real Mode (two bytes)
       ;
+    ; This label is also used with AsmRelocateApLoop. During MP
finalization,
+    ; the code from PM16Mode to SwitchToRealProcEnd is copied to the
start of
+    ; the WakeupBuffer, allowing a parked AP to be booted by an OS.
+    ;
   PM16Mode:
       mov        eax, cr0                    ; Read CR0
       btr        eax, 0                      ; Set PE=0
@@ -487,32 +491,95 @@ PM16Mode:
   SwitchToRealProcEnd:

;-------------------------------------------------------------------------------------


-;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
TopOfApStack, CountTofinish);
+;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment,
Pm16CodeSegment, TopOfApStack, CountTofinish, SevEsAPJumpTable,
WakeupBuffer);
;-------------------------------------------------------------------------------------


   global ASM_PFX(AsmRelocateApLoop)
   ASM_PFX(AsmRelocateApLoop):
   AsmRelocateApLoopStart:
   BITS 64
(3) Unfortunately, the patch only adapts the X64 implementation of the
AsmRelocateApLoopStart() function to the new prototype; the IA32
implementation no longer matches the call site.

(I'm not sure if the intent was for the IA32 version to simply ignore
the new parameters, but even in that case, the "Pm16CodeSegment"
parameter is inserted in the middle of the parameter list, likely
offsetting the rest.)

The problem is foreshadowed even by hunk (2). Namely, in hunk (2), the

    s/mReservedTopOfApStack/StackStart/

replacement is *more difficult* to verify than necessary -- exactly
because "CpuMpData->Pm16CodeSegment" is inserted *before* it.
I can do one of two things here and just put the 3 new parameters at the
end of the function call rather than keeping the code segment parameters
together or update the IA32 call site. Let me see which looks best. But
I'll likely update the IA32 call site no matter what with at least
comments about the parameters that aren't used, either way.

Thanks,
Tom


Thanks
Laszlo