Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 5/5/20 4:39 PM, Laszlo Ersek wrote:
> Hi Tom,
> 
> On 05/01/20 22:17, Lendacky, Thomas wrote:
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2340&data=02%7C01%7Cthomas.lendacky%40amd.com%7C4d398c73a4bc4674d36608d7f13cce1d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243115777198329&sdata=YA9qmWhZDrwGCchGXKDmOQPFqXdDztokQSZeZIq6u18%3D&reserved=0
>>
>> Commit 2db0ccc2d7fe ("UefiCpuPkg: Update CpuExceptionHandlerLib pass
>> XCODE5 tool chain") introduced binary patching into the exception handling
>> support. CPU exception handling is allowed during SEC and this results in
>> binary patching of flash, which should not be done.
>>
>> Separate the changes from commit 2db0ccc2d7fe into an XCODE5 toolchain
>> specific file, Xcode5ExceptionHandlerAsm.nasm, and create new INF files
>> for an XCODE5 version of CpuExceptionHandlerLib. Update the UefiCpuPkg.dsc
>> file to use the new files when the XCODE5 toolchain is used.
>>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>   UefiCpuPkg/UefiCpuPkg.dsc                     |  23 +
>>   .../Xcode5DxeCpuExceptionHandlerLib.inf       |  64 +++
>>   .../Xcode5PeiCpuExceptionHandlerLib.inf       |  63 +++
>>   .../Xcode5SecPeiCpuExceptionHandlerLib.inf    |  55 +++
>>   .../Xcode5SmmCpuExceptionHandlerLib.inf       |  59 +++
> 
> I don't think that paralleling all the existent INF files is necessary
> for XCODE5.
> 
> The binary patching is a problem when the UEFI module containing the
> self-patching CpuExceptionHandlerLib instance executes in-place from
> flash. That applies to: (a) SEC modules, (b) PEI modules that do *not*
> have a DEPEX on "gEfiPeiMemoryDiscoveredPpiGuid". (PEIMs that do have a
> DEPEX on that PPI GUID are only dispatched after the permanent PEI RAM
> has been discovered / published, so they run out of normal RAM.)
> 
> Reviewing the existent INF files, we have:
> 
> - DxeCpuExceptionHandlerLib.inf: for DXE_CORE, DXE_DRIVER,
> UEFI_APPLICATION modules. Self-patching is fine.
> 
> - SmmCpuExceptionHandlerLib.inf: for DXE_SMM_DRIVER modules.
> Self-patching is fine.
> 
> - SecPeiCpuExceptionHandlerLib.inf: SEC is listed explicitly, so here we
> certainly need an alternative.
> 
> - PeiCpuExceptionHandlerLib.inf: unfortunately, the differences of this
> library instance with "SecPeiCpuExceptionHandlerLib.inf" is not obvious;
> only SEC's absence is easily visible.
> 
> If we look at the commit that introduced this lib instance
> (a81abf161666, "UefiCpuPkg/ExceptionLib: Import
> PeiCpuExceptionHandlerLib module", 2016-06-01), we find:
> 
>>      This module could be linked by CpuMpPei driver to handle reserved vector list
>>      and provide spin lock for BSP/APs to prevent dump message corrupted.
> 
> So the library was added explicitly for CpuMpPei's sake -- which looks
> promising, because CpuMpPei certainly depends on
> "gEfiPeiMemoryDiscoveredPpiGuid", as it needs a bunch of RAM for
> offering the multi-processing PPI. That suggests the self-patching is OK
> in "PeiCpuExceptionHandlerLib.inf" too.
> 
> The CpuMpPei DEPEX in question was replaced with a PPI notify callback
> in commit 0a0d5296e448 ("UefiCpuPkg/CpuMpPei: support stack guard
> feature", 2018-09-10). This would be a problem if the self-patching in
> the PeiCpuExceptionHandlerLib instance occurred in the library
> constructor, because the CpuMpPei can now actually be dispatched before
> permanent PEI RAM is available -- and the constructor would run
> immediately.
> 
> Luckily, the lib instance has no CONSTRUCTOR at all, and CpuMpPei calls
> InitializeCpuExceptionHandlers() explicitly in InitializeCpuMpWorker(),
> which is the PPI notify in question. (And per
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2340%23c0&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7C4d398c73a4bc4674d36608d7f13cce1d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243115777198329&amp;sdata=1ucjYymvr7VAF2d2QGyCxQhE8hGe8AFMaW8EYbUUkdM%3D&amp;reserved=0>, the
> self-patching occurs in InitializeCpuExceptionHandlers().)
> 
> Therefore, having two variants of PeiCpuExceptionHandlerLib.inf is also
> unnecessary.
> 
> (1) We only need two variants for "SecPeiCpuExceptionHandlerLib.inf", in
> my opinion.

Ok, I'll rework it to have variants for just SecPeiCpuExceptionHandlerLib.

> 
> (Note: if we check OVMF, we see that PeiCpuExceptionHandlerLib.inf is
> used universally for PEIMs. That's because OVMF is special -- its PEI
> phase runs entirely out of RAM. See also commit f0e6a56a9a2f, "OvmfPkg:
> include UefiCpuPkg/CpuMpPei", 2016-07-15.)
> 
> --*--
> 
> With this patch applied:
> 
> $ diff -u \
>            SecPeiCpuExceptionHandlerLib.inf \
>      Xcode5SecPeiCpuExceptionHandlerLib.inf
> 
>> --- SecPeiCpuExceptionHandlerLib.inf    2020-05-05 18:36:12.813156743 +0200
>> +++ Xcode5SecPeiCpuExceptionHandlerLib.inf      2020-05-05 23:25:24.578572971 +0200
>> @@ -8,7 +8,7 @@
>>
>>   [Defines]
>>     INF_VERSION                    = 0x00010005
>> -  BASE_NAME                      = SecPeiCpuExceptionHandlerLib
>> +  BASE_NAME                      = Xcode5SecPeiCpuExceptionHandlerLib
> 
> OK
> 
>>     MODULE_UNI_FILE                = SecPeiCpuExceptionHandlerLib.uni
> 
> (2) We'll need a separate UNI file here -- also we should customize the
> file-top comment in the INF file -- that explains the difference between
> the XCODE5 and non-XCODE5 variants, briefly.

Ok, will do.

> 
>>     FILE_GUID                      = CA4BBC99-DFC6-4234-B553-8B6586B7B113
> 
> (3) Please generate a new FILE_GUID with "uuidgen".

Ok, thanks, that answers my question that I asked in another email.

> 
>>     MODULE_TYPE                    = PEIM
>> @@ -26,16 +26,20 @@
>>     Ia32/ExceptionTssEntryAsm.nasm
>>     Ia32/ArchExceptionHandler.c
>>     Ia32/ArchInterruptDefs.h
>> +  Ia32/ArchAMDSevVcHandler.c
> 
> (4) Even though the blurb says that this series is based on edk2 commit
> e54310451f1a, some SEV-ES specific parts remain in this patch, and
> should be eliminated. The first example is above.

Ugh. I thought I had that all cleaned up before sending. My bad, I'll fix 
that in the next version.

Thanks,
Tom

> 
>>
>>   [Sources.X64]
>> -  X64/ExceptionHandlerAsm.nasm
>> +  X64/Xcode5ExceptionHandlerAsm.nasm
>>     X64/ArchExceptionHandler.c
>>     X64/ArchInterruptDefs.h
>> +  X64/ArchAMDSevVcHandler.c
> 
> (5) Another SEV-ES change.
> 
>>
>>   [Sources.common]
>>     CpuExceptionCommon.h
>>     CpuExceptionCommon.c
>>     SecPeiCpuException.c
>> +  AMDSevVcHandler.c
>> +  AMDSevVcCommon.h
> 
> (6) ditto
> 
>>
>>   [Packages]
>>     MdePkg/MdePkg.dec
>> @@ -48,3 +52,4 @@
>>     PrintLib
>>     LocalApicLib
>>     PeCoffGetEntryPointLib
>> +  VmgExitLib
> 
> (7) ditto
> 
> Furthermore:
> 
> $ diff -u \
>            ExceptionHandlerAsm.nasm \
>      Xcode5ExceptionHandlerAsm.nasm
> 
>> --- ExceptionHandlerAsm.nasm    2020-05-05 23:26:30.941784203 +0200
>> +++ Xcode5ExceptionHandlerAsm.nasm      2020-05-05 23:25:24.578572971 +0200
>> @@ -18,6 +18,8 @@
>>   ; CommonExceptionHandler()
>>   ;
>>
>> +%define VC_EXCEPTION 29
>> +
>>   extern ASM_PFX(mErrorCodeFlag)    ; Error code flags for exceptions
>>   extern ASM_PFX(mDoFarReturnFlag)  ; Do far return flag
>>   extern ASM_PFX(CommonExceptionHandler)
>> @@ -225,6 +227,9 @@
>>       push    rax
>>
>>   ;; UINT64  Dr0, Dr1, Dr2, Dr3, Dr6, Dr7;
>> +    cmp     qword [rbp + 8], VC_EXCEPTION
>> +    je      VcDebugRegs          ; For SEV-ES (#VC) Debug registers ignored
>> +
>>       mov     rax, dr7
>>       push    rax
>>       mov     rax, dr6
>> @@ -237,7 +242,19 @@
>>       push    rax
>>       mov     rax, dr0
>>       push    rax
>> +    jmp     DrFinish
>> +
>> +VcDebugRegs:
>> +;; UINT64  Dr0, Dr1, Dr2, Dr3, Dr6, Dr7 are skipped for #VC to avoid exception recursion
>> +    xor     rax, rax
>> +    push    rax
>> +    push    rax
>> +    push    rax
>> +    push    rax
>> +    push    rax
>> +    push    rax
>>
>> +DrFinish:
>>   ;; FX_SAVE_STATE_X64 FxSaveState;
>>       sub rsp, 512
>>       mov rdi, rsp
> 
> (8) All of these should be removed -- they should be part of your SEV-ES
> series, on top of this set.
> 
> Thanks,
> Laszlo
>