[edk2-devel] [PATCH v13 45/46] UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use
Lendacky, Thomas
thomas.lendacky at amd.com
Fri Jul 31 14:47:46 UTC 2020
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 at 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.dong at intel.com>
>>>> Cc: Ray Ni <ray.ni at intel.com>
>>>> Cc: Laszlo Ersek <lersek at redhat.com>
>>>> Reviewed-by: Eric Dong <eric.dong at intel.com>
>>>> Signed-off-by: Tom Lendacky <thomas.lendacky at 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 (#63576): https://edk2.groups.io/g/devel/message/63576
Mute This Topic: https://groups.io/mt/75895009/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list