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

["Marvin Häuser" <mhaeuser@posteo.de>]

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