[PATCH v2 0/3] XCODE5 toolchain binary patching fix

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [PATCH v2 0/3] XCODE5 toolchain binary patching fix
@ 2020-05-06 16:32 Lendacky, Thomas
  2020-05-06 16:33 ` [PATCH v2 1/3] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific Lendacky, Thomas
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Lendacky, Thomas @ 2020-05-06 16:32 UTC (permalink / raw)
  To: devel
  Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Liming Gao,
	Eric Dong, Ray Ni, Anthony Perard, Julien Grall, Brijesh Singh,
	Andrew Fish


BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2340

Commit 2db0ccc2d7fe ("UefiCpuPkg: Update CpuExceptionHandlerLib pass
XCODE5 tool chain") introduced binary patching in the
ExceptionHandlerAsm.nasm in order to support the XCODE5 toolchain.
However, the CpuExceptionHandlerLib can be used during SEC phase which
would result in binary patching of flash.

This series creates a new CpuExceptionHandlerLib file to support
the required binary patching for the XCODE5 toolchain, while reverting
the changes from commit 2db0ccc2d7fe in the standard file. As the Pei,
Dxe and SMM versions of the library operate in memory (as opposed to
flash), only the SEC/PEI version is of the library is updated to use the
version of the ExceptionHandlerAsm.nasm that does not perform binary
patching.

This is accomplished in phases:
  - Create a new XCODE5 specific version of the ExceptionHandlerAsm.nasm
    file and update all CpuExceptionHandler INF files to use it while also
    creating a new SEC/PEI CpuExceptionHandler INF file specifically for
    the XCODE5 toolchain.
  - Update all package DSC files that use the SecPeiCpuExceptionHandlerLib
    version of the library to use the XCODE5 version of the library,
    Xcode5SecPeiCpuExceptionHandlerLib, when the XCODE5 toolchain is used.
  - Revert the changes made by commit 2db0ccc2d7fe in the standard file
    and update the SecPeiCpuExceptionHandlerLib.inf file to use the
    standard file.

I don't have access to an XCODE5 toolchain setup, so I have not tested
this with XCODE5. I would like to request that someone who does please
test this.

Also, will this change have an impact on any of the platform builds
outside of this tree? In other words, should the new INF be the one that
uses the reverted ExceptionHandlerAsm.nasm file and it be called something
like BaseSecPeiCpuExceptionHandlerLib.inf?

---

These patches are based on commit:
befd18fca68b ("EmbeddedPkg/EmbeddedPkg.dsc: remove some stale component references")

Cc: Andrew Fish <afish@apple.com>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien@xen.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Ray Ni <ray.ni@intel.com>

Changes since v1:
- Only apply the revert to the Sec/Pei CpuExceptionHandler library and
  leave the Pei, Dxe and Smm versions using the binary patching version.
- Generate a unique file GUID for the new library INF file and create
  a corresponding UNI file.
- Remove any references to SEV-ES (original patches accidentally submitted
  from wrong tree).

Tom Lendacky (3):
  UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific
  OvmfPkg: Use toolchain appropriate CpuExceptionHandlerLib
  UefiCpuPkg/CpuExceptionHandler: Revert CpuExceptionHandler binary
    patching

 OvmfPkg/OvmfPkgIa32.dsc                       |   4 +
 OvmfPkg/OvmfPkgIa32X64.dsc                    |   4 +
 OvmfPkg/OvmfPkgX64.dsc                        |   4 +
 OvmfPkg/OvmfXen.dsc                           |   4 +
 UefiCpuPkg/UefiCpuPkg.dsc                     |   5 +
 .../DxeCpuExceptionHandlerLib.inf             |   2 +-
 .../PeiCpuExceptionHandlerLib.inf             |   2 +-
 .../SmmCpuExceptionHandlerLib.inf             |   2 +-
 .../Xcode5SecPeiCpuExceptionHandlerLib.inf    |  54 +++
 .../X64/ExceptionHandlerAsm.nasm              |  25 +-
 .../X64/Xcode5ExceptionHandlerAsm.nasm        | 396 ++++++++++++++++++
 .../Xcode5SecPeiCpuExceptionHandlerLib.uni    |  17 +
 12 files changed, 497 insertions(+), 22 deletions(-)
 create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
 create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
 create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni

-- 
2.17.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/3] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific
  2020-05-06 16:32 [PATCH v2 0/3] XCODE5 toolchain binary patching fix Lendacky, Thomas
@ 2020-05-06 16:33 ` Lendacky, Thomas
  2020-05-06 19:01   ` Laszlo Ersek
  2020-05-06 16:33 ` [PATCH v2 2/3] OvmfPkg: Use toolchain appropriate CpuExceptionHandlerLib Lendacky, Thomas
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Lendacky, Thomas @ 2020-05-06 16:33 UTC (permalink / raw)
  To: devel
  Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Liming Gao,
	Eric Dong, Ray Ni, Anthony Perard, Julien Grall, Brijesh Singh,
	Andrew Fish

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 a new SEC INF
file for the XCODE5 version of CpuExceptionHandlerLib.

Since binary patching is allowed when running outside of flash, switch
the Dxe, Pei and Smm versions of the CpuExceptionHandlerLib over to use
the Xcode5ExceptionHandlerAsm.nasm file to retain current functionality.

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                     |   5 +
 .../DxeCpuExceptionHandlerLib.inf             |   2 +-
 .../PeiCpuExceptionHandlerLib.inf             |   2 +-
 .../SmmCpuExceptionHandlerLib.inf             |   2 +-
 .../Xcode5SecPeiCpuExceptionHandlerLib.inf    |  54 +++
 .../X64/Xcode5ExceptionHandlerAsm.nasm        | 396 ++++++++++++++++++
 .../Xcode5SecPeiCpuExceptionHandlerLib.uni    |  17 +
 7 files changed, 475 insertions(+), 3 deletions(-)
 create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
 create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
 create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni

diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc
index d28cb5cccb52..264e5a787bce 100644
--- a/UefiCpuPkg/UefiCpuPkg.dsc
+++ b/UefiCpuPkg/UefiCpuPkg.dsc
@@ -59,7 +59,11 @@ [LibraryClasses]
 
 [LibraryClasses.common.SEC]
   PlatformSecLib|UefiCpuPkg/Library/PlatformSecLibNull/PlatformSecLibNull.inf
+!if $(TOOL_CHAIN_TAG) == "XCODE5"
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
+!else
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
+!endif
   HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
   PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
   MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
@@ -126,6 +130,7 @@ [Components.IA32, Components.X64]
   UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
   UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
   UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
+  UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
   UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
   UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
   UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
index e41383573043..61e2ec30b089 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
@@ -28,7 +28,7 @@ [Sources.Ia32]
   Ia32/ArchInterruptDefs.h
 
 [Sources.X64]
-  X64/ExceptionHandlerAsm.nasm
+  X64/Xcode5ExceptionHandlerAsm.nasm
   X64/ArchExceptionHandler.c
   X64/ArchInterruptDefs.h
 
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
index f31423ac0f91..093374944df6 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
@@ -28,7 +28,7 @@ [Sources.Ia32]
   Ia32/ArchInterruptDefs.h
 
 [Sources.X64]
-  X64/ExceptionHandlerAsm.nasm
+  X64/Xcode5ExceptionHandlerAsm.nasm
   X64/ArchExceptionHandler.c
   X64/ArchInterruptDefs.h
 
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
index 66c7f59e3c91..2ffbbccc302f 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
@@ -28,7 +28,7 @@ [Sources.Ia32]
   Ia32/ArchInterruptDefs.h
 
 [Sources.X64]
-  X64/ExceptionHandlerAsm.nasm
+  X64/Xcode5ExceptionHandlerAsm.nasm
   X64/ArchExceptionHandler.c
   X64/ArchInterruptDefs.h
 
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
new file mode 100644
index 000000000000..3ed1378d6fa6
--- /dev/null
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
@@ -0,0 +1,54 @@
+## @file
+#  CPU Exception Handler library instance for SEC/PEI modules.
+#
+#  Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.<BR>
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+#  This is the XCODE5 variant of the SEC/PEI CpuExceptionHandlerLib. This
+#  variant performs binary patching to fix up addresses that allow the
+#  XCODE5 toolchain to be used.
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = Xcode5SecPeiCpuExceptionHandlerLib
+  MODULE_UNI_FILE                = Xcode5SecPeiCpuExceptionHandlerLib.uni
+  FILE_GUID                      = 49C481AF-1621-42F3-8FA1-27C64143E304
+  MODULE_TYPE                    = PEIM
+  VERSION_STRING                 = 1.1
+  LIBRARY_CLASS                  = CpuExceptionHandlerLib|SEC PEI_CORE PEIM
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+#  VALID_ARCHITECTURES           = IA32 X64
+#
+
+[Sources.Ia32]
+  Ia32/ExceptionHandlerAsm.nasm
+  Ia32/ExceptionTssEntryAsm.nasm
+  Ia32/ArchExceptionHandler.c
+  Ia32/ArchInterruptDefs.h
+
+[Sources.X64]
+  X64/Xcode5ExceptionHandlerAsm.nasm
+  X64/ArchExceptionHandler.c
+  X64/ArchInterruptDefs.h
+
+[Sources.common]
+  CpuExceptionCommon.h
+  CpuExceptionCommon.c
+  SecPeiCpuException.c
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  UefiCpuPkg/UefiCpuPkg.dec
+
+[LibraryClasses]
+  BaseLib
+  SerialPortLib
+  PrintLib
+  LocalApicLib
+  PeCoffGetEntryPointLib
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
new file mode 100644
index 000000000000..19198f273137
--- /dev/null
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
@@ -0,0 +1,396 @@
+;------------------------------------------------------------------------------ ;
+; Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+; Module Name:
+;
+;   ExceptionHandlerAsm.Asm
+;
+; Abstract:
+;
+;   x64 CPU Exception Handler
+;
+; Notes:
+;
+;------------------------------------------------------------------------------
+
+;
+; CommonExceptionHandler()
+;
+
+extern ASM_PFX(mErrorCodeFlag)    ; Error code flags for exceptions
+extern ASM_PFX(mDoFarReturnFlag)  ; Do far return flag
+extern ASM_PFX(CommonExceptionHandler)
+
+SECTION .data
+
+DEFAULT REL
+SECTION .text
+
+ALIGN   8
+
+AsmIdtVectorBegin:
+%rep  32
+    db      0x6a        ; push  #VectorNum
+    db      ($ - AsmIdtVectorBegin) / ((AsmIdtVectorEnd - AsmIdtVectorBegin) / 32) ; VectorNum
+    push    rax
+    mov     rax, strict qword 0 ;    mov     rax, ASM_PFX(CommonInterruptEntry)
+    jmp     rax
+%endrep
+AsmIdtVectorEnd:
+
+HookAfterStubHeaderBegin:
+    db      0x6a        ; push
+@VectorNum:
+    db      0          ; 0 will be fixed
+    push    rax
+    mov     rax, strict qword 0 ;     mov     rax, HookAfterStubHeaderEnd
+JmpAbsoluteAddress:
+    jmp     rax
+HookAfterStubHeaderEnd:
+    mov     rax, rsp
+    and     sp,  0xfff0        ; make sure 16-byte aligned for exception context
+    sub     rsp, 0x18           ; reserve room for filling exception data later
+    push    rcx
+    mov     rcx, [rax + 8]
+    bt      [ASM_PFX(mErrorCodeFlag)], ecx
+    jnc     .0
+    push    qword [rsp]             ; push additional rcx to make stack alignment
+.0:
+    xchg    rcx, [rsp]        ; restore rcx, save Exception Number in stack
+    push    qword [rax]             ; push rax into stack to keep code consistence
+
+;---------------------------------------;
+; CommonInterruptEntry                  ;
+;---------------------------------------;
+; The follow algorithm is used for the common interrupt routine.
+; Entry from each interrupt with a push eax and eax=interrupt number
+; Stack frame would be as follows as specified in IA32 manuals:
+;
+; +---------------------+ <-- 16-byte aligned ensured by processor
+; +    Old SS           +
+; +---------------------+
+; +    Old RSP          +
+; +---------------------+
+; +    RFlags           +
+; +---------------------+
+; +    CS               +
+; +---------------------+
+; +    RIP              +
+; +---------------------+
+; +    Error Code       +
+; +---------------------+
+; +   Vector Number     +
+; +---------------------+
+; +    RBP              +
+; +---------------------+ <-- RBP, 16-byte aligned
+; The follow algorithm is used for the common interrupt routine.
+global ASM_PFX(CommonInterruptEntry)
+ASM_PFX(CommonInterruptEntry):
+    cli
+    pop     rax
+    ;
+    ; All interrupt handlers are invoked through interrupt gates, so
+    ; IF flag automatically cleared at the entry point
+    ;
+    xchg    rcx, [rsp]      ; Save rcx into stack and save vector number into rcx
+    and     rcx, 0xFF
+    cmp     ecx, 32         ; Intel reserved vector for exceptions?
+    jae     NoErrorCode
+    bt      [ASM_PFX(mErrorCodeFlag)], ecx
+    jc      HasErrorCode
+
+NoErrorCode:
+
+    ;
+    ; Push a dummy error code on the stack
+    ; to maintain coherent stack map
+    ;
+    push    qword [rsp]
+    mov     qword [rsp + 8], 0
+HasErrorCode:
+    push    rbp
+    mov     rbp, rsp
+    push    0             ; clear EXCEPTION_HANDLER_CONTEXT.OldIdtHandler
+    push    0             ; clear EXCEPTION_HANDLER_CONTEXT.ExceptionDataFlag
+
+    ;
+    ; Stack:
+    ; +---------------------+ <-- 16-byte aligned ensured by processor
+    ; +    Old SS           +
+    ; +---------------------+
+    ; +    Old RSP          +
+    ; +---------------------+
+    ; +    RFlags           +
+    ; +---------------------+
+    ; +    CS               +
+    ; +---------------------+
+    ; +    RIP              +
+    ; +---------------------+
+    ; +    Error Code       +
+    ; +---------------------+
+    ; + RCX / Vector Number +
+    ; +---------------------+
+    ; +    RBP              +
+    ; +---------------------+ <-- RBP, 16-byte aligned
+    ;
+
+    ;
+    ; Since here the stack pointer is 16-byte aligned, so
+    ; EFI_FX_SAVE_STATE_X64 of EFI_SYSTEM_CONTEXT_x64
+    ; is 16-byte aligned
+    ;
+
+;; UINT64  Rdi, Rsi, Rbp, Rsp, Rbx, Rdx, Rcx, Rax;
+;; UINT64  R8, R9, R10, R11, R12, R13, R14, R15;
+    push r15
+    push r14
+    push r13
+    push r12
+    push r11
+    push r10
+    push r9
+    push r8
+    push rax
+    push qword [rbp + 8]   ; RCX
+    push rdx
+    push rbx
+    push qword [rbp + 48]  ; RSP
+    push qword [rbp]       ; RBP
+    push rsi
+    push rdi
+
+;; UINT64  Gs, Fs, Es, Ds, Cs, Ss;  insure high 16 bits of each is zero
+    movzx   rax, word [rbp + 56]
+    push    rax                      ; for ss
+    movzx   rax, word [rbp + 32]
+    push    rax                      ; for cs
+    mov     rax, ds
+    push    rax
+    mov     rax, es
+    push    rax
+    mov     rax, fs
+    push    rax
+    mov     rax, gs
+    push    rax
+
+    mov     [rbp + 8], rcx               ; save vector number
+
+;; UINT64  Rip;
+    push    qword [rbp + 24]
+
+;; UINT64  Gdtr[2], Idtr[2];
+    xor     rax, rax
+    push    rax
+    push    rax
+    sidt    [rsp]
+    mov     bx, word [rsp]
+    mov     rax, qword [rsp + 2]
+    mov     qword [rsp], rax
+    mov     word [rsp + 8], bx
+
+    xor     rax, rax
+    push    rax
+    push    rax
+    sgdt    [rsp]
+    mov     bx, word [rsp]
+    mov     rax, qword [rsp + 2]
+    mov     qword [rsp], rax
+    mov     word [rsp + 8], bx
+
+;; UINT64  Ldtr, Tr;
+    xor     rax, rax
+    str     ax
+    push    rax
+    sldt    ax
+    push    rax
+
+;; UINT64  RFlags;
+    push    qword [rbp + 40]
+
+;; UINT64  Cr0, Cr1, Cr2, Cr3, Cr4, Cr8;
+    mov     rax, cr8
+    push    rax
+    mov     rax, cr4
+    or      rax, 0x208
+    mov     cr4, rax
+    push    rax
+    mov     rax, cr3
+    push    rax
+    mov     rax, cr2
+    push    rax
+    xor     rax, rax
+    push    rax
+    mov     rax, cr0
+    push    rax
+
+;; UINT64  Dr0, Dr1, Dr2, Dr3, Dr6, Dr7;
+    mov     rax, dr7
+    push    rax
+    mov     rax, dr6
+    push    rax
+    mov     rax, dr3
+    push    rax
+    mov     rax, dr2
+    push    rax
+    mov     rax, dr1
+    push    rax
+    mov     rax, dr0
+    push    rax
+
+;; FX_SAVE_STATE_X64 FxSaveState;
+    sub rsp, 512
+    mov rdi, rsp
+    db 0xf, 0xae, 0x7 ;fxsave [rdi]
+
+;; UEFI calling convention for x64 requires that Direction flag in EFLAGs is clear
+    cld
+
+;; UINT32  ExceptionData;
+    push    qword [rbp + 16]
+
+;; Prepare parameter and call
+    mov     rcx, [rbp + 8]
+    mov     rdx, rsp
+    ;
+    ; Per X64 calling convention, allocate maximum parameter stack space
+    ; and make sure RSP is 16-byte aligned
+    ;
+    sub     rsp, 4 * 8 + 8
+    call    ASM_PFX(CommonExceptionHandler)
+    add     rsp, 4 * 8 + 8
+
+    cli
+;; UINT64  ExceptionData;
+    add     rsp, 8
+
+;; FX_SAVE_STATE_X64 FxSaveState;
+
+    mov rsi, rsp
+    db 0xf, 0xae, 0xE ; fxrstor [rsi]
+    add rsp, 512
+
+;; UINT64  Dr0, Dr1, Dr2, Dr3, Dr6, Dr7;
+;; Skip restoration of DRx registers to support in-circuit emualators
+;; or debuggers set breakpoint in interrupt/exception context
+    add     rsp, 8 * 6
+
+;; UINT64  Cr0, Cr1, Cr2, Cr3, Cr4, Cr8;
+    pop     rax
+    mov     cr0, rax
+    add     rsp, 8   ; not for Cr1
+    pop     rax
+    mov     cr2, rax
+    pop     rax
+    mov     cr3, rax
+    pop     rax
+    mov     cr4, rax
+    pop     rax
+    mov     cr8, rax
+
+;; UINT64  RFlags;
+    pop     qword [rbp + 40]
+
+;; UINT64  Ldtr, Tr;
+;; UINT64  Gdtr[2], Idtr[2];
+;; Best not let anyone mess with these particular registers...
+    add     rsp, 48
+
+;; UINT64  Rip;
+    pop     qword [rbp + 24]
+
+;; UINT64  Gs, Fs, Es, Ds, Cs, Ss;
+    pop     rax
+    ; mov     gs, rax ; not for gs
+    pop     rax
+    ; mov     fs, rax ; not for fs
+    ; (X64 will not use fs and gs, so we do not restore it)
+    pop     rax
+    mov     es, rax
+    pop     rax
+    mov     ds, rax
+    pop     qword [rbp + 32]  ; for cs
+    pop     qword [rbp + 56]  ; for ss
+
+;; UINT64  Rdi, Rsi, Rbp, Rsp, Rbx, Rdx, Rcx, Rax;
+;; UINT64  R8, R9, R10, R11, R12, R13, R14, R15;
+    pop     rdi
+    pop     rsi
+    add     rsp, 8               ; not for rbp
+    pop     qword [rbp + 48] ; for rsp
+    pop     rbx
+    pop     rdx
+    pop     rcx
+    pop     rax
+    pop     r8
+    pop     r9
+    pop     r10
+    pop     r11
+    pop     r12
+    pop     r13
+    pop     r14
+    pop     r15
+
+    mov     rsp, rbp
+    pop     rbp
+    add     rsp, 16
+    cmp     qword [rsp - 32], 0  ; check EXCEPTION_HANDLER_CONTEXT.OldIdtHandler
+    jz      DoReturn
+    cmp     qword [rsp - 40], 1  ; check EXCEPTION_HANDLER_CONTEXT.ExceptionDataFlag
+    jz      ErrorCode
+    jmp     qword [rsp - 32]
+ErrorCode:
+    sub     rsp, 8
+    jmp     qword [rsp - 24]
+
+DoReturn:
+    cmp     qword [ASM_PFX(mDoFarReturnFlag)], 0   ; Check if need to do far return instead of IRET
+    jz      DoIret
+    push    rax
+    mov     rax, rsp          ; save old RSP to rax
+    mov     rsp, [rsp + 0x20]
+    push    qword [rax + 0x10]       ; save CS in new location
+    push    qword [rax + 0x8]        ; save EIP in new location
+    push    qword [rax + 0x18]       ; save EFLAGS in new location
+    mov     rax, [rax]        ; restore rax
+    popfq                     ; restore EFLAGS
+    DB      0x48               ; prefix to composite "retq" with next "retf"
+    retf                      ; far return
+DoIret:
+    iretq
+
+;-------------------------------------------------------------------------------------
+;  GetTemplateAddressMap (&AddressMap);
+;-------------------------------------------------------------------------------------
+; comments here for definition of address map
+global ASM_PFX(AsmGetTemplateAddressMap)
+ASM_PFX(AsmGetTemplateAddressMap):
+    lea     rax, [AsmIdtVectorBegin]
+    mov     qword [rcx], rax
+    mov     qword [rcx + 0x8],  (AsmIdtVectorEnd - AsmIdtVectorBegin) / 32
+    lea     rax, [HookAfterStubHeaderBegin]
+    mov     qword [rcx + 0x10], rax
+
+; Fix up CommonInterruptEntry address
+    lea    rax, [ASM_PFX(CommonInterruptEntry)]
+    lea    rcx, [AsmIdtVectorBegin]
+%rep  32
+    mov    qword [rcx + (JmpAbsoluteAddress - 8 - HookAfterStubHeaderBegin)], rax
+    add    rcx, (AsmIdtVectorEnd - AsmIdtVectorBegin) / 32
+%endrep
+; Fix up HookAfterStubHeaderEnd
+    lea    rax, [HookAfterStubHeaderEnd]
+    lea    rcx, [JmpAbsoluteAddress]
+    mov    qword [rcx - 8], rax
+
+    ret
+
+;-------------------------------------------------------------------------------------
+;  AsmVectorNumFixup (*NewVectorAddr, VectorNum, *OldVectorAddr);
+;-------------------------------------------------------------------------------------
+global ASM_PFX(AsmVectorNumFixup)
+ASM_PFX(AsmVectorNumFixup):
+    mov     rax, rdx
+    mov     [rcx + (@VectorNum - HookAfterStubHeaderBegin)], al
+    ret
+
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni
new file mode 100644
index 000000000000..be69992cef09
--- /dev/null
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni
@@ -0,0 +1,17 @@
+// /** @file
+// XCODE5 CPU Exception Handler library instance for SEC/PEI modules.
+//
+// CPU Exception Handler library instance for SEC/PEI modules when built
+// using the XCODE5 toolchain.
+//
+// Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.<BR>
+//
+// SPDX-License-Identifier: BSD-2-Clause-Patent
+//
+// **/
+
+
+#string STR_MODULE_ABSTRACT             #language en-US "CPU Exception Handler library instance for SEC/PEI modules with the XCODE5 toolchain."
+
+#string STR_MODULE_DESCRIPTION          #language en-US "CPU Exception Handler library instance for SEC/PEI modules with the XCODE5 toolchain."
+
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/3] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific
  2020-05-06 16:33 ` [PATCH v2 1/3] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific Lendacky, Thomas
@ 2020-05-06 19:01   ` Laszlo Ersek
  2020-05-06 20:37     ` [edk2-devel] " Lendacky, Thomas
  0 siblings, 1 reply; 9+ messages in thread
From: Laszlo Ersek @ 2020-05-06 19:01 UTC (permalink / raw)
  To: Tom Lendacky, devel
  Cc: Jordan Justen, Ard Biesheuvel, Liming Gao, Eric Dong, Ray Ni,
	Anthony Perard, Julien Grall, Brijesh Singh, Andrew Fish

On 05/06/20 18:33, Tom Lendacky 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 a new SEC INF
> file for the XCODE5 version of CpuExceptionHandlerLib.
> 
> Since binary patching is allowed when running outside of flash, switch
> the Dxe, Pei and Smm versions of the CpuExceptionHandlerLib over to use
> the Xcode5ExceptionHandlerAsm.nasm file to retain current functionality.
> 
> 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                     |   5 +
>  .../DxeCpuExceptionHandlerLib.inf             |   2 +-
>  .../PeiCpuExceptionHandlerLib.inf             |   2 +-
>  .../SmmCpuExceptionHandlerLib.inf             |   2 +-
>  .../Xcode5SecPeiCpuExceptionHandlerLib.inf    |  54 +++
>  .../X64/Xcode5ExceptionHandlerAsm.nasm        | 396 ++++++++++++++++++
>  .../Xcode5SecPeiCpuExceptionHandlerLib.uni    |  17 +
>  7 files changed, 475 insertions(+), 3 deletions(-)
>  create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
>  create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
>  create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc
> index d28cb5cccb52..264e5a787bce 100644
> --- a/UefiCpuPkg/UefiCpuPkg.dsc
> +++ b/UefiCpuPkg/UefiCpuPkg.dsc
> @@ -59,7 +59,11 @@ [LibraryClasses]
>  
>  [LibraryClasses.common.SEC]
>    PlatformSecLib|UefiCpuPkg/Library/PlatformSecLibNull/PlatformSecLibNull.inf
> +!if $(TOOL_CHAIN_TAG) == "XCODE5"
> +  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
> +!else
>    CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
> +!endif
>    HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
>    PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
>    MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
> @@ -126,6 +130,7 @@ [Components.IA32, Components.X64]
>    UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf

(1) I think this lib instance ("SecPeiCpuExceptionHandlerLib.inf") may
not build with XCODE5 at the end of the series, even in stand-alone
mode. Thus I think it should be conditionalized with

!if $(TOOL_CHAIN_TAG) != "XCODE5"
...
!endif

When using XCODE5, we should only build
"Xcode5SecPeiCpuExceptionHandlerLib.inf"; otherwise, we should build
*both* "SecPeiCpuExceptionHandlerLib.inf" and
"Xcode5SecPeiCpuExceptionHandlerLib.inf".

>    UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
>    UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> +  UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
>    UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
>    UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>    UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
> index e41383573043..61e2ec30b089 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
> @@ -28,7 +28,7 @@ [Sources.Ia32]
>    Ia32/ArchInterruptDefs.h
>  
>  [Sources.X64]
> -  X64/ExceptionHandlerAsm.nasm
> +  X64/Xcode5ExceptionHandlerAsm.nasm
>    X64/ArchExceptionHandler.c
>    X64/ArchInterruptDefs.h
>  
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> index f31423ac0f91..093374944df6 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> @@ -28,7 +28,7 @@ [Sources.Ia32]
>    Ia32/ArchInterruptDefs.h
>  
>  [Sources.X64]
> -  X64/ExceptionHandlerAsm.nasm
> +  X64/Xcode5ExceptionHandlerAsm.nasm
>    X64/ArchExceptionHandler.c
>    X64/ArchInterruptDefs.h
>  
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
> index 66c7f59e3c91..2ffbbccc302f 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
> @@ -28,7 +28,7 @@ [Sources.Ia32]
>    Ia32/ArchInterruptDefs.h
>  
>  [Sources.X64]
> -  X64/ExceptionHandlerAsm.nasm
> +  X64/Xcode5ExceptionHandlerAsm.nasm
>    X64/ArchExceptionHandler.c
>    X64/ArchInterruptDefs.h
>  
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
> new file mode 100644
> index 000000000000..3ed1378d6fa6
> --- /dev/null
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
> @@ -0,0 +1,54 @@
> +## @file
> +#  CPU Exception Handler library instance for SEC/PEI modules.
> +#
> +#  Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.<BR>
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent

(2) This is a customized copy of "SecPeiCpuExceptionHandlerLib.inf"; I
think you should prepend your (C) notice.

> +#
> +#  This is the XCODE5 variant of the SEC/PEI CpuExceptionHandlerLib. This
> +#  variant performs binary patching to fix up addresses that allow the
> +#  XCODE5 toolchain to be used.
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = Xcode5SecPeiCpuExceptionHandlerLib
> +  MODULE_UNI_FILE                = Xcode5SecPeiCpuExceptionHandlerLib.uni
> +  FILE_GUID                      = 49C481AF-1621-42F3-8FA1-27C64143E304
> +  MODULE_TYPE                    = PEIM
> +  VERSION_STRING                 = 1.1
> +  LIBRARY_CLASS                  = CpuExceptionHandlerLib|SEC PEI_CORE PEIM
> +
> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +#  VALID_ARCHITECTURES           = IA32 X64
> +#
> +
> +[Sources.Ia32]
> +  Ia32/ExceptionHandlerAsm.nasm
> +  Ia32/ExceptionTssEntryAsm.nasm
> +  Ia32/ArchExceptionHandler.c
> +  Ia32/ArchInterruptDefs.h
> +
> +[Sources.X64]
> +  X64/Xcode5ExceptionHandlerAsm.nasm
> +  X64/ArchExceptionHandler.c
> +  X64/ArchInterruptDefs.h
> +
> +[Sources.common]
> +  CpuExceptionCommon.h
> +  CpuExceptionCommon.c
> +  SecPeiCpuException.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  UefiCpuPkg/UefiCpuPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  SerialPortLib
> +  PrintLib
> +  LocalApicLib
> +  PeCoffGetEntryPointLib
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
> new file mode 100644
> index 000000000000..19198f273137
> --- /dev/null
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
> @@ -0,0 +1,396 @@
> +;------------------------------------------------------------------------------ ;
> +; Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.<BR>
> +; SPDX-License-Identifier: BSD-2-Clause-Patent

This is identical to "ExceptionHandlerAsm.nasm", so I agree a new (C)
notice is not needed.

> +;
> +; Module Name:
> +;
> +;   ExceptionHandlerAsm.Asm
> +;
> +; Abstract:
> +;
> +;   x64 CPU Exception Handler
> +;
> +; Notes:
> +;
> +;------------------------------------------------------------------------------
> +
> +;
> +; CommonExceptionHandler()
> +;
> +
> +extern ASM_PFX(mErrorCodeFlag)    ; Error code flags for exceptions
> +extern ASM_PFX(mDoFarReturnFlag)  ; Do far return flag
> +extern ASM_PFX(CommonExceptionHandler)
> +
> +SECTION .data
> +
> +DEFAULT REL
> +SECTION .text
> +
> +ALIGN   8
> +
> +AsmIdtVectorBegin:
> +%rep  32
> +    db      0x6a        ; push  #VectorNum
> +    db      ($ - AsmIdtVectorBegin) / ((AsmIdtVectorEnd - AsmIdtVectorBegin) / 32) ; VectorNum
> +    push    rax
> +    mov     rax, strict qword 0 ;    mov     rax, ASM_PFX(CommonInterruptEntry)
> +    jmp     rax
> +%endrep
> +AsmIdtVectorEnd:
> +
> +HookAfterStubHeaderBegin:
> +    db      0x6a        ; push
> +@VectorNum:
> +    db      0          ; 0 will be fixed
> +    push    rax
> +    mov     rax, strict qword 0 ;     mov     rax, HookAfterStubHeaderEnd
> +JmpAbsoluteAddress:
> +    jmp     rax
> +HookAfterStubHeaderEnd:
> +    mov     rax, rsp
> +    and     sp,  0xfff0        ; make sure 16-byte aligned for exception context
> +    sub     rsp, 0x18           ; reserve room for filling exception data later
> +    push    rcx
> +    mov     rcx, [rax + 8]
> +    bt      [ASM_PFX(mErrorCodeFlag)], ecx
> +    jnc     .0
> +    push    qword [rsp]             ; push additional rcx to make stack alignment
> +.0:
> +    xchg    rcx, [rsp]        ; restore rcx, save Exception Number in stack
> +    push    qword [rax]             ; push rax into stack to keep code consistence
> +
> +;---------------------------------------;
> +; CommonInterruptEntry                  ;
> +;---------------------------------------;
> +; The follow algorithm is used for the common interrupt routine.
> +; Entry from each interrupt with a push eax and eax=interrupt number
> +; Stack frame would be as follows as specified in IA32 manuals:
> +;
> +; +---------------------+ <-- 16-byte aligned ensured by processor
> +; +    Old SS           +
> +; +---------------------+
> +; +    Old RSP          +
> +; +---------------------+
> +; +    RFlags           +
> +; +---------------------+
> +; +    CS               +
> +; +---------------------+
> +; +    RIP              +
> +; +---------------------+
> +; +    Error Code       +
> +; +---------------------+
> +; +   Vector Number     +
> +; +---------------------+
> +; +    RBP              +
> +; +---------------------+ <-- RBP, 16-byte aligned
> +; The follow algorithm is used for the common interrupt routine.
> +global ASM_PFX(CommonInterruptEntry)
> +ASM_PFX(CommonInterruptEntry):
> +    cli
> +    pop     rax
> +    ;
> +    ; All interrupt handlers are invoked through interrupt gates, so
> +    ; IF flag automatically cleared at the entry point
> +    ;
> +    xchg    rcx, [rsp]      ; Save rcx into stack and save vector number into rcx
> +    and     rcx, 0xFF
> +    cmp     ecx, 32         ; Intel reserved vector for exceptions?
> +    jae     NoErrorCode
> +    bt      [ASM_PFX(mErrorCodeFlag)], ecx
> +    jc      HasErrorCode
> +
> +NoErrorCode:
> +
> +    ;
> +    ; Push a dummy error code on the stack
> +    ; to maintain coherent stack map
> +    ;
> +    push    qword [rsp]
> +    mov     qword [rsp + 8], 0
> +HasErrorCode:
> +    push    rbp
> +    mov     rbp, rsp
> +    push    0             ; clear EXCEPTION_HANDLER_CONTEXT.OldIdtHandler
> +    push    0             ; clear EXCEPTION_HANDLER_CONTEXT.ExceptionDataFlag
> +
> +    ;
> +    ; Stack:
> +    ; +---------------------+ <-- 16-byte aligned ensured by processor
> +    ; +    Old SS           +
> +    ; +---------------------+
> +    ; +    Old RSP          +
> +    ; +---------------------+
> +    ; +    RFlags           +
> +    ; +---------------------+
> +    ; +    CS               +
> +    ; +---------------------+
> +    ; +    RIP              +
> +    ; +---------------------+
> +    ; +    Error Code       +
> +    ; +---------------------+
> +    ; + RCX / Vector Number +
> +    ; +---------------------+
> +    ; +    RBP              +
> +    ; +---------------------+ <-- RBP, 16-byte aligned
> +    ;
> +
> +    ;
> +    ; Since here the stack pointer is 16-byte aligned, so
> +    ; EFI_FX_SAVE_STATE_X64 of EFI_SYSTEM_CONTEXT_x64
> +    ; is 16-byte aligned
> +    ;
> +
> +;; UINT64  Rdi, Rsi, Rbp, Rsp, Rbx, Rdx, Rcx, Rax;
> +;; UINT64  R8, R9, R10, R11, R12, R13, R14, R15;
> +    push r15
> +    push r14
> +    push r13
> +    push r12
> +    push r11
> +    push r10
> +    push r9
> +    push r8
> +    push rax
> +    push qword [rbp + 8]   ; RCX
> +    push rdx
> +    push rbx
> +    push qword [rbp + 48]  ; RSP
> +    push qword [rbp]       ; RBP
> +    push rsi
> +    push rdi
> +
> +;; UINT64  Gs, Fs, Es, Ds, Cs, Ss;  insure high 16 bits of each is zero
> +    movzx   rax, word [rbp + 56]
> +    push    rax                      ; for ss
> +    movzx   rax, word [rbp + 32]
> +    push    rax                      ; for cs
> +    mov     rax, ds
> +    push    rax
> +    mov     rax, es
> +    push    rax
> +    mov     rax, fs
> +    push    rax
> +    mov     rax, gs
> +    push    rax
> +
> +    mov     [rbp + 8], rcx               ; save vector number
> +
> +;; UINT64  Rip;
> +    push    qword [rbp + 24]
> +
> +;; UINT64  Gdtr[2], Idtr[2];
> +    xor     rax, rax
> +    push    rax
> +    push    rax
> +    sidt    [rsp]
> +    mov     bx, word [rsp]
> +    mov     rax, qword [rsp + 2]
> +    mov     qword [rsp], rax
> +    mov     word [rsp + 8], bx
> +
> +    xor     rax, rax
> +    push    rax
> +    push    rax
> +    sgdt    [rsp]
> +    mov     bx, word [rsp]
> +    mov     rax, qword [rsp + 2]
> +    mov     qword [rsp], rax
> +    mov     word [rsp + 8], bx
> +
> +;; UINT64  Ldtr, Tr;
> +    xor     rax, rax
> +    str     ax
> +    push    rax
> +    sldt    ax
> +    push    rax
> +
> +;; UINT64  RFlags;
> +    push    qword [rbp + 40]
> +
> +;; UINT64  Cr0, Cr1, Cr2, Cr3, Cr4, Cr8;
> +    mov     rax, cr8
> +    push    rax
> +    mov     rax, cr4
> +    or      rax, 0x208
> +    mov     cr4, rax
> +    push    rax
> +    mov     rax, cr3
> +    push    rax
> +    mov     rax, cr2
> +    push    rax
> +    xor     rax, rax
> +    push    rax
> +    mov     rax, cr0
> +    push    rax
> +
> +;; UINT64  Dr0, Dr1, Dr2, Dr3, Dr6, Dr7;
> +    mov     rax, dr7
> +    push    rax
> +    mov     rax, dr6
> +    push    rax
> +    mov     rax, dr3
> +    push    rax
> +    mov     rax, dr2
> +    push    rax
> +    mov     rax, dr1
> +    push    rax
> +    mov     rax, dr0
> +    push    rax
> +
> +;; FX_SAVE_STATE_X64 FxSaveState;
> +    sub rsp, 512
> +    mov rdi, rsp
> +    db 0xf, 0xae, 0x7 ;fxsave [rdi]
> +
> +;; UEFI calling convention for x64 requires that Direction flag in EFLAGs is clear
> +    cld
> +
> +;; UINT32  ExceptionData;
> +    push    qword [rbp + 16]
> +
> +;; Prepare parameter and call
> +    mov     rcx, [rbp + 8]
> +    mov     rdx, rsp
> +    ;
> +    ; Per X64 calling convention, allocate maximum parameter stack space
> +    ; and make sure RSP is 16-byte aligned
> +    ;
> +    sub     rsp, 4 * 8 + 8
> +    call    ASM_PFX(CommonExceptionHandler)
> +    add     rsp, 4 * 8 + 8
> +
> +    cli
> +;; UINT64  ExceptionData;
> +    add     rsp, 8
> +
> +;; FX_SAVE_STATE_X64 FxSaveState;
> +
> +    mov rsi, rsp
> +    db 0xf, 0xae, 0xE ; fxrstor [rsi]
> +    add rsp, 512
> +
> +;; UINT64  Dr0, Dr1, Dr2, Dr3, Dr6, Dr7;
> +;; Skip restoration of DRx registers to support in-circuit emualators
> +;; or debuggers set breakpoint in interrupt/exception context
> +    add     rsp, 8 * 6
> +
> +;; UINT64  Cr0, Cr1, Cr2, Cr3, Cr4, Cr8;
> +    pop     rax
> +    mov     cr0, rax
> +    add     rsp, 8   ; not for Cr1
> +    pop     rax
> +    mov     cr2, rax
> +    pop     rax
> +    mov     cr3, rax
> +    pop     rax
> +    mov     cr4, rax
> +    pop     rax
> +    mov     cr8, rax
> +
> +;; UINT64  RFlags;
> +    pop     qword [rbp + 40]
> +
> +;; UINT64  Ldtr, Tr;
> +;; UINT64  Gdtr[2], Idtr[2];
> +;; Best not let anyone mess with these particular registers...
> +    add     rsp, 48
> +
> +;; UINT64  Rip;
> +    pop     qword [rbp + 24]
> +
> +;; UINT64  Gs, Fs, Es, Ds, Cs, Ss;
> +    pop     rax
> +    ; mov     gs, rax ; not for gs
> +    pop     rax
> +    ; mov     fs, rax ; not for fs
> +    ; (X64 will not use fs and gs, so we do not restore it)
> +    pop     rax
> +    mov     es, rax
> +    pop     rax
> +    mov     ds, rax
> +    pop     qword [rbp + 32]  ; for cs
> +    pop     qword [rbp + 56]  ; for ss
> +
> +;; UINT64  Rdi, Rsi, Rbp, Rsp, Rbx, Rdx, Rcx, Rax;
> +;; UINT64  R8, R9, R10, R11, R12, R13, R14, R15;
> +    pop     rdi
> +    pop     rsi
> +    add     rsp, 8               ; not for rbp
> +    pop     qword [rbp + 48] ; for rsp
> +    pop     rbx
> +    pop     rdx
> +    pop     rcx
> +    pop     rax
> +    pop     r8
> +    pop     r9
> +    pop     r10
> +    pop     r11
> +    pop     r12
> +    pop     r13
> +    pop     r14
> +    pop     r15
> +
> +    mov     rsp, rbp
> +    pop     rbp
> +    add     rsp, 16
> +    cmp     qword [rsp - 32], 0  ; check EXCEPTION_HANDLER_CONTEXT.OldIdtHandler
> +    jz      DoReturn
> +    cmp     qword [rsp - 40], 1  ; check EXCEPTION_HANDLER_CONTEXT.ExceptionDataFlag
> +    jz      ErrorCode
> +    jmp     qword [rsp - 32]
> +ErrorCode:
> +    sub     rsp, 8
> +    jmp     qword [rsp - 24]
> +
> +DoReturn:
> +    cmp     qword [ASM_PFX(mDoFarReturnFlag)], 0   ; Check if need to do far return instead of IRET
> +    jz      DoIret
> +    push    rax
> +    mov     rax, rsp          ; save old RSP to rax
> +    mov     rsp, [rsp + 0x20]
> +    push    qword [rax + 0x10]       ; save CS in new location
> +    push    qword [rax + 0x8]        ; save EIP in new location
> +    push    qword [rax + 0x18]       ; save EFLAGS in new location
> +    mov     rax, [rax]        ; restore rax
> +    popfq                     ; restore EFLAGS
> +    DB      0x48               ; prefix to composite "retq" with next "retf"
> +    retf                      ; far return
> +DoIret:
> +    iretq
> +
> +;-------------------------------------------------------------------------------------
> +;  GetTemplateAddressMap (&AddressMap);
> +;-------------------------------------------------------------------------------------
> +; comments here for definition of address map
> +global ASM_PFX(AsmGetTemplateAddressMap)
> +ASM_PFX(AsmGetTemplateAddressMap):
> +    lea     rax, [AsmIdtVectorBegin]
> +    mov     qword [rcx], rax
> +    mov     qword [rcx + 0x8],  (AsmIdtVectorEnd - AsmIdtVectorBegin) / 32
> +    lea     rax, [HookAfterStubHeaderBegin]
> +    mov     qword [rcx + 0x10], rax
> +
> +; Fix up CommonInterruptEntry address
> +    lea    rax, [ASM_PFX(CommonInterruptEntry)]
> +    lea    rcx, [AsmIdtVectorBegin]
> +%rep  32
> +    mov    qword [rcx + (JmpAbsoluteAddress - 8 - HookAfterStubHeaderBegin)], rax
> +    add    rcx, (AsmIdtVectorEnd - AsmIdtVectorBegin) / 32
> +%endrep
> +; Fix up HookAfterStubHeaderEnd
> +    lea    rax, [HookAfterStubHeaderEnd]
> +    lea    rcx, [JmpAbsoluteAddress]
> +    mov    qword [rcx - 8], rax
> +
> +    ret
> +
> +;-------------------------------------------------------------------------------------
> +;  AsmVectorNumFixup (*NewVectorAddr, VectorNum, *OldVectorAddr);
> +;-------------------------------------------------------------------------------------
> +global ASM_PFX(AsmVectorNumFixup)
> +ASM_PFX(AsmVectorNumFixup):
> +    mov     rax, rdx
> +    mov     [rcx + (@VectorNum - HookAfterStubHeaderBegin)], al
> +    ret
> +
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni
> new file mode 100644
> index 000000000000..be69992cef09
> --- /dev/null
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni
> @@ -0,0 +1,17 @@
> +// /** @file
> +// XCODE5 CPU Exception Handler library instance for SEC/PEI modules.
> +//
> +// CPU Exception Handler library instance for SEC/PEI modules when built
> +// using the XCODE5 toolchain.
> +//
> +// Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.<BR>
> +//
> +// SPDX-License-Identifier: BSD-2-Clause-Patent
> +//
> +// **/
> +
> +
> +#string STR_MODULE_ABSTRACT             #language en-US "CPU Exception Handler library instance for SEC/PEI modules with the XCODE5 toolchain."
> +
> +#string STR_MODULE_DESCRIPTION          #language en-US "CPU Exception Handler library instance for SEC/PEI modules with the XCODE5 toolchain."
> +
> 

(3) This is a brand new file; I think you should prepend your (C) notice.

Meta-hint: with patches like this, it sometimes makes sense to format
the series for posting with "--find-copies-harder".

With (1) through (3) updated:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [edk2-devel] [PATCH v2 1/3] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific
  2020-05-06 19:01   ` Laszlo Ersek
@ 2020-05-06 20:37     ` Lendacky, Thomas
  0 siblings, 0 replies; 9+ messages in thread
From: Lendacky, Thomas @ 2020-05-06 20:37 UTC (permalink / raw)
  To: devel, lersek
  Cc: Jordan Justen, Ard Biesheuvel, Liming Gao, Eric Dong, Ray Ni,
	Anthony Perard, Julien Grall, Brijesh Singh, Andrew Fish

On 5/6/20 2:01 PM, Laszlo Ersek via groups.io wrote:
> On 05/06/20 18:33, Tom Lendacky wrote:
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2340&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7C3c47ab80a1554f27afd608d7f1eff6e8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243885258646451&amp;sdata=Xh7E1EB%2B1oOCtc2jgtJ8lsjgcwxgswIbgpL4%2BUwpoOw%3D&amp;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 a new SEC INF
>> file for the XCODE5 version of CpuExceptionHandlerLib.
>>
>> Since binary patching is allowed when running outside of flash, switch
>> the Dxe, Pei and Smm versions of the CpuExceptionHandlerLib over to use
>> the Xcode5ExceptionHandlerAsm.nasm file to retain current functionality.
>>
>> 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                     |   5 +
>>   .../DxeCpuExceptionHandlerLib.inf             |   2 +-
>>   .../PeiCpuExceptionHandlerLib.inf             |   2 +-
>>   .../SmmCpuExceptionHandlerLib.inf             |   2 +-
>>   .../Xcode5SecPeiCpuExceptionHandlerLib.inf    |  54 +++
>>   .../X64/Xcode5ExceptionHandlerAsm.nasm        | 396 ++++++++++++++++++
>>   .../Xcode5SecPeiCpuExceptionHandlerLib.uni    |  17 +
>>   7 files changed, 475 insertions(+), 3 deletions(-)
>>   create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
>>   create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
>>   create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni
>>
>> diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc
>> index d28cb5cccb52..264e5a787bce 100644
>> --- a/UefiCpuPkg/UefiCpuPkg.dsc
>> +++ b/UefiCpuPkg/UefiCpuPkg.dsc
>> @@ -59,7 +59,11 @@ [LibraryClasses]
>>   
>>   [LibraryClasses.common.SEC]
>>     PlatformSecLib|UefiCpuPkg/Library/PlatformSecLibNull/PlatformSecLibNull.inf
>> +!if $(TOOL_CHAIN_TAG) == "XCODE5"
>> +  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
>> +!else
>>     CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
>> +!endif
>>     HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
>>     PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
>>     MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
>> @@ -126,6 +130,7 @@ [Components.IA32, Components.X64]
>>     UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
> 
> (1) I think this lib instance ("SecPeiCpuExceptionHandlerLib.inf") may
> not build with XCODE5 at the end of the series, even in stand-alone
> mode. Thus I think it should be conditionalized with
> 
> !if $(TOOL_CHAIN_TAG) != "XCODE5"
> ...
> !endif
> 
> When using XCODE5, we should only build
> "Xcode5SecPeiCpuExceptionHandlerLib.inf"; otherwise, we should build
> *both* "SecPeiCpuExceptionHandlerLib.inf" and
> ".inf".
> 

This is the area that was resulting in the error when I used the pull
request to run the integration tests. I was using an if/else originally,
I'll try just the if != XCODE5 around SecPeiCpuExceptionHandlerLib.inf and
see if that goes through. If not, Brett Barkelew responded with a
suggestion to add the Xcode5SecPeiCpuExceptionHandlerLib.inf to the ignore
list in the CI yaml file.

For the next version, under [Components], it will look like:

@@ -123,9 +127,12 @@ [Components.IA32, Components.X64]
   UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
   UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
   UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
+!endif
   UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
   UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
+  UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
   UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
   UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
   UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf

>>     UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
>>     UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
>> +  UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
>>     UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
>>     UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>>     UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
>> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
>> index e41383573043..61e2ec30b089 100644
>> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
>> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
>> @@ -28,7 +28,7 @@ [Sources.Ia32]
>>     Ia32/ArchInterruptDefs.h
>>   
>>   [Sources.X64]
>> -  X64/ExceptionHandlerAsm.nasm
>> +  X64/Xcode5ExceptionHandlerAsm.nasm
>>     X64/ArchExceptionHandler.c
>>     X64/ArchInterruptDefs.h
>>   
>> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
>> index f31423ac0f91..093374944df6 100644
>> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
>> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
>> @@ -28,7 +28,7 @@ [Sources.Ia32]
>>     Ia32/ArchInterruptDefs.h
>>   
>>   [Sources.X64]
>> -  X64/ExceptionHandlerAsm.nasm
>> +  X64/Xcode5ExceptionHandlerAsm.nasm
>>     X64/ArchExceptionHandler.c
>>     X64/ArchInterruptDefs.h
>>   
>> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
>> index 66c7f59e3c91..2ffbbccc302f 100644
>> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
>> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
>> @@ -28,7 +28,7 @@ [Sources.Ia32]
>>     Ia32/ArchInterruptDefs.h
>>   
>>   [Sources.X64]
>> -  X64/ExceptionHandlerAsm.nasm
>> +  X64/Xcode5ExceptionHandlerAsm.nasm
>>     X64/ArchExceptionHandler.c
>>     X64/ArchInterruptDefs.h
>>   
>> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
>> new file mode 100644
>> index 000000000000..3ed1378d6fa6
>> --- /dev/null
>> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
>> @@ -0,0 +1,54 @@
>> +## @file
>> +#  CPU Exception Handler library instance for SEC/PEI modules.
>> +#
>> +#  Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.<BR>
>> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> (2) This is a customized copy of "SecPeiCpuExceptionHandlerLib.inf"; I
> think you should prepend your (C) notice.

Ok.

> 
>> +#
>> +#  This is the XCODE5 variant of the SEC/PEI CpuExceptionHandlerLib. This
>> +#  variant performs binary patching to fix up addresses that allow the
>> +#  XCODE5 toolchain to be used.
>> +#
>> +##
>> +
>> +[Defines]
>> +  INF_VERSION                    = 0x00010005
>> +  BASE_NAME                      = Xcode5SecPeiCpuExceptionHandlerLib
>> +  MODULE_UNI_FILE                = Xcode5SecPeiCpuExceptionHandlerLib.uni
>> +  FILE_GUID                      = 49C481AF-1621-42F3-8FA1-27C64143E304
>> +  MODULE_TYPE                    = PEIM
>> +  VERSION_STRING                 = 1.1
>> +  LIBRARY_CLASS                  = CpuExceptionHandlerLib|SEC PEI_CORE PEIM
>> +

<... SNIP ...>

>> +global ASM_PFX(AsmVectorNumFixup)
>> +ASM_PFX(AsmVectorNumFixup):
>> +    mov     rax, rdx
>> +    mov     [rcx + (@VectorNum - HookAfterStubHeaderBegin)], al
>> +    ret
>> +
>> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni
>> new file mode 100644
>> index 000000000000..be69992cef09
>> --- /dev/null
>> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni
>> @@ -0,0 +1,17 @@
>> +// /** @file
>> +// XCODE5 CPU Exception Handler library instance for SEC/PEI modules.
>> +//
>> +// CPU Exception Handler library instance for SEC/PEI modules when built
>> +// using the XCODE5 toolchain.
>> +//
>> +// Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.<BR>
>> +//
>> +// SPDX-License-Identifier: BSD-2-Clause-Patent
>> +//
>> +// **/
>> +
>> +
>> +#string STR_MODULE_ABSTRACT             #language en-US "CPU Exception Handler library instance for SEC/PEI modules with the XCODE5 toolchain."
>> +
>> +#string STR_MODULE_DESCRIPTION          #language en-US "CPU Exception Handler library instance for SEC/PEI modules with the XCODE5 toolchain."
>> +
>>
> 
> (3) This is a brand new file; I think you should prepend your (C) notice.

Ok.

> 
> Meta-hint: with patches like this, it sometimes makes sense to format
> the series for posting with "--find-copies-harder".

Ah, I'll do that on the next version.

Thanks,
Tom

> 
> With (1) through (3) updated:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks,
> Laszlo
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 2/3] OvmfPkg: Use toolchain appropriate CpuExceptionHandlerLib
  2020-05-06 16:32 [PATCH v2 0/3] XCODE5 toolchain binary patching fix Lendacky, Thomas
  2020-05-06 16:33 ` [PATCH v2 1/3] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific Lendacky, Thomas
@ 2020-05-06 16:33 ` Lendacky, Thomas
  2020-05-06 19:04   ` Laszlo Ersek
  2020-05-06 16:33 ` [PATCH v2 3/3] UefiCpuPkg/CpuExceptionHandler: Revert CpuExceptionHandler binary patching Lendacky, Thomas
  2020-05-06 19:53 ` [PATCH v2 0/3] XCODE5 toolchain binary patching fix Laszlo Ersek
  3 siblings, 1 reply; 9+ messages in thread
From: Lendacky, Thomas @ 2020-05-06 16:33 UTC (permalink / raw)
  To: devel
  Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Liming Gao,
	Eric Dong, Ray Ni, Anthony Perard, Julien Grall, Brijesh Singh,
	Andrew Fish, Ard Biesheuvel

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2340

During the SEC phase, use the XCODE5 CpuExceptionHandlerLib library in
place of the standard library when building with the XCODE5 toolchain.
The SEC XCODE5 version of the library performs binary patching and should
only be used when building with the XCODE5 toolchain.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien@xen.org>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 OvmfPkg/OvmfPkgIa32.dsc    | 4 ++++
 OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++++
 OvmfPkg/OvmfPkgX64.dsc     | 4 ++++
 OvmfPkg/OvmfXen.dsc        | 4 ++++
 4 files changed, 16 insertions(+)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 7c8b51f43b66..41ac3202961b 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -246,7 +246,11 @@ [LibraryClasses.common.SEC]
   PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
   PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
   MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
+!if $(TOOL_CHAIN_TAG) == "XCODE5"
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
+!else
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
+!endif
 
 [LibraryClasses.common.PEI_CORE]
   HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index a0596c44168c..c2f11aee2cec 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -250,7 +250,11 @@ [LibraryClasses.common.SEC]
   PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
   PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
   MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
+!if $(TOOL_CHAIN_TAG) == "XCODE5"
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
+!else
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
+!endif
 
 [LibraryClasses.common.PEI_CORE]
   HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 2e764b6b7233..643e6041ad53 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -250,7 +250,11 @@ [LibraryClasses.common.SEC]
   PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
   PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
   MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
+!if $(TOOL_CHAIN_TAG) == "XCODE5"
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
+!else
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
+!endif
 
 [LibraryClasses.common.PEI_CORE]
   HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 8b3615e0b07e..143dc6d5a766 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -228,7 +228,11 @@ [LibraryClasses.common.SEC]
   PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
   PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
   MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
+!if $(TOOL_CHAIN_TAG) == "XCODE5"
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
+!else
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
+!endif
 
 [LibraryClasses.common.PEI_CORE]
   HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 2/3] OvmfPkg: Use toolchain appropriate CpuExceptionHandlerLib
  2020-05-06 16:33 ` [PATCH v2 2/3] OvmfPkg: Use toolchain appropriate CpuExceptionHandlerLib Lendacky, Thomas
@ 2020-05-06 19:04   ` Laszlo Ersek
  0 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2020-05-06 19:04 UTC (permalink / raw)
  To: Tom Lendacky, devel
  Cc: Jordan Justen, Ard Biesheuvel, Liming Gao, Eric Dong, Ray Ni,
	Anthony Perard, Julien Grall, Brijesh Singh, Andrew Fish,
	Ard Biesheuvel

On 05/06/20 18:33, Tom Lendacky wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2340
> 
> During the SEC phase, use the XCODE5 CpuExceptionHandlerLib library in
> place of the standard library when building with the XCODE5 toolchain.
> The SEC XCODE5 version of the library performs binary patching and should
> only be used when building with the XCODE5 toolchain.
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc    | 4 ++++
>  OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++++
>  OvmfPkg/OvmfPkgX64.dsc     | 4 ++++
>  OvmfPkg/OvmfXen.dsc        | 4 ++++
>  4 files changed, 16 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 7c8b51f43b66..41ac3202961b 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -246,7 +246,11 @@ [LibraryClasses.common.SEC]
>    PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
>    PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
>    MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
> +!if $(TOOL_CHAIN_TAG) == "XCODE5"
> +  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
> +!else
>    CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
> +!endif
>  
>  [LibraryClasses.common.PEI_CORE]
>    HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index a0596c44168c..c2f11aee2cec 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -250,7 +250,11 @@ [LibraryClasses.common.SEC]
>    PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
>    PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
>    MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
> +!if $(TOOL_CHAIN_TAG) == "XCODE5"
> +  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
> +!else
>    CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
> +!endif
>  
>  [LibraryClasses.common.PEI_CORE]
>    HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 2e764b6b7233..643e6041ad53 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -250,7 +250,11 @@ [LibraryClasses.common.SEC]
>    PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
>    PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
>    MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
> +!if $(TOOL_CHAIN_TAG) == "XCODE5"
> +  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
> +!else
>    CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
> +!endif
>  
>  [LibraryClasses.common.PEI_CORE]
>    HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index 8b3615e0b07e..143dc6d5a766 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.dsc
> @@ -228,7 +228,11 @@ [LibraryClasses.common.SEC]
>    PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
>    PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
>    MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
> +!if $(TOOL_CHAIN_TAG) == "XCODE5"
> +  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
> +!else
>    CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
> +!endif
>  
>  [LibraryClasses.common.PEI_CORE]
>    HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 3/3] UefiCpuPkg/CpuExceptionHandler: Revert CpuExceptionHandler binary patching
  2020-05-06 16:32 [PATCH v2 0/3] XCODE5 toolchain binary patching fix Lendacky, Thomas
  2020-05-06 16:33 ` [PATCH v2 1/3] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific Lendacky, Thomas
  2020-05-06 16:33 ` [PATCH v2 2/3] OvmfPkg: Use toolchain appropriate CpuExceptionHandlerLib Lendacky, Thomas
@ 2020-05-06 16:33 ` Lendacky, Thomas
  2020-05-06 19:31   ` Laszlo Ersek
  2020-05-06 19:53 ` [PATCH v2 0/3] XCODE5 toolchain binary patching fix Laszlo Ersek
  3 siblings, 1 reply; 9+ messages in thread
From: Lendacky, Thomas @ 2020-05-06 16:33 UTC (permalink / raw)
  To: devel
  Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Liming Gao,
	Eric Dong, Ray Ni, Anthony Perard, Julien Grall, Brijesh Singh,
	Andrew Fish

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2340

Now that an XCODE5 specific CpuExceptionHandlerLib library is in place,
revert the changes made to the ExceptionHandlerAsm.nasm in commit
2db0ccc2d7fe ("UefiCpuPkg: Update CpuExceptionHandlerLib pass XCODE5 tool
chain") so that binary patching of flash code is not performed.

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>
---
 .../X64/ExceptionHandlerAsm.nasm              | 25 +++++--------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
index 19198f273137..3814f9de3703 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
@@ -34,7 +34,7 @@ AsmIdtVectorBegin:
     db      0x6a        ; push  #VectorNum
     db      ($ - AsmIdtVectorBegin) / ((AsmIdtVectorEnd - AsmIdtVectorBegin) / 32) ; VectorNum
     push    rax
-    mov     rax, strict qword 0 ;    mov     rax, ASM_PFX(CommonInterruptEntry)
+    mov     rax, ASM_PFX(CommonInterruptEntry)
     jmp     rax
 %endrep
 AsmIdtVectorEnd:
@@ -44,8 +44,7 @@ HookAfterStubHeaderBegin:
 @VectorNum:
     db      0          ; 0 will be fixed
     push    rax
-    mov     rax, strict qword 0 ;     mov     rax, HookAfterStubHeaderEnd
-JmpAbsoluteAddress:
+    mov     rax, HookAfterStubHeaderEnd
     jmp     rax
 HookAfterStubHeaderEnd:
     mov     rax, rsp
@@ -257,7 +256,8 @@ HasErrorCode:
     ; and make sure RSP is 16-byte aligned
     ;
     sub     rsp, 4 * 8 + 8
-    call    ASM_PFX(CommonExceptionHandler)
+    mov     rax, ASM_PFX(CommonExceptionHandler)
+    call    rax
     add     rsp, 4 * 8 + 8
 
     cli
@@ -365,24 +365,11 @@ DoIret:
 ; comments here for definition of address map
 global ASM_PFX(AsmGetTemplateAddressMap)
 ASM_PFX(AsmGetTemplateAddressMap):
-    lea     rax, [AsmIdtVectorBegin]
+    mov     rax, AsmIdtVectorBegin
     mov     qword [rcx], rax
     mov     qword [rcx + 0x8],  (AsmIdtVectorEnd - AsmIdtVectorBegin) / 32
-    lea     rax, [HookAfterStubHeaderBegin]
+    mov     rax, HookAfterStubHeaderBegin
     mov     qword [rcx + 0x10], rax
-
-; Fix up CommonInterruptEntry address
-    lea    rax, [ASM_PFX(CommonInterruptEntry)]
-    lea    rcx, [AsmIdtVectorBegin]
-%rep  32
-    mov    qword [rcx + (JmpAbsoluteAddress - 8 - HookAfterStubHeaderBegin)], rax
-    add    rcx, (AsmIdtVectorEnd - AsmIdtVectorBegin) / 32
-%endrep
-; Fix up HookAfterStubHeaderEnd
-    lea    rax, [HookAfterStubHeaderEnd]
-    lea    rcx, [JmpAbsoluteAddress]
-    mov    qword [rcx - 8], rax
-
     ret
 
 ;-------------------------------------------------------------------------------------
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 3/3] UefiCpuPkg/CpuExceptionHandler: Revert CpuExceptionHandler binary patching
  2020-05-06 16:33 ` [PATCH v2 3/3] UefiCpuPkg/CpuExceptionHandler: Revert CpuExceptionHandler binary patching Lendacky, Thomas
@ 2020-05-06 19:31   ` Laszlo Ersek
  0 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2020-05-06 19:31 UTC (permalink / raw)
  To: Tom Lendacky, devel
  Cc: Jordan Justen, Ard Biesheuvel, Liming Gao, Eric Dong, Ray Ni,
	Anthony Perard, Julien Grall, Brijesh Singh, Andrew Fish

On 05/06/20 18:33, Tom Lendacky wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2340
> 
> Now that an XCODE5 specific CpuExceptionHandlerLib library is in place,
> revert the changes made to the ExceptionHandlerAsm.nasm in commit
> 2db0ccc2d7fe ("UefiCpuPkg: Update CpuExceptionHandlerLib pass XCODE5 tool
> chain") so that binary patching of flash code is not performed.
> 
> 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>
> ---
>  .../X64/ExceptionHandlerAsm.nasm              | 25 +++++--------------
>  1 file changed, 6 insertions(+), 19 deletions(-)

Apart from the subject update, this patch is identical to the last patch
in the v1 series -- for which I gave R-b:

http://mid.mail-archive.com/df31ace8-ea40-332f-e6a7-1dc40c0e9b5f@redhat.com
https://edk2.groups.io/g/devel/message/58666

So I think my R-b should have been picked up.

Anyway:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 0/3] XCODE5 toolchain binary patching fix
  2020-05-06 16:32 [PATCH v2 0/3] XCODE5 toolchain binary patching fix Lendacky, Thomas
                   ` (2 preceding siblings ...)
  2020-05-06 16:33 ` [PATCH v2 3/3] UefiCpuPkg/CpuExceptionHandler: Revert CpuExceptionHandler binary patching Lendacky, Thomas
@ 2020-05-06 19:53 ` Laszlo Ersek
  3 siblings, 0 replies; 9+ messages in thread
From: Laszlo Ersek @ 2020-05-06 19:53 UTC (permalink / raw)
  To: Tom Lendacky, devel
  Cc: Jordan Justen, Ard Biesheuvel, Liming Gao, Eric Dong, Ray Ni,
	Anthony Perard, Julien Grall, Brijesh Singh, Andrew Fish

On 05/06/20 18:32, Tom Lendacky wrote:
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2340
>
> Commit 2db0ccc2d7fe ("UefiCpuPkg: Update CpuExceptionHandlerLib pass
> XCODE5 tool chain") introduced binary patching in the
> ExceptionHandlerAsm.nasm in order to support the XCODE5 toolchain.
> However, the CpuExceptionHandlerLib can be used during SEC phase which
> would result in binary patching of flash.
>
> This series creates a new CpuExceptionHandlerLib file to support the
> required binary patching for the XCODE5 toolchain, while reverting the
> changes from commit 2db0ccc2d7fe in the standard file. As the Pei, Dxe
> and SMM versions of the library operate in memory (as opposed to
> flash), only the SEC/PEI version is of the library is updated to use
> the version of the ExceptionHandlerAsm.nasm that does not perform
> binary patching.
>
> This is accomplished in phases:
>   - Create a new XCODE5 specific version of the
>     ExceptionHandlerAsm.nasm file and update all CpuExceptionHandler
>     INF files to use it while also creating a new SEC/PEI
>     CpuExceptionHandler INF file specifically for the XCODE5
>     toolchain.
>   - Update all package DSC files that use the
>     SecPeiCpuExceptionHandlerLib version of the library to use the
>     XCODE5 version of the library, Xcode5SecPeiCpuExceptionHandlerLib,
>     when the XCODE5 toolchain is used.
>   - Revert the changes made by commit 2db0ccc2d7fe in the standard
>     file and update the SecPeiCpuExceptionHandlerLib.inf file to use
>     the standard file.
>
> I don't have access to an XCODE5 toolchain setup, so I have not tested
> this with XCODE5. I would like to request that someone who does please
> test this.
>
> Also, will this change have an impact on any of the platform builds
> outside of this tree?

This series takes care of all the "SecPeiCpuExceptionHandlerLib.inf"
occurrences in edk2.

Regarding edk2-platforms, with master being at 644e223bb371
("Platform/RaspberryPi: create DXE phase SerialPortLib version for
RPi3", 2020-05-06):

$ git grep -l \
      'UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf'

Platform/Intel/MinPlatformPkg/Include/Dsc/CorePeiLib.dsc
Platform/Intel/QuarkPlatformPkg/Quark.dsc
Platform/Intel/QuarkPlatformPkg/QuarkMin.dsc
Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgIA32.dsc
Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgX64.dsc

I don't know what's best for these platforms: an XCODE5-compatible
source code that does the wrong thing when run from flash, or an
XCODE5-incompatible source code that does the right thing.

If all of these platforms lived in edk2 proper, I would suggest
inserting patches for them just before patch#3 in this series --
similarly to your OvmfPkg patch. But, these platforms live outside of
edk2, and I don't know if they are ever built with XCODE5...

... You could propose a separate edk2-platforms series:

Platform/Intel/MinPlatformPkg/Include/Dsc/CorePeiLib.dsc
  Chasel Chiu <chasel.chiu@intel.com>
  Nate DeSimone <nathaniel.l.desimone@intel.com>
  Liming Gao <liming.gao@intel.com>

Platform/Intel/QuarkPlatformPkg/Quark.dsc
  Michael D Kinney <michael.d.kinney@intel.com>
  Kelly Steele <kelly.steele@intel.com>

Platform/Intel/QuarkPlatformPkg/QuarkMin.dsc
  Michael D Kinney <michael.d.kinney@intel.com>
  Kelly Steele <kelly.steele@intel.com>

Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgIA32.dsc
  Zailiang Sun <zailiang.sun@intel.com>
  Yi Qian <yi.qian@intel.com>

Platform/Intel/Vlv2TbltDevicePkg/PlatformPkgX64.dsc
  Zailiang Sun <zailiang.sun@intel.com>
  Yi Qian <yi.qian@intel.com>

and we could then delay just the pushing of patch#3 in this series until
the edk2-platforms patches have been merged too.

> In other words, should the new INF be the one that uses the reverted
> ExceptionHandlerAsm.nasm file and it be called something like
> BaseSecPeiCpuExceptionHandlerLib.inf?

Keeping the current (= self-patching) logic associated with the current
filename ("SecPeiCpuExceptionHandlerLib.inf") would certainly eliminate
the headache about out-of-tree platforms. But it would also mean we'd
have to use a special prefix for the *non-broken* SEC INF file. And that
irks me quite a bit. The quirk is in the patching variant, and IMO that
should be reflected by the file name.

Thanks
Laszlo


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-05-06 20:37 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-06 16:32 [PATCH v2 0/3] XCODE5 toolchain binary patching fix Lendacky, Thomas
2020-05-06 16:33 ` [PATCH v2 1/3] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific Lendacky, Thomas
2020-05-06 19:01   ` Laszlo Ersek
2020-05-06 20:37     ` [edk2-devel] " Lendacky, Thomas
2020-05-06 16:33 ` [PATCH v2 2/3] OvmfPkg: Use toolchain appropriate CpuExceptionHandlerLib Lendacky, Thomas
2020-05-06 19:04   ` Laszlo Ersek
2020-05-06 16:33 ` [PATCH v2 3/3] UefiCpuPkg/CpuExceptionHandler: Revert CpuExceptionHandler binary patching Lendacky, Thomas
2020-05-06 19:31   ` Laszlo Ersek
2020-05-06 19:53 ` [PATCH v2 0/3] XCODE5 toolchain binary patching fix Laszlo Ersek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox