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

["Laszlo Ersek" <lersek@redhat.com>]

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@amd.com>
>>>>>
>>>>> 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@intel.com>
>>>>> Cc: Ray Ni <ray.ni@intel.com>
>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>>> Reviewed-by: Eric Dong <eric.dong@intel.com>
>>>>> Signed-off-by: Tom Lendacky <thomas.lendacky@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
>>>>
>