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

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 5/6/20 1:07 PM, Bret Barkelew via groups.io wrote:
> <Quote>
> 
>  > Should that
>  > section not use the !if check and just list both .inf files
>  > (SecPeiCpuExceptionHandlerLib.inf and
>  > Xcode5SecPeiCpuExceptionHandlerLib.inf)?
> 
> Hmmm, this is a very good point; after all, the updated (=reverted)
> "SecPeiCpuExceptionHandlerLib.inf" instance will not build with XCODE5.
> Therefore we should list both lib instance INF files under [Components],
> but make "SecPeiCpuExceptionHandlerLib.inf" conditional on non-XCODE5.
> 
> </Quote>
> 
> Xcode5SecPeiCpuExceptionHandlerLib.inf could be added to the ignore list 
> in the CI yaml file. PR gating CI currently only uses GCC and VS2017/19 
> and shouldn’t have a problem with the reverted lib. This makes 
> Xcode5SecPeiCpuExceptionHandlerLib the exception which can be documented 
> in the ignore list (why it’s being ignored).
> 
> Thoughts?

I'll give those suggestions a try and see how they work. Thanks!

Tom

> 
> - Bret
> 
> *From: *Laszlo Ersek via groups.io <mailto:lersek=redhat.com@groups.io>
> *Sent: *Wednesday, May 6, 2020 9:33 AM
> *To: *Tom Lendacky <mailto:thomas.lendacky@amd.com>; devel@edk2.groups.io 
> <mailto:devel@edk2.groups.io>
> *Cc: *Jordan Justen <mailto:jordan.l.justen@intel.com>; Ard Biesheuvel 
> <mailto:ard.biesheuvel@linaro.org>; Liming Gao 
> <mailto:liming.gao@intel.com>; Eric Dong <mailto:eric.dong@intel.com>; Ray 
> Ni <mailto:ray.ni@intel.com>; Brijesh Singh 
> <mailto:brijesh.singh@amd.com>; Anthony Perard 
> <mailto:anthony.perard@citrix.com>; Benjamin You 
> <mailto:benjamin.you@intel.com>; Guo Dong <mailto:guo.dong@intel.com>; 
> Julien Grall <mailto:julien@xen.org>; Maurice Ma 
> <mailto:maurice.ma@intel.com>; Andrew Fish <mailto:afish@apple.com>
> *Subject: *[EXTERNAL] Re: [edk2-devel] [PATCH 4/4] 
> UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard 
> CpuExceptionHandlerLib
> 
> On 05/06/20 16:35, Tom Lendacky wrote:
>  > On 5/5/20 5:15 PM, Laszlo Ersek via groups.io wrote:
>  >> On 05/01/20 22:17, Lendacky, Thomas wrote:
>  >>> BZ:
>  >>> 
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2340&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7Ca9365fbefc53477c9e3308d7f1db4560%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637243796372782584&amp;sdata=R%2B5IgncDzx7viv3yIwX3MloX1QlzuUJ4bZlKnc%2B0xoI%3D&amp;reserved=0 
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2340&data=02%7C01%7Cthomas.lendacky%40amd.com%7C4d968ebc52544accf1b608d7f1e865f8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243852756214038&sdata=p9f6gdfmOnbTMx7u7Ao4jw6jEqcZ2DVXENwHhwcETjo%3D&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://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopensource.org%2Flicenses%2Fbsd-license.php&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7Ca9365fbefc53477c9e3308d7f1db4560%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637243796372792580&amp;sdata=CZovXRJ6HURgo6sz%2FDi55SUy9IsrJ2pFzcHX%2Bp%2Fv8qA%3D&amp;reserved=0 
> <https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopensource.org%2Flicenses%2Fbsd-license.php&data=02%7C01%7Cthomas.lendacky%40amd.com%7C4d968ebc52544accf1b608d7f1e865f8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243852756214038&sdata=6TFEj80GpLSavcQeRvxn2uCPLVK6HxGN1yKB2PPS7Yk%3D&reserved=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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev.azure.com%2Ftianocore%2Fedk2-ci%2F_build%2Fresults%3FbuildId%3D6516%26view%3Dresults&amp;data=02%7C01%7CBret.Barkelew%40microsoft.com%7Ca9365fbefc53477c9e3308d7f1db4560%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637243796372792580&amp;sdata=N2aYxzfr%2BNw7hkhBTEg0C%2Fm4Hv00xXBZhSz3%2FFgiW1s%3D&amp;reserved=0 
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev.azure.com%2Ftianocore%2Fedk2-ci%2F_build%2Fresults%3FbuildId%3D6516%26view%3Dresults&data=02%7C01%7Cthomas.lendacky%40amd.com%7C4d968ebc52544accf1b608d7f1e865f8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243852756224001&sdata=xccjYXSqyhRJoGrINVWuLVsOql8pAWt5YQjU%2FSsZHWU%3D&reserved=0>).
>  > 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?
> 
> The reason is that the CI code found a library instance (INF) in
> UefiCpuPkg that cannot be built *stand-alone* (i.e. without being
> consumed by a different UEFI module / INF file).
> 
> In core packages, library instances should be buildable stand-alone with
> the "-m" build flag, and for that, the lib instances need to be listed
> in the [Components] section.
> 
>  > Is it due to the
>  > [Components] section of the UefiCpuPkg/UefiCpuPkg.dsc file?
> 
> Yes.
> 
>  > Should that
>  > section not use the !if check and just list both .inf files
>  > (SecPeiCpuExceptionHandlerLib.inf and
>  > Xcode5SecPeiCpuExceptionHandlerLib.inf)?
> 
> Hmmm, this is a very good point; after all, the updated (=reverted)
> "SecPeiCpuExceptionHandlerLib.inf" instance will not build with XCODE5.
> Therefore we should list both lib instance INF files under [Components],
> but make "SecPeiCpuExceptionHandlerLib.inf" conditional on non-XCODE5.
> 
> I wonder how the CI logic will cope with this!
> 
> Thanks,
> Laszlo
> 
> 
>