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 2CAD68036B for ; Wed, 8 Mar 2017 07:20:09 -0800 (PST) Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (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 43D81C0092D8; Wed, 8 Mar 2017 15:19:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-93.phx2.redhat.com [10.3.116.93]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v28FJbof005340; Wed, 8 Mar 2017 10:19:37 -0500 To: "Duran, Leo" , Brijesh Singh References: <148884284887.29188.7643544710695103939.stgit@brijesh-build-machine> <148884286215.29188.1084675072356724555.stgit@brijesh-build-machine> Cc: "jordan.l.justen@intel.com" , "edk2-devel@ml01.01.org" , "Singh, Brijesh" , "Lendacky, Thomas" From: Laszlo Ersek Message-ID: Date: Wed, 8 Mar 2017 16:19:35 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Wed, 08 Mar 2017 15:20:01 +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: Wed, 08 Mar 2017 15:20:09 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/08/17 15:56, Duran, Leo wrote: > For libraries, I've noticed this usage pattern: > > INF File: > - Name: .inf > - Path: xxxPkg/Library//.inf > > INCLUDE File: > - Name: .h > - Path: xxxPkg/Include/Library/.h Correct. What I described earlier was the relationship between and . Namely, is specific to the library instance, and if you have multiple instances for the same library class, then there is a convention for composing from : := Laszlo > > Leo > >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Tuesday, March 07, 2017 4:08 PM >> To: Brijesh Singh >> Cc: jordan.l.justen@intel.com; edk2-devel@ml01.01.org; Duran, Leo >> ; Singh, Brijesh ; Lendacky, >> Thomas >> Subject: Re: [edk2] [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV >> helper library >> >> On 03/07/17 20:14, Brijesh Singh wrote: >>> On Tue, Mar 7, 2017 at 11:06 AM, Laszlo Ersek wrote: >>> >>>> 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? >>>> >>>> >>> This version does not have CRLF, I will ensure that nex rev contains CRLF. >>> >>> >>>>> >>>>> 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. >>> >>> >>> Will do. >>> >>> >>>> >>>> >>>> + 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? >>>> >>>> >>> I should be able to remove the inclusion of this header. >>> >>> >>>>> +#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. >>>> >>>> >>> Both are independent functions, SevActive() function returns whether >>> SEV is active or not. Whereas SevInitialize(), calls SevActive() and >>> if SEV is active then it sets the dynamic PtePcdMemoryEncryptionMask. >>> >>> >>> Please also document the return values (with @retval and/or @return), >>>> even though the explanation is likely trivial. >>>> >>>> I will document it. >>> >>> >>> >>>>> +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. >>>> >>>> >>> Will fix it. In general, are we are okay with MemcryptSevLib naming >>> convensio ? >> >> Regarding library naming conventions, we can consider functions, and library >> instance names. >> >> For functions, I tend to prefer a common prefix, although I don't believe this >> is a hard requirement in edk2. >> >> For library instance names, the naming convention seems to be: >> >> LibClassName >> >> Where describes the phases the library instance can be used in (so, >> Base, Dxe, Pei, PeiDxe, and so on), and describes, very tersely, >> the underlying implementation (for example, Null means the library does >> nothing). I think can be omitted if there's only one library >> instance. >> >> >>> If we are okay with that then I was going to prefix all the function >>> with Sev e.g >>> >>> SevInitialize() : this sets the dynamic PCD >>> SevActive() : returns TRUE when SEV is active >>> >>>> +#endif >>>>> +#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. >>>> >>>> >>> Will do it. >>> >>> >>>>> + >>>>> +RETURN_STATUS >>>>> +EFIAPI >>>>> +MemcryptSevInitialize ( >>>>> + VOID >>>>> + ) >>>>> +{ >>>>> + UINT32 EBX; >>>> >>>> Should be Ebx, IMO. >>>> >>>> >>> Will fix it >>> >>>> + UINT64 MeMask = 0; >>>> >>>> Initialization of local variables is forbidden by the edk2 coding style. >>>> >>>> >>> Will fix it. >>> >>> >>>>> + >>>>> + 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 ... >>>> // >>>> >>>> >>> Will fix it. >>> >>> >>>>> + 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 >> <<. >>>> >>>> >>> I will fix it to use LShiftU64() version. >>> >>> >>>> 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. >>>> >>>> >>> So my rational behind creating a new library was to have a flexibilty >>> of adding more SEV specifc functions. The functions which I have mind is: >>> >>> SevChangeMemoryAttributeEncrypted(Address, Length) : set the >>> encryption attribute SevChangeMemoryAttributeDecrypted(Address, >>> Length) : clear the encryption attribute >>> >>> SevChangeMemoryAttribute*() are not implemented in this RFC and I am >>> working to add in next rev. This will be mainly execute in Dxe phase. >>> These functions will be update the pagetable to clear/set encryption >>> masks. It will be mainly used for Dma libraries and possibly >>> QemuVideoDxe (since framebuffer is shared between HV and Guest >> hence >>> we will need to clear the encryption attribute of framebuffer memory). >>> >>> I would potentially add two libraries: >>> >>> * SevBaseLib: it provides SevActive() and SevInitialize() routines >>> which can be called anytime >>> * SevDxeLib: it will provide routines which can be called used by Dxe >>> drivers. >>> >>> Does it make sense to you. I am open to suggestions. >> >> I think SevActive() and SevInitialize() should become part of PlatformPei only >> (if that's possible). >> >> The upcoming PTE massaging functions could become part of the DMA lib >> stuff that you mention (as functions with external linkage), and then you >> could pull the DMA lib into QemuVideoDxe just to make these functions >> available. >> >> Presently the suggested functions don't seem to justify two (or even >> one) new libclass. >> >> Thanks >> Laszlo >> >>> >>> 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? >>>> >>>> Yes SEV will appear active on both Ia32X64 and X64. I have tried >>>> running >>> the OVMF build with both version. >>> >>> >>>> * 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. >>>> >>>> I have tried Ia3264 and X64 but have not tried Ia32. I will try to >>>> and let >>> you know If I find any issues. >>> >>> >>>>> + 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. >>>> >>>> >>> Will do >>> >>>> + >>>>> + 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. >>>> >>>> >>> Will fix it. >>> >>> >>>>> + >>>>> + 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.PcdPteMemoryEncryptionAddressOr >> Mask >>>>> 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 >>>> >>>> >>> >>> I will go through INI-Style file and git setting etc. >>> >>> >>>> >>>>> @@ -509,6 +510,9 @@ >>>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber|64 >>>>> >> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000 >>>>> >>>>> + # Set memory encryption mask >>>>> + >> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressO >>>> rMask|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.PcdPteMemoryEncryptionAddressO >>>> rMask|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. >>>> >>>> >>> Appriciate your quick feedbacks, I will go through each of them and >>> will try to address in v2. >>> >>> >>>> Thanks! >>>> Laszlo >>>> >>> >>> >>> >