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

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

By the way, which ("%" or "#") should be used for def check in NASM?
I thought we need to use "%" but your patch uses "#".

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Friday, March 31, 2023 5:56 PM
> To: Ard Biesheuvel <ardb@kernel.org>; devel@edk2.groups.io
> 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>; Marvin Häuser <mhaeuser@posteo.de>
> Subject: Re: [edk2-devel] [RFT PATCH v3 2/5]
> UefiCpuPkg/CpuExceptionHandlerLib: Use single SEC/PEI version
> 
> 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
> 
> 
> 
> 
>