From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.158.5]) by mx.groups.io with SMTP id smtpd.web08.17391.1607258639635945184 for ; Sun, 06 Dec 2020 04:43:59 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@ibm.com header.s=pp1 header.b=HEsKIkX0; spf=none, err=permanent DNS error (domain: linux.vnet.ibm.com, ip: 148.163.158.5, mailfrom: dovmurik@linux.vnet.ibm.com) Received: from pps.filterd (m0098414.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 0B6CSN6b038728; Sun, 6 Dec 2020 07:43:55 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=subject : to : cc : references : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=pp1; bh=CmRVBQC0RnvAt2JhQRT2sOAmxZAPXEmWtWCilIeD6fU=; b=HEsKIkX0z3wT+drlTpWlk6FMm1tG0FC13Qtnq9ZjMtsdch24GIlR9IIRsxS1aukvG7NU AMjlfM8ITF/B6BQxHaRFVFM/Yw2Zj9T+Gte5vK3aNQhdaq3PY7qU9wNrfyvuGT1okQZ3 V/2ItwrAxMSbEOPFcXa7h1upuOzTlQU+Pe6p1FBiyeipZqMhvdxU3PZWGeKAFzDgMcoP HRG7OTIgor7lrwl3QeuqEcIetb2gtvJa5v6pTNwrga32w0ew/JjZ6+wwRVpB82FaRaIS u01u6u7MenC1kH9InIbmuoP9OtALsrNEBHFYxy/Hq5PY+6tXRGL2nszBAnDUDDltDNZU aw== Received: from pps.reinject (localhost [127.0.0.1]) by mx0b-001b2d01.pphosted.com with ESMTP id 358rmxpp82-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sun, 06 Dec 2020 07:43:55 -0500 Received: from m0098414.ppops.net (m0098414.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.36/8.16.0.36) with SMTP id 0B6ChCF8077553; Sun, 6 Dec 2020 07:43:55 -0500 Received: from ppma04ams.nl.ibm.com (63.31.33a9.ip4.static.sl-reverse.com [169.51.49.99]) by mx0b-001b2d01.pphosted.com with ESMTP id 358rmxpp7v-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sun, 06 Dec 2020 07:43:54 -0500 Received: from pps.filterd (ppma04ams.nl.ibm.com [127.0.0.1]) by ppma04ams.nl.ibm.com (8.16.0.42/8.16.0.42) with SMTP id 0B6CWt52003493; Sun, 6 Dec 2020 12:43:52 GMT Received: from b06cxnps3075.portsmouth.uk.ibm.com (d06relay10.portsmouth.uk.ibm.com [9.149.109.195]) by ppma04ams.nl.ibm.com with ESMTP id 3583svh1gq-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sun, 06 Dec 2020 12:43:52 +0000 Received: from d06av26.portsmouth.uk.ibm.com (d06av26.portsmouth.uk.ibm.com [9.149.105.62]) by b06cxnps3075.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 0B6ChotF30933338 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Sun, 6 Dec 2020 12:43:50 GMT Received: from d06av26.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 85E40AE045; Sun, 6 Dec 2020 12:43:50 +0000 (GMT) Received: from d06av26.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 1FF2AAE053; Sun, 6 Dec 2020 12:43:47 +0000 (GMT) Received: from [9.160.44.3] (unknown [9.160.44.3]) by d06av26.portsmouth.uk.ibm.com (Postfix) with ESMTP; Sun, 6 Dec 2020 12:43:46 +0000 (GMT) Subject: Re: [PATCH v3 1/3] OvmfPkg/MemEncryptHypercallLib: add library to support SEV hypercalls. To: Ashish Kalra , devel@edk2.groups.io Cc: brijesh.singh@amd.com, tobin@ibm.com, Jon.Grimm@amd.com, Thomas.Lendacky@amd.com, jejb@linux.ibm.com, frankeh@us.ibm.com, dgilbert@redhat.com, lersek@redhat.com, jordan.l.justen@intel.com, ard.biesheuvel@arm.com References: <5d84e29cb02eada513738fb4f0c54a6dfe35f416.1607038824.git.ashish.kalra@amd.com> From: "Dov Murik" Message-ID: <31f7bcce-627c-56b6-444f-83cc41c3e8a2@linux.vnet.ibm.com> Date: Sun, 6 Dec 2020 14:43:45 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.5.1 MIME-Version: 1.0 In-Reply-To: <5d84e29cb02eada513738fb4f0c54a6dfe35f416.1607038824.git.ashish.kalra@amd.com> X-TM-AS-GCONF: 00 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.312,18.0.737 definitions=2020-12-06_06:2020-12-04,2020-12-06 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 mlxscore=0 suspectscore=0 impostorscore=0 bulkscore=0 lowpriorityscore=0 phishscore=0 spamscore=0 mlxlogscore=999 malwarescore=0 adultscore=0 priorityscore=1501 clxscore=1011 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2012060074 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/12/2020 2:03, Ashish Kalra wrote: > From: Ashish Kalra > > Add SEV and SEV-ES hypercall abstraction library to support SEV Page > encryption/deceryption status hypercalls for SEV and SEV-ES guests. > > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > > Signed-off-by: Ashish Kalra > --- > OvmfPkg/Include/Library/MemEncryptHypercallLib.h | 37 +++++++ > OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.c | 105 ++++++++++++++++++++ > OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf | 39 ++++++++ > OvmfPkg/Library/MemEncryptHypercallLib/X64/AsmHelperStub.nasm | 39 ++++++++ > OvmfPkg/OvmfPkgX64.dsc | 1 + > 5 files changed, 221 insertions(+) > > diff --git a/OvmfPkg/Include/Library/MemEncryptHypercallLib.h b/OvmfPkg/Include/Library/MemEncryptHypercallLib.h > new file mode 100644 > index 0000000000..cd46a7f2b3 > --- /dev/null > +++ b/OvmfPkg/Include/Library/MemEncryptHypercallLib.h > @@ -0,0 +1,37 @@ > +/** @file > + > + Define Secure Encrypted Virtualization (SEV) hypercall library. > + > + Copyright (c) 2020, AMD Incorporated. All rights reserved.
> + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#ifndef _MEM_ENCRYPT_HYPERCALL_LIB_H_ > +#define _MEM_ENCRYPT_HYPERCALL_LIB_H_ > + > +#include > + > +#define SEV_PAGE_ENC_HYPERCALL 12 > + > +/** > + This hyercall is used to notify hypervisor when a page is marked as > + 'decrypted' (i.e C-bit removed). > + > + @param[in] PhysicalAddress The physical address that is the start address > + of a memory region. > + @param[in] Length The length of memory region > + @param[in] Mode SetCBit or ClearCBit > + > +**/ > + > +VOID > +EFIAPI > +SetMemoryEncDecHypercall3 ( > + IN UINTN PhysicalAddress, > + IN UINTN Length, > + IN UINTN Mode > + ); (1) I'm not sure why the "3" is needed at the end of the function name. (2) The second argument is called 'Length' here and 'Pages' in the .c file. Which name is correct? If the semantics of the hypercall deals only with whole pages, maybe it'll be better to modify the address to GFN and length to NumberOfPages. This way you don't have to do deal with non-page-aligned addresses or lengths. Of course this may reflect on changes needed in the hypercall implementation itself in KVM. (3) Mode: is this actually a boolean, or the enum which can be SetCBit(=0) or ClearCBit(=1)? Maybe a clearer argument would be "BOOL Encrypted" or "BOOL NewCBitValue"? Of course this has to match semantics of this argument in KVM. > + > +#endif > diff --git a/OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.c b/OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.c > new file mode 100644 > index 0000000000..f1136b7d36 > --- /dev/null > +++ b/OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.c > @@ -0,0 +1,105 @@ > +/** @file > + > + Secure Encrypted Virtualization (SEV) hypercall helper library > + > + Copyright (c) 2020, AMD Incorporated. All rights reserved.
> + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +// > +// Interface exposed by the ASM implementation of the core hypercall > +// > +// > + > +VOID > +EFIAPI > +SetMemoryEncDecHypercall3AsmStub ( > + IN UINTN HypercallNum, > + IN UINTN PhysicalAddress, > + IN UINTN Length, > + IN UINTN Mode > + ); > + > +/** > + This function returns the current CPU privilege level, implemented > + in ASM helper stub. > + > +**/ > + > +UINT8 > +EFIAPI > +GetCurrentCpuPrivilegeLevel ( > + VOID > + ); > + > +STATIC > +VOID > +GhcbSetRegValid ( > + IN OUT GHCB *Ghcb, > + IN GHCB_REGISTER Reg > + ) > +{ > + UINT32 RegIndex; > + UINT32 RegBit; > + > + RegIndex = Reg / 8; > + RegBit = Reg & 0x07; > + > + Ghcb->SaveArea.ValidBitmap[RegIndex] |= (1 << RegBit); > +} > + > +VOID > +EFIAPI > +SetMemoryEncDecHypercall3 ( > + IN PHYSICAL_ADDRESS PhysicalAddress, > + IN UINTN Pages, > + IN UINTN Mode > + ) > +{ > + if (MemEncryptSevEsIsEnabled ()) { > + MSR_SEV_ES_GHCB_REGISTER Msr; > + GHCB *Ghcb; > + BOOLEAN InterruptState; > + UINT64 Status; > + > + Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB); > + Ghcb = Msr.Ghcb; > + > + VmgInit (Ghcb, &InterruptState); > + > + Ghcb->SaveArea.Rax = SEV_PAGE_ENC_HYPERCALL; > + GhcbSetRegValid (Ghcb, GhcbRax); > + Ghcb->SaveArea.Rbx = PhysicalAddress; > + GhcbSetRegValid (Ghcb, GhcbRbx); > + Ghcb->SaveArea.Rcx = Pages; > + GhcbSetRegValid (Ghcb, GhcbRcx); > + Ghcb->SaveArea.Rdx = Mode; > + GhcbSetRegValid (Ghcb, GhcbRdx); > + Ghcb->SaveArea.Cpl = GetCurrentCpuPrivilegeLevel(); > + GhcbSetRegValid (Ghcb, GhcbCpl); > + > + Status = VmgExit (Ghcb, SVM_EXIT_VMMCALL, 0, 0); > + if (Status) { > + DEBUG ((DEBUG_ERROR, "SVM_EXIT_VMMCALL failed %lx\n", Status)); > + } > + VmgDone (Ghcb, InterruptState); > + } else { > + SetMemoryEncDecHypercall3AsmStub ( > + SEV_PAGE_ENC_HYPERCALL, > + PhysicalAddress, > + Pages, > + Mode); > + } > +} > diff --git a/OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf b/OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf > new file mode 100644 > index 0000000000..1936fe5b37 > --- /dev/null > +++ b/OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf > @@ -0,0 +1,39 @@ > +## @file > +# Library provides the hypervisor helper functions for SEV guest > +# > +# Copyright (c) 2020 Advanced Micro Devices. All rights reserved.
> +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +# > +## > + > +[Defines] > + INF_VERSION = 1.25 > + BASE_NAME = MemEncryptHypercallLib > + FILE_GUID = 86f2501e-f128-45f3-91c4-3cff31656ca8 > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = MemEncryptHypercallLib|SEC PEI_CORE PEIM DXE_DRIVER DXE_RUNTIME_DRIVER DXE_SMM_DRIVER UEFI_DRIVER > + > +# > +# The following information is for reference only and not required by the build > +# tools. > +# > +# VALID_ARCHITECTURES = IA32 X64 > +# > + > +[Packages] > + MdeModulePkg/MdeModulePkg.dec > + MdePkg/MdePkg.dec > + UefiCpuPkg/UefiCpuPkg.dec > + OvmfPkg/OvmfPkg.dec > + > +[Sources.X64] > + MemEncryptHypercallLib.c > + X64/AsmHelperStub.nasm > + > +[LibraryClasses] > + BaseLib > + DebugLib > + VmgExitLib > diff --git a/OvmfPkg/Library/MemEncryptHypercallLib/X64/AsmHelperStub.nasm b/OvmfPkg/Library/MemEncryptHypercallLib/X64/AsmHelperStub.nasm > new file mode 100644 > index 0000000000..5d8a7aa85a > --- /dev/null > +++ b/OvmfPkg/Library/MemEncryptHypercallLib/X64/AsmHelperStub.nasm > @@ -0,0 +1,39 @@ > +DEFAULT REL > +SECTION .text > + > +; VOID > +; EFIAPI > +; SetMemoryEncDecHypercall3AsmStub ( > +; IN UINT HypercallNum, > +; IN INTN Arg1, > +; IN INTN Arg2, > +; IN INTN Arg3 > +; ); The name of the function hints that this has something to do with memory enc/dec, but actually this is a generic vmmcall-based hypercall. In your corresponding linux patch you called it kvm_sev_hypercall3, so I assume something like that would be good here too. Also, to support future hypercalls, allow it to return an INTN (value of rax), even though that is not used for the current hypercall in question. > +global ASM_PFX(SetMemoryEncDecHypercall3AsmStub) > +ASM_PFX(SetMemoryEncDecHypercall3AsmStub): > + ; UEFI calling conventions require RBX to > + ; be nonvolatile/callee-saved. > + push rbx > + ; Copy HypercallNumber to rax > + mov rax, rcx > + ; Copy Arg1 to the register expected by KVM > + mov rbx, rdx > + ; Copy Arg2 to register expected by KVM > + mov rcx, r8 > + ; Copy Arg2 to register expected by KVM > + mov rdx, r9 > + ; Call VMMCALL > + vmmcall > + pop rbx > + ret > + > +; UINT8 > +; EFIAPI > +; GetCurrentCpuPrivilegeLevel ( > +; VOID > +; ); > +global ASM_PFX(GetCurrentCpuPrivilegeLevel) > +ASM_PFX(GetCurrentCpuPrivilegeLevel): > + mov ax, cs > + and al, 0x3 > + ret I think GetCurrentCpuPrivilegeLevel can be implemented in C as: return AsmReadCS() & 0x3; > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index e59ae05b73..97c31c7586 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -174,6 +174,7 @@ > VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf > LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf > MemEncryptSevLib|OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf > + MemEncryptHypercallLib|OvmfPkg/Library/MemEncryptHypercallLib/MemEncryptHypercallLib.inf > !if $(SMM_REQUIRE) == FALSE > LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf > !endif > -Dov