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

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

Hi Tom,

On 05/01/20 22:17, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2340
>
> 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://bugzilla.tianocore.org/show_bug.cgi?id=2340#c0>, 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.

(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.

>    FILE_GUID                      = CA4BBC99-DFC6-4234-B553-8B6586B7B113

(3) Please generate a new FILE_GUID with "uuidgen".

>    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.

>
>  [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