- Bret
*From: *Laszlo Ersek via groups.io <mailto:lersek=redhat....@groups.io>
*Sent: *Wednesday, May 6, 2020 9:33 AM
*To: *Tom Lendacky <mailto:thomas.lenda...@amd.com>; devel@edk2.groups.io
<mailto:devel@edk2.groups.io>
*Cc: *Jordan Justen <mailto:jordan.l.jus...@intel.com>; Ard Biesheuvel
<mailto:ard.biesheu...@linaro.org>; Liming Gao
<mailto:liming....@intel.com>; Eric Dong <mailto:eric.d...@intel.com>; Ray
Ni <mailto:ray...@intel.com>; Brijesh Singh
<mailto:brijesh.si...@amd.com>; Anthony Perard
<mailto:anthony.per...@citrix.com>; Benjamin You
<mailto:benjamin....@intel.com>; Guo Dong <mailto:guo.d...@intel.com>;
Julien Grall <mailto:jul...@xen.org>; Maurice Ma
<mailto:maurice...@intel.com>; Andrew Fish <mailto:af...@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&data=02%7C01%7CBret.Barkelew%40microsoft.com%7Ca9365fbefc53477c9e3308d7f1db4560%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637243796372782584&sdata=R%2B5IgncDzx7viv3yIwX3MloX1QlzuUJ4bZlKnc%2B0xoI%3D&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.d...@intel.com>
>>> Cc: Ray Ni <ray...@intel.com>
>>> Cc: Laszlo Ersek <ler...@redhat.com>
>>> Cc: Liming Gao <liming....@intel.com>
>>> Signed-off-by: Tom Lendacky <thomas.lenda...@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&data=02%7C01%7CBret.Barkelew%40microsoft.com%7Ca9365fbefc53477c9e3308d7f1db4560%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637243796372792580&sdata=CZovXRJ6HURgo6sz%2FDi55SUy9IsrJ2pFzcHX%2Bp%2Fv8qA%3D&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 <ler...@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&data=02%7C01%7CBret.Barkelew%40microsoft.com%7Ca9365fbefc53477c9e3308d7f1db4560%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637243796372792580&sdata=N2aYxzfr%2BNw7hkhBTEg0C%2Fm4Hv00xXBZhSz3%2FFgiW1s%3D&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
