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.lenda...@amd.com> >>>>> >>>>> BZ: >>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&data=02%7C01%7Cthomas.lendacky%40amd.com%7Cb8c77cf296c949d2bbd808d8354f542b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637317962138028351&sdata=HISHZmLjOc%2FfgVrBm8MlNeDAk453NJ64%2B51bETZj4rk%3D&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.d...@intel.com> >>>>> Cc: Ray Ni <ray...@intel.com> >>>>> Cc: Laszlo Ersek <ler...@redhat.com> >>>>> Reviewed-by: Eric Dong <eric.d...@intel.com> >>>>> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com> >>>>> --- >>>>> 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 >>>> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#63600): https://edk2.groups.io/g/devel/message/63600 Mute This Topic: https://groups.io/mt/75895009/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
