Re: [edk2-devel] [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib

["Liming Gao" <liming.gao@intel.com>]

Thomas:

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Lendacky, Thomas
> Sent: Wednesday, May 6, 2020 10:35 PM
> To: devel@edk2.groups.io; lersek@redhat.com
> Cc: Justen, Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Gao, Liming <liming.gao@intel.com>;
> Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Brijesh Singh <brijesh.singh@amd.com>; Anthony Perard
> <anthony.perard@citrix.com>; You, Benjamin <benjamin.you@intel.com>; Dong, Guo <guo.dong@intel.com>; Julien Grall
> <julien@xen.org>; Ma, Maurice <maurice.ma@intel.com>; Andrew Fish <afish@apple.com>
> Subject: Re: [edk2-devel] [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib
> 
> On 5/5/20 5:15 PM, Laszlo Ersek via groups.io wrote:
> > On 05/01/20 22:17, Lendacky, Thomas wrote:
> >> BZ:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2340&amp;d
> ata=02%7C01%7Cthomas.lendacky%40amd.com%7Cd2ec699d2c644a55724008d7f141d96f%7C3dd8961fe4884e608e11a82d994e183d%
> 7C0%7C0%7C637243137431443098&amp;sdata=oTTju7144KZc8VCmQqu74UilIOzQyji9jlO%2BMJeZYyU%3D&amp;reserved=0
> >>
> >> Now that an XCODE5 specific CpuExceptionHandlerLib library is in place,
> >> revert the changes made to the ExceptionHandlerAsm.nasm in commit
> >> 2db0ccc2d7fe ("UefiCpuPkg: Update CpuExceptionHandlerLib pass XCODE5 tool
> >> chain") so that binary patching of flash code is not performed.
> >>
> >> Cc: Eric Dong <eric.dong@intel.com>
> >> Cc: Ray Ni <ray.ni@intel.com>
> >> Cc: Laszlo Ersek <lersek@redhat.com>
> >> Cc: Liming Gao <liming.gao@intel.com>
> >> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> >> ---
> >>   .../X64/ExceptionHandlerAsm.nasm              | 25 +++++--------------
> >>   1 file changed, 6 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> >> index 19198f273137..3814f9de3703 100644
> >> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> >> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> >> @@ -34,7 +34,7 @@ AsmIdtVectorBegin:
> >>       db      0x6a        ; push  #VectorNum
> >>       db      ($ - AsmIdtVectorBegin) / ((AsmIdtVectorEnd - AsmIdtVectorBegin) / 32) ; VectorNum
> >>       push    rax
> >> -    mov     rax, strict qword 0 ;    mov     rax, ASM_PFX(CommonInterruptEntry)
> >> +    mov     rax, ASM_PFX(CommonInterruptEntry)
> >>       jmp     rax
> >>   %endrep
> >>   AsmIdtVectorEnd:
> >> @@ -44,8 +44,7 @@ HookAfterStubHeaderBegin:
> >>   @VectorNum:
> >>       db      0          ; 0 will be fixed
> >>       push    rax
> >> -    mov     rax, strict qword 0 ;     mov     rax, HookAfterStubHeaderEnd
> >> -JmpAbsoluteAddress:
> >> +    mov     rax, HookAfterStubHeaderEnd
> >>       jmp     rax
> >>   HookAfterStubHeaderEnd:
> >>       mov     rax, rsp
> >> @@ -257,7 +256,8 @@ HasErrorCode:
> >>       ; and make sure RSP is 16-byte aligned
> >>       ;
> >>       sub     rsp, 4 * 8 + 8
> >> -    call    ASM_PFX(CommonExceptionHandler)
> >> +    mov     rax, ASM_PFX(CommonExceptionHandler)
> >> +    call    rax
> >>       add     rsp, 4 * 8 + 8
> >>
> >>       cli
> >> @@ -365,24 +365,11 @@ DoIret:
> >>   ; comments here for definition of address map
> >>   global ASM_PFX(AsmGetTemplateAddressMap)
> >>   ASM_PFX(AsmGetTemplateAddressMap):
> >> -    lea     rax, [AsmIdtVectorBegin]
> >> +    mov     rax, AsmIdtVectorBegin
> >>       mov     qword [rcx], rax
> >>       mov     qword [rcx + 0x8],  (AsmIdtVectorEnd - AsmIdtVectorBegin) / 32
> >> -    lea     rax, [HookAfterStubHeaderBegin]
> >> +    mov     rax, HookAfterStubHeaderBegin
> >>       mov     qword [rcx + 0x10], rax
> >> -
> >> -; Fix up CommonInterruptEntry address
> >> -    lea    rax, [ASM_PFX(CommonInterruptEntry)]
> >> -    lea    rcx, [AsmIdtVectorBegin]
> >> -%rep  32
> >> -    mov    qword [rcx + (JmpAbsoluteAddress - 8 - HookAfterStubHeaderBegin)], rax
> >> -    add    rcx, (AsmIdtVectorEnd - AsmIdtVectorBegin) / 32
> >> -%endrep
> >> -; Fix up HookAfterStubHeaderEnd
> >> -    lea    rax, [HookAfterStubHeaderEnd]
> >> -    lea    rcx, [JmpAbsoluteAddress]
> >> -    mov    qword [rcx - 8], rax
> >> -
> >>       ret
> >>
> >>   ;-------------------------------------------------------------------------------------
> >>
> >
> > With this patch applied, the differences with the "original" remain:
> >
> > $ git diff 2db0ccc2d7fe^..HEAD -- \
> >        UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> >
> >> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> >> index ba8993d84b0b..3814f9de3703 100644
> >> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> >> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> >> @@ -1,12 +1,6 @@
> >>   ;------------------------------------------------------------------------------ ;
> >> -; Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.<BR>
> >> -; This program and the accompanying materials
> >> -; are licensed and made available under the terms and conditions of the BSD License
> >> -; which accompanies this distribution.  The full text of the license may be found at
> >> -; https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopensource.org%2Flicenses%2Fbsd-
> license.php&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7Cd2ec699d2c644a55724008d7f141d96f%7C3dd8961fe4884e608e
> 11a82d994e183d%7C0%7C0%7C637243137431443098&amp;sdata=SZAc83Y%2BwZauGcj47EDgc10fnxSucy2ljeI9PcaJSvE%3D&amp;reser
> ved=0.
> >> -;
> >> -; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> >> -; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> >> +; Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.<BR>
> >> +; SPDX-License-Identifier: BSD-2-Clause-Patent
> >>   ;
> >>   ; Module Name:
> >>   ;
> >
> > This is expected.
> >
> >> @@ -189,17 +183,19 @@ HasErrorCode:
> >>       push    rax
> >>       push    rax
> >>       sidt    [rsp]
> >> -    xchg    rax, [rsp + 2]
> >> -    xchg    rax, [rsp]
> >> -    xchg    rax, [rsp + 8]
> >> +    mov     bx, word [rsp]
> >> +    mov     rax, qword [rsp + 2]
> >> +    mov     qword [rsp], rax
> >> +    mov     word [rsp + 8], bx
> >>
> >>       xor     rax, rax
> >>       push    rax
> >>       push    rax
> >>       sgdt    [rsp]
> >> -    xchg    rax, [rsp + 2]
> >> -    xchg    rax, [rsp]
> >> -    xchg    rax, [rsp + 8]
> >> +    mov     bx, word [rsp]
> >> +    mov     rax, qword [rsp + 2]
> >> +    mov     qword [rsp], rax
> >> +    mov     word [rsp + 8], bx
> >>
> >>   ;; UINT64  Ldtr, Tr;
> >>       xor     rax, rax
> >>
> >
> > Also expected, from commit f4c898f2b2db
> > ("UefiCpuPkg/CpuExceptionHandlerLib: Fix split lock", 2019-09-20).
> >
> > Therefore, for this patch:
> >
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> >
> > *However*, this revert must be restricted to the original
> > "SecPeiCpuExceptionHandlerLib.inf" instance, i.e. where binary patching
> > is not acceptable. (Otherwise, in combination with my request (1) under
> > patch#1, we'd needlessly break the PEI / DXE / SMM lib instances under
> > XCODE5.)
> >
> > (1) Therefore, please insert a new patch between patches #1 and #2, such
> > that the new patch flip
> >
> > - PeiCpuExceptionHandlerLib.inf
> > - DxeCpuExceptionHandlerLib.inf
> > - SmmCpuExceptionHandlerLib.inf
> >
> > to using "Xcode5ExceptionHandlerAsm.nasm".
> >
> > (If you wish, you can squash these modifications into the updated
> > patch#1, rather than inserting them as a separate patch between #1 and
> > #2.)
> >
> >
> > In summary, I suggest the following end-state:
> >
> > - we should have a self-patching NASM file, and one without
> > self-patching,
> >
> > - the self-patching variant should be called
> > "Xcode5ExceptionHandlerAsm.nasm" (because the *only* reason for the
> > self-patching is xcode5),
> >
> > - we should have 5 INF files in total,
> >
> > - "PeiCpuExceptionHandlerLib.inf", "DxeCpuExceptionHandlerLib.inf",
> > "SmmCpuExceptionHandlerLib.inf" should use
> > "Xcode5ExceptionHandlerAsm.nasm" (the self-patching is harmless),
> >
> > - "SecPeiCpuExceptionHandlerLib.inf" should use
> > "ExceptionHandlerAsm.nasm" (self-patching is invalid, so don't do it),
> >
> > - "Xcode5SecPeiCpuExceptionHandlerLib.inf" should use
> > "Xcode5ExceptionHandlerAsm.nasm" (the self-patching is invalid, but we
> > can't avoid it when building with XCODE5),
> >
> > - platforms should resolve the CpuExceptionHandlerLib class to
> > "Xcode5SecPeiCpuExceptionHandlerLib.inf" only for the XCODE5 toolchain
> > *and* for the SEC phase.
> 
> Ok, I have v2 ready to go, but when I ran it through the integration tests
> using a pull request I received some errors (see
> https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=6516&view=results).
> The error is the same in all cases and the error message is:
> 
> CRITICAL -
> UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
> not in UefiCpuPkg/UefiCpuPkg.dsc
> 
> Any idea about the reason for this message? Is it due to the [Components]
> section of the UefiCpuPkg/UefiCpuPkg.dsc file? Should that section not use
> the !if check and just list both .inf files
> (SecPeiCpuExceptionHandlerLib.inf and Xcode5SecPeiCpuExceptionHandlerLib.inf)?
> 

Yes. Package DSC [Components] section should list all module INF files, then verify their build. 

Thanks
Liming
> Thanks,
> Tom
> 
> >
> > Thanks,
> > Laszlo
> >
> >
> >
> >
> 
>