Re: [edk2-devel] [PATCH v8 26/46] OvmfPkg/VmgExitLib: Add support for DR7 Read/Write NAE events

["Laszlo Ersek" <lersek@redhat.com>]

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