From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web10.4383.1590580471775986448 for ; Wed, 27 May 2020 04:54:32 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=X5lPUCq8; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1590580470; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=AaFc1xTU3U9u5VWLtOygmisaWd6Y77EsjOHVERbBwQk=; b=X5lPUCq8/sloFeOurriGjx9J95/h0nElpI9LX2VJhDcQ8AajbJB9Fcl+aysxZsYG4DGFvR 36wh0Kv9fPydBFr+SBuGmeIKXyftjI/s+DvFp1ahIYfVtcWKOvSXWU+2leLVK5L6irUu4l ETq+jHGoX2ufcsMhSEhqGQrurIsKbzM= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-334-8-_-RSDHNZOVOKjmExVjDA-1; Wed, 27 May 2020 07:54:24 -0400 X-MC-Unique: 8-_-RSDHNZOVOKjmExVjDA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 8D2BC18FF661; Wed, 27 May 2020 11:54:22 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-77.ams2.redhat.com [10.36.113.77]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1CBF8648D7; Wed, 27 May 2020 11:54:19 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v8 26/46] OvmfPkg/VmgExitLib: Add support for DR7 Read/Write NAE events To: Tom Lendacky , devel@edk2.groups.io Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , Brijesh Singh , Ard Biesheuvel References: <7015dcd00bb075c9875b3e4f5507a3281817cef4.1589925074.git.thomas.lendacky@amd.com> <4fcce31f-d324-2c24-4831-d2aecfb5508a@redhat.com> <035a8ff3-8e63-1471-541e-22a8d1cd8180@amd.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 27 May 2020 13:54:18 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <035a8ff3-8e63-1471-541e-22a8d1cd8180@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit 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 >>> Cc: Ray Ni >>> Cc: Jordan Justen >>> Cc: Laszlo Ersek >>> Cc: Ard Biesheuvel >>> Signed-off-by: Tom Lendacky >>> --- >>>   .../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 >>> >> >