[RFT PATCH v2 0/6] UefiCpuPkg, OvmfPkf: Simplify CpuExceptionHandlerLib

[RFT PATCH v2 0/6] UefiCpuPkg, OvmfPkf: Simplify CpuExceptionHandlerLib

[Ard Biesheuvel]

We have a special version of CpuExceptionHandlerLib for XCODE5, whose
linker (LLD) does not permit absolute symbol references in read-only
sections.

Instead of fixing this up at runtime (which is done by writing the fixed
up values to the .text section, which we'd prefer to avoid), pass the
appropriate linker switches to allow these absolute relocations.

Changes since v2:
- pass linker switches to permit absolute relocations in read-only
  regions, and keep all code in .text

Cc: Ni, Ray <ray.ni@intel.com>
Cc: Andrew Fish <afish@apple.com>
Cc: "Kinney, Michael D" <michael.d.kinney@intel.com>
Cc: "Liu, Zhiguang" <zhiguang.liu@intel.com>
Cc: Rebecca Cran <rebecca@bsdio.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>

Ard Biesheuvel (6):
  BaseTools/tools_def XCODE: Link X64 with -read_only_relocs suppress
  BaseTools/tools_def CLANGDWARF: Permit text relocations
  UefiCpuPkg/CpuExceptionHandlerLib: Use single SEC/PEI version
  UefiCpuPkg/CpuExceptionHandlerLib: Remove needless runtime fixups
  OvmfPkg: Drop special Xcode5 version of exception handler library
  UefiCpuPkg/CpuExceptionHandlerLib: Drop special XCODE5 version

 BaseTools/Conf/tools_def.template                                                                                |   8 +-
 OvmfPkg/AmdSev/AmdSevX64.dsc                                                                                     |   4 -
 OvmfPkg/CloudHv/CloudHvX64.dsc                                                                                   |   4 -
 OvmfPkg/IntelTdx/IntelTdxX64.dsc                                                                                 |   4 -
 OvmfPkg/Microvm/MicrovmX64.dsc                                                                                   |   4 -
 OvmfPkg/OvmfPkgIa32.dsc                                                                                          |   4 -
 OvmfPkg/OvmfPkgIa32X64.dsc                                                                                       |   4 -
 OvmfPkg/OvmfPkgX64.dsc                                                                                           |   4 -
 OvmfPkg/OvmfXen.dsc                                                                                              |   4 -
 UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf                                          |   2 +-
 UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf                                          |   2 +-
 UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf                                       |   2 +-
 UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf                                          |   2 +-
 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm                                           |  92 ++++++++++++++---
 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/{Xcode5ExceptionHandlerAsm.nasm => SecPeiExceptionHandlerAsm.nasm} | 103 +++-----------------
 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf                                 |  65 ------------
 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni                                 |  18 ----
 17 files changed, 98 insertions(+), 228 deletions(-)
 rename UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/{Xcode5ExceptionHandlerAsm.nasm => SecPeiExceptionHandlerAsm.nasm} (70%)
 delete mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
 delete mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni

-- 
2.39.2

[RFT PATCH v2 1/6] BaseTools/tools_def XCODE: Link X64 with -read_only_relocs suppress

[Ard Biesheuvel]

Earlier XCODE versions did not support the -read_only_relocs suppress
linker option, which suppresses errors resulting from absolute
relocations emitted into read-only sections when building PIE
executables.

This requires a rather messy workaround in the CPU exception handler
libraries, to permit absolute relocations in code that may get copied
from a template in some cases.

Fortunately, this seems to be permitted now, so add the option for X64
as well (it was already present for IA32).

This will allows us to simplify the CPU exception handler libraries in
subsequent patches.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 BaseTools/Conf/tools_def.template | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index ae43101853870c6d..1855f1038b1571e4 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -3010,9 +3010,9 @@ RELEASE_XCODE5_IA32_CC_FLAGS   = -arch i386 -c    -Os       -Wall -Werror -inclu
 ##################
 # X64 definitions
 ##################
-  DEBUG_XCODE5_X64_DLINK_FLAGS      = -arch x86_64 -u _$(IMAGE_ENTRY_POINT) -e _$(IMAGE_ENTRY_POINT) -preload -segalign 0x20  -pie -all_load -dead_strip -seg1addr 0x240 -map $(DEST_DIR_DEBUG)/$(BASE_NAME).map
-  NOOPT_XCODE5_X64_DLINK_FLAGS      = -arch x86_64 -u _$(IMAGE_ENTRY_POINT) -e _$(IMAGE_ENTRY_POINT) -preload -segalign 0x20  -pie -all_load -dead_strip -seg1addr 0x240 -map $(DEST_DIR_DEBUG)/$(BASE_NAME).map
-RELEASE_XCODE5_X64_DLINK_FLAGS      = -arch x86_64 -u _$(IMAGE_ENTRY_POINT) -e _$(IMAGE_ENTRY_POINT) -preload -segalign 0x20  -pie -all_load -dead_strip -seg1addr 0x240 -map $(DEST_DIR_DEBUG)/$(BASE_NAME).map
+  DEBUG_XCODE5_X64_DLINK_FLAGS      = -arch x86_64 -u _$(IMAGE_ENTRY_POINT) -e _$(IMAGE_ENTRY_POINT) -preload -segalign 0x20  -pie -all_load -dead_strip -seg1addr 0x240 -read_only_relocs suppress -map $(DEST_DIR_DEBUG)/$(BASE_NAME).map
+  NOOPT_XCODE5_X64_DLINK_FLAGS      = -arch x86_64 -u _$(IMAGE_ENTRY_POINT) -e _$(IMAGE_ENTRY_POINT) -preload -segalign 0x20  -pie -all_load -dead_strip -seg1addr 0x240 -read_only_relocs suppress -map $(DEST_DIR_DEBUG)/$(BASE_NAME).map
+RELEASE_XCODE5_X64_DLINK_FLAGS      = -arch x86_64 -u _$(IMAGE_ENTRY_POINT) -e _$(IMAGE_ENTRY_POINT) -preload -segalign 0x20  -pie -all_load -dead_strip -seg1addr 0x240 -read_only_relocs suppress -map $(DEST_DIR_DEBUG)/$(BASE_NAME).map
 
 *_XCODE5_X64_SLINK_FLAGS      = -static -o
   DEBUG_XCODE5_X64_ASM_FLAGS  = -arch x86_64 -g
-- 
2.39.2

[RFT PATCH v2 2/6] BaseTools/tools_def CLANGDWARF: Permit text relocations

[Ard Biesheuvel]

We rely on PIE executables to get the codegen that is suitable for
PE/COFF conversion where the resulting executables can be loaded
anywhere in the address space.

However, ELF linkers may default to disallowing text relocations in PIE
executables, as this would require text segments to be updated at
runtime, which is bad for security and increases the copy-on-write
footprint of ELF executables and shared libraries.

However, none of those concerns apply to PE/COFF executables in the
context of EFI, which are copied into memory rather than mmap()'ed, and
fixed up by the loader before launch.

So pass -z notext to the LLD linker to permit runtime relocations in
read-only sections.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 BaseTools/Conf/tools_def.template | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/BaseTools/Conf/tools_def.template b/BaseTools/Conf/tools_def.template
index 1855f1038b1571e4..0c6fc4a3011c8d5c 100755
--- a/BaseTools/Conf/tools_def.template
+++ b/BaseTools/Conf/tools_def.template
@@ -2870,7 +2870,7 @@ DEFINE CLANGDWARF_X64_PREFIX        = ENV(CLANG_BIN)
 DEFINE CLANGDWARF_IA32_X64_DLINK_COMMON   = -nostdlib -Wl,-q,--gc-sections -z max-page-size=0x40
 DEFINE CLANGDWARF_DLINK2_FLAGS_COMMON     = -Wl,--script=$(EDK_TOOLS_PATH)/Scripts/ClangBase.lds
 DEFINE CLANGDWARF_IA32_X64_ASLDLINK_FLAGS = DEF(CLANGDWARF_IA32_X64_DLINK_COMMON) -Wl,--defsym=PECOFF_HEADER_SIZE=0 DEF(CLANGDWARF_DLINK2_FLAGS_COMMON) -Wl,--entry,ReferenceAcpiTable -u ReferenceAcpiTable
-DEFINE CLANGDWARF_IA32_X64_DLINK_FLAGS    = DEF(CLANGDWARF_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map,--whole-archive
+DEFINE CLANGDWARF_IA32_X64_DLINK_FLAGS    = DEF(CLANGDWARF_IA32_X64_DLINK_COMMON) -Wl,--entry,$(IMAGE_ENTRY_POINT) -u $(IMAGE_ENTRY_POINT) -Wl,-Map,$(DEST_DIR_DEBUG)/$(BASE_NAME).map,--whole-archive -Wl,-z,notext
 DEFINE CLANGDWARF_IA32_DLINK2_FLAGS       = -Wl,--defsym=PECOFF_HEADER_SIZE=0x220 DEF(CLANGDWARF_DLINK2_FLAGS_COMMON)
 DEFINE CLANGDWARF_X64_DLINK2_FLAGS        = -Wl,--defsym=PECOFF_HEADER_SIZE=0x228 DEF(CLANGDWARF_DLINK2_FLAGS_COMMON)
 
-- 
2.39.2

[RFT PATCH v2 3/6] UefiCpuPkg/CpuExceptionHandlerLib: Use single SEC/PEI version

[Ard Biesheuvel]

Currently, we use the non-Xcode5 version of ExceptionHandlerAsm.nasm
only for the SEC and PEI phases, and this version was not compatible
with the XCODE or LLD linkers, which do not permit absolute relocations
in read-only sections by default. This has been fixed now, so we can use
it for all toolchains.

Let's rename the .nasm file to reflect the fact that is used for the
SecPei flavor of this library only, and while at it, remove some
unnecessary absolute references.

Since this makes the generic version compatible with the XCODE, let's
use this [smaller] version for XCODE5 builds too.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf                                 | 2 +-
 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/{ExceptionHandlerAsm.nasm => SecPeiExceptionHandlerAsm.nasm} | 7 +++----
 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf                           | 2 +-
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
index df44371fe018e06d..10c5c5f2e5d203f6 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
@@ -28,7 +28,7 @@ [Sources.Ia32]
   Ia32/ArchInterruptDefs.h
 
 [Sources.X64]
-  X64/ExceptionHandlerAsm.nasm
+  X64/SecPeiExceptionHandlerAsm.nasm
   X64/ArchExceptionHandler.c
   X64/ArchInterruptDefs.h
 
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/SecPeiExceptionHandlerAsm.nasm
similarity index 95%
rename from UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
rename to UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/SecPeiExceptionHandlerAsm.nasm
index aaf8d622e6f3b8f1..585298768a66af6a 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/SecPeiExceptionHandlerAsm.nasm
@@ -276,8 +276,7 @@ DrFinish:
     ; and make sure RSP is 16-byte aligned
     ;
     sub     rsp, 4 * 8 + 8
-    mov     rax, ASM_PFX(CommonExceptionHandler)
-    call    rax
+    call    ASM_PFX(CommonExceptionHandler)
     add     rsp, 4 * 8 + 8
 
     cli
@@ -384,10 +383,10 @@ DoIret:
 ; comments here for definition of address map
 global ASM_PFX(AsmGetTemplateAddressMap)
 ASM_PFX(AsmGetTemplateAddressMap):
-    mov     rax, AsmIdtVectorBegin
+    lea     rax, [AsmIdtVectorBegin]
     mov     qword [rcx], rax
     mov     qword [rcx + 0x8],  (AsmIdtVectorEnd - AsmIdtVectorBegin) / 32
-    mov     rax, HookAfterStubHeaderBegin
+    lea     rax, [HookAfterStubHeaderBegin]
     mov     qword [rcx + 0x10], rax
     ret
 
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
index 619b39d7f1de9ae3..c58fbb0d74500e48 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
@@ -33,7 +33,7 @@ [Sources.Ia32]
   Ia32/ArchInterruptDefs.h
 
 [Sources.X64]
-  X64/Xcode5ExceptionHandlerAsm.nasm
+  X64/SecPeiExceptionHandlerAsm.nasm
   X64/ArchExceptionHandler.c
   X64/ArchInterruptDefs.h
 
-- 
2.39.2

[RFT PATCH v2 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Remove needless runtime fixups

[Ard Biesheuvel]

Recent versions of the XCODE linker can be instructed to permit text
relocations, so we no longer have to work around this, which is
especially nice as our workaround assumes that the .text section is
mapped both writable and executable at the same time.

So remove the runtime fixups and instead, just emit the absolute
references into the .text section.

While at it, rename the Xcode5ExceptionHandlerAsm.nasm source file and
drop the Xcode5 prefix: this code is used by other toolchains too.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf                                    |  2 +-
 UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf                                    |  2 +-
 UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf                                    |  2 +-
 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/{Xcode5ExceptionHandlerAsm.nasm => ExceptionHandlerAsm.nasm} | 18 ++----------------
 4 files changed, 5 insertions(+), 19 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
index d0f82095cf926e99..1b2dde746d154706 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/Xcode5ExceptionHandlerAsm.nasm
+  X64/ExceptionHandlerAsm.nasm
   X64/ArchExceptionHandler.c
   X64/ArchInterruptDefs.h
 
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
index 5339f8e604045801..86248cea3e97cedb 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/Xcode5ExceptionHandlerAsm.nasm
+  X64/ExceptionHandlerAsm.nasm
   X64/ArchExceptionHandler.c
   X64/ArchInterruptDefs.h
 
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
index 8f8a5dab79303f87..0eed594be8660302 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/Xcode5ExceptionHandlerAsm.nasm
+  X64/ExceptionHandlerAsm.nasm
   X64/ArchExceptionHandler.c
   X64/ArchInterruptDefs.h
 
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
similarity index 92%
rename from UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
rename to UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
index 957478574253e619..10af4cfcdb6b1ea2 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
@@ -59,7 +59,7 @@ AsmIdtVectorBegin:
 %rep  256
     push    strict dword %[Vector] ; This instruction pushes sign-extended 8-byte value on stack
     push    rax
-    mov     rax, strict qword 0    ; mov     rax, ASM_PFX(CommonInterruptEntry)
+    mov     rax, ASM_PFX(CommonInterruptEntry)
     jmp     rax
 %assign Vector Vector+1
 %endrep
@@ -69,8 +69,7 @@ HookAfterStubHeaderBegin:
     push    strict dword 0      ; 0 will be fixed
 VectorNum:
     push    rax
-    mov     rax, strict qword 0 ;     mov     rax, HookAfterStubHeaderEnd
-JmpAbsoluteAddress:
+    mov     rax, HookAfterStubHeaderEnd
     jmp     rax
 HookAfterStubHeaderEnd:
     mov     rax, rsp
@@ -456,19 +455,6 @@ ASM_PFX(AsmGetTemplateAddressMap):
     mov     qword [rcx + 0x8],  (AsmIdtVectorEnd - AsmIdtVectorBegin) / 256
     lea     rax, [HookAfterStubHeaderBegin]
     mov     qword [rcx + 0x10], rax
-
-; Fix up CommonInterruptEntry address
-    lea    rax, [ASM_PFX(CommonInterruptEntry)]
-    lea    rcx, [AsmIdtVectorBegin]
-%rep  256
-    mov    qword [rcx + (JmpAbsoluteAddress - 8 - HookAfterStubHeaderBegin)], rax
-    add    rcx, (AsmIdtVectorEnd - AsmIdtVectorBegin) / 256
-%endrep
-; Fix up HookAfterStubHeaderEnd
-    lea    rax, [HookAfterStubHeaderEnd]
-    lea    rcx, [JmpAbsoluteAddress]
-    mov    qword [rcx - 8], rax
-
     ret
 
 ;-------------------------------------------------------------------------------------
-- 
2.39.2

[RFT PATCH v2 5/6] OvmfPkg: Drop special Xcode5 version of exception handler library

[Ard Biesheuvel]

The generic and XCODE5 versions of this library are now identical, so
drop the special case. The library will be removed entirely in a
subsequent patch.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 OvmfPkg/AmdSev/AmdSevX64.dsc     | 4 ----
 OvmfPkg/CloudHv/CloudHvX64.dsc   | 4 ----
 OvmfPkg/IntelTdx/IntelTdxX64.dsc | 4 ----
 OvmfPkg/Microvm/MicrovmX64.dsc   | 4 ----
 OvmfPkg/OvmfPkgIa32.dsc          | 4 ----
 OvmfPkg/OvmfPkgIa32X64.dsc       | 4 ----
 OvmfPkg/OvmfPkgX64.dsc           | 4 ----
 OvmfPkg/OvmfXen.dsc              | 4 ----
 8 files changed, 32 deletions(-)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index c005e474dd826759..943c4eed9831a1c5 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -224,11 +224,7 @@ [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
   CcExitLib|OvmfPkg/Library/CcExitLib/SecCcExitLib.inf
   MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf
 
diff --git a/OvmfPkg/CloudHv/CloudHvX64.dsc b/OvmfPkg/CloudHv/CloudHvX64.dsc
index b9820cc14bee0693..cc2dd925bc940ea8 100644
--- a/OvmfPkg/CloudHv/CloudHvX64.dsc
+++ b/OvmfPkg/CloudHv/CloudHvX64.dsc
@@ -270,11 +270,7 @@ [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
   CcExitLib|OvmfPkg/Library/CcExitLib/SecCcExitLib.inf
   MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf
 
diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.dsc b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
index 5c56858d063b96bf..f734409055400859 100644
--- a/OvmfPkg/IntelTdx/IntelTdxX64.dsc
+++ b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
@@ -238,11 +238,7 @@ [LibraryClasses.common.SEC]
   ExtractGuidedSectionLib|MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSectionLib.inf
   PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
   MemoryAllocationLib|EmbeddedPkg/Library/PrePiMemoryAllocationLib/PrePiMemoryAllocationLib.inf
-!if $(TOOL_CHAIN_TAG) == "XCODE5"
-  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
-!else
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
-!endif
   CcExitLib|OvmfPkg/Library/CcExitLib/SecCcExitLib.inf
   MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf
   PrePiHobListPointerLib|OvmfPkg/IntelTdx/PrePiHobListPointerLibTdx/PrePiHobListPointerLibTdx.inf
diff --git a/OvmfPkg/Microvm/MicrovmX64.dsc b/OvmfPkg/Microvm/MicrovmX64.dsc
index 384b0b7afc74e90f..e9aab515592ffcec 100644
--- a/OvmfPkg/Microvm/MicrovmX64.dsc
+++ b/OvmfPkg/Microvm/MicrovmX64.dsc
@@ -272,11 +272,7 @@ [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
   CcExitLib|OvmfPkg/Library/CcExitLib/SecCcExitLib.inf
   MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf
 
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index a6db902f54ece86f..86177bb948999435 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -275,11 +275,7 @@ [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
   MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf
 
 [LibraryClasses.common.PEI_CORE]
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 076fc0353de02aaa..065b5445064712d9 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -280,11 +280,7 @@ [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
   MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf
 
 [LibraryClasses.common.PEI_CORE]
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index b2f3d14cd94d5fff..3d405cd4ade07900 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -297,11 +297,7 @@ [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" || $(TOOL_CHAIN_TAG) == "CLANGDWARF"
-  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
-!else
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
-!endif
   CcExitLib|OvmfPkg/Library/CcExitLib/SecCcExitLib.inf
   MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf
   CcProbeLib|OvmfPkg/Library/CcProbeLib/SecPeiCcProbeLib.inf
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 990225d2dd05d3a8..8bfc16c2d3d6aabe 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -247,11 +247,7 @@ [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
   MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLib.inf
 
 [LibraryClasses.common.PEI_CORE]
-- 
2.39.2

[RFT PATCH v2 6/6] UefiCpuPkg/CpuExceptionHandlerLib: Drop special XCODE5 version

[Ard Biesheuvel]

This library is no longer used or needed, so let's remove it.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf | 65 --------------------
 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni | 18 ------
 2 files changed, 83 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
deleted file mode 100644
index c58fbb0d74500e48..0000000000000000
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
+++ /dev/null
@@ -1,65 +0,0 @@
-## @file
-#  CPU Exception Handler library instance for SEC/PEI modules.
-#
-#  Copyright (C) 2020, Advanced Micro Devices, Inc. All rights reserved.<BR>
-#  Copyright (c) 2012 - 2022, 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/SecPeiExceptionHandlerAsm.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
-  CcExitLib
-
-[Pcd]
-  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuStackSwitchExceptionList
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize
-
-[FeaturePcd]
-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ## CONSUMES
-
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni
deleted file mode 100644
index a63b25f39d992775..0000000000000000
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni
+++ /dev/null
@@ -1,18 +0,0 @@
-// /** @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) 2020, Advanced Micro Devices, Inc. All rights reserved.<BR>
-// 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.39.2

Re: [edk2-devel] [RFT PATCH v2 1/6] BaseTools/tools_def XCODE: Link X64 with -read_only_relocs suppress

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 1569 bytes --]

Hi Ard,

Sorry, I cannot preserve the CC list as the groups.io interface doesn't seem to allow it. Can you please CC me on future revisions?

This patch will badly corrupt binaries. I cannot cite a source right now (if you want me to, please remind me in your response, so I can look it up tomorrow), but for X64 (but not IA32, which is why this is enabled there), relocs are relative to the first *writable* segment. In other words, any relocation to __TEXT will badly corrupt binaries this way.

In AUDK, we support this with two essential changes. The first is that we always generate a writable dummy segment at the beginning of the address space [1], making the relocs relative to the image base. The second is that in ocmtoc, our fork of the abandoned (and pretty badly-bugged) Apple mtoc, we explicitly require this segment to be present and verify its virtual address is the minimum virtual address [2]. It is then omitted from the conversion process [3]. I suggest you replicate these changes and fully switch to ocmtoc for XCODE5 builds.

Best regards,
Marvin

[1]
https://github.com/acidanthera/audk/blob/c382e9c571c7d5f39ba53b46a0c723c7943f33c5/BaseTools/Conf/tools_def.template#L2976-L2988

[2]
https://github.com/acidanthera/ocmtoc/blob/b0152c51beae264770c3faf0d213f9594ee043be/efitools/mtoc.c#L1097-L1123
https://github.com/acidanthera/ocmtoc/blob/b0152c51beae264770c3faf0d213f9594ee043be/efitools/mtoc.c#L1204-L1214

[3]
https://github.com/acidanthera/ocmtoc/blob/b0152c51beae264770c3faf0d213f9594ee043be/efitools/mtoc.c#L1307-L1311

[-- Attachment #2: Type: text/html, Size: 2165 bytes --]

Re: [edk2-devel] [RFT PATCH v2 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Remove needless runtime fixups

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 5883 bytes --]

Hi Ard,

Thanks for these important updates!

On Thu, Mar 30, 2023 at 02:21 PM, Ard Biesheuvel wrote:

> 
> Recent versions of the XCODE linker can be instructed to permit text
> relocations, so we no longer have to work around this, which is
> especially nice as our workaround assumes that the .text section is
> mapped both writable and executable at the same time.
> 
> So remove the runtime fixups and instead, just emit the absolute
> references into the .text section.
> 
> While at it, rename the Xcode5ExceptionHandlerAsm.nasm source file and
> drop the Xcode5 prefix: this code is used by other toolchains too.
> 
> Signed-off-by: Ard Biesheuvel <ardb@...>
> ---
> UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf =
> | 2 +-
> UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf =
> | 2 +-
> UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf =
> | 2 +-
> UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/{Xcode5ExceptionHandlerAsm.n=
> 
> asm =3D> ExceptionHandlerAsm.nasm} | 18 ++----------------
> 4 files changed, 5 insertions(+), 19 deletions(-)
> 
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandl=
> erLib.inf
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandle=
> rLib.inf
> index d0f82095cf926e99..1b2dde746d154706 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.i=
> nf
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.i=
> nf
> @@ -28,7 +28,7 @@ [Sources.Ia32]
> Ia32/ArchInterruptDefs.h=0D
> =0D
> [Sources.X64]=0D
> - X64/Xcode5ExceptionHandlerAsm.nasm=0D
> + X64/ExceptionHandlerAsm.nasm=0D
> X64/ArchExceptionHandler.c=0D
> X64/ArchInterruptDefs.h=0D
> =0D
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandl=
> erLib.inf
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandle=
> rLib.inf
> index 5339f8e604045801..86248cea3e97cedb 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.i=
> nf
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.i=
> nf
> @@ -28,7 +28,7 @@ [Sources.Ia32]
> Ia32/ArchInterruptDefs.h=0D
> =0D
> [Sources.X64]=0D
> - X64/Xcode5ExceptionHandlerAsm.nasm=0D
> + X64/ExceptionHandlerAsm.nasm=0D
> X64/ArchExceptionHandler.c=0D
> X64/ArchInterruptDefs.h=0D
> =0D
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandl=
> erLib.inf
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandle=
> rLib.inf
> index 8f8a5dab79303f87..0eed594be8660302 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.i=
> nf
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.i=
> nf
> @@ -28,7 +28,7 @@ [Sources.Ia32]
> Ia32/ArchInterruptDefs.h=0D
> =0D
> [Sources.X64]=0D
> - X64/Xcode5ExceptionHandlerAsm.nasm=0D
> + X64/ExceptionHandlerAsm.nasm=0D
> X64/ArchExceptionHandler.c=0D
> X64/ArchInterruptDefs.h=0D
> =0D
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionH=
> andlerAsm.nasm
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHan=
> dlerAsm.nasm
> similarity index 92%
> rename from
> UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHa=
> ndlerAsm.nasm
> rename to
> UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm=
> .nasm
> index 957478574253e619..10af4cfcdb6b1ea2 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerA=
> sm.nasm
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> @@ -59,7 +59,7 @@ AsmIdtVectorBegin:
> %rep 256=0D
> push strict dword %[Vector] ; This instruction pushes sign-extended=
> 8-byte value on stack=0D
> push rax=0D
> - mov rax, strict qword 0 ; mov rax, ASM_PFX(CommonInterruptE=
> ntry)=0D
> + mov rax, ASM_PFX(CommonInterruptEntry)=0D

I'm fairly certain this can be a relative reference, as the code doesn't seem to be copied away (as opposed to HookAfterStubHeaderBegin). If true, this would save 256 relocs, which sounds quite nice. Would you mind verifying? Thanks!

If you want to take things a step further, this is how we merged the SEC/PEI and DXE/SMM variants: https://github.com/acidanthera/audk/commit/9646f2c4bc0475e0635b60b7c7828999a1d40dcb ( https://github.com/acidanthera/audk/commit/9646f2c4bc0475e0635b60b7c7828999a1d40dcb )
(There is a bug visible in the changes, which was fixed only later, so take this only as a PoC).

Best regards,
Marvin

> 
> jmp rax=0D
> %assign Vector Vector+1=0D
> %endrep=0D
> @@ -69,8 +69,7 @@ HookAfterStubHeaderBegin:
> push strict dword 0 ; 0 will be fixed=0D
> VectorNum:=0D
> push rax=0D
> - mov rax, strict qword 0 ; mov rax, HookAfterStubHeaderEnd=
> =0D
> -JmpAbsoluteAddress:=0D
> + mov rax, HookAfterStubHeaderEnd=0D
> jmp rax=0D
> HookAfterStubHeaderEnd:=0D
> mov rax, rsp=0D
> @@ -456,19 +455,6 @@ ASM_PFX(AsmGetTemplateAddressMap):
> mov qword [rcx + 0x8], (AsmIdtVectorEnd - AsmIdtVectorBegin) / 25=
> 6=0D
> lea rax, [HookAfterStubHeaderBegin]=0D
> mov qword [rcx + 0x10], rax=0D
> -=0D
> -; Fix up CommonInterruptEntry address=0D
> - lea rax, [ASM_PFX(CommonInterruptEntry)]=0D
> - lea rcx, [AsmIdtVectorBegin]=0D
> -%rep 256=0D
> - mov qword [rcx + (JmpAbsoluteAddress - 8 - HookAfterStubHeaderBegin=
> )], rax=0D
> - add rcx, (AsmIdtVectorEnd - AsmIdtVectorBegin) / 256=0D
> -%endrep=0D
> -; Fix up HookAfterStubHeaderEnd=0D
> - lea rax, [HookAfterStubHeaderEnd]=0D
> - lea rcx, [JmpAbsoluteAddress]=0D
> - mov qword [rcx - 8], rax=0D
> -=0D
> ret=0D
> =0D
> ;-------------------------------------------------------------------------=
> 
> ------------=0D
> --=20
> 2.39.2

[-- Attachment #2: Type: text/html, Size: 6196 bytes --]

Re: [edk2-devel] [RFT PATCH v2 5/6] OvmfPkg: Drop special Xcode5 version of exception handler library

[Yao, Jiewen]

Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel
> Sent: Friday, March 31, 2023 5:21 AM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb@kernel.org>; Ni; Ni, Ray <ray.ni@intel.com>;
> Andrew Fish <afish@apple.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>;
> Rebecca Cran <rebecca@bsdio.com>; Tom Lendacky
> <thomas.lendacky@amd.com>
> Subject: [edk2-devel] [RFT PATCH v2 5/6] OvmfPkg: Drop special Xcode5
> version of exception handler library
> 
> The generic and XCODE5 versions of this library are now identical, so
> drop the special case. The library will be removed entirely in a
> subsequent patch.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  OvmfPkg/AmdSev/AmdSevX64.dsc     | 4 ----
>  OvmfPkg/CloudHv/CloudHvX64.dsc   | 4 ----
>  OvmfPkg/IntelTdx/IntelTdxX64.dsc | 4 ----
>  OvmfPkg/Microvm/MicrovmX64.dsc   | 4 ----
>  OvmfPkg/OvmfPkgIa32.dsc          | 4 ----
>  OvmfPkg/OvmfPkgIa32X64.dsc       | 4 ----
>  OvmfPkg/OvmfPkgX64.dsc           | 4 ----
>  OvmfPkg/OvmfXen.dsc              | 4 ----
>  8 files changed, 32 deletions(-)
> 
> diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc
> b/OvmfPkg/AmdSev/AmdSevX64.dsc
> index c005e474dd826759..943c4eed9831a1c5 100644
> --- a/OvmfPkg/AmdSev/AmdSevX64.dsc
> +++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
> @@ -224,11 +224,7 @@ [LibraryClasses.common.SEC]
>    PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
> 
> 
> PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/P
> eiServicesTablePointerLibIdt.inf
> 
> 
> MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemory
> AllocationLib.inf
> 
> -!if $(TOOL_CHAIN_TAG) == "XCODE5"
> 
> -
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcod
> e5SecPeiCpuExceptionHandlerLib.inf
> 
> -!else
> 
> 
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPe
> iCpuExceptionHandlerLib.inf
> 
> -!endif
> 
>    CcExitLib|OvmfPkg/Library/CcExitLib/SecCcExitLib.inf
> 
> 
> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncry
> ptSevLib.inf
> 
> 
> 
> diff --git a/OvmfPkg/CloudHv/CloudHvX64.dsc
> b/OvmfPkg/CloudHv/CloudHvX64.dsc
> index b9820cc14bee0693..cc2dd925bc940ea8 100644
> --- a/OvmfPkg/CloudHv/CloudHvX64.dsc
> +++ b/OvmfPkg/CloudHv/CloudHvX64.dsc
> @@ -270,11 +270,7 @@ [LibraryClasses.common.SEC]
>    PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
> 
> 
> PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/P
> eiServicesTablePointerLibIdt.inf
> 
> 
> MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemory
> AllocationLib.inf
> 
> -!if $(TOOL_CHAIN_TAG) == "XCODE5"
> 
> -
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcod
> e5SecPeiCpuExceptionHandlerLib.inf
> 
> -!else
> 
> 
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPe
> iCpuExceptionHandlerLib.inf
> 
> -!endif
> 
>    CcExitLib|OvmfPkg/Library/CcExitLib/SecCcExitLib.inf
> 
> 
> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncry
> ptSevLib.inf
> 
> 
> 
> diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.dsc
> b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
> index 5c56858d063b96bf..f734409055400859 100644
> --- a/OvmfPkg/IntelTdx/IntelTdxX64.dsc
> +++ b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
> @@ -238,11 +238,7 @@ [LibraryClasses.common.SEC]
> 
> ExtractGuidedSectionLib|MdePkg/Library/BaseExtractGuidedSectionLib/Bas
> eExtractGuidedSectionLib.inf
> 
> 
> PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/P
> eiServicesTablePointerLibIdt.inf
> 
> 
> MemoryAllocationLib|EmbeddedPkg/Library/PrePiMemoryAllocationLib/Pre
> PiMemoryAllocationLib.inf
> 
> -!if $(TOOL_CHAIN_TAG) == "XCODE5"
> 
> -
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcod
> e5SecPeiCpuExceptionHandlerLib.inf
> 
> -!else
> 
> 
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPe
> iCpuExceptionHandlerLib.inf
> 
> -!endif
> 
>    CcExitLib|OvmfPkg/Library/CcExitLib/SecCcExitLib.inf
> 
> 
> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncry
> ptSevLib.inf
> 
> 
> PrePiHobListPointerLib|OvmfPkg/IntelTdx/PrePiHobListPointerLibTdx/PrePiH
> obListPointerLibTdx.inf
> 
> diff --git a/OvmfPkg/Microvm/MicrovmX64.dsc
> b/OvmfPkg/Microvm/MicrovmX64.dsc
> index 384b0b7afc74e90f..e9aab515592ffcec 100644
> --- a/OvmfPkg/Microvm/MicrovmX64.dsc
> +++ b/OvmfPkg/Microvm/MicrovmX64.dsc
> @@ -272,11 +272,7 @@ [LibraryClasses.common.SEC]
>    PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
> 
> 
> PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/P
> eiServicesTablePointerLibIdt.inf
> 
> 
> MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemory
> AllocationLib.inf
> 
> -!if $(TOOL_CHAIN_TAG) == "XCODE5"
> 
> -
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcod
> e5SecPeiCpuExceptionHandlerLib.inf
> 
> -!else
> 
> 
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPe
> iCpuExceptionHandlerLib.inf
> 
> -!endif
> 
>    CcExitLib|OvmfPkg/Library/CcExitLib/SecCcExitLib.inf
> 
> 
> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncry
> ptSevLib.inf
> 
> 
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index a6db902f54ece86f..86177bb948999435 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -275,11 +275,7 @@ [LibraryClasses.common.SEC]
>    PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
> 
> 
> PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/P
> eiServicesTablePointerLibIdt.inf
> 
> 
> MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemory
> AllocationLib.inf
> 
> -!if $(TOOL_CHAIN_TAG) == "XCODE5"
> 
> -
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcod
> e5SecPeiCpuExceptionHandlerLib.inf
> 
> -!else
> 
> 
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPe
> iCpuExceptionHandlerLib.inf
> 
> -!endif
> 
> 
> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncry
> ptSevLib.inf
> 
> 
> 
>  [LibraryClasses.common.PEI_CORE]
> 
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 076fc0353de02aaa..065b5445064712d9 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -280,11 +280,7 @@ [LibraryClasses.common.SEC]
>    PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
> 
> 
> PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/P
> eiServicesTablePointerLibIdt.inf
> 
> 
> MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemory
> AllocationLib.inf
> 
> -!if $(TOOL_CHAIN_TAG) == "XCODE5"
> 
> -
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcod
> e5SecPeiCpuExceptionHandlerLib.inf
> 
> -!else
> 
> 
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPe
> iCpuExceptionHandlerLib.inf
> 
> -!endif
> 
> 
> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncry
> ptSevLib.inf
> 
> 
> 
>  [LibraryClasses.common.PEI_CORE]
> 
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index b2f3d14cd94d5fff..3d405cd4ade07900 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -297,11 +297,7 @@ [LibraryClasses.common.SEC]
>    PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
> 
> 
> PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/P
> eiServicesTablePointerLibIdt.inf
> 
> 
> MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemory
> AllocationLib.inf
> 
> -!if $(TOOL_CHAIN_TAG) == "XCODE5" || $(TOOL_CHAIN_TAG) ==
> "CLANGDWARF"
> 
> -
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcod
> e5SecPeiCpuExceptionHandlerLib.inf
> 
> -!else
> 
> 
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPe
> iCpuExceptionHandlerLib.inf
> 
> -!endif
> 
>    CcExitLib|OvmfPkg/Library/CcExitLib/SecCcExitLib.inf
> 
> 
> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncry
> ptSevLib.inf
> 
>    CcProbeLib|OvmfPkg/Library/CcProbeLib/SecPeiCcProbeLib.inf
> 
> diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
> index 990225d2dd05d3a8..8bfc16c2d3d6aabe 100644
> --- a/OvmfPkg/OvmfXen.dsc
> +++ b/OvmfPkg/OvmfXen.dsc
> @@ -247,11 +247,7 @@ [LibraryClasses.common.SEC]
>    PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
> 
> 
> PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/P
> eiServicesTablePointerLibIdt.inf
> 
> 
> MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemory
> AllocationLib.inf
> 
> -!if $(TOOL_CHAIN_TAG) == "XCODE5"
> 
> -
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcod
> e5SecPeiCpuExceptionHandlerLib.inf
> 
> -!else
> 
> 
> CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPe
> iCpuExceptionHandlerLib.inf
> 
> -!endif
> 
> 
> MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncry
> ptSevLib.inf
> 
> 
> 
>  [LibraryClasses.common.PEI_CORE]
> 
> --
> 2.39.2
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#102216):
> https://edk2.groups.io/g/devel/message/102216
> Mute This Topic: https://groups.io/mt/97960768/1772286
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [jiewen.yao@intel.com]
> -=-=-=-=-=-=
> 

Re: [RFT PATCH v2 3/6] UefiCpuPkg/CpuExceptionHandlerLib: Use single SEC/PEI version

[Ni, Ray]

Thanks for the change.

But it doesn't highlight another impact due to this change: CET logic is removed from the SEC/PEI version.
It's not an issue because CET is only enabled in SMM environment today.
But better to highlight the impact in the commit message, and explicitly say that limitation in the SecPeiCpuExceptionHandlerLib.inf file.

Thanks,
Ray

-----Original Message-----
From: Ard Biesheuvel <ardb@kernel.org> 
Sent: Friday, March 31, 2023 5:21 AM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <ardb@kernel.org>; Ni; Ni, Ray <ray.ni@intel.com>; Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>; Rebecca Cran <rebecca@bsdio.com>; Tom Lendacky <thomas.lendacky@amd.com>
Subject: [RFT PATCH v2 3/6] UefiCpuPkg/CpuExceptionHandlerLib: Use single SEC/PEI version

Currently, we use the non-Xcode5 version of ExceptionHandlerAsm.nasm
only for the SEC and PEI phases, and this version was not compatible
with the XCODE or LLD linkers, which do not permit absolute relocations
in read-only sections by default. This has been fixed now, so we can use
it for all toolchains.

Let's rename the .nasm file to reflect the fact that is used for the
SecPei flavor of this library only, and while at it, remove some
unnecessary absolute references.

Since this makes the generic version compatible with the XCODE, let's
use this [smaller] version for XCODE5 builds too.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf                                 | 2 +-
 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/{ExceptionHandlerAsm.nasm => SecPeiExceptionHandlerAsm.nasm} | 7 +++----
 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf                           | 2 +-
 3 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
index df44371fe018e06d..10c5c5f2e5d203f6 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
@@ -28,7 +28,7 @@ [Sources.Ia32]
   Ia32/ArchInterruptDefs.h

 

 [Sources.X64]

-  X64/ExceptionHandlerAsm.nasm

+  X64/SecPeiExceptionHandlerAsm.nasm

   X64/ArchExceptionHandler.c

   X64/ArchInterruptDefs.h

 

diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/SecPeiExceptionHandlerAsm.nasm
similarity index 95%
rename from UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
rename to UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/SecPeiExceptionHandlerAsm.nasm
index aaf8d622e6f3b8f1..585298768a66af6a 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/SecPeiExceptionHandlerAsm.nasm
@@ -276,8 +276,7 @@ DrFinish:
     ; and make sure RSP is 16-byte aligned

     ;

     sub     rsp, 4 * 8 + 8

-    mov     rax, ASM_PFX(CommonExceptionHandler)

-    call    rax

+    call    ASM_PFX(CommonExceptionHandler)

     add     rsp, 4 * 8 + 8

 

     cli

@@ -384,10 +383,10 @@ DoIret:
 ; comments here for definition of address map

 global ASM_PFX(AsmGetTemplateAddressMap)

 ASM_PFX(AsmGetTemplateAddressMap):

-    mov     rax, AsmIdtVectorBegin

+    lea     rax, [AsmIdtVectorBegin]

     mov     qword [rcx], rax

     mov     qword [rcx + 0x8],  (AsmIdtVectorEnd - AsmIdtVectorBegin) / 32

-    mov     rax, HookAfterStubHeaderBegin

+    lea     rax, [HookAfterStubHeaderBegin]

     mov     qword [rcx + 0x10], rax

     ret

 

diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
index 619b39d7f1de9ae3..c58fbb0d74500e48 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
@@ -33,7 +33,7 @@ [Sources.Ia32]
   Ia32/ArchInterruptDefs.h

 

 [Sources.X64]

-  X64/Xcode5ExceptionHandlerAsm.nasm

+  X64/SecPeiExceptionHandlerAsm.nasm

   X64/ArchExceptionHandler.c

   X64/ArchInterruptDefs.h

 

-- 
2.39.2

Re: [edk2-devel] [RFT PATCH v2 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Remove needless runtime fixups

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
Sent: Friday, March 31, 2023 5:21 AM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <ardb@kernel.org>; Ni; Ni, Ray <ray.ni@intel.com>; Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>; Rebecca Cran <rebecca@bsdio.com>; Tom Lendacky <thomas.lendacky@amd.com>
Subject: [edk2-devel] [RFT PATCH v2 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Remove needless runtime fixups

Recent versions of the XCODE linker can be instructed to permit text
relocations, so we no longer have to work around this, which is
especially nice as our workaround assumes that the .text section is
mapped both writable and executable at the same time.

So remove the runtime fixups and instead, just emit the absolute
references into the .text section.

While at it, rename the Xcode5ExceptionHandlerAsm.nasm source file and
drop the Xcode5 prefix: this code is used by other toolchains too.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf                                    |  2 +-
 UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf                                    |  2 +-
 UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf                                    |  2 +-
 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/{Xcode5ExceptionHandlerAsm.nasm => ExceptionHandlerAsm.nasm} | 18 ++----------------
 4 files changed, 5 insertions(+), 19 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
index d0f82095cf926e99..1b2dde746d154706 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/Xcode5ExceptionHandlerAsm.nasm

+  X64/ExceptionHandlerAsm.nasm

   X64/ArchExceptionHandler.c

   X64/ArchInterruptDefs.h

 

diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
index 5339f8e604045801..86248cea3e97cedb 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/Xcode5ExceptionHandlerAsm.nasm

+  X64/ExceptionHandlerAsm.nasm

   X64/ArchExceptionHandler.c

   X64/ArchInterruptDefs.h

 

diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
index 8f8a5dab79303f87..0eed594be8660302 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/Xcode5ExceptionHandlerAsm.nasm

+  X64/ExceptionHandlerAsm.nasm

   X64/ArchExceptionHandler.c

   X64/ArchInterruptDefs.h

 

diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
similarity index 92%
rename from UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
rename to UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
index 957478574253e619..10af4cfcdb6b1ea2 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
@@ -59,7 +59,7 @@ AsmIdtVectorBegin:
 %rep  256

     push    strict dword %[Vector] ; This instruction pushes sign-extended 8-byte value on stack

     push    rax

-    mov     rax, strict qword 0    ; mov     rax, ASM_PFX(CommonInterruptEntry)

+    mov     rax, ASM_PFX(CommonInterruptEntry)

     jmp     rax

 %assign Vector Vector+1

 %endrep

@@ -69,8 +69,7 @@ HookAfterStubHeaderBegin:
     push    strict dword 0      ; 0 will be fixed

 VectorNum:

     push    rax

-    mov     rax, strict qword 0 ;     mov     rax, HookAfterStubHeaderEnd

-JmpAbsoluteAddress:

+    mov     rax, HookAfterStubHeaderEnd

     jmp     rax

 HookAfterStubHeaderEnd:

     mov     rax, rsp

@@ -456,19 +455,6 @@ ASM_PFX(AsmGetTemplateAddressMap):
     mov     qword [rcx + 0x8],  (AsmIdtVectorEnd - AsmIdtVectorBegin) / 256

     lea     rax, [HookAfterStubHeaderBegin]

     mov     qword [rcx + 0x10], rax

-

-; Fix up CommonInterruptEntry address

-    lea    rax, [ASM_PFX(CommonInterruptEntry)]

-    lea    rcx, [AsmIdtVectorBegin]

-%rep  256

-    mov    qword [rcx + (JmpAbsoluteAddress - 8 - HookAfterStubHeaderBegin)], rax

-    add    rcx, (AsmIdtVectorEnd - AsmIdtVectorBegin) / 256

-%endrep

-; Fix up HookAfterStubHeaderEnd

-    lea    rax, [HookAfterStubHeaderEnd]

-    lea    rcx, [JmpAbsoluteAddress]

-    mov    qword [rcx - 8], rax

-

     ret

 

 ;-------------------------------------------------------------------------------------

-- 
2.39.2



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102215): https://edk2.groups.io/g/devel/message/102215
Mute This Topic: https://groups.io/mt/97960766/1712937
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
-=-=-=-=-=-=

Re: [edk2-devel] [RFT PATCH v2 6/6] UefiCpuPkg/CpuExceptionHandlerLib: Drop special XCODE5 version

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
Sent: Friday, March 31, 2023 5:21 AM
To: devel@edk2.groups.io
Cc: Ard Biesheuvel <ardb@kernel.org>; Ni; Ni, Ray <ray.ni@intel.com>; Andrew Fish <afish@apple.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>; Rebecca Cran <rebecca@bsdio.com>; Tom Lendacky <thomas.lendacky@amd.com>
Subject: [edk2-devel] [RFT PATCH v2 6/6] UefiCpuPkg/CpuExceptionHandlerLib: Drop special XCODE5 version

This library is no longer used or needed, so let's remove it.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf | 65 --------------------
 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni | 18 ------
 2 files changed, 83 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
deleted file mode 100644
index c58fbb0d74500e48..0000000000000000
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
+++ /dev/null
@@ -1,65 +0,0 @@
-## @file

-#  CPU Exception Handler library instance for SEC/PEI modules.

-#

-#  Copyright (C) 2020, Advanced Micro Devices, Inc. All rights reserved.<BR>

-#  Copyright (c) 2012 - 2022, 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/SecPeiExceptionHandlerAsm.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

-  CcExitLib

-

-[Pcd]

-  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard

-  gUefiCpuPkgTokenSpaceGuid.PcdCpuStackSwitchExceptionList

-  gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize

-

-[FeaturePcd]

-  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ## CONSUMES

-

diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni
deleted file mode 100644
index a63b25f39d992775..0000000000000000
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni
+++ /dev/null
@@ -1,18 +0,0 @@
-// /** @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) 2020, Advanced Micro Devices, Inc. All rights reserved.<BR>

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



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#102217): https://edk2.groups.io/g/devel/message/102217
Mute This Topic: https://groups.io/mt/97960772/1712937
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
-=-=-=-=-=-=

Re: [edk2-devel] [RFT PATCH v2 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Remove needless runtime fixups

[Ni, Ray]

[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]


diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionH=
andlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHan=
dlerAsm.nasm
similarity index 92%
rename from UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHa=
ndlerAsm.nasm
rename to UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm=
.nasm
index 957478574253e619..10af4cfcdb6b1ea2 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerA=
sm.nasm
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
@@ -59,7 +59,7 @@ AsmIdtVectorBegin:
%rep 256=0D
push strict dword %[Vector] ; This instruction pushes sign-extended=
8-byte value on stack=0D
push rax=0D
- mov rax, strict qword 0 ; mov rax, ASM_PFX(CommonInterruptE=
ntry)=0D
+ mov rax, ASM_PFX(CommonInterruptEntry)=0D

I'm fairly certain this can be a relative reference, as the code doesn't seem to be copied away (as opposed to HookAfterStubHeaderBegin). If true, this would save 256 relocs, which sounds quite nice. Would you mind verifying? Thanks!

Marvin,

You are right the mov can be replaced with “lea”. But, we need to analyze if using lea, the size of each idt stub is fixed or variant (today the size is fixed, so StubSize = TotalSize / 256).

Another thing we need to evaluate is the impact to “Hook after” feature because “hook after” stub is very like the idt stub.

I think we can firstly use Ard’s solution. Later to evaluate to eliminate unnecessary absolute reference.

Thanks,

Ray

[-- Attachment #2: Type: text/html, Size: 3494 bytes --]

Re: [edk2-devel] [RFT PATCH v2 1/6] BaseTools/tools_def XCODE: Link X64 with -read_only_relocs suppress

[Ard Biesheuvel]

Hi Marvin,

Thanks for the context.


On Thu, 30 Mar 2023 at 23:54, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
> Hi Ard,
>
> Sorry, I cannot preserve the CC list as the groups.io interface doesn't seem to allow it. Can you please CC me on future revisions?
>
> This patch will badly corrupt binaries. I cannot cite a source right now (if you want me to, please remind me in your response, so I can look it up tomorrow), but for X64 (but not IA32, which is why this is enabled there), relocs are relative to the first *writable* segment. In other words, any relocation to __TEXT will badly corrupt binaries this way.
>

OMG.

I can't believe how buggy all this stuff is. But I can confirm that
the resulting binaries don't look right, even though they appear to
boot fine. In particular, when I dump the PE relocations using
llvm-readobj --coff-basereloc, I don't see any relocations referring
to the .text section.

> In AUDK, we support this with two essential changes. The first is that we always generate a writable dummy segment at the beginning of the address space [1], making the relocs relative to the image base. The second is that in ocmtoc, our fork of the abandoned (and pretty badly-bugged) Apple mtoc, we explicitly require this segment to be present and verify its virtual address is the minimum virtual address [2]. It is then omitted from the conversion process [3]. I suggest you replicate these changes and fully switch to ocmtoc for XCODE5 builds.
>

I'm not going to do any of that. Instead, I am going to drop this
change, and do the following:

- modify the SecPei version of CpuExceptionHandlerLib to put the
vector templates in .data, as I proposed before. This works around the
issue, and given that SEC/PEI is assumed to be read-only anyway (as it
may execute in place from flash) and does not use page alignment for
the sections due to size constraints, it is reasonable to assume that
.text and .data will be mapped executable anyway.

- update the version that performs the runtime fixups to only do so
when using the XCODE toolchain - we can phase that out once we drop
XCODE support.

Re: [edk2-devel] [RFT PATCH v2 3/6] UefiCpuPkg/CpuExceptionHandlerLib: Use single SEC/PEI version

[Ard Biesheuvel]

On Fri, 31 Mar 2023 at 06:24, Ni, Ray <ray.ni@intel.com> wrote:
>
> Thanks for the change.
>
> But it doesn't highlight another impact due to this change: CET logic is removed from the SEC/PEI version.

No the ordinary SEC/PEI version does not have the CET logic - only the
XCODE version does.

> It's not an issue because CET is only enabled in SMM environment today.
> But better to highlight the impact in the commit message, and explicitly say that limitation in the SecPeiCpuExceptionHandlerLib.inf file.
>

I am going to have to revisit this anyway, but I will put some more
context about this in the commit log.

Re: [edk2-devel] [RFT PATCH v2 3/6] UefiCpuPkg/CpuExceptionHandlerLib: Use single SEC/PEI version

[Ni, Ray]

Thanks!
Overall, the patch greatly simplifies the CpuExceptionHandlerLib.
Even I sometimes am lost of which C/NASM contributes to which instance.

With the enhanced commit log, Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel
> Sent: Friday, March 31, 2023 3:41 PM
> To: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>
> Cc: Andrew Fish <afish@apple.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>;
> Rebecca Cran <rebecca@bsdio.com>; Tom Lendacky
> <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [RFT PATCH v2 3/6]
> UefiCpuPkg/CpuExceptionHandlerLib: Use single SEC/PEI version
> 
> On Fri, 31 Mar 2023 at 06:24, Ni, Ray <ray.ni@intel.com> wrote:
> >
> > Thanks for the change.
> >
> > But it doesn't highlight another impact due to this change: CET logic is
> removed from the SEC/PEI version.
> 
> No the ordinary SEC/PEI version does not have the CET logic - only the
> XCODE version does.
> 
> > It's not an issue because CET is only enabled in SMM environment today.
> > But better to highlight the impact in the commit message, and explicitly say
> that limitation in the SecPeiCpuExceptionHandlerLib.inf file.
> >
> 
> I am going to have to revisit this anyway, but I will put some more
> context about this in the commit log.
> 
> 
> 
> 

Re: [edk2-devel] [RFT PATCH v2 4/6] UefiCpuPkg/CpuExceptionHandlerLib: Remove needless runtime fixups

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 1842 bytes --]

Hi Ray,

Definitely - this is just an optimisation “while we’re at it“, nothing urgent.

Best regards,
Marvin

> On 31. Mar 2023, at 07:09, Ni, Ray <ray.ni@intel.com> wrote:
> 
> 
> 
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionH=
> andlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHan=
> dlerAsm.nasm
> similarity index 92%
> rename from UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHa=
> ndlerAsm.nasm
> rename to UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm=
> .nasm
> index 957478574253e619..10af4cfcdb6b1ea2 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerA=
> sm.nasm
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> @@ -59,7 +59,7 @@ AsmIdtVectorBegin:
> %rep 256=0D
> push strict dword %[Vector] ; This instruction pushes sign-extended=
> 8-byte value on stack=0D
> push rax=0D
> - mov rax, strict qword 0 ; mov rax, ASM_PFX(CommonInterruptE=
> ntry)=0D
> + mov rax, ASM_PFX(CommonInterruptEntry)=0D
> I'm fairly certain this can be a relative reference, as the code doesn't seem to be copied away (as opposed to HookAfterStubHeaderBegin). If true, this would save 256 relocs, which sounds quite nice. Would you mind verifying? Thanks!
> 
> Marvin,
> 
> You are right the mov can be replaced with “lea”. But, we need to analyze if using lea, the size of each idt stub is fixed or variant (today the size is fixed, so StubSize = TotalSize / 256).
> 
> Another thing we need to evaluate is the impact to “Hook after” feature because “hook after” stub is very like the idt stub.
> 
> I think we can firstly use Ard’s solution. Later to evaluate to eliminate unnecessary absolute reference.
> 
> Thanks,
> 
> Ray

[-- Attachment #2: Type: text/html, Size: 3455 bytes --]

Re: [edk2-devel] [RFT PATCH v2 1/6] BaseTools/tools_def XCODE: Link X64 with -read_only_relocs suppress

[Marvin Häuser]

> On 31. Mar 2023, at 09:39, Ard Biesheuvel <ardb@kernel.org> wrote:
> Hi Marvin,
> 
> Thanks for the context.
> 
> 
> On Thu, 30 Mar 2023 at 23:54, Marvin Häuser <mhaeuser@posteo.de> wrote:
>> 
>> Hi Ard,
>> 
>> Sorry, I cannot preserve the CC list as the groups.io interface doesn't seem to allow it. Can you please CC me on future revisions?
>> 
>> This patch will badly corrupt binaries. I cannot cite a source right now (if you want me to, please remind me in your response, so I can look it up tomorrow), but for X64 (but not IA32, which is why this is enabled there), relocs are relative to the first *writable* segment. In other words, any relocation to __TEXT will badly corrupt binaries this way.
> 
> OMG.
> 
> I can't believe how buggy all this stuff is. But I can confirm that
> the resulting binaries don't look right, even though they appear to
> boot fine.

Codegen does not change from the suppress flag, so there will be no additional text relocs beyond those you introduced. As they target the exception handler, I guess you’d need to actively provoke the broken code paths (and may end up with a nice recursion :) ).

> In particular, when I dump the PE relocations using
> llvm-readobj --coff-basereloc, I don't see any relocations referring
> to the .text section.
> 
>> In AUDK, we support this with two essential changes. The first is that we always generate a writable dummy segment at the beginning of the address space [1], making the relocs relative to the image base. The second is that in ocmtoc, our fork of the abandoned (and pretty badly-bugged) Apple mtoc, we explicitly require this segment to be present and verify its virtual address is the minimum virtual address [2]. It is then omitted from the conversion process [3]. I suggest you replicate these changes and fully switch to ocmtoc for XCODE5 builds.
> 
> I'm not going to do any of that. Instead, I am going to drop this
> change, and do the following:
> 
> - modify the SecPei version of CpuExceptionHandlerLib to put the
> vector templates in .data, as I proposed before. This works around the
> issue, and given that SEC/PEI is assumed to be read-only anyway (as it
> may execute in place from flash) and does not use page alignment for
> the sections due to size constraints, it is reasonable to assume that
> .text and .data will be mapped executable anyway.

Well, that assumption is more than fair to make for the status quo platforms, but this is just another rock in the way of doing things the right way (even if it’s just VMs).

Cc Gerd for an OVMF security perspective. Is PEI-time memory protection something you’d be interested in in the future?

> 
> - update the version that performs the runtime fixups to only do so
> when using the XCODE toolchain - we can phase that out once we drop
> XCODE support.

I agree if there’s an actual plan on doing that. We depend on XCODE5 downstream, but I think it would literally be easier for us if the upstream version was dropped than rebasing against hacks that our slightly modded variant does not require.

Cc Andrew and Rebecca. I don’t know anyone else who might still be using XCODE5. Any objections to dropping it? If so, any plans to pick up my proposed changes instead?

Best regards,
Marvin

Re: [edk2-devel] [RFT PATCH v2 1/6] BaseTools/tools_def XCODE: Link X64 with -read_only_relocs suppress

[Ard Biesheuvel]

On Fri, 31 Mar 2023 at 10:29, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> > On 31. Mar 2023, at 09:39, Ard Biesheuvel <ardb@kernel.org> wrote:
> > Hi Marvin,
> >
> > Thanks for the context.
> >
> >
> > On Thu, 30 Mar 2023 at 23:54, Marvin Häuser <mhaeuser@posteo.de> wrote:
> >>
> >> Hi Ard,
> >>
> >> Sorry, I cannot preserve the CC list as the groups.io interface doesn't seem to allow it. Can you please CC me on future revisions?
> >>
> >> This patch will badly corrupt binaries. I cannot cite a source right now (if you want me to, please remind me in your response, so I can look it up tomorrow), but for X64 (but not IA32, which is why this is enabled there), relocs are relative to the first *writable* segment. In other words, any relocation to __TEXT will badly corrupt binaries this way.
> >
> > OMG.
> >
> > I can't believe how buggy all this stuff is. But I can confirm that
> > the resulting binaries don't look right, even though they appear to
> > boot fine.
>
> Codegen does not change from the suppress flag, so there will be no additional text relocs beyond those you introduced. As they target the exception handler, I guess you’d need to actively provoke the broken code paths (and may end up with a nice recursion :) ).
>

I understand that the codegen is the same. I was specifically talking
about the PE relocations, which seem to be lacking entirely.

> > In particular, when I dump the PE relocations using
> > llvm-readobj --coff-basereloc, I don't see any relocations referring
> > to the .text section.
> >
> >> In AUDK, we support this with two essential changes. The first is that we always generate a writable dummy segment at the beginning of the address space [1], making the relocs relative to the image base. The second is that in ocmtoc, our fork of the abandoned (and pretty badly-bugged) Apple mtoc, we explicitly require this segment to be present and verify its virtual address is the minimum virtual address [2]. It is then omitted from the conversion process [3]. I suggest you replicate these changes and fully switch to ocmtoc for XCODE5 builds.
> >
> > I'm not going to do any of that. Instead, I am going to drop this
> > change, and do the following:
> >
> > - modify the SecPei version of CpuExceptionHandlerLib to put the
> > vector templates in .data, as I proposed before. This works around the
> > issue, and given that SEC/PEI is assumed to be read-only anyway (as it
> > may execute in place from flash) and does not use page alignment for
> > the sections due to size constraints, it is reasonable to assume that
> > .text and .data will be mapped executable anyway.
>
> Well, that assumption is more than fair to make for the status quo platforms, but this is just another rock in the way of doing things the right way (even if it’s just VMs).
>

How so? SEC and PEI could be mapped read-only today, it's just that we
never bother.

> Cc Gerd for an OVMF security perspective. Is PEI-time memory protection something you’d be interested in in the future?
>

My WXN series for ARM maps all PEIMs read-only, and turns off
shadowing entirely (which makes no sense for VMs). So we have most of
what we need to do that, and this change has no bearing on that.

> >
> > - update the version that performs the runtime fixups to only do so
> > when using the XCODE toolchain - we can phase that out once we drop
> > XCODE support.
>
> I agree if there’s an actual plan on doing that. We depend on XCODE5 downstream, but I think it would literally be easier for us if the upstream version was dropped than rebasing against hacks that our slightly modded variant does not require.
>
> Cc Andrew and Rebecca. I don’t know anyone else who might still be using XCODE5. Any objections to dropping it? If so, any plans to pick up my proposed changes instead?
>

I wouldn't mind dropping it. In fact, I'm wondering - given that you
need to install nasm and iasl anyway - if it wouldn't make more sense
to use the CLANGPDB toolchain on macOS, and avoid the mtoc mess
entirely?

Re: [edk2-devel] [RFT PATCH v2 1/6] BaseTools/tools_def XCODE: Link X64 with -read_only_relocs suppress

[Gerd Hoffmann]

  Hi,

> > - modify the SecPei version of CpuExceptionHandlerLib to put the
> > vector templates in .data, as I proposed before. This works around the
> > issue, and given that SEC/PEI is assumed to be read-only anyway (as it
> > may execute in place from flash) and does not use page alignment for
> > the sections due to size constraints, it is reasonable to assume that
> > .text and .data will be mapped executable anyway.
> 
> Well, that assumption is more than fair to make for the status quo
> platforms, but this is just another rock in the way of doing things
> the right way (even if it’s just VMs).
> 
> Cc Gerd for an OVMF security perspective. Is PEI-time memory
> protection something you’d be interested in in the future?

Given that PEI is expected to be able to run from read-only storage
the easiest way to apply X^W rules would be to just map the whole
PEI firmware volume as R-X when executing from RAM (which is the case
for OVMF).

I've fixed OVMF PEI modules last year to *not* use global variables,
so OVMF is not a special case any more and mapping OVMF PEI readonly
should work just fine.

So Ard's approach looks sane to me.

take care,
  Gerd

Re: [edk2-devel] [RFT PATCH v2 1/6] BaseTools/tools_def XCODE: Link X64 with -read_only_relocs suppress

[Marvin Häuser]

> On 31. Mar 2023, at 10:59, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> On Fri, 31 Mar 2023 at 10:29, Marvin Häuser <mhaeuser@posteo.de> wrote:
>> 
>> 
>>>> On 31. Mar 2023, at 09:39, Ard Biesheuvel <ardb@kernel.org> wrote:
>>> Hi Marvin,
>>> 
>>> Thanks for the context.
>>> 
>>> 
>>> On Thu, 30 Mar 2023 at 23:54, Marvin Häuser <mhaeuser@posteo.de> wrote:
>>>> 
>>>> Hi Ard,
>>>> 
>>>> Sorry, I cannot preserve the CC list as the groups.io interface doesn't seem to allow it. Can you please CC me on future revisions?
>>>> 
>>>> This patch will badly corrupt binaries. I cannot cite a source right now (if you want me to, please remind me in your response, so I can look it up tomorrow), but for X64 (but not IA32, which is why this is enabled there), relocs are relative to the first *writable* segment. In other words, any relocation to __TEXT will badly corrupt binaries this way.
>>> 
>>> OMG.
>>> 
>>> I can't believe how buggy all this stuff is. But I can confirm that
>>> the resulting binaries don't look right, even though they appear to
>>> boot fine.
>> 
>> Codegen does not change from the suppress flag, so there will be no additional text relocs beyond those you introduced. As they target the exception handler, I guess you’d need to actively provoke the broken code paths (and may end up with a nice recursion :) ).
>> 
> 
> I understand that the codegen is the same. I was specifically talking
> about the PE relocations, which seem to be lacking entirely.

Sure, I was just elaborating on the “appear to boot fine” part, which does make sense. When I last tried, the relocs were not absent but underflown. Might be mtoc drops them somehow, I think I inspected the Mach-O directly. Whatever, you reproduce the issue. :)

> 
>>> In particular, when I dump the PE relocations using
>>> llvm-readobj --coff-basereloc, I don't see any relocations referring
>>> to the .text section.
>>> 
>>>> In AUDK, we support this with two essential changes. The first is that we always generate a writable dummy segment at the beginning of the address space [1], making the relocs relative to the image base. The second is that in ocmtoc, our fork of the abandoned (and pretty badly-bugged) Apple mtoc, we explicitly require this segment to be present and verify its virtual address is the minimum virtual address [2]. It is then omitted from the conversion process [3]. I suggest you replicate these changes and fully switch to ocmtoc for XCODE5 builds.
>>> 
>>> I'm not going to do any of that. Instead, I am going to drop this
>>> change, and do the following:
>>> 
>>> - modify the SecPei version of CpuExceptionHandlerLib to put the
>>> vector templates in .data, as I proposed before. This works around the
>>> issue, and given that SEC/PEI is assumed to be read-only anyway (as it
>>> may execute in place from flash) and does not use page alignment for
>>> the sections due to size constraints, it is reasonable to assume that
>>> .text and .data will be mapped executable anyway.
>> 
>> Well, that assumption is more than fair to make for the status quo platforms, but this is just another rock in the way of doing things the right way (even if it’s just VMs).
>> 
> 
> How so? SEC and PEI could be mapped read-only today, it's just that we
> never bother.

I’m not concerned about read-only but NX.

> 
>> Cc Gerd for an OVMF security perspective. Is PEI-time memory protection something you’d be interested in in the future?
>> 
> 
> My WXN series for ARM maps all PEIMs read-only, and turns off
> shadowing entirely (which makes no sense for VMs). So we have most of
> what we need to do that, and this change has no bearing on that.

Well yes, if everything is read-only, you guarantee W^X implicitly, but downstream we have plans for the full deal including NX data. It’s however shelved for the distant future, so as long as this is changed with the intention of reverting it once XCODE5 is fixed or dropped, that sounds fine to me. I just don’t like the notion of intentionally breaking the memory permission model as a hack. I rather hope we’ll make some swift progress on removing XCODE5 as a source of frustration. :)

> 
>>> 
>>> - update the version that performs the runtime fixups to only do so
>>> when using the XCODE toolchain - we can phase that out once we drop
>>> XCODE support.
>> 
>> I agree if there’s an actual plan on doing that. We depend on XCODE5 downstream, but I think it would literally be easier for us if the upstream version was dropped than rebasing against hacks that our slightly modded variant does not require.
>> 
>> Cc Andrew and Rebecca. I don’t know anyone else who might still be using XCODE5. Any objections to dropping it? If so, any plans to pick up my proposed changes instead?
>> 
> 
> I wouldn't mind dropping it. In fact, I'm wondering - given that you
> need to install nasm and iasl anyway - if it wouldn't make more sense
> to use the CLANGPDB toolchain on macOS, and avoid the mtoc mess
> entirely?

I’d say using XCODE5 is a historical thing for us. Years ago, Vitaly evaluated both CLANG38 and CLANGPDB and found various things including debugging to be badly broken. In fact, CLANG38 turned out to have issues like misaligning UINT64s *for years*. However, those issues might have been fixed and it’s not impossible Vitaly will give it another try eventually. In any case, I think our downstream variant of XCODE5 doesn’t require any level of special care, so it doesn’t really matter to us.

(Another thing to consider is despite the bugs are fixed, mtoc has a much higher overall code quality and more safety checks than GenFw, which is used for CLANGDWARF.)

The upstream toolchain has no future in my opinion, as mtoc has been deprecated and already failed to compile certain things (like it lacked Standalone MM types). The reason it still “worked” was because homebrew silently shipped a variant with a subset of our ocmtoc patches. So as I see it, taking our changes or dropping it entirely are the only sane options, even regardless of this particular issue you’re trying to fix. Personally, I have no preference.

Best regards,
Marvin

Re: [edk2-devel] [RFT PATCH v2 1/6] BaseTools/tools_def XCODE: Link X64 with -read_only_relocs suppress

[Ard Biesheuvel]

On Fri, 31 Mar 2023 at 11:27, Marvin Häuser <mhaeuser@posteo.de> wrote:
>
>
> > On 31. Mar 2023, at 10:59, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Fri, 31 Mar 2023 at 10:29, Marvin Häuser <mhaeuser@posteo.de> wrote:
> >>
> >>
> >>>> On 31. Mar 2023, at 09:39, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>> Hi Marvin,
> >>>
> >>> Thanks for the context.
> >>>
> >>>
> >>> On Thu, 30 Mar 2023 at 23:54, Marvin Häuser <mhaeuser@posteo.de> wrote:
> >>>>
> >>>> Hi Ard,
> >>>>
> >>>> Sorry, I cannot preserve the CC list as the groups.io interface doesn't seem to allow it. Can you please CC me on future revisions?
> >>>>
> >>>> This patch will badly corrupt binaries. I cannot cite a source right now (if you want me to, please remind me in your response, so I can look it up tomorrow), but for X64 (but not IA32, which is why this is enabled there), relocs are relative to the first *writable* segment. In other words, any relocation to __TEXT will badly corrupt binaries this way.
> >>>
> >>> OMG.
> >>>
> >>> I can't believe how buggy all this stuff is. But I can confirm that
> >>> the resulting binaries don't look right, even though they appear to
> >>> boot fine.
> >>
> >> Codegen does not change from the suppress flag, so there will be no additional text relocs beyond those you introduced. As they target the exception handler, I guess you’d need to actively provoke the broken code paths (and may end up with a nice recursion :) ).
> >>
> >
> > I understand that the codegen is the same. I was specifically talking
> > about the PE relocations, which seem to be lacking entirely.
>
> Sure, I was just elaborating on the “appear to boot fine” part, which does make sense. When I last tried, the relocs were not absent but underflown. Might be mtoc drops them somehow, I think I inspected the Mach-O directly. Whatever, you reproduce the issue. :)
>

Fair enough.

> >
> >>> In particular, when I dump the PE relocations using
> >>> llvm-readobj --coff-basereloc, I don't see any relocations referring
> >>> to the .text section.
> >>>
> >>>> In AUDK, we support this with two essential changes. The first is that we always generate a writable dummy segment at the beginning of the address space [1], making the relocs relative to the image base. The second is that in ocmtoc, our fork of the abandoned (and pretty badly-bugged) Apple mtoc, we explicitly require this segment to be present and verify its virtual address is the minimum virtual address [2]. It is then omitted from the conversion process [3]. I suggest you replicate these changes and fully switch to ocmtoc for XCODE5 builds.
> >>>
> >>> I'm not going to do any of that. Instead, I am going to drop this
> >>> change, and do the following:
> >>>
> >>> - modify the SecPei version of CpuExceptionHandlerLib to put the
> >>> vector templates in .data, as I proposed before. This works around the
> >>> issue, and given that SEC/PEI is assumed to be read-only anyway (as it
> >>> may execute in place from flash) and does not use page alignment for
> >>> the sections due to size constraints, it is reasonable to assume that
> >>> .text and .data will be mapped executable anyway.
> >>
> >> Well, that assumption is more than fair to make for the status quo platforms, but this is just another rock in the way of doing things the right way (even if it’s just VMs).
> >>
> >
> > How so? SEC and PEI could be mapped read-only today, it's just that we
> > never bother.
>
> I’m not concerned about read-only but NX.
>

We don't have writable data in SEC or PEI, so this would require SEC,
PEI_CORE and every PEIM to have split .text and .rodata, and round
them up to page size. Not sure this is worth it, especially given the
fact that CoCo targets seems to be skipping the PEI phase entirely.

> >
> >> Cc Gerd for an OVMF security perspective. Is PEI-time memory protection something you’d be interested in in the future?
> >>
> >
> > My WXN series for ARM maps all PEIMs read-only, and turns off
> > shadowing entirely (which makes no sense for VMs). So we have most of
> > what we need to do that, and this change has no bearing on that.
>
> Well yes, if everything is read-only, you guarantee W^X implicitly, but downstream we have plans for the full deal including NX data. It’s however shelved for the distant future, so as long as this is changed with the intention of reverting it once XCODE5 is fixed or dropped, that sounds fine to me. I just don’t like the notion of intentionally breaking the memory permission model as a hack. I rather hope we’ll make some swift progress on removing XCODE5 as a source of frustration. :)
>

Pardon my bluntness, but why should I care about the shelved future
plans of some downstream project?

> >
> >>>
> >>> - update the version that performs the runtime fixups to only do so
> >>> when using the XCODE toolchain - we can phase that out once we drop
> >>> XCODE support.
> >>
> >> I agree if there’s an actual plan on doing that. We depend on XCODE5 downstream, but I think it would literally be easier for us if the upstream version was dropped than rebasing against hacks that our slightly modded variant does not require.
> >>
> >> Cc Andrew and Rebecca. I don’t know anyone else who might still be using XCODE5. Any objections to dropping it? If so, any plans to pick up my proposed changes instead?
> >>
> >
> > I wouldn't mind dropping it. In fact, I'm wondering - given that you
> > need to install nasm and iasl anyway - if it wouldn't make more sense
> > to use the CLANGPDB toolchain on macOS, and avoid the mtoc mess
> > entirely?
>
> I’d say using XCODE5 is a historical thing for us. Years ago, Vitaly evaluated both CLANG38 and CLANGPDB and found various things including debugging to be badly broken. In fact, CLANG38 turned out to have issues like misaligning UINT64s *for years*.

Wow, that is very bad. Was that reported to the mailing list?

> However, those issues might have been fixed and it’s not impossible Vitaly will give it another try eventually. In any case, I think our downstream variant of XCODE5 doesn’t require any level of special care, so it doesn’t really matter to us.
>
> (Another thing to consider is despite the bugs are fixed, mtoc has a much higher overall code quality and more safety checks than GenFw, which is used for CLANGDWARF.)
>
> The upstream toolchain has no future in my opinion, as mtoc has been deprecated and already failed to compile certain things (like it lacked Standalone MM types). The reason it still “worked” was because homebrew silently shipped a variant with a subset of our ocmtoc patches. So as I see it, taking our changes or dropping it entirely are the only sane options, even regardless of this particular issue you’re trying to fix. Personally, I have no preference.
>

I think both GenFw and mtoc are horrible hacks that should be phased
out once we can - with good cross-architecture Clang support for
native PE binaries, I'd hope macOS could move to CLANGPDB for all
targets.

Re: [edk2-devel] [RFT PATCH v2 1/6] BaseTools/tools_def XCODE: Link X64 with -read_only_relocs suppress

[Marvin Häuser]

[-- Attachment #1: Type: text/plain, Size: 9276 bytes --]


> On 31. Mar 2023, at 11:36, Ard Biesheuvel <ardb@kernel.org> wrote:
> 
> On Fri, 31 Mar 2023 at 11:27, Marvin Häuser <mhaeuser@posteo.de> wrote:
>> 
>> 
>>>> On 31. Mar 2023, at 10:59, Ard Biesheuvel <ardb@kernel.org> wrote:
>>> 
>>> On Fri, 31 Mar 2023 at 10:29, Marvin Häuser <mhaeuser@posteo.de> wrote:
>>>> 
>>>> 
>>>>>> On 31. Mar 2023, at 09:39, Ard Biesheuvel <ardb@kernel.org> wrote:
>>>>> Hi Marvin,
>>>>> 
>>>>> Thanks for the context.
>>>>> 
>>>>> 
>>>>> On Thu, 30 Mar 2023 at 23:54, Marvin Häuser <mhaeuser@posteo.de> wrote:
>>>>>> 
>>>>>> Hi Ard,
>>>>>> 
>>>>>> Sorry, I cannot preserve the CC list as the groups.io interface doesn't seem to allow it. Can you please CC me on future revisions?
>>>>>> 
>>>>>> This patch will badly corrupt binaries. I cannot cite a source right now (if you want me to, please remind me in your response, so I can look it up tomorrow), but for X64 (but not IA32, which is why this is enabled there), relocs are relative to the first *writable* segment. In other words, any relocation to __TEXT will badly corrupt binaries this way.
>>>>> 
>>>>> OMG.
>>>>> 
>>>>> I can't believe how buggy all this stuff is. But I can confirm that
>>>>> the resulting binaries don't look right, even though they appear to
>>>>> boot fine.
>>>> 
>>>> Codegen does not change from the suppress flag, so there will be no additional text relocs beyond those you introduced. As they target the exception handler, I guess you’d need to actively provoke the broken code paths (and may end up with a nice recursion :) ).
>>>> 
>>> 
>>> I understand that the codegen is the same. I was specifically talking
>>> about the PE relocations, which seem to be lacking entirely.
>> 
>> Sure, I was just elaborating on the “appear to boot fine” part, which does make sense. When I last tried, the relocs were not absent but underflown. Might be mtoc drops them somehow, I think I inspected the Mach-O directly. Whatever, you reproduce the issue. :)
>> 
> 
> Fair enough.
> 
>>> 
>>>>> In particular, when I dump the PE relocations using
>>>>> llvm-readobj --coff-basereloc, I don't see any relocations referring
>>>>> to the .text section.
>>>>> 
>>>>>> In AUDK, we support this with two essential changes. The first is that we always generate a writable dummy segment at the beginning of the address space [1], making the relocs relative to the image base. The second is that in ocmtoc, our fork of the abandoned (and pretty badly-bugged) Apple mtoc, we explicitly require this segment to be present and verify its virtual address is the minimum virtual address [2]. It is then omitted from the conversion process [3]. I suggest you replicate these changes and fully switch to ocmtoc for XCODE5 builds.
>>>>> 
>>>>> I'm not going to do any of that. Instead, I am going to drop this
>>>>> change, and do the following:
>>>>> 
>>>>> - modify the SecPei version of CpuExceptionHandlerLib to put the
>>>>> vector templates in .data, as I proposed before. This works around the
>>>>> issue, and given that SEC/PEI is assumed to be read-only anyway (as it
>>>>> may execute in place from flash) and does not use page alignment for
>>>>> the sections due to size constraints, it is reasonable to assume that
>>>>> .text and .data will be mapped executable anyway.
>>>> 
>>>> Well, that assumption is more than fair to make for the status quo platforms, but this is just another rock in the way of doing things the right way (even if it’s just VMs).
>>>> 
>>> 
>>> How so? SEC and PEI could be mapped read-only today, it's just that we
>>> never bother.
>> 
>> I’m not concerned about read-only but NX.
>> 
> 
> We don't have writable data in SEC or PEI, so this would require SEC,
> PEI_CORE and every PEIM to have split .text and .rodata, and round
> them up to page size. Not sure this is worth it, especially given the
> fact that CoCo targets seems to be skipping the PEI phase entirely.

CoCo = Confidential Computing? Right, I actually hope that’s true. :) But there are also some plans for real hardware here.

> 
>>> 
>>>> Cc Gerd for an OVMF security perspective. Is PEI-time memory protection something you’d be interested in in the future?
>>>> 
>>> 
>>> My WXN series for ARM maps all PEIMs read-only, and turns off
>>> shadowing entirely (which makes no sense for VMs). So we have most of
>>> what we need to do that, and this change has no bearing on that.
>> 
>> Well yes, if everything is read-only, you guarantee W^X implicitly, but downstream we have plans for the full deal including NX data. It’s however shelved for the distant future, so as long as this is changed with the intention of reverting it once XCODE5 is fixed or dropped, that sounds fine to me. I just don’t like the notion of intentionally breaking the memory permission model as a hack. I rather hope we’ll make some swift progress on removing XCODE5 as a source of frustration. :)
>> 
> 
> Pardon my bluntness, but why should I care about the shelved future
> plans of some downstream project?

No worries. The part you should care about is that this violates a well-established, well-reasoned, and important convention. This generates objectively broken binaries that only happen to work due to current implementation details. The “future plans” part was an explanation of why I’m persistent about it, stating that some folks want to depend on said convention. As your change affects XCODE5 only (and thus there will be no future changes that rely on this hack), I’m fine to drop this. Basically I was scared this will become part of the design and folks will magically start depending on this hack. :)

> 
>>> 
>>>>> 
>>>>> - update the version that performs the runtime fixups to only do so
>>>>> when using the XCODE toolchain - we can phase that out once we drop
>>>>> XCODE support.
>>>> 
>>>> I agree if there’s an actual plan on doing that. We depend on XCODE5 downstream, but I think it would literally be easier for us if the upstream version was dropped than rebasing against hacks that our slightly modded variant does not require.
>>>> 
>>>> Cc Andrew and Rebecca. I don’t know anyone else who might still be using XCODE5. Any objections to dropping it? If so, any plans to pick up my proposed changes instead?
>>>> 
>>> 
>>> I wouldn't mind dropping it. In fact, I'm wondering - given that you
>>> need to install nasm and iasl anyway - if it wouldn't make more sense
>>> to use the CLANGPDB toolchain on macOS, and avoid the mtoc mess
>>> entirely?
>> 
>> I’d say using XCODE5 is a historical thing for us. Years ago, Vitaly evaluated both CLANG38 and CLANGPDB and found various things including debugging to be badly broken. In fact, CLANG38 turned out to have issues like misaligning UINT64s *for years*.
> 
> Wow, that is very bad. Was that reported to the mailing list?

Yes, with my 2021’s patch ignored, of course. Pedro’s respin was merged though: https://github.com/tianocore/edk2/commit/c5d68ef6e7553ab2894f541eba4e982428ecbd53

> 
>> However, those issues might have been fixed and it’s not impossible Vitaly will give it another try eventually. In any case, I think our downstream variant of XCODE5 doesn’t require any level of special care, so it doesn’t really matter to us.
>> 
>> (Another thing to consider is despite the bugs are fixed, mtoc has a much higher overall code quality and more safety checks than GenFw, which is used for CLANGDWARF.)
>> 
>> The upstream toolchain has no future in my opinion, as mtoc has been deprecated and already failed to compile certain things (like it lacked Standalone MM types). The reason it still “worked” was because homebrew silently shipped a variant with a subset of our ocmtoc patches. So as I see it, taking our changes or dropping it entirely are the only sane options, even regardless of this particular issue you’re trying to fix. Personally, I have no preference.
>> 
> 
> I think both GenFw and mtoc are horrible hacks that should be phased
> out once we can - with good cross-architecture Clang support for
> native PE binaries, I'd hope macOS could move to CLANGPDB for all
> targets.

That’s a bit too utopian (or actually, dystopian). First, to my understanding, this will also break GCC, no? Maybe there’s good support for generating PEs now, who knows. As far as I’m aware, pairing PE with DWARF isn’t really a well-supported thing either. But again, old experiences, may be better now.

Yet, even if all of this works fine now, this is still a PE lock-in. For cross-platform and cross-format support, something like GenFw and mtoc is unavoidable. In fact, my current thesis topic is designing a replacement for the TE format and a tool to generate it from PEs and ELFs. In contrast to GenFw and mtoc, the design is not to attempt to translate the details of one format into another, but to define an abstract model of an UEFI image file and use this both on the consumer side (a generic loader library) and the producer side (the generation tool uses an intermediate representation for conversion rather than doing format-to-format). This actually works very well. :)

Best regards,
Marvin

[-- Attachment #2: Type: text/html, Size: 21072 bytes --]

Re: [edk2-devel] [RFT PATCH v2 1/6] BaseTools/tools_def XCODE: Link X64 with -read_only_relocs suppress

[Gerd Hoffmann]

  Hi,

> > However, those issues might have been fixed and it’s not impossible
> > Vitaly will give it another try eventually. In any case, I think our
> > downstream variant of XCODE5 doesn’t require any level of special
> > care, so it doesn’t really matter to us.
> >
> > (Another thing to consider is despite the bugs are fixed, mtoc has a
> > much higher overall code quality and more safety checks than GenFw,
> > which is used for CLANGDWARF.)
> >
> > The upstream toolchain has no future in my opinion, as mtoc has been
> > deprecated and already failed to compile certain things (like it
> > lacked Standalone MM types). The reason it still “worked” was
> > because homebrew silently shipped a variant with a subset of our
> > ocmtoc patches. So as I see it, taking our changes or dropping it
> > entirely are the only sane options, even regardless of this
> > particular issue you’re trying to fix. Personally, I have no
> > preference.
> 
> I think both GenFw and mtoc are horrible hacks that should be phased
> out once we can - with good cross-architecture Clang support for
> native PE binaries, I'd hope macOS could move to CLANGPDB for all
> targets.

What is the difference between CLANGPDB and CLANGDWARF?  Just the debug
info format?

What is the support status?  Is CLANGDWARF expected to build edk2 on all
platforms?  Including cross-builds?  Or will that work only after
Rebecca's toolchain fix/cleanup series being merged?

Should we eventually switch from gcc to clang on linux too?

take care,
  Gerd

Re: [edk2-devel] [RFT PATCH v2 1/6] BaseTools/tools_def XCODE: Link X64 with -read_only_relocs suppress

[Ard Biesheuvel]

On Fri, 31 Mar 2023 at 12:53, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > However, those issues might have been fixed and it’s not impossible
> > > Vitaly will give it another try eventually. In any case, I think our
> > > downstream variant of XCODE5 doesn’t require any level of special
> > > care, so it doesn’t really matter to us.
> > >
> > > (Another thing to consider is despite the bugs are fixed, mtoc has a
> > > much higher overall code quality and more safety checks than GenFw,
> > > which is used for CLANGDWARF.)
> > >
> > > The upstream toolchain has no future in my opinion, as mtoc has been
> > > deprecated and already failed to compile certain things (like it
> > > lacked Standalone MM types). The reason it still “worked” was
> > > because homebrew silently shipped a variant with a subset of our
> > > ocmtoc patches. So as I see it, taking our changes or dropping it
> > > entirely are the only sane options, even regardless of this
> > > particular issue you’re trying to fix. Personally, I have no
> > > preference.
> >
> > I think both GenFw and mtoc are horrible hacks that should be phased
> > out once we can - with good cross-architecture Clang support for
> > native PE binaries, I'd hope macOS could move to CLANGPDB for all
> > targets.
>
> What is the difference between CLANGPDB and CLANGDWARF?  Just the debug
> info format?
>

No, it uses the LLVM tools to generate PE binaries directly.

> What is the support status?  Is CLANGDWARF expected to build edk2 on all
> platforms?  Including cross-builds?  Or will that work only after
> Rebecca's toolchain fix/cleanup series being merged?
>

Yes, that what I was hoping for - LLD supports all architectures,
which is why I insisted that CLANGDWARF should use LLD on ARM/AARCH64
as well.

That way, anyone can build all targets on any host.

> Should we eventually switch from gcc to clang on linux too?
>

When using ELF to PE/COFF conversion, it doesn't make that much of a difference.

Re: [edk2-devel] [RFT PATCH v2 1/6] BaseTools/tools_def XCODE: Link X64 with -read_only_relocs suppress

[Marvin Häuser]

Hi Gerd,

> On 31. Mar 2023, at 12:53, Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
>   Hi,
> 
>>> However, those issues might have been fixed and it’s not impossible
>>> Vitaly will give it another try eventually. In any case, I think our
>>> downstream variant of XCODE5 doesn’t require any level of special
>>> care, so it doesn’t really matter to us.
>>> 
>>> (Another thing to consider is despite the bugs are fixed, mtoc has a
>>> much higher overall code quality and more safety checks than GenFw,
>>> which is used for CLANGDWARF.)
>>> 
>>> The upstream toolchain has no future in my opinion, as mtoc has been
>>> deprecated and already failed to compile certain things (like it
>>> lacked Standalone MM types). The reason it still “worked” was
>>> because homebrew silently shipped a variant with a subset of our
>>> ocmtoc patches. So as I see it, taking our changes or dropping it
>>> entirely are the only sane options, even regardless of this
>>> particular issue you’re trying to fix. Personally, I have no
>>> preference.
>> 
>> I think both GenFw and mtoc are horrible hacks that should be phased
>> out once we can - with good cross-architecture Clang support for
>> native PE binaries, I'd hope macOS could move to CLANGPDB for all
>> targets.
> 
> What is the difference between CLANGPDB and CLANGDWARF?  Just the debug
> info format?

DWARF generates an ELF (which is utilized by UefiPayloadPkg) and PDB generates a PE directly.

Best regards,
Marvin

> 
> What is the support status?  Is CLANGDWARF expected to build edk2 on all
> platforms?  Including cross-builds?  Or will that work only after
> Rebecca's toolchain fix/cleanup series being merged?
> 
> Should we eventually switch from gcc to clang on linux too?
> 
> take care,
>  Gerd
> 

Re: [edk2-devel] [RFT PATCH v2 1/6] BaseTools/tools_def XCODE: Link X64 with -read_only_relocs suppress

[Rebecca Cran]

On 3/31/23 2:29 AM, Marvin Häuser wrote:
> I agree if there’s an actual plan on doing that. We depend on XCODE5 downstream, but I think it would literally be easier for us if the upstream version was dropped than rebasing against hacks that our slightly modded variant does not require.
>
> Cc Andrew and Rebecca. I don’t know anyone else who might still be using XCODE5. Any objections to dropping it? If so, any plans to pick up my proposed changes instead?

I've only been using XCODE5 to test if it still works. I do all of my 
development work on Linux or FreeBSD.


-- 

Rebecca Cran

Re: [edk2-devel] [RFT PATCH v2 1/6] BaseTools/tools_def XCODE: Link X64 with -read_only_relocs suppress

[Marvin Häuser]

> On 31. Mar 2023, at 16:58, Rebecca Cran <rebecca@bsdio.com> wrote:
> 
> On 3/31/23 2:29 AM, Marvin Häuser wrote:
>> I agree if there’s an actual plan on doing that. We depend on XCODE5 downstream, but I think it would literally be easier for us if the upstream version was dropped than rebasing against hacks that our slightly modded variant does not require.
>> 
>> Cc Andrew and Rebecca. I don’t know anyone else who might still be using XCODE5. Any objections to dropping it? If so, any plans to pick up my proposed changes instead?
> 
> I've only been using XCODE5 to test if it still works. I do all of my development work on Linux or FreeBSD.

I checked the list and most of the traffic regarding XCODE5 is basically Andrew and you. So if you don't need this toolchain...

@Andrew Would you agree it's fair for Apple to maintain XCODE5 and its oddities downstream? I know someone mentioned the ARM transition, but I think you still support UEFI for HV things, don't you? The alternative would be we revamp the upstream XCODE5 toolchain and mtoc with our changes from AUDK and ocmtoc, but this isn't feasible without strong support from Apple (our previous patches to mtoc were ignored). I don't think the burden easily fixable XCODE5 oddities put on the general codebase are acceptable going forward. Thanks!

Best regards,
Marvin

> 
> 
> -- 
> 
> Rebecca Cran
> 
>