From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 0287580346 for ; Tue, 7 Mar 2017 09:06:15 -0800 (PST) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 7D690C056791; Tue, 7 Mar 2017 17:06:15 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-184.phx2.redhat.com [10.3.116.184]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v27H6CWi004695; Tue, 7 Mar 2017 12:06:13 -0500 To: Brijesh Singh , jordan.l.justen@intel.com, edk2-devel@ml01.01.org References: <148884284887.29188.7643544710695103939.stgit@brijesh-build-machine> <148884286215.29188.1084675072356724555.stgit@brijesh-build-machine> Cc: Thomas.Lendacky@amd.com, leo.duran@amd.com, brijesh.sing@amd.com From: Laszlo Ersek Message-ID: Date: Tue, 7 Mar 2017 18:06:11 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.1 MIME-Version: 1.0 In-Reply-To: <148884286215.29188.1084675072356724555.stgit@brijesh-build-machine> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Tue, 07 Mar 2017 17:06:15 +0000 (UTC) Subject: Re: [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV helper library X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 07 Mar 2017 17:06:15 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 03/07/17 00:27, Brijesh Singh wrote: > The library contain common helper routines for SEV feature. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Brijesh Singh > --- > OvmfPkg/Include/Library/MemcryptSevLib.h | 42 +++++++++++++ > OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c | 66 +++++++++++++++++++++ > OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf | 44 ++++++++++++++ > OvmfPkg/OvmfPkgIa32X64.dsc | 4 + > OvmfPkg/OvmfPkgX64.dsc | 4 + > 5 files changed, 160 insertions(+) > create mode 100644 OvmfPkg/Include/Library/MemcryptSevLib.h > create mode 100644 OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c > create mode 100644 OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf New files -- can you please double-check they are CRLF terminated? > > diff --git a/OvmfPkg/Include/Library/MemcryptSevLib.h b/OvmfPkg/Include/Library/MemcryptSevLib.h > new file mode 100644 > index 0000000..89f9c86 > --- /dev/null > +++ b/OvmfPkg/Include/Library/MemcryptSevLib.h > @@ -0,0 +1,42 @@ > +/** @file Please add one or two sentences about the purpose of this library class. > + Copyright (C) 2017 Advanced Micro Devices. > + > + This program and the accompanying materials are licensed and made available > + under the terms and conditions of the BSD License which accompanies this > + distribution. The full text of the license may be found at > + http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT > + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +**/ > + > +#ifndef __MEMCRYPT_SEV_LIB_H__ > +#define __MEMCRYPT_SEV_LIB_H__ > + > +#include I think it shouldn't be necessary to include this, especially not for a BASE library (class). What type exactly did you need this include for? > +#include > + > +/** > + > + Initialize SEV memory encryption > + > +**/ > + > +RETURN_STATUS > +EFIAPI > +MemcryptSevInitialize ( > + VOID > + ); > + > +/** > + > + Return TRUE when SEV is active otherwise FALSE > + > + **/ Is this function restricted to code that runs after a successful invocation of MemcryptSevInitialize()? If so, please document that. Please also document the return values (with @retval and/or @return), even though the explanation is likely trivial. > +BOOLEAN > +EFIAPI > +SevActive ( > + VOID > + ); > + I'd prefer if we could use a common prefix for the two functions, something that is unique to / characteristic of the new library class. > +#endif > diff --git a/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c > new file mode 100644 > index 0000000..2d60b75 > --- /dev/null > +++ b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.c > @@ -0,0 +1,66 @@ > +/** @file > + > + Copyright (c) 2017, AMD Incorporated. All rights reserved.
> + > + This program and the accompanying materials > + are licensed and made available under the terms and conditions of the BSD License > + which accompanies this distribution. The full text of the license may be found at > + http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ > + > +#include "Uefi.h" > +#include > +#include > +#include > +#include > + > +#define KVM_FEATURE_MEMORY_ENCRYPTION 0x100 Can you move all these magic constants (CPUID leaves as well) to OvmfPkg/Include/IndustryStandard/? I guess the filename could be "AmdSev.h", or something similar. Such header files are also prime locations to capture the great standards references that you added to the blurb. > + > +RETURN_STATUS > +EFIAPI > +MemcryptSevInitialize ( > + VOID > + ) > +{ > + UINT32 EBX; Should be Ebx, IMO. > + UINT64 MeMask = 0; Initialization of local variables is forbidden by the edk2 coding style. > + > + if (SevActive ()) { Aha! So it's the other way around than I expected. > + // CPUID Fn8000_001f[EBX] - Bit 5:0 (memory encryption bit position) Comment style is // // CPUID ... // > + AsmCpuid(0x8000001f, NULL, &EBX, NULL, NULL); > + MeMask = (1ULL << (EBX & 0x3f)); You can't bit shift 64-bit values in edk2 with the C-language operator; it won't build for Ia32. Please either use LShiftU64() here, or -- if you are sure the code will never be reached in Ia32 guests -- use (UINTN)1, and shift that with <<. BTW, in what PI phases do you intend to use this library? For example, in the Ia32X64 build of OVMF, the PEI phase runs as 32-bit, and the DXE phase runs in 64-bit. Hm, in the next patch, it seems that the library is put to use in PlatformPei (and only there). Since these functions are really small, I think I would prefer having a new file under OvmfPkg/PlatformPei only. More on the Ia32X64 build of OVMF -- assuming the PEI phase runs in 32-bit, but the DXE phase (and the OS) run in 64-bit, does SEV appear active when the 32-bit PlatformPei module queries it? * If it doesn't, that's a problem, because then the PCD will be set incorrectly. In this case, it might make sense to limit SEV only to the X64 build of OVMF. * If SEV does appear active when the 32-bit PlatformPei module queries it, then we definitely need to use LShiftU64 here. Have you tested this series in all three builds of OVMF? (Ia32, Ia32X64, and X64?) I'm not asking about SMM, I can see it's on the TODO list. > + DEBUG ((DEBUG_INFO, "KVM Secure Encrypted Virtualization (SEV) is enabled\n")); > + DEBUG ((DEBUG_INFO, "MemEncryptionMask 0x%lx\n", MeMask)); > + } > + > + PcdSet64S (PcdPteMemoryEncryptionAddressOrMask, MeMask); Whiile we do exped PcdSet64S to succeed, please add a separate Status variable, and add an ASSERT_RETURN_ERROR on that. > + > + return RETURN_SUCCESS; > +} > + > +BOOLEAN > +EFIAPI > +SevActive ( > + VOID > + ) > +{ > + UINT32 KVMFeatures, EAX; Should be KvmFeatures and Eax. > + > + // Check if KVM memory encyption feature is set comment style > + AsmCpuid(0x40000001, &KVMFeatures, NULL, NULL, NULL); > + if (KVMFeatures & KVM_FEATURE_MEMORY_ENCRYPTION) { > + > + // Check whether SEV is enabled > + // CPUID Fn8000_001f[EAX] - Bit 0 (SEV is enabled) > + AsmCpuid(0x8000001f, &EAX, NULL, NULL, NULL); Tom commented on this -- the result is not checked. > + > + return TRUE; > + } > + > + return FALSE; > +} > + > diff --git a/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf > new file mode 100644 > index 0000000..8e8d7e0 > --- /dev/null > +++ b/OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf > @@ -0,0 +1,44 @@ > +## @file > +# > +# Copyright (c) 2017 Advanced Micro Devices. All rights reserved.
> +# > +# This program and the accompanying materials > +# are licensed and made available under the terms and conditions of the BSD License > +# which accompanies this distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +# > +# > +## > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = MemcryptSevLib > + FILE_GUID = c1594631-3888-4be4-949f-9c630dbc842b > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = MemcryptSevLib > + > +# > +# The following information is for reference only and not required by the build tools. > +# > +# VALID_ARCHITECTURES = IA32 X64 > +# > + > +[Packages] > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > + OvmfPkg/OvmfPkg.dec > + UefiCpuPkg/UefiCpuPkg.dec > + > +[Sources] > + MemcryptSevLib.c > + > +[LibraryClasses] > + BaseLib > + DebugLib > + PcdLib > + > +[Pcd] > + gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > index 56f7ff9..a35e1d2 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -129,6 +129,7 @@ > QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf > VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf > LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf > + MemcryptSevLib|OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf > !if $(SMM_REQUIRE) == FALSE > LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf > !endif Please configure your git instance so that it includes the section names of INI-style files in hunk headers: https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-05 see xfuncname there, and: https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-09 > @@ -509,6 +510,9 @@ > gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64 > gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000 > > + # Set memory encryption mask > + gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0 > + > !if $(SMM_REQUIRE) == TRUE > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01 > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index d0b0b0e..5d853d6 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -129,6 +129,7 @@ > QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf > VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf > LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf > + MemcryptSevLib|OvmfPkg/Library/MemcryptSevLib/MemcryptSevLib.inf > !if $(SMM_REQUIRE) == FALSE > LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf > !endif > @@ -508,6 +509,9 @@ > gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64 > gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000 > > + # Set memory encryption mask > + gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0 > + > !if $(SMM_REQUIRE) == TRUE > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01 > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmApSyncTimeout|100000 > The OvmfPkgIa32.dsc file is not modified. The MemcryptSevLib class is not resolved for the Ia32 build, hence PlatformPei will fail to build in the next patch. Please build and regression-test all three builds of OVMF (SMM disabled, for now) in the development of this feature. Regression-testing should in particular include S3 suspend/resume (again, with SMM disabled, for now). If it breaks (and it is expected to break), please extend the S3Verification() function so that it prevents the firmware from booting (with a reasonable error message) if it sees that SEV is active and S3 was *not* disabled on the QEMU command line. Losing a guest at S3 resume is very annoying, it's better not to boot in that case (and to ask the user to disable S3 on the QEMU command line). Anyhow, I think these functions should go directly under OvmfPkg/PlatformPei, into a new file. I may have missed a few things, but I'm (theoretically) at a conference, and right now it seems more correct for me to give (perhaps spotty) feedback *quickly*, than to let my backlog pile up. Thanks! Laszlo