public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH 0/4] XCODE5 toolchain binary patching fix
@ 2020-05-01 20:17 Lendacky, Thomas
  2020-05-01 20:17 ` [PATCH 1/4] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific Lendacky, Thomas
                   ` (4 more replies)
  0 siblings, 5 replies; 17+ messages in thread
From: Lendacky, Thomas @ 2020-05-01 20:17 UTC (permalink / raw)
  To: devel
  Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Liming Gao,
	Eric Dong, Ray Ni, Brijesh Singh, Anthony Perard, Benjamin You,
	Guo Dong, Julien Grall, Maurice Ma, Andrew Fish, Hao A Wu,
	Jian J Wang, Michael D Kinney


BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2340

Commit 2db0ccc2d7fe ("UefiCpuPkg: Update CpuExceptionHandlerLib pass
XCODE5 tool chain") introduced binary patching in the
ExceptionHandlerAsm.nasm in order to support the XCODE5 toolchain.
However, the CpuExceptionHandlerLib can be used during SEC phase which
would result in binary patching of flash.

This series creates a new CpuExceptionHandlerLib file to support
the required binary patching for the XCODE5 toolchain, while reverting
the changes from commit 2db0ccc2d7fe in the standard file.

This is accomplished in phases:
  - Create a new XCODE5 specific version of the ExceptionHandlerAsm.nasm
    file
  - Update the DSC files that use the CpuExceptionHandlerLib library to
    to use the XCODE5 version of the library when the XCODE5 toolchain
    is used.
  - Revert the changes made by commit 2db0ccc2d7fe in the standard file.

I don't have access to an XCODE5 toolchain setup, so I have not tested
this with XCODE5. I would like to request that someone who does please
test this.

---

These patches are based on commit:
e54310451f1a ("OvmfPkg: Add VBE2 mode info structure to LegacyVgaBios.h")

Cc: Andrew Fish <afish@apple.com>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Benjamin You <benjamin.you@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien@xen.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Ray Ni <ray.ni@intel.com>

Tom Lendacky (4):
  UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific
  UefiPayloadPkg: Use toolchain appropriate CpuExceptionHandlerLib
  OvmfPkg: Use toolchain appropriate CpuExceptionHandlerLib
  UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard
    CpuExceptionHandlerLib

 OvmfPkg/OvmfPkgIa32.dsc                       |  20 +
 OvmfPkg/OvmfPkgIa32X64.dsc                    |  20 +
 OvmfPkg/OvmfPkgX64.dsc                        |  20 +
 OvmfPkg/OvmfXen.dsc                           |  16 +
 UefiCpuPkg/UefiCpuPkg.dsc                     |  23 +
 UefiPayloadPkg/UefiPayloadPkgIa32.dsc         |   8 +
 .../Xcode5DxeCpuExceptionHandlerLib.inf       |  64 +++
 .../Xcode5PeiCpuExceptionHandlerLib.inf       |  63 +++
 .../Xcode5SecPeiCpuExceptionHandlerLib.inf    |  55 +++
 .../Xcode5SmmCpuExceptionHandlerLib.inf       |  59 +++
 .../X64/ExceptionHandlerAsm.nasm              |  25 +-
 .../X64/Xcode5ExceptionHandlerAsm.nasm        | 413 ++++++++++++++++++
 12 files changed, 767 insertions(+), 19 deletions(-)
 create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
 create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5PeiCpuExceptionHandlerLib.inf
 create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
 create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SmmCpuExceptionHandlerLib.inf
 create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm

-- 
2.17.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/4] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific
  2020-05-01 20:17 [PATCH 0/4] XCODE5 toolchain binary patching fix Lendacky, Thomas
@ 2020-05-01 20:17 ` Lendacky, Thomas
  2020-05-05 21:39   ` [edk2-devel] " Laszlo Ersek
  2020-05-01 20:17 ` [PATCH 2/4] UefiPayloadPkg: Use toolchain appropriate CpuExceptionHandlerLib Lendacky, Thomas
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 17+ messages in thread
From: Lendacky, Thomas @ 2020-05-01 20:17 UTC (permalink / raw)
  To: devel
  Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Liming Gao,
	Eric Dong, Ray Ni, Brijesh Singh, Anthony Perard, Benjamin You,
	Guo Dong, Julien Grall, Maurice Ma, Andrew Fish

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2340

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 new INF files
for an XCODE5 version of CpuExceptionHandlerLib. Update the UefiCpuPkg.dsc
file to use the new files when the XCODE5 toolchain is used.

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                     |  23 +
 .../Xcode5DxeCpuExceptionHandlerLib.inf       |  64 +++
 .../Xcode5PeiCpuExceptionHandlerLib.inf       |  63 +++
 .../Xcode5SecPeiCpuExceptionHandlerLib.inf    |  55 +++
 .../Xcode5SmmCpuExceptionHandlerLib.inf       |  59 +++
 .../X64/Xcode5ExceptionHandlerAsm.nasm        | 413 ++++++++++++++++++
 6 files changed, 677 insertions(+)
 create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
 create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5PeiCpuExceptionHandlerLib.inf
 create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
 create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SmmCpuExceptionHandlerLib.inf
 create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm

diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc
index d28cb5cccb52..ad298011232d 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/SecPeiCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
+!endif
   HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
   PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
   MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
@@ -73,12 +77,20 @@ [LibraryClasses.common.PEIM]
 
 [LibraryClasses.IA32.PEIM, LibraryClasses.X64.PEIM]
   PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5PeiCpuExceptionHandlerLib.inf
+!endif
 
 [LibraryClasses.common.DXE_DRIVER]
   MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
+!endif
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
   RegisterCpuFeaturesLib|UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.inf
 
@@ -86,7 +98,11 @@ [LibraryClasses.common.DXE_SMM_DRIVER]
   SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.inf
   MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLib/SmmMemoryAllocationLib.inf
   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SmmCpuExceptionHandlerLib.inf
+!endif
 
 [LibraryClasses.common.UEFI_APPLICATION]
   UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
@@ -122,10 +138,17 @@ [Components.IA32, Components.X64]
   UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
   UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
   UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
   UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
   UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
   UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
+!else
+  UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
+  UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
+  UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SmmCpuExceptionHandlerLib.inf
+  UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5PeiCpuExceptionHandlerLib.inf
+!endif
   UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
   UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
   UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
new file mode 100644
index 000000000000..ef37efec6246
--- /dev/null
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
@@ -0,0 +1,64 @@
+## @file
+#  CPU Exception Handler library instance for DXE modules.
+#
+#  Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved.<BR>
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = Xcode5DxeCpuExceptionHandlerLib
+  MODULE_UNI_FILE                = DxeCpuExceptionHandlerLib.uni
+  FILE_GUID                      = B6E9835A-EDCF-4748-98A8-27D3C722E02D
+  MODULE_TYPE                    = DXE_DRIVER
+  VERSION_STRING                 = 1.1
+  LIBRARY_CLASS                  = CpuExceptionHandlerLib|DXE_CORE DXE_DRIVER UEFI_APPLICATION
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+#  VALID_ARCHITECTURES           = IA32 X64
+#
+
+[Sources.Ia32]
+  Ia32/ExceptionHandlerAsm.nasm
+  Ia32/ExceptionTssEntryAsm.nasm
+  Ia32/ArchExceptionHandler.c
+  Ia32/ArchInterruptDefs.h
+  Ia32/ArchAMDSevVcHandler.c
+
+[Sources.X64]
+  X64/Xcode5ExceptionHandlerAsm.nasm
+  X64/ArchExceptionHandler.c
+  X64/ArchInterruptDefs.h
+  X64/ArchAMDSevVcHandler.c
+
+[Sources.common]
+  CpuExceptionCommon.h
+  CpuExceptionCommon.c
+  PeiDxeSmmCpuException.c
+  DxeException.c
+  AMDSevVcHandler.c
+  AMDSevVcCommon.h
+
+[Pcd]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuStackSwitchExceptionList
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  UefiCpuPkg/UefiCpuPkg.dec
+
+[LibraryClasses]
+  BaseLib
+  SerialPortLib
+  PrintLib
+  SynchronizationLib
+  LocalApicLib
+  PeCoffGetEntryPointLib
+  MemoryAllocationLib
+  DebugLib
+  VmgExitLib
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5PeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5PeiCpuExceptionHandlerLib.inf
new file mode 100644
index 000000000000..830ed1eb8bad
--- /dev/null
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5PeiCpuExceptionHandlerLib.inf
@@ -0,0 +1,63 @@
+## @file
+#  CPU Exception Handler library instance for PEI module.
+#
+#  Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = Xcode5PeiCpuExceptionHandlerLib
+  MODULE_UNI_FILE                = PeiCpuExceptionHandlerLib.uni
+  FILE_GUID                      = 980DDA67-44A6-4897-99E6-275290B71F9E
+  MODULE_TYPE                    = PEIM
+  VERSION_STRING                 = 1.1
+  LIBRARY_CLASS                  = CpuExceptionHandlerLib|PEI_CORE PEIM
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+#  VALID_ARCHITECTURES           = IA32 X64
+#
+
+[Sources.Ia32]
+  Ia32/ExceptionHandlerAsm.nasm
+  Ia32/ExceptionTssEntryAsm.nasm
+  Ia32/ArchExceptionHandler.c
+  Ia32/ArchInterruptDefs.h
+  Ia32/ArchAMDSevVcHandler.c
+
+[Sources.X64]
+  X64/Xcode5ExceptionHandlerAsm.nasm
+  X64/ArchExceptionHandler.c
+  X64/ArchInterruptDefs.h
+  X64/ArchAMDSevVcHandler.c
+
+[Sources.common]
+  CpuExceptionCommon.h
+  CpuExceptionCommon.c
+  PeiCpuException.c
+  PeiDxeSmmCpuException.c
+  AMDSevVcHandler.c
+  AMDSevVcCommon.h
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  UefiCpuPkg/UefiCpuPkg.dec
+
+[LibraryClasses]
+  BaseLib
+  SerialPortLib
+  PrintLib
+  LocalApicLib
+  PeCoffGetEntryPointLib
+  HobLib
+  MemoryAllocationLib
+  SynchronizationLib
+  VmgExitLib
+
+[Pcd]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard    # CONSUMES
+
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
new file mode 100644
index 000000000000..36420be22faa
--- /dev/null
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
@@ -0,0 +1,55 @@
+## @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
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = Xcode5SecPeiCpuExceptionHandlerLib
+  MODULE_UNI_FILE                = SecPeiCpuExceptionHandlerLib.uni
+  FILE_GUID                      = CA4BBC99-DFC6-4234-B553-8B6586B7B113
+  MODULE_TYPE                    = PEIM
+  VERSION_STRING                 = 1.1
+  LIBRARY_CLASS                  = CpuExceptionHandlerLib|SEC PEI_CORE PEIM
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+#  VALID_ARCHITECTURES           = IA32 X64
+#
+
+[Sources.Ia32]
+  Ia32/ExceptionHandlerAsm.nasm
+  Ia32/ExceptionTssEntryAsm.nasm
+  Ia32/ArchExceptionHandler.c
+  Ia32/ArchInterruptDefs.h
+  Ia32/ArchAMDSevVcHandler.c
+
+[Sources.X64]
+  X64/Xcode5ExceptionHandlerAsm.nasm
+  X64/ArchExceptionHandler.c
+  X64/ArchInterruptDefs.h
+  X64/ArchAMDSevVcHandler.c
+
+[Sources.common]
+  CpuExceptionCommon.h
+  CpuExceptionCommon.c
+  SecPeiCpuException.c
+  AMDSevVcHandler.c
+  AMDSevVcCommon.h
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  UefiCpuPkg/UefiCpuPkg.dec
+
+[LibraryClasses]
+  BaseLib
+  SerialPortLib
+  PrintLib
+  LocalApicLib
+  PeCoffGetEntryPointLib
+  VmgExitLib
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SmmCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SmmCpuExceptionHandlerLib.inf
new file mode 100644
index 000000000000..8f71a45c86d5
--- /dev/null
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SmmCpuExceptionHandlerLib.inf
@@ -0,0 +1,59 @@
+## @file
+#  CPU Exception Handler library instance for SMM modules.
+#
+#  Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved.<BR>
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = Xcode5SmmCpuExceptionHandlerLib
+  MODULE_UNI_FILE                = SmmCpuExceptionHandlerLib.uni
+  FILE_GUID                      = 8D2C439B-3981-42ff-9CE5-1B50ECA502D6
+  MODULE_TYPE                    = DXE_SMM_DRIVER
+  VERSION_STRING                 = 1.1
+  LIBRARY_CLASS                  = CpuExceptionHandlerLib|DXE_SMM_DRIVER
+
+#
+# The following information is for reference only and not required by the build tools.
+#
+#  VALID_ARCHITECTURES           = IA32 X64
+#
+
+[Sources.Ia32]
+  Ia32/ExceptionHandlerAsm.nasm
+  Ia32/ExceptionTssEntryAsm.nasm
+  Ia32/ArchExceptionHandler.c
+  Ia32/ArchInterruptDefs.h
+  Ia32/ArchAMDSevVcHandler.c
+
+[Sources.X64]
+  X64/Xcode5ExceptionHandlerAsm.nasm
+  X64/ArchExceptionHandler.c
+  X64/ArchInterruptDefs.h
+  X64/ArchAMDSevVcHandler.c
+
+[Sources.common]
+  CpuExceptionCommon.h
+  CpuExceptionCommon.c
+  PeiDxeSmmCpuException.c
+  SmmException.c
+  AMDSevVcHandler.c
+  AMDSevVcCommon.h
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  UefiCpuPkg/UefiCpuPkg.dec
+
+[LibraryClasses]
+  BaseLib
+  SerialPortLib
+  PrintLib
+  SynchronizationLib
+  LocalApicLib
+  PeCoffGetEntryPointLib
+  DebugLib
+  VmgExitLib
+
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
new file mode 100644
index 000000000000..26cae56cc5cf
--- /dev/null
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
@@ -0,0 +1,413 @@
+;------------------------------------------------------------------------------ ;
+; Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.<BR>
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+; Module Name:
+;
+;   ExceptionHandlerAsm.Asm
+;
+; Abstract:
+;
+;   x64 CPU Exception Handler
+;
+; Notes:
+;
+;------------------------------------------------------------------------------
+
+;
+; CommonExceptionHandler()
+;
+
+%define VC_EXCEPTION 29
+
+extern ASM_PFX(mErrorCodeFlag)    ; Error code flags for exceptions
+extern ASM_PFX(mDoFarReturnFlag)  ; Do far return flag
+extern ASM_PFX(CommonExceptionHandler)
+
+SECTION .data
+
+DEFAULT REL
+SECTION .text
+
+ALIGN   8
+
+AsmIdtVectorBegin:
+%rep  32
+    db      0x6a        ; push  #VectorNum
+    db      ($ - AsmIdtVectorBegin) / ((AsmIdtVectorEnd - AsmIdtVectorBegin) / 32) ; VectorNum
+    push    rax
+    mov     rax, strict qword 0 ;    mov     rax, ASM_PFX(CommonInterruptEntry)
+    jmp     rax
+%endrep
+AsmIdtVectorEnd:
+
+HookAfterStubHeaderBegin:
+    db      0x6a        ; push
+@VectorNum:
+    db      0          ; 0 will be fixed
+    push    rax
+    mov     rax, strict qword 0 ;     mov     rax, HookAfterStubHeaderEnd
+JmpAbsoluteAddress:
+    jmp     rax
+HookAfterStubHeaderEnd:
+    mov     rax, rsp
+    and     sp,  0xfff0        ; make sure 16-byte aligned for exception context
+    sub     rsp, 0x18           ; reserve room for filling exception data later
+    push    rcx
+    mov     rcx, [rax + 8]
+    bt      [ASM_PFX(mErrorCodeFlag)], ecx
+    jnc     .0
+    push    qword [rsp]             ; push additional rcx to make stack alignment
+.0:
+    xchg    rcx, [rsp]        ; restore rcx, save Exception Number in stack
+    push    qword [rax]             ; push rax into stack to keep code consistence
+
+;---------------------------------------;
+; CommonInterruptEntry                  ;
+;---------------------------------------;
+; The follow algorithm is used for the common interrupt routine.
+; Entry from each interrupt with a push eax and eax=interrupt number
+; Stack frame would be as follows as specified in IA32 manuals:
+;
+; +---------------------+ <-- 16-byte aligned ensured by processor
+; +    Old SS           +
+; +---------------------+
+; +    Old RSP          +
+; +---------------------+
+; +    RFlags           +
+; +---------------------+
+; +    CS               +
+; +---------------------+
+; +    RIP              +
+; +---------------------+
+; +    Error Code       +
+; +---------------------+
+; +   Vector Number     +
+; +---------------------+
+; +    RBP              +
+; +---------------------+ <-- RBP, 16-byte aligned
+; The follow algorithm is used for the common interrupt routine.
+global ASM_PFX(CommonInterruptEntry)
+ASM_PFX(CommonInterruptEntry):
+    cli
+    pop     rax
+    ;
+    ; All interrupt handlers are invoked through interrupt gates, so
+    ; IF flag automatically cleared at the entry point
+    ;
+    xchg    rcx, [rsp]      ; Save rcx into stack and save vector number into rcx
+    and     rcx, 0xFF
+    cmp     ecx, 32         ; Intel reserved vector for exceptions?
+    jae     NoErrorCode
+    bt      [ASM_PFX(mErrorCodeFlag)], ecx
+    jc      HasErrorCode
+
+NoErrorCode:
+
+    ;
+    ; Push a dummy error code on the stack
+    ; to maintain coherent stack map
+    ;
+    push    qword [rsp]
+    mov     qword [rsp + 8], 0
+HasErrorCode:
+    push    rbp
+    mov     rbp, rsp
+    push    0             ; clear EXCEPTION_HANDLER_CONTEXT.OldIdtHandler
+    push    0             ; clear EXCEPTION_HANDLER_CONTEXT.ExceptionDataFlag
+
+    ;
+    ; Stack:
+    ; +---------------------+ <-- 16-byte aligned ensured by processor
+    ; +    Old SS           +
+    ; +---------------------+
+    ; +    Old RSP          +
+    ; +---------------------+
+    ; +    RFlags           +
+    ; +---------------------+
+    ; +    CS               +
+    ; +---------------------+
+    ; +    RIP              +
+    ; +---------------------+
+    ; +    Error Code       +
+    ; +---------------------+
+    ; + RCX / Vector Number +
+    ; +---------------------+
+    ; +    RBP              +
+    ; +---------------------+ <-- RBP, 16-byte aligned
+    ;
+
+    ;
+    ; Since here the stack pointer is 16-byte aligned, so
+    ; EFI_FX_SAVE_STATE_X64 of EFI_SYSTEM_CONTEXT_x64
+    ; is 16-byte aligned
+    ;
+
+;; UINT64  Rdi, Rsi, Rbp, Rsp, Rbx, Rdx, Rcx, Rax;
+;; UINT64  R8, R9, R10, R11, R12, R13, R14, R15;
+    push r15
+    push r14
+    push r13
+    push r12
+    push r11
+    push r10
+    push r9
+    push r8
+    push rax
+    push qword [rbp + 8]   ; RCX
+    push rdx
+    push rbx
+    push qword [rbp + 48]  ; RSP
+    push qword [rbp]       ; RBP
+    push rsi
+    push rdi
+
+;; UINT64  Gs, Fs, Es, Ds, Cs, Ss;  insure high 16 bits of each is zero
+    movzx   rax, word [rbp + 56]
+    push    rax                      ; for ss
+    movzx   rax, word [rbp + 32]
+    push    rax                      ; for cs
+    mov     rax, ds
+    push    rax
+    mov     rax, es
+    push    rax
+    mov     rax, fs
+    push    rax
+    mov     rax, gs
+    push    rax
+
+    mov     [rbp + 8], rcx               ; save vector number
+
+;; UINT64  Rip;
+    push    qword [rbp + 24]
+
+;; UINT64  Gdtr[2], Idtr[2];
+    xor     rax, rax
+    push    rax
+    push    rax
+    sidt    [rsp]
+    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]
+    mov     bx, word [rsp]
+    mov     rax, qword [rsp + 2]
+    mov     qword [rsp], rax
+    mov     word [rsp + 8], bx
+
+;; UINT64  Ldtr, Tr;
+    xor     rax, rax
+    str     ax
+    push    rax
+    sldt    ax
+    push    rax
+
+;; UINT64  RFlags;
+    push    qword [rbp + 40]
+
+;; UINT64  Cr0, Cr1, Cr2, Cr3, Cr4, Cr8;
+    mov     rax, cr8
+    push    rax
+    mov     rax, cr4
+    or      rax, 0x208
+    mov     cr4, rax
+    push    rax
+    mov     rax, cr3
+    push    rax
+    mov     rax, cr2
+    push    rax
+    xor     rax, rax
+    push    rax
+    mov     rax, cr0
+    push    rax
+
+;; UINT64  Dr0, Dr1, Dr2, Dr3, Dr6, Dr7;
+    cmp     qword [rbp + 8], VC_EXCEPTION
+    je      VcDebugRegs          ; For SEV-ES (#VC) Debug registers ignored
+
+    mov     rax, dr7
+    push    rax
+    mov     rax, dr6
+    push    rax
+    mov     rax, dr3
+    push    rax
+    mov     rax, dr2
+    push    rax
+    mov     rax, dr1
+    push    rax
+    mov     rax, dr0
+    push    rax
+    jmp     DrFinish
+
+VcDebugRegs:
+;; UINT64  Dr0, Dr1, Dr2, Dr3, Dr6, Dr7 are skipped for #VC to avoid exception recursion
+    xor     rax, rax
+    push    rax
+    push    rax
+    push    rax
+    push    rax
+    push    rax
+    push    rax
+
+DrFinish:
+;; FX_SAVE_STATE_X64 FxSaveState;
+    sub rsp, 512
+    mov rdi, rsp
+    db 0xf, 0xae, 0x7 ;fxsave [rdi]
+
+;; UEFI calling convention for x64 requires that Direction flag in EFLAGs is clear
+    cld
+
+;; UINT32  ExceptionData;
+    push    qword [rbp + 16]
+
+;; Prepare parameter and call
+    mov     rcx, [rbp + 8]
+    mov     rdx, rsp
+    ;
+    ; Per X64 calling convention, allocate maximum parameter stack space
+    ; and make sure RSP is 16-byte aligned
+    ;
+    sub     rsp, 4 * 8 + 8
+    call    ASM_PFX(CommonExceptionHandler)
+    add     rsp, 4 * 8 + 8
+
+    cli
+;; UINT64  ExceptionData;
+    add     rsp, 8
+
+;; FX_SAVE_STATE_X64 FxSaveState;
+
+    mov rsi, rsp
+    db 0xf, 0xae, 0xE ; fxrstor [rsi]
+    add rsp, 512
+
+;; UINT64  Dr0, Dr1, Dr2, Dr3, Dr6, Dr7;
+;; Skip restoration of DRx registers to support in-circuit emualators
+;; or debuggers set breakpoint in interrupt/exception context
+    add     rsp, 8 * 6
+
+;; UINT64  Cr0, Cr1, Cr2, Cr3, Cr4, Cr8;
+    pop     rax
+    mov     cr0, rax
+    add     rsp, 8   ; not for Cr1
+    pop     rax
+    mov     cr2, rax
+    pop     rax
+    mov     cr3, rax
+    pop     rax
+    mov     cr4, rax
+    pop     rax
+    mov     cr8, rax
+
+;; UINT64  RFlags;
+    pop     qword [rbp + 40]
+
+;; UINT64  Ldtr, Tr;
+;; UINT64  Gdtr[2], Idtr[2];
+;; Best not let anyone mess with these particular registers...
+    add     rsp, 48
+
+;; UINT64  Rip;
+    pop     qword [rbp + 24]
+
+;; UINT64  Gs, Fs, Es, Ds, Cs, Ss;
+    pop     rax
+    ; mov     gs, rax ; not for gs
+    pop     rax
+    ; mov     fs, rax ; not for fs
+    ; (X64 will not use fs and gs, so we do not restore it)
+    pop     rax
+    mov     es, rax
+    pop     rax
+    mov     ds, rax
+    pop     qword [rbp + 32]  ; for cs
+    pop     qword [rbp + 56]  ; for ss
+
+;; UINT64  Rdi, Rsi, Rbp, Rsp, Rbx, Rdx, Rcx, Rax;
+;; UINT64  R8, R9, R10, R11, R12, R13, R14, R15;
+    pop     rdi
+    pop     rsi
+    add     rsp, 8               ; not for rbp
+    pop     qword [rbp + 48] ; for rsp
+    pop     rbx
+    pop     rdx
+    pop     rcx
+    pop     rax
+    pop     r8
+    pop     r9
+    pop     r10
+    pop     r11
+    pop     r12
+    pop     r13
+    pop     r14
+    pop     r15
+
+    mov     rsp, rbp
+    pop     rbp
+    add     rsp, 16
+    cmp     qword [rsp - 32], 0  ; check EXCEPTION_HANDLER_CONTEXT.OldIdtHandler
+    jz      DoReturn
+    cmp     qword [rsp - 40], 1  ; check EXCEPTION_HANDLER_CONTEXT.ExceptionDataFlag
+    jz      ErrorCode
+    jmp     qword [rsp - 32]
+ErrorCode:
+    sub     rsp, 8
+    jmp     qword [rsp - 24]
+
+DoReturn:
+    cmp     qword [ASM_PFX(mDoFarReturnFlag)], 0   ; Check if need to do far return instead of IRET
+    jz      DoIret
+    push    rax
+    mov     rax, rsp          ; save old RSP to rax
+    mov     rsp, [rsp + 0x20]
+    push    qword [rax + 0x10]       ; save CS in new location
+    push    qword [rax + 0x8]        ; save EIP in new location
+    push    qword [rax + 0x18]       ; save EFLAGS in new location
+    mov     rax, [rax]        ; restore rax
+    popfq                     ; restore EFLAGS
+    DB      0x48               ; prefix to composite "retq" with next "retf"
+    retf                      ; far return
+DoIret:
+    iretq
+
+;-------------------------------------------------------------------------------------
+;  GetTemplateAddressMap (&AddressMap);
+;-------------------------------------------------------------------------------------
+; comments here for definition of address map
+global ASM_PFX(AsmGetTemplateAddressMap)
+ASM_PFX(AsmGetTemplateAddressMap):
+    lea     rax, [AsmIdtVectorBegin]
+    mov     qword [rcx], rax
+    mov     qword [rcx + 0x8],  (AsmIdtVectorEnd - AsmIdtVectorBegin) / 32
+    lea     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
+
+;-------------------------------------------------------------------------------------
+;  AsmVectorNumFixup (*NewVectorAddr, VectorNum, *OldVectorAddr);
+;-------------------------------------------------------------------------------------
+global ASM_PFX(AsmVectorNumFixup)
+ASM_PFX(AsmVectorNumFixup):
+    mov     rax, rdx
+    mov     [rcx + (@VectorNum - HookAfterStubHeaderBegin)], al
+    ret
+
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 2/4] UefiPayloadPkg: Use toolchain appropriate CpuExceptionHandlerLib
  2020-05-01 20:17 [PATCH 0/4] XCODE5 toolchain binary patching fix Lendacky, Thomas
  2020-05-01 20:17 ` [PATCH 1/4] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific Lendacky, Thomas
@ 2020-05-01 20:17 ` Lendacky, Thomas
  2020-05-05 22:19   ` [edk2-devel] " Laszlo Ersek
  2020-05-01 20:17 ` [PATCH 3/4] OvmfPkg: " Lendacky, Thomas
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 17+ messages in thread
From: Lendacky, Thomas @ 2020-05-01 20:17 UTC (permalink / raw)
  To: devel
  Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Liming Gao,
	Eric Dong, Ray Ni, Brijesh Singh, Anthony Perard, Benjamin You,
	Guo Dong, Julien Grall, Maurice Ma, Andrew Fish

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2340

Use the XCODE5 CpuExceptionHandlerLib library in place of the standard
library when building with the XCODE5 toolchain. The XCODE5 version of
the library performs binary patching and should only be used when building
with the XCODE5 toolchain.

Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Benjamin You <benjamin.you@intel.com>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 UefiPayloadPkg/UefiPayloadPkgIa32.dsc | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
index d52945442e0e..2bf7aafd8c77 100644
--- a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
@@ -232,7 +232,11 @@ [LibraryClasses.common.DXE_CORE]
 !if $(SOURCE_DEBUG_ENABLE)
   DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgentLib.inf
 !endif
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
+!endif
 
 [LibraryClasses.common.DXE_DRIVER]
   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
@@ -243,7 +247,11 @@ [LibraryClasses.common.DXE_DRIVER]
 !if $(SOURCE_DEBUG_ENABLE)
   DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgentLib.inf
 !endif
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
+!endif
   MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
 
 [LibraryClasses.common.DXE_RUNTIME_DRIVER]
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 3/4] OvmfPkg: Use toolchain appropriate CpuExceptionHandlerLib
  2020-05-01 20:17 [PATCH 0/4] XCODE5 toolchain binary patching fix Lendacky, Thomas
  2020-05-01 20:17 ` [PATCH 1/4] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific Lendacky, Thomas
  2020-05-01 20:17 ` [PATCH 2/4] UefiPayloadPkg: Use toolchain appropriate CpuExceptionHandlerLib Lendacky, Thomas
@ 2020-05-01 20:17 ` Lendacky, Thomas
  2020-05-05 21:49   ` [edk2-devel] " Laszlo Ersek
  2020-05-01 20:17 ` [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib Lendacky, Thomas
       [not found] ` <160B00E54624ADAA.10991@groups.io>
  4 siblings, 1 reply; 17+ messages in thread
From: Lendacky, Thomas @ 2020-05-01 20:17 UTC (permalink / raw)
  To: devel
  Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Liming Gao,
	Eric Dong, Ray Ni, Brijesh Singh, Anthony Perard, Benjamin You,
	Guo Dong, Julien Grall, Maurice Ma, Andrew Fish, Ard Biesheuvel

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2340

Use the XCODE5 CpuExceptionHandlerLib library in place of the standard
library when building with the XCODE5 toolchain. The XCODE5 version of
the library performs binary patching and should only be used when building
with the XCODE5 toolchain.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Julien Grall <julien@xen.org>
Cc: Liming Gao <liming.gao@intel.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 OvmfPkg/OvmfPkgIa32.dsc    | 20 ++++++++++++++++++++
 OvmfPkg/OvmfPkgIa32X64.dsc | 20 ++++++++++++++++++++
 OvmfPkg/OvmfPkgX64.dsc     | 20 ++++++++++++++++++++
 OvmfPkg/OvmfXen.dsc        | 16 ++++++++++++++++
 4 files changed, 76 insertions(+)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index fcd9779b5ba2..f27fcd7e1087 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -245,7 +245,11 @@ [LibraryClasses.common.SEC]
   PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
   PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
   MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
+!endif
 
 [LibraryClasses.common.PEI_CORE]
   HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
@@ -283,7 +287,11 @@ [LibraryClasses.common.PEIM]
 !if $(SOURCE_DEBUG_ENABLE) == TRUE
   DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentLib.inf
 !endif
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5PeiCpuExceptionHandlerLib.inf
+!endif
   MpInitLib|UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
   PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
@@ -309,7 +317,11 @@ [LibraryClasses.common.DXE_CORE]
 !if $(SOURCE_DEBUG_ENABLE) == TRUE
   DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgentLib.inf
 !endif
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
+!endif
   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
 
 [LibraryClasses.common.DXE_RUNTIME_DRIVER]
@@ -362,7 +374,11 @@ [LibraryClasses.common.DXE_DRIVER]
   PlatformBootManagerLib|OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
   PlatformBmPrintScLib|OvmfPkg/Library/PlatformBmPrintScLib/PlatformBmPrintScLib.inf
   QemuBootOrderLib|OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
+!endif
 !if $(SMM_REQUIRE) == TRUE
   LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.inf
 !else
@@ -413,7 +429,11 @@ [LibraryClasses.common.DXE_SMM_DRIVER]
 !else
   DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
 !endif
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SmmCpuExceptionHandlerLib.inf
+!endif
 !if $(SOURCE_DEBUG_ENABLE) == TRUE
   DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgentLib.inf
 !endif
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 1626d2415a2c..0d3c3559d15f 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -249,7 +249,11 @@ [LibraryClasses.common.SEC]
   PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
   PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
   MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
+!endif
 
 [LibraryClasses.common.PEI_CORE]
   HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
@@ -287,7 +291,11 @@ [LibraryClasses.common.PEIM]
 !if $(SOURCE_DEBUG_ENABLE) == TRUE
   DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentLib.inf
 !endif
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5PeiCpuExceptionHandlerLib.inf
+!endif
   MpInitLib|UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
   PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
@@ -313,7 +321,11 @@ [LibraryClasses.common.DXE_CORE]
 !if $(SOURCE_DEBUG_ENABLE) == TRUE
   DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgentLib.inf
 !endif
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
+!endif
   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
 
 [LibraryClasses.common.DXE_RUNTIME_DRIVER]
@@ -366,7 +378,11 @@ [LibraryClasses.common.DXE_DRIVER]
   PlatformBootManagerLib|OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
   PlatformBmPrintScLib|OvmfPkg/Library/PlatformBmPrintScLib/PlatformBmPrintScLib.inf
   QemuBootOrderLib|OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
+!endif
 !if $(SMM_REQUIRE) == TRUE
   LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.inf
 !else
@@ -417,7 +433,11 @@ [LibraryClasses.common.DXE_SMM_DRIVER]
 !else
   DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
 !endif
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SmmCpuExceptionHandlerLib.inf
+!endif
 !if $(SOURCE_DEBUG_ENABLE) == TRUE
   DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgentLib.inf
 !endif
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 65cfe957761b..c713e65db6f7 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -249,7 +249,11 @@ [LibraryClasses.common.SEC]
   PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
   PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
   MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
+!endif
 
 [LibraryClasses.common.PEI_CORE]
   HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
@@ -287,7 +291,11 @@ [LibraryClasses.common.PEIM]
 !if $(SOURCE_DEBUG_ENABLE) == TRUE
   DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentLib.inf
 !endif
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5PeiCpuExceptionHandlerLib.inf
+!endif
   MpInitLib|UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
   PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
@@ -313,7 +321,11 @@ [LibraryClasses.common.DXE_CORE]
 !if $(SOURCE_DEBUG_ENABLE) == TRUE
   DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgentLib.inf
 !endif
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
+!endif
   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
 
 [LibraryClasses.common.DXE_RUNTIME_DRIVER]
@@ -366,7 +378,11 @@ [LibraryClasses.common.DXE_DRIVER]
   PlatformBootManagerLib|OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
   PlatformBmPrintScLib|OvmfPkg/Library/PlatformBmPrintScLib/PlatformBmPrintScLib.inf
   QemuBootOrderLib|OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
+!endif
 !if $(SMM_REQUIRE) == TRUE
   LockBoxLib|MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxDxeLib.inf
 !else
@@ -417,7 +433,11 @@ [LibraryClasses.common.DXE_SMM_DRIVER]
 !else
   DebugLib|OvmfPkg/Library/PlatformDebugLibIoPort/PlatformDebugLibIoPort.inf
 !endif
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SmmCpuExceptionHandlerLib.inf
+!endif
 !if $(SOURCE_DEBUG_ENABLE) == TRUE
   DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/SmmDebugAgentLib.inf
 !endif
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 8b3615e0b07e..4f1a4f7980a3 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -228,7 +228,11 @@ [LibraryClasses.common.SEC]
   PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
   PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
   MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
+!endif
 
 [LibraryClasses.common.PEI_CORE]
   HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
@@ -256,7 +260,11 @@ [LibraryClasses.common.PEIM]
 !if $(SOURCE_DEBUG_ENABLE) == TRUE
   DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/SecPeiDebugAgentLib.inf
 !endif
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5PeiCpuExceptionHandlerLib.inf
+!endif
   MpInitLib|UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
   QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/PeiQemuFwCfgS3LibFwCfg.inf
   PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
@@ -271,7 +279,11 @@ [LibraryClasses.common.DXE_CORE]
 !if $(SOURCE_DEBUG_ENABLE) == TRUE
   DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgentLib.inf
 !endif
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
+!endif
   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
 
 [LibraryClasses.common.DXE_RUNTIME_DRIVER]
@@ -306,7 +318,11 @@ [LibraryClasses.common.DXE_DRIVER]
   PlatformBootManagerLib|OvmfPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
   PlatformBmPrintScLib|OvmfPkg/Library/PlatformBmPrintScLib/PlatformBmPrintScLib.inf
   QemuBootOrderLib|OvmfPkg/Library/QemuBootOrderLib/QemuBootOrderLib.inf
+!if $(TOOL_CHAIN_TAG) != "XCODE5"
   CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
+!else
+  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
+!endif
   LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
 !if $(SOURCE_DEBUG_ENABLE) == TRUE
   DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgentLib.inf
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib
  2020-05-01 20:17 [PATCH 0/4] XCODE5 toolchain binary patching fix Lendacky, Thomas
                   ` (2 preceding siblings ...)
  2020-05-01 20:17 ` [PATCH 3/4] OvmfPkg: " Lendacky, Thomas
@ 2020-05-01 20:17 ` Lendacky, Thomas
  2020-05-01 20:49   ` [EXTERNAL] [edk2-devel] " Bret Barkelew
  2020-05-05 22:15   ` Laszlo Ersek
       [not found] ` <160B00E54624ADAA.10991@groups.io>
  4 siblings, 2 replies; 17+ messages in thread
From: Lendacky, Thomas @ 2020-05-01 20:17 UTC (permalink / raw)
  To: devel
  Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Liming Gao,
	Eric Dong, Ray Ni, Brijesh Singh, Anthony Perard, Benjamin You,
	Guo Dong, Julien Grall, Maurice Ma, Andrew Fish

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2340

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
 
 ;-------------------------------------------------------------------------------------
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [EXTERNAL] [edk2-devel] [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib
  2020-05-01 20:17 ` [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib Lendacky, Thomas
@ 2020-05-01 20:49   ` Bret Barkelew
  2020-05-05 22:15   ` Laszlo Ersek
  1 sibling, 0 replies; 17+ messages in thread
From: Bret Barkelew @ 2020-05-01 20:49 UTC (permalink / raw)
  To: devel@edk2.groups.io, thomas.lendacky@amd.com
  Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Liming Gao,
	Eric Dong, Ray Ni, Brijesh Singh, Anthony Perard, Benjamin You,
	Guo Dong, Julien Grall, Maurice Ma, Andrew Fish

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

Acked-by: Bret Barkelew <bret.barkelew@microsoft.com>

- Bret

________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Lendacky, Thomas via groups.io <thomas.lendacky=amd.com@groups.io>
Sent: Friday, May 1, 2020 1:17:41 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: Jordan Justen <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Liming Gao <liming.gao@intel.com>; Eric Dong <eric.dong@intel.com>; Ray Ni <ray.ni@intel.com>; Brijesh Singh <brijesh.singh@amd.com>; Anthony Perard <anthony.perard@citrix.com>; Benjamin You <benjamin.you@intel.com>; Guo Dong <guo.dong@intel.com>; Julien Grall <julien@xen.org>; Maurice Ma <maurice.ma@intel.com>; Andrew Fish <afish@apple.com>
Subject: [EXTERNAL] [edk2-devel] [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib

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%7Cc233bc94949a424f66bf08d7ee0cc626%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637239610952998859&amp;sdata=rm%2FpiCDgatUHtU9PEQGWTzFT70XmkpbIukV7eiHWHeo%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

 ;-------------------------------------------------------------------------------------
--
2.17.1





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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific
       [not found] ` <160B00E54624ADAA.10991@groups.io>
@ 2020-05-05 18:50   ` Lendacky, Thomas
  0 siblings, 0 replies; 17+ messages in thread
From: Lendacky, Thomas @ 2020-05-05 18:50 UTC (permalink / raw)
  To: devel
  Cc: Jordan Justen, Laszlo Ersek, Ard Biesheuvel, Liming Gao,
	Eric Dong, Ray Ni, Brijesh Singh, Anthony Perard, Benjamin You,
	Guo Dong, Julien Grall, Maurice Ma, Andrew Fish

On 5/1/20 3:17 PM, Lendacky, Thomas via groups.io wrote:
> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2340&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7Ca1268b8e6d554b6c55ca08d7ee0cbf84%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637239610850064292&amp;sdata=soYzRSutJ%2Bepugf2XnzRUpMCLa1GaKoBD%2FX1F4HPCt8%3D&amp;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 new INF files
> for an XCODE5 version of CpuExceptionHandlerLib. Update the UefiCpuPkg.dsc
> file to use the new files when the XCODE5 toolchain is used.

I used the same FILE_GUID for the new INF files as the old ones. I wasn't 
sure if they should get new GUIDs or use the same GUID as the original 
since only one of the libraries will/should be used at a time.

Let me know if they need new GUIDs and I'll update and repost the series.

Thanks,
Tom

> 
> 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                     |  23 +
>   .../Xcode5DxeCpuExceptionHandlerLib.inf       |  64 +++
>   .../Xcode5PeiCpuExceptionHandlerLib.inf       |  63 +++
>   .../Xcode5SecPeiCpuExceptionHandlerLib.inf    |  55 +++
>   .../Xcode5SmmCpuExceptionHandlerLib.inf       |  59 +++
>   .../X64/Xcode5ExceptionHandlerAsm.nasm        | 413 ++++++++++++++++++
>   6 files changed, 677 insertions(+)
>   create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
>   create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5PeiCpuExceptionHandlerLib.inf
>   create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
>   create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SmmCpuExceptionHandlerLib.inf
>   create mode 100644 UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
> 
> diff --git a/UefiCpuPkg/UefiCpuPkg.dsc b/UefiCpuPkg/UefiCpuPkg.dsc
> index d28cb5cccb52..ad298011232d 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/SecPeiCpuExceptionHandlerLib.inf
> +!else
> +  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
> +!endif
>     HobLib|MdePkg/Library/PeiHobLib/PeiHobLib.inf
>     PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
>     MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
> @@ -73,12 +77,20 @@ [LibraryClasses.common.PEIM]
>   
>   [LibraryClasses.IA32.PEIM, LibraryClasses.X64.PEIM]
>     PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
> +!if $(TOOL_CHAIN_TAG) != "XCODE5"
>     CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> +!else
> +  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5PeiCpuExceptionHandlerLib.inf
> +!endif
>   
>   [LibraryClasses.common.DXE_DRIVER]
>     MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
>     HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> +!if $(TOOL_CHAIN_TAG) != "XCODE5"
>     CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
> +!else
> +  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
> +!endif
>     MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>     RegisterCpuFeaturesLib|UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.inf
>   
> @@ -86,7 +98,11 @@ [LibraryClasses.common.DXE_SMM_DRIVER]
>     SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.inf
>     MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLib/SmmMemoryAllocationLib.inf
>     HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
> +!if $(TOOL_CHAIN_TAG) != "XCODE5"
>     CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
> +!else
> +  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SmmCpuExceptionHandlerLib.inf
> +!endif
>   
>   [LibraryClasses.common.UEFI_APPLICATION]
>     UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
> @@ -122,10 +138,17 @@ [Components.IA32, Components.X64]
>     UefiCpuPkg/Library/BaseXApicLib/BaseXApicLib.inf
>     UefiCpuPkg/Library/BaseXApicX2ApicLib/BaseXApicX2ApicLib.inf
>     UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
> +!if $(TOOL_CHAIN_TAG) != "XCODE5"
>     UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
>     UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
>     UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
>     UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> +!else
> +  UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
> +  UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
> +  UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SmmCpuExceptionHandlerLib.inf
> +  UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5PeiCpuExceptionHandlerLib.inf
> +!endif
>     UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf
>     UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>     UefiCpuPkg/Library/MpInitLibUp/MpInitLibUp.inf
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
> new file mode 100644
> index 000000000000..ef37efec6246
> --- /dev/null
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
> @@ -0,0 +1,64 @@
> +## @file
> +#  CPU Exception Handler library instance for DXE modules.
> +#
> +#  Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved.<BR>
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = Xcode5DxeCpuExceptionHandlerLib
> +  MODULE_UNI_FILE                = DxeCpuExceptionHandlerLib.uni
> +  FILE_GUID                      = B6E9835A-EDCF-4748-98A8-27D3C722E02D
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.1
> +  LIBRARY_CLASS                  = CpuExceptionHandlerLib|DXE_CORE DXE_DRIVER UEFI_APPLICATION
> +
> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +#  VALID_ARCHITECTURES           = IA32 X64
> +#
> +
> +[Sources.Ia32]
> +  Ia32/ExceptionHandlerAsm.nasm
> +  Ia32/ExceptionTssEntryAsm.nasm
> +  Ia32/ArchExceptionHandler.c
> +  Ia32/ArchInterruptDefs.h
> +  Ia32/ArchAMDSevVcHandler.c
> +
> +[Sources.X64]
> +  X64/Xcode5ExceptionHandlerAsm.nasm
> +  X64/ArchExceptionHandler.c
> +  X64/ArchInterruptDefs.h
> +  X64/ArchAMDSevVcHandler.c
> +
> +[Sources.common]
> +  CpuExceptionCommon.h
> +  CpuExceptionCommon.c
> +  PeiDxeSmmCpuException.c
> +  DxeException.c
> +  AMDSevVcHandler.c
> +  AMDSevVcCommon.h
> +
> +[Pcd]
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuStackSwitchExceptionList
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  UefiCpuPkg/UefiCpuPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  SerialPortLib
> +  PrintLib
> +  SynchronizationLib
> +  LocalApicLib
> +  PeCoffGetEntryPointLib
> +  MemoryAllocationLib
> +  DebugLib
> +  VmgExitLib
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5PeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5PeiCpuExceptionHandlerLib.inf
> new file mode 100644
> index 000000000000..830ed1eb8bad
> --- /dev/null
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5PeiCpuExceptionHandlerLib.inf
> @@ -0,0 +1,63 @@
> +## @file
> +#  CPU Exception Handler library instance for PEI module.
> +#
> +#  Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = Xcode5PeiCpuExceptionHandlerLib
> +  MODULE_UNI_FILE                = PeiCpuExceptionHandlerLib.uni
> +  FILE_GUID                      = 980DDA67-44A6-4897-99E6-275290B71F9E
> +  MODULE_TYPE                    = PEIM
> +  VERSION_STRING                 = 1.1
> +  LIBRARY_CLASS                  = CpuExceptionHandlerLib|PEI_CORE PEIM
> +
> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +#  VALID_ARCHITECTURES           = IA32 X64
> +#
> +
> +[Sources.Ia32]
> +  Ia32/ExceptionHandlerAsm.nasm
> +  Ia32/ExceptionTssEntryAsm.nasm
> +  Ia32/ArchExceptionHandler.c
> +  Ia32/ArchInterruptDefs.h
> +  Ia32/ArchAMDSevVcHandler.c
> +
> +[Sources.X64]
> +  X64/Xcode5ExceptionHandlerAsm.nasm
> +  X64/ArchExceptionHandler.c
> +  X64/ArchInterruptDefs.h
> +  X64/ArchAMDSevVcHandler.c
> +
> +[Sources.common]
> +  CpuExceptionCommon.h
> +  CpuExceptionCommon.c
> +  PeiCpuException.c
> +  PeiDxeSmmCpuException.c
> +  AMDSevVcHandler.c
> +  AMDSevVcCommon.h
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  UefiCpuPkg/UefiCpuPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  SerialPortLib
> +  PrintLib
> +  LocalApicLib
> +  PeCoffGetEntryPointLib
> +  HobLib
> +  MemoryAllocationLib
> +  SynchronizationLib
> +  VmgExitLib
> +
> +[Pcd]
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard    # CONSUMES
> +
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
> new file mode 100644
> index 000000000000..36420be22faa
> --- /dev/null
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
> @@ -0,0 +1,55 @@
> +## @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
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = Xcode5SecPeiCpuExceptionHandlerLib
> +  MODULE_UNI_FILE                = SecPeiCpuExceptionHandlerLib.uni
> +  FILE_GUID                      = CA4BBC99-DFC6-4234-B553-8B6586B7B113
> +  MODULE_TYPE                    = PEIM
> +  VERSION_STRING                 = 1.1
> +  LIBRARY_CLASS                  = CpuExceptionHandlerLib|SEC PEI_CORE PEIM
> +
> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +#  VALID_ARCHITECTURES           = IA32 X64
> +#
> +
> +[Sources.Ia32]
> +  Ia32/ExceptionHandlerAsm.nasm
> +  Ia32/ExceptionTssEntryAsm.nasm
> +  Ia32/ArchExceptionHandler.c
> +  Ia32/ArchInterruptDefs.h
> +  Ia32/ArchAMDSevVcHandler.c
> +
> +[Sources.X64]
> +  X64/Xcode5ExceptionHandlerAsm.nasm
> +  X64/ArchExceptionHandler.c
> +  X64/ArchInterruptDefs.h
> +  X64/ArchAMDSevVcHandler.c
> +
> +[Sources.common]
> +  CpuExceptionCommon.h
> +  CpuExceptionCommon.c
> +  SecPeiCpuException.c
> +  AMDSevVcHandler.c
> +  AMDSevVcCommon.h
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  UefiCpuPkg/UefiCpuPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  SerialPortLib
> +  PrintLib
> +  LocalApicLib
> +  PeCoffGetEntryPointLib
> +  VmgExitLib
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SmmCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SmmCpuExceptionHandlerLib.inf
> new file mode 100644
> index 000000000000..8f71a45c86d5
> --- /dev/null
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SmmCpuExceptionHandlerLib.inf
> @@ -0,0 +1,59 @@
> +## @file
> +#  CPU Exception Handler library instance for SMM modules.
> +#
> +#  Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved.<BR>
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = Xcode5SmmCpuExceptionHandlerLib
> +  MODULE_UNI_FILE                = SmmCpuExceptionHandlerLib.uni
> +  FILE_GUID                      = 8D2C439B-3981-42ff-9CE5-1B50ECA502D6
> +  MODULE_TYPE                    = DXE_SMM_DRIVER
> +  VERSION_STRING                 = 1.1
> +  LIBRARY_CLASS                  = CpuExceptionHandlerLib|DXE_SMM_DRIVER
> +
> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +#  VALID_ARCHITECTURES           = IA32 X64
> +#
> +
> +[Sources.Ia32]
> +  Ia32/ExceptionHandlerAsm.nasm
> +  Ia32/ExceptionTssEntryAsm.nasm
> +  Ia32/ArchExceptionHandler.c
> +  Ia32/ArchInterruptDefs.h
> +  Ia32/ArchAMDSevVcHandler.c
> +
> +[Sources.X64]
> +  X64/Xcode5ExceptionHandlerAsm.nasm
> +  X64/ArchExceptionHandler.c
> +  X64/ArchInterruptDefs.h
> +  X64/ArchAMDSevVcHandler.c
> +
> +[Sources.common]
> +  CpuExceptionCommon.h
> +  CpuExceptionCommon.c
> +  PeiDxeSmmCpuException.c
> +  SmmException.c
> +  AMDSevVcHandler.c
> +  AMDSevVcCommon.h
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  UefiCpuPkg/UefiCpuPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  SerialPortLib
> +  PrintLib
> +  SynchronizationLib
> +  LocalApicLib
> +  PeCoffGetEntryPointLib
> +  DebugLib
> +  VmgExitLib
> +
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
> new file mode 100644
> index 000000000000..26cae56cc5cf
> --- /dev/null
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
> @@ -0,0 +1,413 @@
> +;------------------------------------------------------------------------------ ;
> +; Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.<BR>
> +; SPDX-License-Identifier: BSD-2-Clause-Patent
> +;
> +; Module Name:
> +;
> +;   ExceptionHandlerAsm.Asm
> +;
> +; Abstract:
> +;
> +;   x64 CPU Exception Handler
> +;
> +; Notes:
> +;
> +;------------------------------------------------------------------------------
> +
> +;
> +; CommonExceptionHandler()
> +;
> +
> +%define VC_EXCEPTION 29
> +
> +extern ASM_PFX(mErrorCodeFlag)    ; Error code flags for exceptions
> +extern ASM_PFX(mDoFarReturnFlag)  ; Do far return flag
> +extern ASM_PFX(CommonExceptionHandler)
> +
> +SECTION .data
> +
> +DEFAULT REL
> +SECTION .text
> +
> +ALIGN   8
> +
> +AsmIdtVectorBegin:
> +%rep  32
> +    db      0x6a        ; push  #VectorNum
> +    db      ($ - AsmIdtVectorBegin) / ((AsmIdtVectorEnd - AsmIdtVectorBegin) / 32) ; VectorNum
> +    push    rax
> +    mov     rax, strict qword 0 ;    mov     rax, ASM_PFX(CommonInterruptEntry)
> +    jmp     rax
> +%endrep
> +AsmIdtVectorEnd:
> +
> +HookAfterStubHeaderBegin:
> +    db      0x6a        ; push
> +@VectorNum:
> +    db      0          ; 0 will be fixed
> +    push    rax
> +    mov     rax, strict qword 0 ;     mov     rax, HookAfterStubHeaderEnd
> +JmpAbsoluteAddress:
> +    jmp     rax
> +HookAfterStubHeaderEnd:
> +    mov     rax, rsp
> +    and     sp,  0xfff0        ; make sure 16-byte aligned for exception context
> +    sub     rsp, 0x18           ; reserve room for filling exception data later
> +    push    rcx
> +    mov     rcx, [rax + 8]
> +    bt      [ASM_PFX(mErrorCodeFlag)], ecx
> +    jnc     .0
> +    push    qword [rsp]             ; push additional rcx to make stack alignment
> +.0:
> +    xchg    rcx, [rsp]        ; restore rcx, save Exception Number in stack
> +    push    qword [rax]             ; push rax into stack to keep code consistence
> +
> +;---------------------------------------;
> +; CommonInterruptEntry                  ;
> +;---------------------------------------;
> +; The follow algorithm is used for the common interrupt routine.
> +; Entry from each interrupt with a push eax and eax=interrupt number
> +; Stack frame would be as follows as specified in IA32 manuals:
> +;
> +; +---------------------+ <-- 16-byte aligned ensured by processor
> +; +    Old SS           +
> +; +---------------------+
> +; +    Old RSP          +
> +; +---------------------+
> +; +    RFlags           +
> +; +---------------------+
> +; +    CS               +
> +; +---------------------+
> +; +    RIP              +
> +; +---------------------+
> +; +    Error Code       +
> +; +---------------------+
> +; +   Vector Number     +
> +; +---------------------+
> +; +    RBP              +
> +; +---------------------+ <-- RBP, 16-byte aligned
> +; The follow algorithm is used for the common interrupt routine.
> +global ASM_PFX(CommonInterruptEntry)
> +ASM_PFX(CommonInterruptEntry):
> +    cli
> +    pop     rax
> +    ;
> +    ; All interrupt handlers are invoked through interrupt gates, so
> +    ; IF flag automatically cleared at the entry point
> +    ;
> +    xchg    rcx, [rsp]      ; Save rcx into stack and save vector number into rcx
> +    and     rcx, 0xFF
> +    cmp     ecx, 32         ; Intel reserved vector for exceptions?
> +    jae     NoErrorCode
> +    bt      [ASM_PFX(mErrorCodeFlag)], ecx
> +    jc      HasErrorCode
> +
> +NoErrorCode:
> +
> +    ;
> +    ; Push a dummy error code on the stack
> +    ; to maintain coherent stack map
> +    ;
> +    push    qword [rsp]
> +    mov     qword [rsp + 8], 0
> +HasErrorCode:
> +    push    rbp
> +    mov     rbp, rsp
> +    push    0             ; clear EXCEPTION_HANDLER_CONTEXT.OldIdtHandler
> +    push    0             ; clear EXCEPTION_HANDLER_CONTEXT.ExceptionDataFlag
> +
> +    ;
> +    ; Stack:
> +    ; +---------------------+ <-- 16-byte aligned ensured by processor
> +    ; +    Old SS           +
> +    ; +---------------------+
> +    ; +    Old RSP          +
> +    ; +---------------------+
> +    ; +    RFlags           +
> +    ; +---------------------+
> +    ; +    CS               +
> +    ; +---------------------+
> +    ; +    RIP              +
> +    ; +---------------------+
> +    ; +    Error Code       +
> +    ; +---------------------+
> +    ; + RCX / Vector Number +
> +    ; +---------------------+
> +    ; +    RBP              +
> +    ; +---------------------+ <-- RBP, 16-byte aligned
> +    ;
> +
> +    ;
> +    ; Since here the stack pointer is 16-byte aligned, so
> +    ; EFI_FX_SAVE_STATE_X64 of EFI_SYSTEM_CONTEXT_x64
> +    ; is 16-byte aligned
> +    ;
> +
> +;; UINT64  Rdi, Rsi, Rbp, Rsp, Rbx, Rdx, Rcx, Rax;
> +;; UINT64  R8, R9, R10, R11, R12, R13, R14, R15;
> +    push r15
> +    push r14
> +    push r13
> +    push r12
> +    push r11
> +    push r10
> +    push r9
> +    push r8
> +    push rax
> +    push qword [rbp + 8]   ; RCX
> +    push rdx
> +    push rbx
> +    push qword [rbp + 48]  ; RSP
> +    push qword [rbp]       ; RBP
> +    push rsi
> +    push rdi
> +
> +;; UINT64  Gs, Fs, Es, Ds, Cs, Ss;  insure high 16 bits of each is zero
> +    movzx   rax, word [rbp + 56]
> +    push    rax                      ; for ss
> +    movzx   rax, word [rbp + 32]
> +    push    rax                      ; for cs
> +    mov     rax, ds
> +    push    rax
> +    mov     rax, es
> +    push    rax
> +    mov     rax, fs
> +    push    rax
> +    mov     rax, gs
> +    push    rax
> +
> +    mov     [rbp + 8], rcx               ; save vector number
> +
> +;; UINT64  Rip;
> +    push    qword [rbp + 24]
> +
> +;; UINT64  Gdtr[2], Idtr[2];
> +    xor     rax, rax
> +    push    rax
> +    push    rax
> +    sidt    [rsp]
> +    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]
> +    mov     bx, word [rsp]
> +    mov     rax, qword [rsp + 2]
> +    mov     qword [rsp], rax
> +    mov     word [rsp + 8], bx
> +
> +;; UINT64  Ldtr, Tr;
> +    xor     rax, rax
> +    str     ax
> +    push    rax
> +    sldt    ax
> +    push    rax
> +
> +;; UINT64  RFlags;
> +    push    qword [rbp + 40]
> +
> +;; UINT64  Cr0, Cr1, Cr2, Cr3, Cr4, Cr8;
> +    mov     rax, cr8
> +    push    rax
> +    mov     rax, cr4
> +    or      rax, 0x208
> +    mov     cr4, rax
> +    push    rax
> +    mov     rax, cr3
> +    push    rax
> +    mov     rax, cr2
> +    push    rax
> +    xor     rax, rax
> +    push    rax
> +    mov     rax, cr0
> +    push    rax
> +
> +;; UINT64  Dr0, Dr1, Dr2, Dr3, Dr6, Dr7;
> +    cmp     qword [rbp + 8], VC_EXCEPTION
> +    je      VcDebugRegs          ; For SEV-ES (#VC) Debug registers ignored
> +
> +    mov     rax, dr7
> +    push    rax
> +    mov     rax, dr6
> +    push    rax
> +    mov     rax, dr3
> +    push    rax
> +    mov     rax, dr2
> +    push    rax
> +    mov     rax, dr1
> +    push    rax
> +    mov     rax, dr0
> +    push    rax
> +    jmp     DrFinish
> +
> +VcDebugRegs:
> +;; UINT64  Dr0, Dr1, Dr2, Dr3, Dr6, Dr7 are skipped for #VC to avoid exception recursion
> +    xor     rax, rax
> +    push    rax
> +    push    rax
> +    push    rax
> +    push    rax
> +    push    rax
> +    push    rax
> +
> +DrFinish:
> +;; FX_SAVE_STATE_X64 FxSaveState;
> +    sub rsp, 512
> +    mov rdi, rsp
> +    db 0xf, 0xae, 0x7 ;fxsave [rdi]
> +
> +;; UEFI calling convention for x64 requires that Direction flag in EFLAGs is clear
> +    cld
> +
> +;; UINT32  ExceptionData;
> +    push    qword [rbp + 16]
> +
> +;; Prepare parameter and call
> +    mov     rcx, [rbp + 8]
> +    mov     rdx, rsp
> +    ;
> +    ; Per X64 calling convention, allocate maximum parameter stack space
> +    ; and make sure RSP is 16-byte aligned
> +    ;
> +    sub     rsp, 4 * 8 + 8
> +    call    ASM_PFX(CommonExceptionHandler)
> +    add     rsp, 4 * 8 + 8
> +
> +    cli
> +;; UINT64  ExceptionData;
> +    add     rsp, 8
> +
> +;; FX_SAVE_STATE_X64 FxSaveState;
> +
> +    mov rsi, rsp
> +    db 0xf, 0xae, 0xE ; fxrstor [rsi]
> +    add rsp, 512
> +
> +;; UINT64  Dr0, Dr1, Dr2, Dr3, Dr6, Dr7;
> +;; Skip restoration of DRx registers to support in-circuit emualators
> +;; or debuggers set breakpoint in interrupt/exception context
> +    add     rsp, 8 * 6
> +
> +;; UINT64  Cr0, Cr1, Cr2, Cr3, Cr4, Cr8;
> +    pop     rax
> +    mov     cr0, rax
> +    add     rsp, 8   ; not for Cr1
> +    pop     rax
> +    mov     cr2, rax
> +    pop     rax
> +    mov     cr3, rax
> +    pop     rax
> +    mov     cr4, rax
> +    pop     rax
> +    mov     cr8, rax
> +
> +;; UINT64  RFlags;
> +    pop     qword [rbp + 40]
> +
> +;; UINT64  Ldtr, Tr;
> +;; UINT64  Gdtr[2], Idtr[2];
> +;; Best not let anyone mess with these particular registers...
> +    add     rsp, 48
> +
> +;; UINT64  Rip;
> +    pop     qword [rbp + 24]
> +
> +;; UINT64  Gs, Fs, Es, Ds, Cs, Ss;
> +    pop     rax
> +    ; mov     gs, rax ; not for gs
> +    pop     rax
> +    ; mov     fs, rax ; not for fs
> +    ; (X64 will not use fs and gs, so we do not restore it)
> +    pop     rax
> +    mov     es, rax
> +    pop     rax
> +    mov     ds, rax
> +    pop     qword [rbp + 32]  ; for cs
> +    pop     qword [rbp + 56]  ; for ss
> +
> +;; UINT64  Rdi, Rsi, Rbp, Rsp, Rbx, Rdx, Rcx, Rax;
> +;; UINT64  R8, R9, R10, R11, R12, R13, R14, R15;
> +    pop     rdi
> +    pop     rsi
> +    add     rsp, 8               ; not for rbp
> +    pop     qword [rbp + 48] ; for rsp
> +    pop     rbx
> +    pop     rdx
> +    pop     rcx
> +    pop     rax
> +    pop     r8
> +    pop     r9
> +    pop     r10
> +    pop     r11
> +    pop     r12
> +    pop     r13
> +    pop     r14
> +    pop     r15
> +
> +    mov     rsp, rbp
> +    pop     rbp
> +    add     rsp, 16
> +    cmp     qword [rsp - 32], 0  ; check EXCEPTION_HANDLER_CONTEXT.OldIdtHandler
> +    jz      DoReturn
> +    cmp     qword [rsp - 40], 1  ; check EXCEPTION_HANDLER_CONTEXT.ExceptionDataFlag
> +    jz      ErrorCode
> +    jmp     qword [rsp - 32]
> +ErrorCode:
> +    sub     rsp, 8
> +    jmp     qword [rsp - 24]
> +
> +DoReturn:
> +    cmp     qword [ASM_PFX(mDoFarReturnFlag)], 0   ; Check if need to do far return instead of IRET
> +    jz      DoIret
> +    push    rax
> +    mov     rax, rsp          ; save old RSP to rax
> +    mov     rsp, [rsp + 0x20]
> +    push    qword [rax + 0x10]       ; save CS in new location
> +    push    qword [rax + 0x8]        ; save EIP in new location
> +    push    qword [rax + 0x18]       ; save EFLAGS in new location
> +    mov     rax, [rax]        ; restore rax
> +    popfq                     ; restore EFLAGS
> +    DB      0x48               ; prefix to composite "retq" with next "retf"
> +    retf                      ; far return
> +DoIret:
> +    iretq
> +
> +;-------------------------------------------------------------------------------------
> +;  GetTemplateAddressMap (&AddressMap);
> +;-------------------------------------------------------------------------------------
> +; comments here for definition of address map
> +global ASM_PFX(AsmGetTemplateAddressMap)
> +ASM_PFX(AsmGetTemplateAddressMap):
> +    lea     rax, [AsmIdtVectorBegin]
> +    mov     qword [rcx], rax
> +    mov     qword [rcx + 0x8],  (AsmIdtVectorEnd - AsmIdtVectorBegin) / 32
> +    lea     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
> +
> +;-------------------------------------------------------------------------------------
> +;  AsmVectorNumFixup (*NewVectorAddr, VectorNum, *OldVectorAddr);
> +;-------------------------------------------------------------------------------------
> +global ASM_PFX(AsmVectorNumFixup)
> +ASM_PFX(AsmVectorNumFixup):
> +    mov     rax, rdx
> +    mov     [rcx + (@VectorNum - HookAfterStubHeaderBegin)], al
> +    ret
> +
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific
  2020-05-01 20:17 ` [PATCH 1/4] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific Lendacky, Thomas
@ 2020-05-05 21:39   ` Laszlo Ersek
  2020-05-05 22:09     ` Lendacky, Thomas
  0 siblings, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2020-05-05 21:39 UTC (permalink / raw)
  To: devel, thomas.lendacky
  Cc: Jordan Justen, Ard Biesheuvel, Liming Gao, Eric Dong, Ray Ni,
	Brijesh Singh, Anthony Perard, Benjamin You, Guo Dong,
	Julien Grall, Maurice Ma, Andrew Fish

Hi Tom,

On 05/01/20 22:17, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2340
>
> 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 new INF files
> for an XCODE5 version of CpuExceptionHandlerLib. Update the UefiCpuPkg.dsc
> file to use the new files when the XCODE5 toolchain is used.
>
> 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                     |  23 +
>  .../Xcode5DxeCpuExceptionHandlerLib.inf       |  64 +++
>  .../Xcode5PeiCpuExceptionHandlerLib.inf       |  63 +++
>  .../Xcode5SecPeiCpuExceptionHandlerLib.inf    |  55 +++
>  .../Xcode5SmmCpuExceptionHandlerLib.inf       |  59 +++

I don't think that paralleling all the existent INF files is necessary
for XCODE5.

The binary patching is a problem when the UEFI module containing the
self-patching CpuExceptionHandlerLib instance executes in-place from
flash. That applies to: (a) SEC modules, (b) PEI modules that do *not*
have a DEPEX on "gEfiPeiMemoryDiscoveredPpiGuid". (PEIMs that do have a
DEPEX on that PPI GUID are only dispatched after the permanent PEI RAM
has been discovered / published, so they run out of normal RAM.)

Reviewing the existent INF files, we have:

- DxeCpuExceptionHandlerLib.inf: for DXE_CORE, DXE_DRIVER,
UEFI_APPLICATION modules. Self-patching is fine.

- SmmCpuExceptionHandlerLib.inf: for DXE_SMM_DRIVER modules.
Self-patching is fine.

- SecPeiCpuExceptionHandlerLib.inf: SEC is listed explicitly, so here we
certainly need an alternative.

- PeiCpuExceptionHandlerLib.inf: unfortunately, the differences of this
library instance with "SecPeiCpuExceptionHandlerLib.inf" is not obvious;
only SEC's absence is easily visible.

If we look at the commit that introduced this lib instance
(a81abf161666, "UefiCpuPkg/ExceptionLib: Import
PeiCpuExceptionHandlerLib module", 2016-06-01), we find:

>     This module could be linked by CpuMpPei driver to handle reserved vector list
>     and provide spin lock for BSP/APs to prevent dump message corrupted.

So the library was added explicitly for CpuMpPei's sake -- which looks
promising, because CpuMpPei certainly depends on
"gEfiPeiMemoryDiscoveredPpiGuid", as it needs a bunch of RAM for
offering the multi-processing PPI. That suggests the self-patching is OK
in "PeiCpuExceptionHandlerLib.inf" too.

The CpuMpPei DEPEX in question was replaced with a PPI notify callback
in commit 0a0d5296e448 ("UefiCpuPkg/CpuMpPei: support stack guard
feature", 2018-09-10). This would be a problem if the self-patching in
the PeiCpuExceptionHandlerLib instance occurred in the library
constructor, because the CpuMpPei can now actually be dispatched before
permanent PEI RAM is available -- and the constructor would run
immediately.

Luckily, the lib instance has no CONSTRUCTOR at all, and CpuMpPei calls
InitializeCpuExceptionHandlers() explicitly in InitializeCpuMpWorker(),
which is the PPI notify in question. (And per
<https://bugzilla.tianocore.org/show_bug.cgi?id=2340#c0>, the
self-patching occurs in InitializeCpuExceptionHandlers().)

Therefore, having two variants of PeiCpuExceptionHandlerLib.inf is also
unnecessary.

(1) We only need two variants for "SecPeiCpuExceptionHandlerLib.inf", in
my opinion.

(Note: if we check OVMF, we see that PeiCpuExceptionHandlerLib.inf is
used universally for PEIMs. That's because OVMF is special -- its PEI
phase runs entirely out of RAM. See also commit f0e6a56a9a2f, "OvmfPkg:
include UefiCpuPkg/CpuMpPei", 2016-07-15.)

--*--

With this patch applied:

$ diff -u \
          SecPeiCpuExceptionHandlerLib.inf \
    Xcode5SecPeiCpuExceptionHandlerLib.inf

> --- SecPeiCpuExceptionHandlerLib.inf    2020-05-05 18:36:12.813156743 +0200
> +++ Xcode5SecPeiCpuExceptionHandlerLib.inf      2020-05-05 23:25:24.578572971 +0200
> @@ -8,7 +8,7 @@
>
>  [Defines]
>    INF_VERSION                    = 0x00010005
> -  BASE_NAME                      = SecPeiCpuExceptionHandlerLib
> +  BASE_NAME                      = Xcode5SecPeiCpuExceptionHandlerLib

OK

>    MODULE_UNI_FILE                = SecPeiCpuExceptionHandlerLib.uni

(2) We'll need a separate UNI file here -- also we should customize the
file-top comment in the INF file -- that explains the difference between
the XCODE5 and non-XCODE5 variants, briefly.

>    FILE_GUID                      = CA4BBC99-DFC6-4234-B553-8B6586B7B113

(3) Please generate a new FILE_GUID with "uuidgen".

>    MODULE_TYPE                    = PEIM
> @@ -26,16 +26,20 @@
>    Ia32/ExceptionTssEntryAsm.nasm
>    Ia32/ArchExceptionHandler.c
>    Ia32/ArchInterruptDefs.h
> +  Ia32/ArchAMDSevVcHandler.c

(4) Even though the blurb says that this series is based on edk2 commit
e54310451f1a, some SEV-ES specific parts remain in this patch, and
should be eliminated. The first example is above.

>
>  [Sources.X64]
> -  X64/ExceptionHandlerAsm.nasm
> +  X64/Xcode5ExceptionHandlerAsm.nasm
>    X64/ArchExceptionHandler.c
>    X64/ArchInterruptDefs.h
> +  X64/ArchAMDSevVcHandler.c

(5) Another SEV-ES change.

>
>  [Sources.common]
>    CpuExceptionCommon.h
>    CpuExceptionCommon.c
>    SecPeiCpuException.c
> +  AMDSevVcHandler.c
> +  AMDSevVcCommon.h

(6) ditto

>
>  [Packages]
>    MdePkg/MdePkg.dec
> @@ -48,3 +52,4 @@
>    PrintLib
>    LocalApicLib
>    PeCoffGetEntryPointLib
> +  VmgExitLib

(7) ditto

Furthermore:

$ diff -u \
          ExceptionHandlerAsm.nasm \
    Xcode5ExceptionHandlerAsm.nasm

> --- ExceptionHandlerAsm.nasm    2020-05-05 23:26:30.941784203 +0200
> +++ Xcode5ExceptionHandlerAsm.nasm      2020-05-05 23:25:24.578572971 +0200
> @@ -18,6 +18,8 @@
>  ; CommonExceptionHandler()
>  ;
>
> +%define VC_EXCEPTION 29
> +
>  extern ASM_PFX(mErrorCodeFlag)    ; Error code flags for exceptions
>  extern ASM_PFX(mDoFarReturnFlag)  ; Do far return flag
>  extern ASM_PFX(CommonExceptionHandler)
> @@ -225,6 +227,9 @@
>      push    rax
>
>  ;; UINT64  Dr0, Dr1, Dr2, Dr3, Dr6, Dr7;
> +    cmp     qword [rbp + 8], VC_EXCEPTION
> +    je      VcDebugRegs          ; For SEV-ES (#VC) Debug registers ignored
> +
>      mov     rax, dr7
>      push    rax
>      mov     rax, dr6
> @@ -237,7 +242,19 @@
>      push    rax
>      mov     rax, dr0
>      push    rax
> +    jmp     DrFinish
> +
> +VcDebugRegs:
> +;; UINT64  Dr0, Dr1, Dr2, Dr3, Dr6, Dr7 are skipped for #VC to avoid exception recursion
> +    xor     rax, rax
> +    push    rax
> +    push    rax
> +    push    rax
> +    push    rax
> +    push    rax
> +    push    rax
>
> +DrFinish:
>  ;; FX_SAVE_STATE_X64 FxSaveState;
>      sub rsp, 512
>      mov rdi, rsp

(8) All of these should be removed -- they should be part of your SEV-ES
series, on top of this set.

Thanks,
Laszlo


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [PATCH 3/4] OvmfPkg: Use toolchain appropriate CpuExceptionHandlerLib
  2020-05-01 20:17 ` [PATCH 3/4] OvmfPkg: " Lendacky, Thomas
@ 2020-05-05 21:49   ` Laszlo Ersek
  0 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2020-05-05 21:49 UTC (permalink / raw)
  To: devel, thomas.lendacky
  Cc: Jordan Justen, Ard Biesheuvel, Liming Gao, Eric Dong, Ray Ni,
	Brijesh Singh, Anthony Perard, Benjamin You, Guo Dong,
	Julien Grall, Maurice Ma, Andrew Fish, Ard Biesheuvel

On 05/01/20 22:17, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2340
> 
> Use the XCODE5 CpuExceptionHandlerLib library in place of the standard
> library when building with the XCODE5 toolchain. The XCODE5 version of
> the library performs binary patching and should only be used when building
> with the XCODE5 toolchain.
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Cc: Anthony Perard <anthony.perard@citrix.com>
> Cc: Julien Grall <julien@xen.org>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc    | 20 ++++++++++++++++++++
>  OvmfPkg/OvmfPkgIa32X64.dsc | 20 ++++++++++++++++++++
>  OvmfPkg/OvmfPkgX64.dsc     | 20 ++++++++++++++++++++
>  OvmfPkg/OvmfXen.dsc        | 16 ++++++++++++++++
>  4 files changed, 76 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index fcd9779b5ba2..f27fcd7e1087 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -245,7 +245,11 @@ [LibraryClasses.common.SEC]
>    PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
>    PeiServicesTablePointerLib|MdePkg/Library/PeiServicesTablePointerLibIdt/PeiServicesTablePointerLibIdt.inf
>    MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
> +!if $(TOOL_CHAIN_TAG) != "XCODE5"
>    CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
> +!else
> +  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
> +!endif

(1) In accordance with my point (1) under patch#1, please keep the SEC
phase hunks only, for all four DSC files, and drop the rest of the hunks.

Only SecPeiCpuExceptionHandlerLib needs to be conditionalized.

(2) Style request: given that we have an "!else" branch too, please use
the "==" operator with the "!if".

I think that the double-negation arising from testing "!=" first, and
then having an "!else", is hard to read.

Thanks!
Laszlo


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [PATCH 1/4] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific
  2020-05-05 21:39   ` [edk2-devel] " Laszlo Ersek
@ 2020-05-05 22:09     ` Lendacky, Thomas
  0 siblings, 0 replies; 17+ messages in thread
From: Lendacky, Thomas @ 2020-05-05 22:09 UTC (permalink / raw)
  To: Laszlo Ersek, devel
  Cc: Jordan Justen, Ard Biesheuvel, Liming Gao, Eric Dong, Ray Ni,
	Brijesh Singh, Anthony Perard, Benjamin You, Guo Dong,
	Julien Grall, Maurice Ma, Andrew Fish

On 5/5/20 4:39 PM, Laszlo Ersek wrote:
> Hi Tom,
> 
> 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;data=02%7C01%7Cthomas.lendacky%40amd.com%7C4d398c73a4bc4674d36608d7f13cce1d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243115777198329&amp;sdata=YA9qmWhZDrwGCchGXKDmOQPFqXdDztokQSZeZIq6u18%3D&amp;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 new INF files
>> for an XCODE5 version of CpuExceptionHandlerLib. Update the UefiCpuPkg.dsc
>> file to use the new files when the XCODE5 toolchain is used.
>>
>> 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                     |  23 +
>>   .../Xcode5DxeCpuExceptionHandlerLib.inf       |  64 +++
>>   .../Xcode5PeiCpuExceptionHandlerLib.inf       |  63 +++
>>   .../Xcode5SecPeiCpuExceptionHandlerLib.inf    |  55 +++
>>   .../Xcode5SmmCpuExceptionHandlerLib.inf       |  59 +++
> 
> I don't think that paralleling all the existent INF files is necessary
> for XCODE5.
> 
> The binary patching is a problem when the UEFI module containing the
> self-patching CpuExceptionHandlerLib instance executes in-place from
> flash. That applies to: (a) SEC modules, (b) PEI modules that do *not*
> have a DEPEX on "gEfiPeiMemoryDiscoveredPpiGuid". (PEIMs that do have a
> DEPEX on that PPI GUID are only dispatched after the permanent PEI RAM
> has been discovered / published, so they run out of normal RAM.)
> 
> Reviewing the existent INF files, we have:
> 
> - DxeCpuExceptionHandlerLib.inf: for DXE_CORE, DXE_DRIVER,
> UEFI_APPLICATION modules. Self-patching is fine.
> 
> - SmmCpuExceptionHandlerLib.inf: for DXE_SMM_DRIVER modules.
> Self-patching is fine.
> 
> - SecPeiCpuExceptionHandlerLib.inf: SEC is listed explicitly, so here we
> certainly need an alternative.
> 
> - PeiCpuExceptionHandlerLib.inf: unfortunately, the differences of this
> library instance with "SecPeiCpuExceptionHandlerLib.inf" is not obvious;
> only SEC's absence is easily visible.
> 
> If we look at the commit that introduced this lib instance
> (a81abf161666, "UefiCpuPkg/ExceptionLib: Import
> PeiCpuExceptionHandlerLib module", 2016-06-01), we find:
> 
>>      This module could be linked by CpuMpPei driver to handle reserved vector list
>>      and provide spin lock for BSP/APs to prevent dump message corrupted.
> 
> So the library was added explicitly for CpuMpPei's sake -- which looks
> promising, because CpuMpPei certainly depends on
> "gEfiPeiMemoryDiscoveredPpiGuid", as it needs a bunch of RAM for
> offering the multi-processing PPI. That suggests the self-patching is OK
> in "PeiCpuExceptionHandlerLib.inf" too.
> 
> The CpuMpPei DEPEX in question was replaced with a PPI notify callback
> in commit 0a0d5296e448 ("UefiCpuPkg/CpuMpPei: support stack guard
> feature", 2018-09-10). This would be a problem if the self-patching in
> the PeiCpuExceptionHandlerLib instance occurred in the library
> constructor, because the CpuMpPei can now actually be dispatched before
> permanent PEI RAM is available -- and the constructor would run
> immediately.
> 
> Luckily, the lib instance has no CONSTRUCTOR at all, and CpuMpPei calls
> InitializeCpuExceptionHandlers() explicitly in InitializeCpuMpWorker(),
> which is the PPI notify in question. (And per
> <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2340%23c0&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7C4d398c73a4bc4674d36608d7f13cce1d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243115777198329&amp;sdata=1ucjYymvr7VAF2d2QGyCxQhE8hGe8AFMaW8EYbUUkdM%3D&amp;reserved=0>, the
> self-patching occurs in InitializeCpuExceptionHandlers().)
> 
> Therefore, having two variants of PeiCpuExceptionHandlerLib.inf is also
> unnecessary.
> 
> (1) We only need two variants for "SecPeiCpuExceptionHandlerLib.inf", in
> my opinion.

Ok, I'll rework it to have variants for just SecPeiCpuExceptionHandlerLib.

> 
> (Note: if we check OVMF, we see that PeiCpuExceptionHandlerLib.inf is
> used universally for PEIMs. That's because OVMF is special -- its PEI
> phase runs entirely out of RAM. See also commit f0e6a56a9a2f, "OvmfPkg:
> include UefiCpuPkg/CpuMpPei", 2016-07-15.)
> 
> --*--
> 
> With this patch applied:
> 
> $ diff -u \
>            SecPeiCpuExceptionHandlerLib.inf \
>      Xcode5SecPeiCpuExceptionHandlerLib.inf
> 
>> --- SecPeiCpuExceptionHandlerLib.inf    2020-05-05 18:36:12.813156743 +0200
>> +++ Xcode5SecPeiCpuExceptionHandlerLib.inf      2020-05-05 23:25:24.578572971 +0200
>> @@ -8,7 +8,7 @@
>>
>>   [Defines]
>>     INF_VERSION                    = 0x00010005
>> -  BASE_NAME                      = SecPeiCpuExceptionHandlerLib
>> +  BASE_NAME                      = Xcode5SecPeiCpuExceptionHandlerLib
> 
> OK
> 
>>     MODULE_UNI_FILE                = SecPeiCpuExceptionHandlerLib.uni
> 
> (2) We'll need a separate UNI file here -- also we should customize the
> file-top comment in the INF file -- that explains the difference between
> the XCODE5 and non-XCODE5 variants, briefly.

Ok, will do.

> 
>>     FILE_GUID                      = CA4BBC99-DFC6-4234-B553-8B6586B7B113
> 
> (3) Please generate a new FILE_GUID with "uuidgen".

Ok, thanks, that answers my question that I asked in another email.

> 
>>     MODULE_TYPE                    = PEIM
>> @@ -26,16 +26,20 @@
>>     Ia32/ExceptionTssEntryAsm.nasm
>>     Ia32/ArchExceptionHandler.c
>>     Ia32/ArchInterruptDefs.h
>> +  Ia32/ArchAMDSevVcHandler.c
> 
> (4) Even though the blurb says that this series is based on edk2 commit
> e54310451f1a, some SEV-ES specific parts remain in this patch, and
> should be eliminated. The first example is above.

Ugh. I thought I had that all cleaned up before sending. My bad, I'll fix 
that in the next version.

Thanks,
Tom

> 
>>
>>   [Sources.X64]
>> -  X64/ExceptionHandlerAsm.nasm
>> +  X64/Xcode5ExceptionHandlerAsm.nasm
>>     X64/ArchExceptionHandler.c
>>     X64/ArchInterruptDefs.h
>> +  X64/ArchAMDSevVcHandler.c
> 
> (5) Another SEV-ES change.
> 
>>
>>   [Sources.common]
>>     CpuExceptionCommon.h
>>     CpuExceptionCommon.c
>>     SecPeiCpuException.c
>> +  AMDSevVcHandler.c
>> +  AMDSevVcCommon.h
> 
> (6) ditto
> 
>>
>>   [Packages]
>>     MdePkg/MdePkg.dec
>> @@ -48,3 +52,4 @@
>>     PrintLib
>>     LocalApicLib
>>     PeCoffGetEntryPointLib
>> +  VmgExitLib
> 
> (7) ditto
> 
> Furthermore:
> 
> $ diff -u \
>            ExceptionHandlerAsm.nasm \
>      Xcode5ExceptionHandlerAsm.nasm
> 
>> --- ExceptionHandlerAsm.nasm    2020-05-05 23:26:30.941784203 +0200
>> +++ Xcode5ExceptionHandlerAsm.nasm      2020-05-05 23:25:24.578572971 +0200
>> @@ -18,6 +18,8 @@
>>   ; CommonExceptionHandler()
>>   ;
>>
>> +%define VC_EXCEPTION 29
>> +
>>   extern ASM_PFX(mErrorCodeFlag)    ; Error code flags for exceptions
>>   extern ASM_PFX(mDoFarReturnFlag)  ; Do far return flag
>>   extern ASM_PFX(CommonExceptionHandler)
>> @@ -225,6 +227,9 @@
>>       push    rax
>>
>>   ;; UINT64  Dr0, Dr1, Dr2, Dr3, Dr6, Dr7;
>> +    cmp     qword [rbp + 8], VC_EXCEPTION
>> +    je      VcDebugRegs          ; For SEV-ES (#VC) Debug registers ignored
>> +
>>       mov     rax, dr7
>>       push    rax
>>       mov     rax, dr6
>> @@ -237,7 +242,19 @@
>>       push    rax
>>       mov     rax, dr0
>>       push    rax
>> +    jmp     DrFinish
>> +
>> +VcDebugRegs:
>> +;; UINT64  Dr0, Dr1, Dr2, Dr3, Dr6, Dr7 are skipped for #VC to avoid exception recursion
>> +    xor     rax, rax
>> +    push    rax
>> +    push    rax
>> +    push    rax
>> +    push    rax
>> +    push    rax
>> +    push    rax
>>
>> +DrFinish:
>>   ;; FX_SAVE_STATE_X64 FxSaveState;
>>       sub rsp, 512
>>       mov rdi, rsp
> 
> (8) All of these should be removed -- they should be part of your SEV-ES
> series, on top of this set.
> 
> Thanks,
> Laszlo
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib
  2020-05-01 20:17 ` [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib Lendacky, Thomas
  2020-05-01 20:49   ` [EXTERNAL] [edk2-devel] " Bret Barkelew
@ 2020-05-05 22:15   ` Laszlo Ersek
  2020-05-06 14:35     ` Lendacky, Thomas
  1 sibling, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2020-05-05 22:15 UTC (permalink / raw)
  To: devel, thomas.lendacky
  Cc: Jordan Justen, Ard Biesheuvel, Liming Gao, Eric Dong, Ray Ni,
	Brijesh Singh, Anthony Perard, Benjamin You, Guo Dong,
	Julien Grall, Maurice Ma, Andrew Fish

On 05/01/20 22:17, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2340
>
> 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
> -; http://opensource.org/licenses/bsd-license.php.
> -;
> -; 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.

Thanks,
Laszlo


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [PATCH 2/4] UefiPayloadPkg: Use toolchain appropriate CpuExceptionHandlerLib
  2020-05-01 20:17 ` [PATCH 2/4] UefiPayloadPkg: Use toolchain appropriate CpuExceptionHandlerLib Lendacky, Thomas
@ 2020-05-05 22:19   ` Laszlo Ersek
  0 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2020-05-05 22:19 UTC (permalink / raw)
  To: devel, thomas.lendacky
  Cc: Jordan Justen, Ard Biesheuvel, Liming Gao, Eric Dong, Ray Ni,
	Brijesh Singh, Anthony Perard, Benjamin You, Guo Dong,
	Julien Grall, Maurice Ma, Andrew Fish

On 05/01/20 22:17, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2340
> 
> Use the XCODE5 CpuExceptionHandlerLib library in place of the standard
> library when building with the XCODE5 toolchain. The XCODE5 version of
> the library performs binary patching and should only be used when building
> with the XCODE5 toolchain.
> 
> Cc: Maurice Ma <maurice.ma@intel.com>
> Cc: Guo Dong <guo.dong@intel.com>
> Cc: Benjamin You <benjamin.you@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  UefiPayloadPkg/UefiPayloadPkgIa32.dsc | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> index d52945442e0e..2bf7aafd8c77 100644
> --- a/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> +++ b/UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> @@ -232,7 +232,11 @@ [LibraryClasses.common.DXE_CORE]
>  !if $(SOURCE_DEBUG_ENABLE)
>    DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgentLib.inf
>  !endif
> +!if $(TOOL_CHAIN_TAG) != "XCODE5"
>    CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
> +!else
> +  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
> +!endif
>  
>  [LibraryClasses.common.DXE_DRIVER]
>    PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf
> @@ -243,7 +247,11 @@ [LibraryClasses.common.DXE_DRIVER]
>  !if $(SOURCE_DEBUG_ENABLE)
>    DebugAgentLib|SourceLevelDebugPkg/Library/DebugAgent/DxeDebugAgentLib.inf
>  !endif
> +!if $(TOOL_CHAIN_TAG) != "XCODE5"
>    CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
> +!else
> +  CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5DxeCpuExceptionHandlerLib.inf
> +!endif
>    MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
>  
>  [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> 

This patch should be dropped, as it only modifies the lib class
resolutions for DXE_CORE and DXE_DRIVER modules; in those modules, the
self-patching is harmless.

Thanks
Laszlo


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib
  2020-05-05 22:15   ` Laszlo Ersek
@ 2020-05-06 14:35     ` Lendacky, Thomas
  2020-05-06 14:53       ` Liming Gao
  2020-05-06 16:33       ` Laszlo Ersek
  0 siblings, 2 replies; 17+ messages in thread
From: Lendacky, Thomas @ 2020-05-06 14:35 UTC (permalink / raw)
  To: devel, lersek
  Cc: Jordan Justen, Ard Biesheuvel, Liming Gao, Eric Dong, Ray Ni,
	Brijesh Singh, Anthony Perard, Benjamin You, Guo Dong,
	Julien Grall, Maurice Ma, Andrew Fish

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;data=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%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243137431443098&amp;sdata=SZAc83Y%2BwZauGcj47EDgc10fnxSucy2ljeI9PcaJSvE%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://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)?

Thanks,
Tom

> 
> Thanks,
> Laszlo
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib
  2020-05-06 14:35     ` Lendacky, Thomas
@ 2020-05-06 14:53       ` Liming Gao
  2020-05-06 16:33       ` Laszlo Ersek
  1 sibling, 0 replies; 17+ messages in thread
From: Liming Gao @ 2020-05-06 14:53 UTC (permalink / raw)
  To: devel@edk2.groups.io, thomas.lendacky@amd.com, lersek@redhat.com
  Cc: Justen, Jordan L, Ard Biesheuvel, Dong, Eric, Ni, Ray,
	Brijesh Singh, Anthony Perard, You, Benjamin, Dong, Guo,
	Julien Grall, Ma, Maurice, Andrew Fish

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [edk2-devel] [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib
  2020-05-06 14:35     ` Lendacky, Thomas
  2020-05-06 14:53       ` Liming Gao
@ 2020-05-06 16:33       ` Laszlo Ersek
  2020-05-06 18:07         ` [EXTERNAL] " Bret Barkelew
  1 sibling, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2020-05-06 16:33 UTC (permalink / raw)
  To: Tom Lendacky, devel
  Cc: Jordan Justen, Ard Biesheuvel, Liming Gao, Eric Dong, Ray Ni,
	Brijesh Singh, Anthony Perard, Benjamin You, Guo Dong,
	Julien Grall, Maurice Ma, Andrew Fish

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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2340&amp;data=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%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243137431443098&amp;sdata=SZAc83Y%2BwZauGcj47EDgc10fnxSucy2ljeI9PcaJSvE%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://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?

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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [EXTERNAL] Re: [edk2-devel] [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib
  2020-05-06 16:33       ` Laszlo Ersek
@ 2020-05-06 18:07         ` Bret Barkelew
  2020-05-06 19:51           ` Lendacky, Thomas
  0 siblings, 1 reply; 17+ messages in thread
From: Bret Barkelew @ 2020-05-06 18:07 UTC (permalink / raw)
  To: devel@edk2.groups.io, lersek@redhat.com, Tom Lendacky
  Cc: Jordan Justen, Ard Biesheuvel, Liming Gao, Eric Dong, Ray Ni,
	Brijesh Singh, Anthony Perard, Benjamin You, Guo Dong,
	Julien Grall, Maurice Ma, Andrew Fish

[-- 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 --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [EXTERNAL] Re: [edk2-devel] [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib
  2020-05-06 18:07         ` [EXTERNAL] " Bret Barkelew
@ 2020-05-06 19:51           ` Lendacky, Thomas
  0 siblings, 0 replies; 17+ messages in thread
From: Lendacky, Thomas @ 2020-05-06 19:51 UTC (permalink / raw)
  To: devel, bret.barkelew, lersek@redhat.com
  Cc: Jordan Justen, Ard Biesheuvel, Liming Gao, Eric Dong, Ray Ni,
	Brijesh Singh, Anthony Perard, Benjamin You, Guo Dong,
	Julien Grall, Maurice Ma, Andrew Fish

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2020-05-06 19:51 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-01 20:17 [PATCH 0/4] XCODE5 toolchain binary patching fix Lendacky, Thomas
2020-05-01 20:17 ` [PATCH 1/4] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific Lendacky, Thomas
2020-05-05 21:39   ` [edk2-devel] " Laszlo Ersek
2020-05-05 22:09     ` Lendacky, Thomas
2020-05-01 20:17 ` [PATCH 2/4] UefiPayloadPkg: Use toolchain appropriate CpuExceptionHandlerLib Lendacky, Thomas
2020-05-05 22:19   ` [edk2-devel] " Laszlo Ersek
2020-05-01 20:17 ` [PATCH 3/4] OvmfPkg: " Lendacky, Thomas
2020-05-05 21:49   ` [edk2-devel] " Laszlo Ersek
2020-05-01 20:17 ` [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib Lendacky, Thomas
2020-05-01 20:49   ` [EXTERNAL] [edk2-devel] " Bret Barkelew
2020-05-05 22:15   ` Laszlo Ersek
2020-05-06 14:35     ` Lendacky, Thomas
2020-05-06 14:53       ` Liming Gao
2020-05-06 16:33       ` Laszlo Ersek
2020-05-06 18:07         ` [EXTERNAL] " Bret Barkelew
2020-05-06 19:51           ` Lendacky, Thomas
     [not found] ` <160B00E54624ADAA.10991@groups.io>
2020-05-05 18:50   ` [edk2-devel] [PATCH 1/4] UefiCpuPkg/CpuExceptionHandler: Make XCODE5 changes toolchain specific Lendacky, Thomas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox