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.d...@intel.com>
>>> Cc: Ray Ni <ray...@intel.com>
>>> Cc: Jordan Justen <jordan.l.jus...@intel.com>
>>> Cc: Laszlo Ersek <ler...@redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheu...@arm.com>
>>> Signed-off-by: Tom Lendacky <thomas.lenda...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
