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

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

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