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&data=02%7C01%7Cthomas.lendacky%40amd.com%7C4d398c73a4bc4674d36608d7f13cce1d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243115777198329&sdata=YA9qmWhZDrwGCchGXKDmOQPFqXdDztokQSZeZIq6u18%3D&reserved=0
Commit 2db0ccc2d7fe ("UefiCpuPkg: Update CpuExceptionHandlerLib pass
XCODE5 tool chain") introduced binary patching into the exception handling
support. CPU exception handling is allowed during SEC and this results in
binary patching of flash, which should not be done.
Separate the changes from commit 2db0ccc2d7fe into an XCODE5 toolchain
specific file, Xcode5ExceptionHandlerAsm.nasm, and create 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.d...@intel.com>
Cc: Ray Ni <ray...@intel.com>
Cc: Laszlo Ersek <ler...@redhat.com>
Cc: Liming Gao <liming....@intel.com>
Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
---
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&data=02%7C01%7Cthomas.lendacky%40amd.com%7C4d398c73a4bc4674d36608d7f13cce1d%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243115777198329&sdata=1ucjYymvr7VAF2d2QGyCxQhE8hGe8AFMaW8EYbUUkdM%3D&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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#58665): https://edk2.groups.io/g/devel/message/58665
Mute This Topic: https://groups.io/mt/73406888/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
