Re: [edk2-devel] [PATCH v2 1/3] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific

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

On 5/6/20 2:01 PM, Laszlo Ersek via groups.io wrote:
> On 05/06/20 18:33, Tom Lendacky wrote:
>> BZ: 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%7C3c47ab80a1554f27afd608d7f1eff6e8%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243885258646451&sdata=Xh7E1EB%2B1oOCtc2jgtJ8lsjgcwxgswIbgpL4%2BUwpoOw%3D&reserved=0
>>
>> Commit 2db0ccc2d7fe ("UefiCpuPkg: Update CpuExceptionHandlerLib pass
>> XCODE5 tool chain") introduced binary patching into the exception handling
>> support. CPU exception handling is allowed during SEC and this results in
>> binary patching of flash, which should not be done.
>>
>> Separate the changes from commit 2db0ccc2d7fe into an XCODE5 toolchain
>> specific file, Xcode5ExceptionHandlerAsm.nasm, and create a new SEC INF
>> file for the XCODE5 version of CpuExceptionHandlerLib.
>>
>> Since binary patching is allowed when running outside of flash, switch
>> the Dxe, Pei and Smm versions of the CpuExceptionHandlerLib over to use
>> the Xcode5ExceptionHandlerAsm.nasm file to retain current functionality.
>>
>> 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>
>> ---
>>   UefiCpuPkg/UefiCpuPkg.dsc                     |   5 +
>>   .../DxeCpuExceptionHandlerLib.inf             |   2 +-
>>   .../PeiCpuExceptionHandlerLib.inf             |   2 +-
>>   .../SmmCpuExceptionHandlerLib.inf             |   2 +-
>>   .../Xcode5SecPeiCpuExceptionHandlerLib.inf    |  54 +++
>>   .../X64/Xcode5ExceptionHandlerAsm.nasm        | 396 ++++++++++++++++++
>>   .../Xcode5SecPeiCpuExceptionHandlerLib.uni    |  17 +
>>   7 files changed, 475 insertions(+), 3 deletions(-)
>>   create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
>>   create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
>>   create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni
>>
>> diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc
>> index d28cb5cccb52..264e5a787bce 100644
>> --- a/UefiCpuPkg/UefiCpuPkg.dsc
>> +++ b/UefiCpuPkg/UefiCpuPkg.dsc
>> @@ -59,7 +59,11 @@ [LibraryClasses]
>>   
>>   [LibraryClasses.common.SEC]
>>     PlatformSecLib|UefiCpuPkg/Library/PlatformSecLibNull/PlatformSecLibNull.inf
>> +!if $(TOOL_CHAIN_TAG) == "XCODE5"
>> +  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
>> +!else
>>     CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
>> +!endif
>>     HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
>>     PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
>>     MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
>> @@ -126,6 +130,7 @@ [Components.IA32, Components.X64]
>>     UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
> 
> (1) I think this lib instance ("SecPeiCpuExceptionHandlerLib.inf") may
> not build with XCODE5 at the end of the series, even in stand-alone
> mode. Thus I think it should be conditionalized with
> 
> !if $(TOOL_CHAIN_TAG) != "XCODE5"
> ...
> !endif
> 
> When using XCODE5, we should only build
> "Xcode5SecPeiCpuExceptionHandlerLib.inf"; otherwise, we should build
> *both* "SecPeiCpuExceptionHandlerLib.inf" and
> ".inf".
> 

This is the area that was resulting in the error when I used the pull
request to run the integration tests. I was using an if/else originally,
I'll try just the if != XCODE5 around SecPeiCpuExceptionHandlerLib.inf and
see if that goes through. If not, Brett Barkelew responded with a
suggestion to add the Xcode5SecPeiCpuExceptionHandlerLib.inf to the ignore
list in the CI yaml file.

For the next version, under [Components], it will look like:

@@ -123,9 +127,12 @@ [Components.IA32, Components.X64]
   UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
   UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
   UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
+!endif
   UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
   UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
+  UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
   UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
   UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
   UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf

>>     UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
>>     UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
>> +  UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
>>     UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
>>     UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>>     UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
>> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
>> index e41383573043..61e2ec30b089 100644
>> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
>> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
>> @@ -28,7 +28,7 @@ [Sources.Ia32]
>>     Ia32/ArchInterruptDefs.h
>>   
>>   [Sources.X64]
>> -  X64/ExceptionHandlerAsm.nasm
>> +  X64/Xcode5ExceptionHandlerAsm.nasm
>>     X64/ArchExceptionHandler.c
>>     X64/ArchInterruptDefs.h
>>   
>> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
>> index f31423ac0f91..093374944df6 100644
>> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
>> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
>> @@ -28,7 +28,7 @@ [Sources.Ia32]
>>     Ia32/ArchInterruptDefs.h
>>   
>>   [Sources.X64]
>> -  X64/ExceptionHandlerAsm.nasm
>> +  X64/Xcode5ExceptionHandlerAsm.nasm
>>     X64/ArchExceptionHandler.c
>>     X64/ArchInterruptDefs.h
>>   
>> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
>> index 66c7f59e3c91..2ffbbccc302f 100644
>> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
>> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
>> @@ -28,7 +28,7 @@ [Sources.Ia32]
>>     Ia32/ArchInterruptDefs.h
>>   
>>   [Sources.X64]
>> -  X64/ExceptionHandlerAsm.nasm
>> +  X64/Xcode5ExceptionHandlerAsm.nasm
>>     X64/ArchExceptionHandler.c
>>     X64/ArchInterruptDefs.h
>>   
>> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
>> new file mode 100644
>> index 000000000000..3ed1378d6fa6
>> --- /dev/null
>> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
>> @@ -0,0 +1,54 @@
>> +## @file
>> +#  CPU Exception Handler library instance for SEC/PEI modules.
>> +#
>> +#  Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.<BR>
>> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> (2) This is a customized copy of "SecPeiCpuExceptionHandlerLib.inf"; I
> think you should prepend your (C) notice.

Ok.

> 
>> +#
>> +#  This is the XCODE5 variant of the SEC/PEI CpuExceptionHandlerLib. This
>> +#  variant performs binary patching to fix up addresses that allow the
>> +#  XCODE5 toolchain to be used.
>> +#
>> +##
>> +
>> +[Defines]
>> +  INF_VERSION                    = 0x00010005
>> +  BASE_NAME                      = Xcode5SecPeiCpuExceptionHandlerLib
>> +  MODULE_UNI_FILE                = Xcode5SecPeiCpuExceptionHandlerLib.uni
>> +  FILE_GUID                      = 49C481AF-1621-42F3-8FA1-27C64143E304
>> +  MODULE_TYPE                    = PEIM
>> +  VERSION_STRING                 = 1.1
>> +  LIBRARY_CLASS                  = CpuExceptionHandlerLib|SEC PEI_CORE PEIM
>> +

<... SNIP ...>

>> +global ASM_PFX(AsmVectorNumFixup)
>> +ASM_PFX(AsmVectorNumFixup):
>> +    mov     rax, rdx
>> +    mov     [rcx + (@VectorNum - HookAfterStubHeaderBegin)], al
>> +    ret
>> +
>> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni
>> new file mode 100644
>> index 000000000000..be69992cef09
>> --- /dev/null
>> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.uni
>> @@ -0,0 +1,17 @@
>> +// /** @file
>> +// XCODE5 CPU Exception Handler library instance for SEC/PEI modules.
>> +//
>> +// CPU Exception Handler library instance for SEC/PEI modules when built
>> +// using the XCODE5 toolchain.
>> +//
>> +// Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.<BR>
>> +//
>> +// SPDX-License-Identifier: BSD-2-Clause-Patent
>> +//
>> +// **/
>> +
>> +
>> +#string STR_MODULE_ABSTRACT             #language en-US "CPU Exception Handler library instance for SEC/PEI modules with the XCODE5 toolchain."
>> +
>> +#string STR_MODULE_DESCRIPTION          #language en-US "CPU Exception Handler library instance for SEC/PEI modules with the XCODE5 toolchain."
>> +
>>
> 
> (3) This is a brand new file; I think you should prepend your (C) notice.

Ok.

> 
> Meta-hint: with patches like this, it sometimes makes sense to format
> the series for posting with "--find-copies-harder".

Ah, I'll do that on the next version.

Thanks,
Tom

> 
> With (1) through (3) updated:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks,
> Laszlo
> 
> 
> 
>