Re: [edk2-devel] [PATCH v13 45/46] UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use
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) , + 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 +
Re: [edk2-devel] [PATCH v13 45/46] UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use
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) , >>> >>> + 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
Re: [edk2-devel] [PATCH v13 45/46] UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use
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) , >> >> + 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
Re: [edk2-devel] [PATCH v13 45/46] UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use
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) , +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 -cmpqword [rsp + 56], 0 +cmpqword [rsp + 56], 0 ; SevEsAPJumpTable je NoSevEs ; @@ -539,16 +539,17 @@ BITS 64 NoSevEs: cli ; Disable interrupt before switching to 32-bit mode -movrax, [rsp + 48] ; CountTofinish +movrax, [rsp + 40] ; CountTofinish lock dec dword [rax] ; (*CountTofinish)-- +movr10, [rsp + 48] ; Pm16CodeSegment movrax, [rsp + 56] ; SevEsAPJumpTable movrbx, [rsp + 64] ; WakeupBuffer -movrsp, [rsp + 40] ; TopOfApStack +movrsp, 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 BZ:
Re: [edk2-devel] [PATCH v13 45/46] UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use
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) , +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 -cmpqword [rsp + 56], 0 +cmpqword [rsp + 56], 0 ; SevEsAPJumpTable je NoSevEs ; @@ -539,16 +539,17 @@ BITS 64 NoSevEs: cli ; Disable interrupt before switching to 32-bit mode -movrax, [rsp + 48] ; CountTofinish +movrax, [rsp + 40] ; CountTofinish lock dec dword [rax] ; (*CountTofinish)-- +movr10, [rsp + 48] ; Pm16CodeSegment movrax, [rsp + 56] ; SevEsAPJumpTable movrbx, [rsp + 64] ; WakeupBuffer -movrsp, [rsp + 40] ; TopOfApStack +movrsp, 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 >>> >>> BZ: >>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198data=02%7C01%7Cthomas.lendacky%40amd.com%7Cb8c77cf296c949d2bbd808d8354f542b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637317962138028351sdata=HISHZmLjOc%2FfgVrBm8MlNeDAk453NJ64%2B51bETZj4rk%3Dreserved=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
Re: [edk2-devel] [PATCH v13 45/46] UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use
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 BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198data=02%7C01%7Cthomas.lendacky%40amd.com%7Cb8c77cf296c949d2bbd808d8354f542b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637317962138028351sdata=HISHZmLjOc%2FfgVrBm8MlNeDAk453NJ64%2B51bETZj4rk%3Dreserved=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 Cc: Ray Ni Cc: Laszlo Ersek Reviewed-by: Eric Dong Signed-off-by: Tom 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,0x10)/\370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\linux. InstallProtocolInterface: [EfiLoadedImageProtocol] 853A03A8 Loading driver at 0x00083E72000 EntryPoint=0x00083E76680 InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 853A0510 ProtectUefiImageCommon - 0x853A03A8 - 0x83E72000 - 0x00E75000 FSOpen: Open '370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\initrd' Success PixelBlueGreenRedReserved8BitPerColor ConvertPages: range 40 - 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:
Re: [edk2-devel] [PATCH v13 45/46] UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use
Hi Tom, On 07/30/20 20:43, Tom Lendacky wrote: > From: Tom 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 > Cc: Ray Ni > Cc: Laszlo Ersek > Reviewed-by: Eric Dong > Signed-off-by: Tom 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,0x10)/\370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\linux. > InstallProtocolInterface: [EfiLoadedImageProtocol] 853A03A8 > Loading driver at 0x00083E72000 EntryPoint=0x00083E76680 > InstallProtocolInterface: [EfiLoadedImageDevicePathProtocol] 853A0510 > ProtectUefiImageCommon - 0x853A03A8 > - 0x83E72000 - 0x00E75000 > FSOpen: Open '370ac550dcaa48b88f1ca75ad903b0e7\4.16.7-100.fc26.i686\initrd' > Success > PixelBlueGreenRedReserved8BitPerColor > ConvertPages: range 40 - 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 > @@
[edk2-devel] [PATCH v13 45/46] UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use
From: Tom 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
Cc: Ray Ni
Cc: Laszlo Ersek
Reviewed-by: Eric Dong
Signed-off-by: Tom 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
#include
#include
+#include
#include
#include
@@ -85,6 +86,13 @@ GetWakeupBuffer (
{
EFI_STATUS Status;
EFI_PHYSICAL_ADDRESSStartAddress;
+ 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),
);
@@ -159,8 +167,10 @@ GetSevEsAPMemory (
VOID
)
{
- EFI_STATUSStatus;
- EFI_PHYSICAL_ADDRESS StartAddress;
+ EFI_STATUSStatus;
+ 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 (
BOOLEANMwaitSupport;
ASM_RELOCATE_AP_LOOP AsmRelocateApLoopFunc;
UINTN ProcessorNumber;
+ UINTN StackStart;
MpInitLibWhoAmI ();
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)
+CpuMpData->Pm16CodeSegment,
+StackStart - ProcessorNumber * AP_SAFE_STACK_SIZE,
+(UINTN) ,
+CpuMpData->SevEsAPBuffer,
+CpuMpData->WakeupBuffer
);
//
// It should never reach here
@@ -374,6 +403,21 @@ MpInitChangeApLoopCallback (
while (mNumberToFinish > 0) {
CpuPause ();
}
+
+
