From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM10-MW2-obe.outbound.protection.outlook.com (NAM10-MW2-obe.outbound.protection.outlook.com [40.107.94.47]) by mx.groups.io with SMTP id smtpd.web11.52658.1590505565665994922 for ; Tue, 26 May 2020 08:06:05 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@amdcloud.onmicrosoft.com header.s=selector2-amdcloud-onmicrosoft-com header.b=0TsIlBDV; spf=none, err=SPF record not found (domain: amd.com, ip: 40.107.94.47, mailfrom: thomas.lendacky@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=jf16ZMaIboddRgoLO3ael382qvFZPjPS3RSimmE1qHxUUb5xspjjTDtSXcNqjcPtXBarAjHRe+C0aFjaV2YeOSFHgNQx+YRZurI6+vLa0vVjBAQe1Eqf7qRVYCnyASxWU1Ntcy/6eOF6QRlIA6Mr+eUjRI9u7FJoOuBunm+zMdIR3jM/589/vm8E4uFthkjEx/SKKMk92jCqFQwXRQYfcZGWsetKeuS/Dd6M/hRzvbzWcShXOs3IKtP1XRanVoRpCm5x7OgNKxm7gyMSdIXlcaEMUvCycFvVKr+B/h6RAkkw8gf0dlIBHVsRvce4Ie7TO96mSXjl7YV1y+XeECJimw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=mtoKhC2wPCvkezFMi+OMd41UEqPsQ3rMEGqJoYKt41k=; b=G2sG+ndMtL7b4XcXvUkSEGzqhUf9HOLKXt+xFZeiQ5b+h+NoxD3CWUumBANxKm3vD9ml6a8rz0sWf7Wi1i2RFZ167XUZ2/vKnKH7XJwS/a/W9EuZ+1EGsflgwCHS5Ck6oQd+JtaeO3/zC2/kEHdmoyLrMQCM8f5Zpr7BF5Cf5iItPdDPZASiVJik6LmjNiaq3mqdDO3oTqEQ5pE/E7xgMxPx3JLxQE1VrWK6YEJNg7ddnSdWynADqnZAN+9OwLZQHPqdLobhTbdaw5xJdl1MF2vLf0QU25Giy51/3z1WTtQgz1Wz0Fb+u8oJb9EODKtjuTd6LFwkn70R0R+7LmnB/g== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amdcloud.onmicrosoft.com; s=selector2-amdcloud-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=mtoKhC2wPCvkezFMi+OMd41UEqPsQ3rMEGqJoYKt41k=; b=0TsIlBDV8v8uo2oYu+crgNncTwTuZi8wD0HJkFrLHgvMQEN5mQca4uJ6zex+S7Zl5z8UJAwWThuGiBK4GPLRt/fv9FS3GrEQ961lD5yCA2TdLBFArcMpvWTEHWmIXEswZdKU8SYrKFG4pSm3pl45q3Y8fikVBxR6Qs2dBVljBFc= Authentication-Results: arm.com; dkim=none (message not signed) header.d=none;arm.com; dmarc=none action=none header.from=amd.com; Received: from DM5PR12MB1355.namprd12.prod.outlook.com (2603:10b6:3:6e::7) by DM5PR12MB1244.namprd12.prod.outlook.com (2603:10b6:3:73::15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3021.27; Tue, 26 May 2020 15:06:04 +0000 Received: from DM5PR12MB1355.namprd12.prod.outlook.com ([fe80::4ce1:9947:9681:c8b1]) by DM5PR12MB1355.namprd12.prod.outlook.com ([fe80::4ce1:9947:9681:c8b1%10]) with mapi id 15.20.3021.029; Tue, 26 May 2020 15:06:04 +0000 Subject: Re: [edk2-devel] [PATCH v8 26/46] OvmfPkg/VmgExitLib: Add support for DR7 Read/Write NAE events To: Laszlo Ersek , 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> From: "Lendacky, Thomas" Message-ID: <035a8ff3-8e63-1471-541e-22a8d1cd8180@amd.com> Date: Tue, 26 May 2020 10:06:01 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 In-Reply-To: <4fcce31f-d324-2c24-4831-d2aecfb5508a@redhat.com> X-ClientProxiedBy: SN4PR0401CA0011.namprd04.prod.outlook.com (2603:10b6:803:21::21) To DM5PR12MB1355.namprd12.prod.outlook.com (2603:10b6:3:6e::7) Return-Path: thomas.lendacky@amd.com MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from office-linux.texastahm.com (67.79.209.213) by SN4PR0401CA0011.namprd04.prod.outlook.com (2603:10b6:803:21::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3021.23 via Frontend Transport; Tue, 26 May 2020 15:06:03 +0000 X-Originating-IP: [67.79.209.213] X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-HT: Tenant X-MS-Office365-Filtering-Correlation-Id: 8e933d46-2be5-4555-70b3-08d801864f18 X-MS-TrafficTypeDiagnostic: DM5PR12MB1244: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:10000; X-Forefront-PRVS: 041517DFAB X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: ej7A87arpPtjLy6r6Taic6lOV116K1RX1Edn0FTU3hU9kW9/ITwwZQmF2BovAXb+oimC+kpZA0tadKSFbOVZiBnZT402fAGnbxe+qzMgtSwmas7CFajMu2Tf20hTWwVC+z5aK0RMcGENJvbBEwuqb6PkP1KErRwrRKM3fAEcRP3cBD24wxJo17RizOvHzAppY1oVjspG6ry5Hv6qbq4R4zf/YOHvgCp5YFVyxVQrpPILERrAvNO4vsvoRDBK9+4/gHINKA5iWhxdnCL/EB9ZzUXvs8zxFnaPNbP0PEYxsHNHzzSLRemv6idLxy05khIJBLglB1/B4CQqVY5uhdIMt3pcWo2C3Q4Tc9HafH4bwr1YcBv+Os/q/gHPkhMGtDyOCNDOqIV/4WCpizyhIaFrpaCoRVJb4LZLdOFooJ4UwjvA2QhgBvukjI/piHZV1fgGrqhHSQycLYd/DW2g8or8tw== X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DM5PR12MB1355.namprd12.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(4636009)(396003)(39860400002)(136003)(366004)(376002)(346002)(31696002)(186003)(16526019)(26005)(66476007)(53546011)(6506007)(52116002)(956004)(2906002)(66556008)(66946007)(31686004)(45080400002)(30864003)(2616005)(478600001)(5660300002)(54906003)(316002)(6512007)(6486002)(8936002)(36756003)(4326008)(966005)(8676002)(19627235002)(86362001)(43740500002);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: HXn3raX3RbxEVBGdRR8TWgYc1VmpUYU3xgyERUzEAyd7qlDIU0qkX2/vLI/Mi0pa/jC0Mn8NVWfqT5yXEwVlLqAS3G7gHySRhsXEp8naoaMfLzgLA1umKEueqy4iVc0OKheDi4I2E6If+reMQ0um5jXXOvQn0hbGVKhno5ILaJeyG3jGtLZoBdZumJ5Dzc42NR4s/T6RL5BQ8GBC9Uv+kh/deHcYNOTdZb1zz2+r+6rOtghPs/9OtZzJH8BSCIwHKT9mcRm/6DfMA3WjcDDfBJ7GlAuv6DWTmJa90GOt1TkeMZbKgZvxn+7RmYf3HW0DtMM/J0dSW5brmI2ukfLF8FRDlPiKhxSCBExvbw86wdIuXS7CiUfyb3bYc2rLGOV+Qo2FaaKLZkAPR0VGMPsInOkEMC/uTZ/Z8bfhxazVJdp88YlQEVoijtgsiwVIGEsxZcoNBZIJEq8lANMH74cNFeYEZasuO23l0+7L6jzFiCA= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 8e933d46-2be5-4555-70b3-08d801864f18 X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 May 2020 15:06:03.9551 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: 6iNGUgumSeqVuujzEDuSeZULJTha75DwR9PWftxP+LZTkbNE7TOKC3OyhihxgdeXwRZh6MuxNRpH2g/+/fJ34A== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR12MB1244 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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. 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 >> >