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

["Bret Barkelew" <bret.barkelew@microsoft.com>]

[-- Attachment #1: Type: text/plain, Size: 11812 bytes --]

<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?

- 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
>>>
>>>
>>> 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.
>>>
>>> -;
>>> -; 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).
> 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





[-- Attachment #2: Type: text/html, Size: 20879 bytes --]