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.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 (#63622): https://edk2.groups.io/g/devel/message/63622 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] -=-=-=-=-=-=-=-=-=-=-=-
