Re: [RFT PATCH v3 2/5] UefiCpuPkg/CpuExceptionHandlerLib: Use single SEC/PEI version

["Ni, Ray" <ray.ni@intel.com>]

Ard,
Thanks for the detailed commit messages. That really helps me to understand why XCODE version
was needed.

However, I feel it would be great if you can "highlight" what are changed by this patch.
The following is just an example. You can reword as you like.

1. Change for non-XCODE SecPeiCpuExceptionHandlerLib:
   * Use SecPeiExceptionHandlerAsm.nasm (renamed from ExceptionHandlerAsm.nasm)
   * Removed some unnecessary absolute references
   * (32 IDT stubs are still in .text.)
2. Change for XCODE SecPeiCpuExceptionHandlerLib:
   * Use SecPeiExceptionHandlerAsm.nasm instead of Xcode5ExceptionHandlerAsm.nasm
   * CET logic is not in SecPeiExceptionHandlerAsm.nasm (but aligns to non-XCODE lib instance)
   * Fixed a bug that does runtime fixup in TEXT section in SPI flash.
   * Emitted the code carrying the absolute symbol references into the .data which XCODE or
      LLD linkers allow.
     Then fixup can be done by other build tools such as GenFv if the code runs in SPI flash,
     or by PE coff loader if the code is loaded to memory.

Again, thanks for the quick patches just because I asked some XCODE questions.

Thanks,
Ray

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Friday, March 31, 2023 5:15 PM
> To: devel@edk2.groups.io
> Cc: Ard Biesheuvel <ardb@kernel.org>; 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>;
> Marvin Häuser <mhaeuser@posteo.de>
> Subject: [RFT PATCH v3 2/5] 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.
> 
> Given that SEC and PEI code typically executes in place from flash and
> does not use page alignment for sections, we can simply emit the code
> carrying the absolute symbol references into the .data segment instead.
> This works around the linker's objections, and the resulting image will
> be mapped executable in its entirety anyway. Since this is only needed
> for XCODE, let's make this change conditionally using a preprocessor
> macro.
> 
> 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.
> 
> Also update the Xcode specific version of this library, and use this
> source file instead. This is necesessary, as the Xcode specific version
> modifies its own code at runtime, which is not permitted in SEC or PEI.
> Note that this also removes CET support from the Xcode5 specific build
> of the SEC/PEI version of this library, but this is not needed this
> early in any case, and this aligns it with other toolchains, which use
> this version of the library, which does not have CET support either.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> 
> UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib
> .inf                                 |  4 +++-
> 
> UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/{ExceptionHandlerAsm.na
> sm => SecPeiExceptionHandlerAsm.nasm} | 12 ++++++++----
> 
> UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHa
> ndlerLib.inf                           |  4 +++-
>  3 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandler
> Lib.inf
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandler
> Lib.inf
> index df44371fe018e06d..885bb6638ab58620 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandler
> Lib.inf
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandler
> Lib.inf
> @@ -28,7 +28,7 @@ [Sources.Ia32]
>    Ia32/ArchInterruptDefs.h
> 
> 
> 
>  [Sources.X64]
> 
> -  X64/ExceptionHandlerAsm.nasm
> 
> +  X64/SecPeiExceptionHandlerAsm.nasm
> 
>    X64/ArchExceptionHandler.c
> 
>    X64/ArchInterruptDefs.h
> 
> 
> 
> @@ -58,3 +58,5 @@ [Pcd]
>  [FeaturePcd]
> 
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ##
> CONSUMES
> 
> 
> 
> +[BuildOptions]
> 
> +  XCODE:*_*_X64_PP_FLAGS = -DNO_ABSOLUTE_RELOCS_IN_TEXT
> 
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.n
> asm
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/SecPeiExceptionHandler
> Asm.nasm
> similarity index 94%
> rename from
> UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nas
> m
> rename to
> UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/SecPeiExceptionHandlerA
> sm.nasm
> index aaf8d622e6f3b8f1..ec45c60181906c14 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.n
> asm
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/SecPeiExceptionHandler
> Asm.nasm
> @@ -27,7 +27,9 @@ extern ASM_PFX(CommonExceptionHandler)
>  SECTION .data
> 
> 
> 
>  DEFAULT REL
> 
> +#ifndef NO_ABSOLUTE_RELOCS_IN_TEXT
> 
>  SECTION .text
> 
> +#endif
> 
> 
> 
>  ALIGN   8
> 
> 
> 
> @@ -51,6 +53,9 @@ HookAfterStubHeaderBegin:
>      push    rax
> 
>      mov     rax, HookAfterStubHeaderEnd
> 
>      jmp     rax
> 
> +
> 
> +SECTION .text
> 
> +
> 
>  HookAfterStubHeaderEnd:
> 
>      mov     rax, rsp
> 
>      and     sp,  0xfff0        ; make sure 16-byte aligned for exception context
> 
> @@ -276,8 +281,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 +388,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/Xcode5SecPeiCpuException
> HandlerLib.inf
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuException
> HandlerLib.inf
> index 619b39d7f1de9ae3..17f872bb15eb0ff7 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuException
> HandlerLib.inf
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuException
> HandlerLib.inf
> @@ -33,7 +33,7 @@ [Sources.Ia32]
>    Ia32/ArchInterruptDefs.h
> 
> 
> 
>  [Sources.X64]
> 
> -  X64/Xcode5ExceptionHandlerAsm.nasm
> 
> +  X64/SecPeiExceptionHandlerAsm.nasm
> 
>    X64/ArchExceptionHandler.c
> 
>    X64/ArchInterruptDefs.h
> 
> 
> 
> @@ -63,3 +63,5 @@ [Pcd]
>  [FeaturePcd]
> 
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ##
> CONSUMES
> 
> 
> 
> +[BuildOptions]
> 
> +  XCODE:*_*_X64_PP_FLAGS = -DNO_ABSOLUTE_RELOCS_IN_TEXT
> 
> --
> 2.39.2