BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
Under SEV-ES, a DR7 read or write intercept generates a #VC exception.
The #VC handler must provide special support to the guest for this. On
a DR7 write, the #VC handler must cache the value and issue a VMGEXIT
to notify the hypervisor of the write. However, the #VC handler must
not actually set the value of the DR7 register. On a DR7 read, the #VC
handler must return the cached value of the DR7 register to the guest.
VMGEXIT is not invoked for a DR7 register read.
To avoid exception recursion, a #VC exception will not try to read and
push the actual debug registers into the EFI_SYSTEM_CONTEXT_X64 struct
and instead push zeroes. The #VC exception handler does not make use of
the debug registers from saved context.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
.../Library/VmgExitLib/X64/VmgExitVcHandler.c | 105 ++++++++++++++++++
.../X64/ExceptionHandlerAsm.nasm | 17 +++
.../X64/Xcode5ExceptionHandlerAsm.nasm | 17 +++
3 files changed, 139 insertions(+)
diff --git a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
index b028b20f255a..e4072d79d704 100644
--- a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
@@ -14,6 +14,16 @@
#define CR4_OSXSAVE (1 << 18)
+#define DR7_RESET_VALUE 0x400
+
+//
+// Per-CPU data mapping structure
+//
+typedef struct {
+ BOOLEAN Dr7Cached;
+ UINT64 Dr7;
+} SEV_ES_PER_CPU_DATA;
+
//
// Instruction execution mode definition
//
@@ -1494,6 +1504,93 @@ RdtscExit (
return 0;
}
+/**
+ Handle a DR7 register write event.
+
+ Use the VMGEXIT instruction to handle a DR7 write event.
+
+ @param[in, out] Ghcb Pointer to the Guest-Hypervisor Communication
+ Block
+ @param[in, out] Regs x64 processor context
+ @param[in] InstructionData Instruction parsing context
+
+ @retval 0 Event handled successfully
+ @retval Others New exception value to propagate
+
+**/
+STATIC
+UINT64
+Dr7WriteExit (
+ IN OUT GHCB *Ghcb,
+ IN OUT EFI_SYSTEM_CONTEXT_X64 *Regs,
+ IN SEV_ES_INSTRUCTION_DATA *InstructionData
+ )
+{
+ SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
+ SEV_ES_PER_CPU_DATA *SevEsData;
+ INTN *Register;
+ UINT64 Status;
+
+ Ext = &InstructionData->Ext;
+ SevEsData = (SEV_ES_PER_CPU_DATA *) (Ghcb + 1);
+
+ DecodeModRm (Regs, InstructionData);
+
+ /* MOV DRn always treats MOD == 3 no matter how encoded */
+ Register = GetRegisterPointer (Regs, Ext->ModRm.Rm);
+
+ /* Using a value of 0 for ExitInfo1 means RAX holds the value */
+ Ghcb->SaveArea.Rax = *Register;
+ GhcbSetRegValid (Ghcb, GhcbRax);
+
+ Status = VmgExit (Ghcb, SVM_EXIT_DR7_WRITE, 0, 0);
+ if (Status) {
+ return Status;
+ }
+
+ SevEsData->Dr7 = *Register;
+ SevEsData->Dr7Cached = TRUE;
+
+ return 0;
+}
+
+/**
+ Handle a DR7 register read event.
+
+ Use the VMGEXIT instruction to handle a DR7 read event.
+
+ @param[in, out] Ghcb Pointer to the Guest-Hypervisor Communication
+ Block
+ @param[in, out] Regs x64 processor context
+ @param[in] InstructionData Instruction parsing context
+
+ @retval 0 Event handled successfully
+
+**/
+STATIC
+UINT64
+Dr7ReadExit (
+ IN OUT GHCB *Ghcb,
+ IN OUT EFI_SYSTEM_CONTEXT_X64 *Regs,
+ IN SEV_ES_INSTRUCTION_DATA *InstructionData
+ )
+{
+ SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
+ SEV_ES_PER_CPU_DATA *SevEsData;
+ INTN *Register;
+
+ Ext = &InstructionData->Ext;
+ SevEsData = (SEV_ES_PER_CPU_DATA *) (Ghcb + 1);
+
+ DecodeModRm (Regs, InstructionData);
+
+ /* MOV DRn always treats MOD == 3 no matter how encoded */
+ Register = GetRegisterPointer (Regs, Ext->ModRm.Rm);
+ *Register = (SevEsData->Dr7Cached) ? SevEsData->Dr7 : DR7_RESET_VALUE;
+
+ return 0;
+}
+
/**
Handle a #VC exception.
@@ -1538,6 +1635,14 @@ VmgExitHandleVc (
ExitCode = Regs->ExceptionData;
switch (ExitCode) {
+ case SVM_EXIT_DR7_READ:
+ NaeExit = Dr7ReadExit;
+ break;
+
+ case SVM_EXIT_DR7_WRITE:
+ NaeExit = Dr7WriteExit;
+ break;
+
case SVM_EXIT_RDTSC:
NaeExit = RdtscExit;
break;
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
index 3814f9de3703..2a5545ecfd41 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
@@ -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)
@@ -224,6 +226,9 @@ HasErrorCode:
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
@@ -236,7 +241,19 @@ HasErrorCode:
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
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
index 19198f273137..26cae56cc5cf 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
@@ -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 @@ HasErrorCode:
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 @@ HasErrorCode:
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
--
2.17.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#59882): https://edk2.groups.io/g/devel/message/59882
Mute This Topic: https://groups.io/mt/74336582/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 05/19/20 23:50, Lendacky, Thomas wrote: > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198 > > Under SEV-ES, a DR7 read or write intercept generates a #VC exception. > The #VC handler must provide special support to the guest for this. On > a DR7 write, the #VC handler must cache the value and issue a VMGEXIT > to notify the hypervisor of the write. However, the #VC handler must > not actually set the value of the DR7 register. On a DR7 read, the #VC > handler must return the cached value of the DR7 register to the guest. > VMGEXIT is not invoked for a DR7 register read. > > To avoid exception recursion, a #VC exception will not try to read and > push the actual debug registers into the EFI_SYSTEM_CONTEXT_X64 struct > and instead push zeroes. The #VC exception handler does not make use of > the debug registers from saved context. > > Cc: Eric Dong <eric.dong@intel.com> > Cc: Ray Ni <ray.ni@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Ard Biesheuvel <ard.biesheuvel@arm.com> > Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> > --- > .../Library/VmgExitLib/X64/VmgExitVcHandler.c | 105 ++++++++++++++++++ > .../X64/ExceptionHandlerAsm.nasm | 17 +++ > .../X64/Xcode5ExceptionHandlerAsm.nasm | 17 +++ > 3 files changed, 139 insertions(+) My brain is mush (the NPF/MMIO patch wasn't easy), so I'll pick up the review at this patch next week. Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#60155): https://edk2.groups.io/g/devel/message/60155 Mute This Topic: https://groups.io/mt/74336582/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 05/19/20 23:50, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>
> Under SEV-ES, a DR7 read or write intercept generates a #VC exception.
> The #VC handler must provide special support to the guest for this. On
> a DR7 write, the #VC handler must cache the value and issue a VMGEXIT
> to notify the hypervisor of the write. However, the #VC handler must
> not actually set the value of the DR7 register. On a DR7 read, the #VC
> handler must return the cached value of the DR7 register to the guest.
> VMGEXIT is not invoked for a DR7 register read.
>
> To avoid exception recursion, a #VC exception will not try to read and
> push the actual debug registers into the EFI_SYSTEM_CONTEXT_X64 struct
> and instead push zeroes. The #VC exception handler does not make use of
> the debug registers from saved context.
AFAICS the following patches introcuce / reiterate the per-CPU page concept:
- "MdeModulePkg/DxeIplPeim: Support GHCB pages when creating page
tables" (v8 05/46)
- "OvmfPkg: Create a GHCB page for use during Sec phase" (v8 29/46)
- "OvmfPkg: Create GHCB pages for use during Pei and Dxe phase" (v8 31/46)
I find it somewhat difficult to locate those patches and to learn about
the per-cpu pages from them. The first patch listed above belongs to a
different package. And the two other patches listed above do not precede
(but follow) the present patch.
(1) Therefore please include a paragraph about the per-cpu pages in the
commit message of this patch.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
> .../Library/VmgExitLib/X64/VmgExitVcHandler.c | 105 ++++++++++++++++++
> .../X64/ExceptionHandlerAsm.nasm | 17 +++
> .../X64/Xcode5ExceptionHandlerAsm.nasm | 17 +++
> 3 files changed, 139 insertions(+)
Please pass "--stat=1000 --stat-graph-width=20" to git-format-patch;
that way, the pathnames will not be truncated, and the graph to the
right will still not be wider than 20 chars.
Why I'm requesting this (and unfortunately there is no way to make the
second switch above permanent, in the git config): because I almost
missed that this patch modifies both UefiCpuPkg and OvmfPkg. It would
have been obvious from the diffstat (if the pathnames had not been
truncated).
(2) Please split the UefiCpuPkg hunks to a separate patch, if possible.
(Or maybe consider squashing those hunks into patch
"UefiCpuPkg/CpuExceptionHandler: Add base support for the #VC exception"
(v8 11/46), if the UefiCpuPkg owners prefer that.)
>
> diff --git a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
> index b028b20f255a..e4072d79d704 100644
> --- a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
> +++ b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
> @@ -14,6 +14,16 @@
>
> #define CR4_OSXSAVE (1 << 18)
>
> +#define DR7_RESET_VALUE 0x400
(3) From the Intel SDM, this looks like a standard value. I'd say if we
deem it important enough for turning into a macro, then it belongs
elsewhere (in some more visible header file).
Otherwise (given that we only use it once, below), I think we could
simply open-code it at the location of use, with a comment.
> +
> +//
> +// Per-CPU data mapping structure
> +//
> +typedef struct {
> + BOOLEAN Dr7Cached;
> + UINT64 Dr7;
> +} SEV_ES_PER_CPU_DATA;
> +
> //
> // Instruction execution mode definition
> //
> @@ -1494,6 +1504,93 @@ RdtscExit (
> return 0;
> }
>
> +/**
> + Handle a DR7 register write event.
> +
> + Use the VMGEXIT instruction to handle a DR7 write event.
> +
> + @param[in, out] Ghcb Pointer to the Guest-Hypervisor Communication
> + Block
> + @param[in, out] Regs x64 processor context
> + @param[in] InstructionData Instruction parsing context
> +
> + @retval 0 Event handled successfully
> + @retval Others New exception value to propagate
> +
> +**/
> +STATIC
> +UINT64
> +Dr7WriteExit (
> + IN OUT GHCB *Ghcb,
> + IN OUT EFI_SYSTEM_CONTEXT_X64 *Regs,
> + IN SEV_ES_INSTRUCTION_DATA *InstructionData
> + )
> +{
> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
> + SEV_ES_PER_CPU_DATA *SevEsData;
> + INTN *Register;
(4) This should be UINT64, per my earlier request.
> + UINT64 Status;
> +
> + Ext = &InstructionData->Ext;
> + SevEsData = (SEV_ES_PER_CPU_DATA *) (Ghcb + 1);
> +
> + DecodeModRm (Regs, InstructionData);
> +
> + /* MOV DRn always treats MOD == 3 no matter how encoded */
(5) comment style
> + Register = GetRegisterPointer (Regs, Ext->ModRm.Rm);
> +
> + /* Using a value of 0 for ExitInfo1 means RAX holds the value */
(6) comment style
> + Ghcb->SaveArea.Rax = *Register;
> + GhcbSetRegValid (Ghcb, GhcbRax);
> +
> + Status = VmgExit (Ghcb, SVM_EXIT_DR7_WRITE, 0, 0);
> + if (Status) {
(7) please compare with 0 explicitly
> + return Status;
> + }
> +
> + SevEsData->Dr7 = *Register;
> + SevEsData->Dr7Cached = TRUE;
Hmmm... I'm wondering where this BOOLEAN gets re-set to FALSE on a
platform reset.
In patch "OvmfPkg: Create GHCB pages for use during Pei and Dxe phase",
in function AmdSevEsInitialize(), we have a ZeroMem(). That should cover
it for PEI and DXE; OK.
(8) In patch "OvmfPkg: Create a GHCB page for use during Sec phase"
however, we don't seem to zero out the per-cpu page itself (which
resides just after PcdOvmfSecGhcbBase).
Do we do that elsewhere? (Sorry if I'm just not seeing it.)
I'm asking because, after a platform reset, SevEsData->Dr7Cached may
read as TRUE in SEC at the very first access (it lives at a fixed
location, and QEMU platform reset does not clear RAM). And so we could
return the value cached from the previous boot rather than 0x400.
> +
> + return 0;
> +}
> +
> +/**
> + Handle a DR7 register read event.
> +
> + Use the VMGEXIT instruction to handle a DR7 read event.
> +
> + @param[in, out] Ghcb Pointer to the Guest-Hypervisor Communication
> + Block
> + @param[in, out] Regs x64 processor context
> + @param[in] InstructionData Instruction parsing context
> +
> + @retval 0 Event handled successfully
> +
> +**/
> +STATIC
> +UINT64
> +Dr7ReadExit (
> + IN OUT GHCB *Ghcb,
> + IN OUT EFI_SYSTEM_CONTEXT_X64 *Regs,
> + IN SEV_ES_INSTRUCTION_DATA *InstructionData
> + )
> +{
> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
> + SEV_ES_PER_CPU_DATA *SevEsData;
> + INTN *Register;
(9) Should be UINT64.
> +
> + Ext = &InstructionData->Ext;
> + SevEsData = (SEV_ES_PER_CPU_DATA *) (Ghcb + 1);
> +
> + DecodeModRm (Regs, InstructionData);
> +
> + /* MOV DRn always treats MOD == 3 no matter how encoded */
(10) Please fix the comment style.
> + Register = GetRegisterPointer (Regs, Ext->ModRm.Rm);
> + *Register = (SevEsData->Dr7Cached) ? SevEsData->Dr7 : DR7_RESET_VALUE;
> +
> + return 0;
> +}
> +
> /**
> Handle a #VC exception.
>
> @@ -1538,6 +1635,14 @@ VmgExitHandleVc (
>
> ExitCode = Regs->ExceptionData;
> switch (ExitCode) {
> + case SVM_EXIT_DR7_READ:
> + NaeExit = Dr7ReadExit;
> + break;
> +
> + case SVM_EXIT_DR7_WRITE:
> + NaeExit = Dr7WriteExit;
> + break;
> +
> case SVM_EXIT_RDTSC:
> NaeExit = RdtscExit;
> break;
Stopping here (before the UefiCpuPkg hunks).
Thanks!
Laszlo
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> index 3814f9de3703..2a5545ecfd41 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> @@ -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)
> @@ -224,6 +226,9 @@ HasErrorCode:
> 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
> @@ -236,7 +241,19 @@ HasErrorCode:
> 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
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
> index 19198f273137..26cae56cc5cf 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
> @@ -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 @@ HasErrorCode:
> 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 @@ HasErrorCode:
> 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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60211): https://edk2.groups.io/g/devel/message/60211
Mute This Topic: https://groups.io/mt/74336582/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 5/25/20 9:47 AM, Laszlo Ersek wrote:
> On 05/19/20 23:50, Lendacky, Thomas wrote:
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&data=02%7C01%7Cthomas.lendacky%40amd.com%7C8d75a8b2107f4def062c08d800ba8795%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260148432921212&sdata=WNj6rvvOB%2FeVbeozpvRTXmrqFZEQuEjzEOGIU9KvJVs%3D&reserved=0
>>
>> Under SEV-ES, a DR7 read or write intercept generates a #VC exception.
>> The #VC handler must provide special support to the guest for this. On
>> a DR7 write, the #VC handler must cache the value and issue a VMGEXIT
>> to notify the hypervisor of the write. However, the #VC handler must
>> not actually set the value of the DR7 register. On a DR7 read, the #VC
>> handler must return the cached value of the DR7 register to the guest.
>> VMGEXIT is not invoked for a DR7 register read.
>>
>> To avoid exception recursion, a #VC exception will not try to read and
>> push the actual debug registers into the EFI_SYSTEM_CONTEXT_X64 struct
>> and instead push zeroes. The #VC exception handler does not make use of
>> the debug registers from saved context.
>
> AFAICS the following patches introcuce / reiterate the per-CPU page concept:
>
> - "MdeModulePkg/DxeIplPeim: Support GHCB pages when creating page
> tables" (v8 05/46)
> - "OvmfPkg: Create a GHCB page for use during Sec phase" (v8 29/46)
> - "OvmfPkg: Create GHCB pages for use during Pei and Dxe phase" (v8 31/46)
>
> I find it somewhat difficult to locate those patches and to learn about
> the per-cpu pages from them. The first patch listed above belongs to a
> different package. And the two other patches listed above do not precede
> (but follow) the present patch.
>
> (1) Therefore please include a paragraph about the per-cpu pages in the
> commit message of this patch.
Will do.
>
>>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>> .../Library/VmgExitLib/X64/VmgExitVcHandler.c | 105 ++++++++++++++++++
>> .../X64/ExceptionHandlerAsm.nasm | 17 +++
>> .../X64/Xcode5ExceptionHandlerAsm.nasm | 17 +++
>> 3 files changed, 139 insertions(+)
>
> Please pass "--stat=1000 --stat-graph-width=20" to git-format-patch;
> that way, the pathnames will not be truncated, and the graph to the
> right will still not be wider than 20 chars.
>
> Why I'm requesting this (and unfortunately there is no way to make the
> second switch above permanent, in the git config): because I almost
> missed that this patch modifies both UefiCpuPkg and OvmfPkg. It would
> have been obvious from the diffstat (if the pathnames had not been
> truncated).
>
> (2) Please split the UefiCpuPkg hunks to a separate patch, if possible.
>
> (Or maybe consider squashing those hunks into patch
> "UefiCpuPkg/CpuExceptionHandler: Add base support for the #VC exception"
> (v8 11/46), if the UefiCpuPkg owners prefer that.)
It would probably fit nicely into the existing patch. I'll look and either
move it to there or create a new patch.
>
>>
>> diff --git a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
>> index b028b20f255a..e4072d79d704 100644
>> --- a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
>> +++ b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
>> @@ -14,6 +14,16 @@
>>
>> #define CR4_OSXSAVE (1 << 18)
>>
>> +#define DR7_RESET_VALUE 0x400
>
> (3) From the Intel SDM, this looks like a standard value. I'd say if we
> deem it important enough for turning into a macro, then it belongs
> elsewhere (in some more visible header file).
>
> Otherwise (given that we only use it once, below), I think we could
> simply open-code it at the location of use, with a comment.
I'll do the latter.
>
>> +
>> +//
>> +// Per-CPU data mapping structure
>> +//
>> +typedef struct {
>> + BOOLEAN Dr7Cached;
>> + UINT64 Dr7;
>> +} SEV_ES_PER_CPU_DATA;
>> +
>> //
>> // Instruction execution mode definition
>> //
>> @@ -1494,6 +1504,93 @@ RdtscExit (
>> return 0;
>> }
>>
>> +/**
>> + Handle a DR7 register write event.
>> +
>> + Use the VMGEXIT instruction to handle a DR7 write event.
>> +
>> + @param[in, out] Ghcb Pointer to the Guest-Hypervisor Communication
>> + Block
>> + @param[in, out] Regs x64 processor context
>> + @param[in] InstructionData Instruction parsing context
>> +
>> + @retval 0 Event handled successfully
>> + @retval Others New exception value to propagate
>> +
>> +**/
>> +STATIC
>> +UINT64
>> +Dr7WriteExit (
>> + IN OUT GHCB *Ghcb,
>> + IN OUT EFI_SYSTEM_CONTEXT_X64 *Regs,
>> + IN SEV_ES_INSTRUCTION_DATA *InstructionData
>> + )
>> +{
>> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
>> + SEV_ES_PER_CPU_DATA *SevEsData;
>> + INTN *Register;
>
> (4) This should be UINT64, per my earlier request.
>
>> + UINT64 Status;
>> +
>> + Ext = &InstructionData->Ext;
>> + SevEsData = (SEV_ES_PER_CPU_DATA *) (Ghcb + 1);
>> +
>> + DecodeModRm (Regs, InstructionData);
>> +
>> + /* MOV DRn always treats MOD == 3 no matter how encoded */
>
> (5) comment style
>
>> + Register = GetRegisterPointer (Regs, Ext->ModRm.Rm);
>> +
>> + /* Using a value of 0 for ExitInfo1 means RAX holds the value */
>
> (6) comment style
>
>> + Ghcb->SaveArea.Rax = *Register;
>> + GhcbSetRegValid (Ghcb, GhcbRax);
>> +
>> + Status = VmgExit (Ghcb, SVM_EXIT_DR7_WRITE, 0, 0);
>> + if (Status) {
>
> (7) please compare with 0 explicitly
4 - 7 will be taken care of.
>
>> + return Status;
>> + }
>> +
>> + SevEsData->Dr7 = *Register;
>> + SevEsData->Dr7Cached = TRUE;
>
> Hmmm... I'm wondering where this BOOLEAN gets re-set to FALSE on a
> platform reset.
>
> In patch "OvmfPkg: Create GHCB pages for use during Pei and Dxe phase",
> in function AmdSevEsInitialize(), we have a ZeroMem(). That should cover
> it for PEI and DXE; OK.
>
> (8) In patch "OvmfPkg: Create a GHCB page for use during Sec phase"
> however, we don't seem to zero out the per-cpu page itself (which
> resides just after PcdOvmfSecGhcbBase).
>
> Do we do that elsewhere? (Sorry if I'm just not seeing it.)
>
> I'm asking because, after a platform reset, SevEsData->Dr7Cached may
> read as TRUE in SEC at the very first access (it lives at a fixed
> location, and QEMU platform reset does not clear RAM). And so we could
> return the value cached from the previous boot rather than 0x400.
An SEV-ES guest can't be rebooted/reset without restarting Qemu because
the guest register can't be changed by the hypervisor. So a full reboot
isn't initially supported SEV-ES.
But, yes, this should still clear it to be safe for any future support.
I'll find an appropriate place to zero it out.
>
>
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + Handle a DR7 register read event.
>> +
>> + Use the VMGEXIT instruction to handle a DR7 read event.
>> +
>> + @param[in, out] Ghcb Pointer to the Guest-Hypervisor Communication
>> + Block
>> + @param[in, out] Regs x64 processor context
>> + @param[in] InstructionData Instruction parsing context
>> +
>> + @retval 0 Event handled successfully
>> +
>> +**/
>> +STATIC
>> +UINT64
>> +Dr7ReadExit (
>> + IN OUT GHCB *Ghcb,
>> + IN OUT EFI_SYSTEM_CONTEXT_X64 *Regs,
>> + IN SEV_ES_INSTRUCTION_DATA *InstructionData
>> + )
>> +{
>> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
>> + SEV_ES_PER_CPU_DATA *SevEsData;
>> + INTN *Register;
>
> (9) Should be UINT64.
>
>> +
>> + Ext = &InstructionData->Ext;
>> + SevEsData = (SEV_ES_PER_CPU_DATA *) (Ghcb + 1);
>> +
>> + DecodeModRm (Regs, InstructionData);
>> +
>> + /* MOV DRn always treats MOD == 3 no matter how encoded */
>
> (10) Please fix the comment style.
>
>> + Register = GetRegisterPointer (Regs, Ext->ModRm.Rm);
>> + *Register = (SevEsData->Dr7Cached) ? SevEsData->Dr7 : DR7_RESET_VALUE;
>> +
>> + return 0;
>> +}
>> +
>> /**
>> Handle a #VC exception.
>>
>> @@ -1538,6 +1635,14 @@ VmgExitHandleVc (
>>
>> ExitCode = Regs->ExceptionData;
>> switch (ExitCode) {
>> + case SVM_EXIT_DR7_READ:
>> + NaeExit = Dr7ReadExit;
>> + break;
>> +
>> + case SVM_EXIT_DR7_WRITE:
>> + NaeExit = Dr7WriteExit;
>> + break;
>> +
>> case SVM_EXIT_RDTSC:
>> NaeExit = RdtscExit;
>> break;
>
> Stopping here (before the UefiCpuPkg hunks).
9 - 10 to be taken care of.
Thanks,
Tom
>
> Thanks!
> Laszlo
>
>> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
>> index 3814f9de3703..2a5545ecfd41 100644
>> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
>> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
>> @@ -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)
>> @@ -224,6 +226,9 @@ HasErrorCode:
>> 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
>> @@ -236,7 +241,19 @@ HasErrorCode:
>> 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
>> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
>> index 19198f273137..26cae56cc5cf 100644
>> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
>> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
>> @@ -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 @@ HasErrorCode:
>> 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 @@ HasErrorCode:
>> 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
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60282): https://edk2.groups.io/g/devel/message/60282
Mute This Topic: https://groups.io/mt/74336582/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 05/26/20 17:06, Tom Lendacky wrote:
> On 5/25/20 9:47 AM, Laszlo Ersek wrote:
>> On 05/19/20 23:50, Lendacky, Thomas wrote:
>>> BZ:
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&data=02%7C01%7Cthomas.lendacky%40amd.com%7C8d75a8b2107f4def062c08d800ba8795%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260148432921212&sdata=WNj6rvvOB%2FeVbeozpvRTXmrqFZEQuEjzEOGIU9KvJVs%3D&reserved=0
>>>
>>>
>>> Under SEV-ES, a DR7 read or write intercept generates a #VC exception.
>>> The #VC handler must provide special support to the guest for this. On
>>> a DR7 write, the #VC handler must cache the value and issue a VMGEXIT
>>> to notify the hypervisor of the write. However, the #VC handler must
>>> not actually set the value of the DR7 register. On a DR7 read, the #VC
>>> handler must return the cached value of the DR7 register to the guest.
>>> VMGEXIT is not invoked for a DR7 register read.
>>>
>>> To avoid exception recursion, a #VC exception will not try to read and
>>> push the actual debug registers into the EFI_SYSTEM_CONTEXT_X64 struct
>>> and instead push zeroes. The #VC exception handler does not make use of
>>> the debug registers from saved context.
>>
>> AFAICS the following patches introcuce / reiterate the per-CPU page
>> concept:
>>
>> - "MdeModulePkg/DxeIplPeim: Support GHCB pages when creating page
>> tables" (v8 05/46)
>> - "OvmfPkg: Create a GHCB page for use during Sec phase" (v8 29/46)
>> - "OvmfPkg: Create GHCB pages for use during Pei and Dxe phase" (v8
>> 31/46)
>>
>> I find it somewhat difficult to locate those patches and to learn about
>> the per-cpu pages from them. The first patch listed above belongs to a
>> different package. And the two other patches listed above do not precede
>> (but follow) the present patch.
>>
>> (1) Therefore please include a paragraph about the per-cpu pages in the
>> commit message of this patch.
>
> Will do.
>
>>
>>>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>> ---
>>> .../Library/VmgExitLib/X64/VmgExitVcHandler.c | 105 ++++++++++++++++++
>>> .../X64/ExceptionHandlerAsm.nasm | 17 +++
>>> .../X64/Xcode5ExceptionHandlerAsm.nasm | 17 +++
>>> 3 files changed, 139 insertions(+)
>>
>> Please pass "--stat=1000 --stat-graph-width=20" to git-format-patch;
>> that way, the pathnames will not be truncated, and the graph to the
>> right will still not be wider than 20 chars.
>>
>> Why I'm requesting this (and unfortunately there is no way to make the
>> second switch above permanent, in the git config): because I almost
>> missed that this patch modifies both UefiCpuPkg and OvmfPkg. It would
>> have been obvious from the diffstat (if the pathnames had not been
>> truncated).
>>
>> (2) Please split the UefiCpuPkg hunks to a separate patch, if possible.
>>
>> (Or maybe consider squashing those hunks into patch
>> "UefiCpuPkg/CpuExceptionHandler: Add base support for the #VC exception"
>> (v8 11/46), if the UefiCpuPkg owners prefer that.)
>
> It would probably fit nicely into the existing patch. I'll look and
> either move it to there or create a new patch.
>
>>
>>>
>>> diff --git a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
>>> b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
>>> index b028b20f255a..e4072d79d704 100644
>>> --- a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
>>> +++ b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
>>> @@ -14,6 +14,16 @@
>>> #define CR4_OSXSAVE (1 << 18)
>>> +#define DR7_RESET_VALUE 0x400
>>
>> (3) From the Intel SDM, this looks like a standard value. I'd say if we
>> deem it important enough for turning into a macro, then it belongs
>> elsewhere (in some more visible header file).
>>
>> Otherwise (given that we only use it once, below), I think we could
>> simply open-code it at the location of use, with a comment.
>
> I'll do the latter.
>
>>
>>> +
>>> +//
>>> +// Per-CPU data mapping structure
>>> +//
>>> +typedef struct {
>>> + BOOLEAN Dr7Cached;
>>> + UINT64 Dr7;
>>> +} SEV_ES_PER_CPU_DATA;
>>> +
>>> //
>>> // Instruction execution mode definition
>>> //
>>> @@ -1494,6 +1504,93 @@ RdtscExit (
>>> return 0;
>>> }
>>> +/**
>>> + Handle a DR7 register write event.
>>> +
>>> + Use the VMGEXIT instruction to handle a DR7 write event.
>>> +
>>> + @param[in, out] Ghcb Pointer to the Guest-Hypervisor
>>> Communication
>>> + Block
>>> + @param[in, out] Regs x64 processor context
>>> + @param[in] InstructionData Instruction parsing context
>>> +
>>> + @retval 0 Event handled successfully
>>> + @retval Others New exception value to propagate
>>> +
>>> +**/
>>> +STATIC
>>> +UINT64
>>> +Dr7WriteExit (
>>> + IN OUT GHCB *Ghcb,
>>> + IN OUT EFI_SYSTEM_CONTEXT_X64 *Regs,
>>> + IN SEV_ES_INSTRUCTION_DATA *InstructionData
>>> + )
>>> +{
>>> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
>>> + SEV_ES_PER_CPU_DATA *SevEsData;
>>> + INTN *Register;
>>
>> (4) This should be UINT64, per my earlier request.
>>
>>> + UINT64 Status;
>>> +
>>> + Ext = &InstructionData->Ext;
>>> + SevEsData = (SEV_ES_PER_CPU_DATA *) (Ghcb + 1);
>>> +
>>> + DecodeModRm (Regs, InstructionData);
>>> +
>>> + /* MOV DRn always treats MOD == 3 no matter how encoded */
>>
>> (5) comment style
>>
>>> + Register = GetRegisterPointer (Regs, Ext->ModRm.Rm);
>>> +
>>> + /* Using a value of 0 for ExitInfo1 means RAX holds the value */
>>
>> (6) comment style
>>
>>> + Ghcb->SaveArea.Rax = *Register;
>>> + GhcbSetRegValid (Ghcb, GhcbRax);
>>> +
>>> + Status = VmgExit (Ghcb, SVM_EXIT_DR7_WRITE, 0, 0);
>>> + if (Status) {
>>
>> (7) please compare with 0 explicitly
>
> 4 - 7 will be taken care of.
>
>>
>>> + return Status;
>>> + }
>>> +
>>> + SevEsData->Dr7 = *Register;
>>> + SevEsData->Dr7Cached = TRUE;
>>
>> Hmmm... I'm wondering where this BOOLEAN gets re-set to FALSE on a
>> platform reset.
>>
>> In patch "OvmfPkg: Create GHCB pages for use during Pei and Dxe phase",
>> in function AmdSevEsInitialize(), we have a ZeroMem(). That should cover
>> it for PEI and DXE; OK.
>>
>> (8) In patch "OvmfPkg: Create a GHCB page for use during Sec phase"
>> however, we don't seem to zero out the per-cpu page itself (which
>> resides just after PcdOvmfSecGhcbBase).
>>
>> Do we do that elsewhere? (Sorry if I'm just not seeing it.)
>>
>> I'm asking because, after a platform reset, SevEsData->Dr7Cached may
>> read as TRUE in SEC at the very first access (it lives at a fixed
>> location, and QEMU platform reset does not clear RAM). And so we could
>> return the value cached from the previous boot rather than 0x400.
>
> An SEV-ES guest can't be rebooted/reset without restarting Qemu because
> the guest register can't be changed by the hypervisor. So a full reboot
> isn't initially supported SEV-ES.
Apologies for missing that!
>
> But, yes, this should still clear it to be safe for any future support.
> I'll find an appropriate place to zero it out.
With your explanation above, about platform reset, I think I'm happy
with the current handling of "Dr7Cached". So I'd like to leave the
choice to you: please either add the clearing, or document in the commit
message and/or the code that platform reset will not happen. Whichever
you like more.
Thank you!
Laszlo
>
>>
>>
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +/**
>>> + Handle a DR7 register read event.
>>> +
>>> + Use the VMGEXIT instruction to handle a DR7 read event.
>>> +
>>> + @param[in, out] Ghcb Pointer to the Guest-Hypervisor
>>> Communication
>>> + Block
>>> + @param[in, out] Regs x64 processor context
>>> + @param[in] InstructionData Instruction parsing context
>>> +
>>> + @retval 0 Event handled successfully
>>> +
>>> +**/
>>> +STATIC
>>> +UINT64
>>> +Dr7ReadExit (
>>> + IN OUT GHCB *Ghcb,
>>> + IN OUT EFI_SYSTEM_CONTEXT_X64 *Regs,
>>> + IN SEV_ES_INSTRUCTION_DATA *InstructionData
>>> + )
>>> +{
>>> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
>>> + SEV_ES_PER_CPU_DATA *SevEsData;
>>> + INTN *Register;
>>
>> (9) Should be UINT64.
>>
>>> +
>>> + Ext = &InstructionData->Ext;
>>> + SevEsData = (SEV_ES_PER_CPU_DATA *) (Ghcb + 1);
>>> +
>>> + DecodeModRm (Regs, InstructionData);
>>> +
>>> + /* MOV DRn always treats MOD == 3 no matter how encoded */
>>
>> (10) Please fix the comment style.
>>
>>> + Register = GetRegisterPointer (Regs, Ext->ModRm.Rm);
>>> + *Register = (SevEsData->Dr7Cached) ? SevEsData->Dr7 :
>>> DR7_RESET_VALUE;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> /**
>>> Handle a #VC exception.
>>> @@ -1538,6 +1635,14 @@ VmgExitHandleVc (
>>> ExitCode = Regs->ExceptionData;
>>> switch (ExitCode) {
>>> + case SVM_EXIT_DR7_READ:
>>> + NaeExit = Dr7ReadExit;
>>> + break;
>>> +
>>> + case SVM_EXIT_DR7_WRITE:
>>> + NaeExit = Dr7WriteExit;
>>> + break;
>>> +
>>> case SVM_EXIT_RDTSC:
>>> NaeExit = RdtscExit;
>>> break;
>>
>> Stopping here (before the UefiCpuPkg hunks).
>
> 9 - 10 to be taken care of.
>
> Thanks,
> Tom
>
>>
>> Thanks!
>> Laszlo
>>
>>> diff --git
>>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
>>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
>>> index 3814f9de3703..2a5545ecfd41 100644
>>> ---
>>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
>>> +++
>>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
>>> @@ -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)
>>> @@ -224,6 +226,9 @@ HasErrorCode:
>>> 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
>>> @@ -236,7 +241,19 @@ HasErrorCode:
>>> 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
>>> diff --git
>>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
>>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
>>>
>>> index 19198f273137..26cae56cc5cf 100644
>>> ---
>>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
>>>
>>> +++
>>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAsm.nasm
>>>
>>> @@ -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 @@ HasErrorCode:
>>> 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 @@ HasErrorCode:
>>> 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
>>>
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60339): https://edk2.groups.io/g/devel/message/60339
Mute This Topic: https://groups.io/mt/74336582/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.