From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (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 3A8B321CE73F0 for ; Thu, 6 Jul 2017 09:44:10 -0700 (PDT) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga105.jf.intel.com with ESMTP; 06 Jul 2017 09:45:50 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,318,1496127600"; d="scan'208";a="283051951" Received: from dwsu-mobl.amr.corp.intel.com (HELO localhost) ([10.254.57.106]) by fmsmga004.fm.intel.com with ESMTP; 06 Jul 2017 09:45:49 -0700 MIME-Version: 1.0 To: Brijesh Singh , edk2-devel@lists.01.org Message-ID: <149935954954.18473.9057866656909328887@jljusten-skl> From: Jordan Justen In-Reply-To: <2c3b8722-0e91-af09-5d3b-c5751dd53a9f@amd.com> Cc: brijesh.singh@amd.com, Thomas.Lendacky@amd.com, leo.duran@amd.com, Jeff Fan , Liming Gao , Laszlo Ersek , Jiewen Yao , Michael D Kinney References: <1495809845-32472-1-git-send-email-brijesh.singh@amd.com> <2c3b8722-0e91-af09-5d3b-c5751dd53a9f@amd.com> User-Agent: alot/0.5.1 Date: Thu, 06 Jul 2017 09:45:49 -0700 Subject: Re: [PATCH v6 00/17] x86: Secure Encrypted Virtualization (AMD) X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 06 Jul 2017 16:44:10 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 2017-07-05 15:31:20, Brijesh Singh wrote: > Hi Jordan and Laszlo, > = > Ping. > = > It has been a while, Do you have any further feedbacks on this series ? > If you want then I can rebase the patches before you commit into upstream= repos. > = I'm still dissappointed by the APRIORI usage. As I understand it, you are also dissatisfied with this approach and you hope to improve things by somehow hooking into DXE Core. Is that true? If so, can you create a bugzilla regarding this feature? When would you plan to work to address that? I guess with that resolved, you could add an Acked-by from me. In general, it'd also be nice to move the processor features to more generic places, although that may be challenging if the next step is some kind of platform hook from DXE Core. Maybe if the DXE Core calls out to some protocol or signals an event then a driver in UefiCpuPkg could handle the protocol implementation to modify the page tables. -Jordan > = > On 05/26/2017 09:43 AM, Brijesh Singh wrote: > > The patch series provides support for AMD's new Secure Encrypted > > Virtualization (SEV) feature. > > = > > SEV is an extension to the AMD-V architecture which supports running > > multiple VMs under the control of a hypervisor. The SEV feature allows > > the memory contents of a virtual machine (VM) to be transparently encry= pted > > with a key unique to the guest VM. The memory controller contains a > > high performance encryption engine which can be programmed with multiple > > keys for use by a different VMs in the system. The programming and > > management of these keys is handled by the AMD Secure Processor firmware > > which exposes a commands for these tasks. > > = > > SEV guest VMs have the concept of private and shared memory. Private m= emory is > > encrypted with the guest-specific key, while shared memory may be encry= pted > > with hypervisor key. Certain types of memory (namely instruction pages= and > > guest page tables) are always treated as private memory by the hardware. > > For data memory, SEV guest VMs can choose which pages they would like t= o be > > private. The choice is done using the standard CPU page tables using th= e C-bit, > > and is fully controlled by the guest. Due to security reasons all the D= MA > > operations inside the guest must be performed on shared pages (C-bit c= lear). > > Note that since C-bit is only controllable by the guest OS when it is o= perating > > in 64-bit or 32-bit PAE mode, in all other modes the SEV hardware force= s the > > C-bit to a 1. > > = > > The following links provide additional details: > > = > > AMD Memory Encryption whitepaper: > > http://amd-dev.wpengine.netdna-cdn.com/wordpress/media/2013/12/AMD_Memo= ry_Encryption_Whitepaper_v7-Public.pdf > > = > > AMD64 Architecture Programmer's Manual: > > http://support.amd.com/TechDocs/24593.pdf > > SME is section 7.10 > > SEV is section 15.34 > > = > > Secure Encrypted Virutualization Key Management: > > http://support.amd.com/TechDocs/55766_SEV-KM API_Specification.pdf > > = > > KVM Forum Presentation: > > http://www.linux-kvm.org/images/7/74/02x08A-Thomas_Lendacky-AMDs_Virtua= lizatoin_Memory_Encryption_Technology.pdf > > = > > [1] http://marc.info/?l=3Dlinux-mm&m=3D148846752931115&w=3D2 > > = > > --- > > = > > Patch series is based on commit aff463c825a3 > > (Vlv2TbltDevicePkg/FvbRuntimeDxe: correct NumOfLba vararg type in Era= seBlocks()) > > = > > https://github.com/codomania/edk2/tree/v6 > > = > > The patch series is tested with OvmfIa32.dsc, OvmfIa32X64.dsc and OvmfX= 64.dsc. > > Since memory encryption bit is not accessiable when processor is in 32-= bit mode > > hence any DMA access in this mode would cause assert. I have also teste= d the > > suspend and resume path, it seems to be working fine. I still need to w= ork to > > finish adding the SEV Dma support in QemuFwCfgS3Lib package (see TODO). > > = > > Changes since v5: > > - add placeholder gIoMmuAbsentProtocolGuid > > - add PlatformHasIoMmuLib > > - fix indentation > > = > > Changes since v4: > > - decouple IoMmu protocol implementation from AmdSevDxe into a sepera= te > > IoMmuDxe driver. And introduce a placeholder protocol to provide the > > dependency support for the dependent modules. > > - update debug messages to use gEfiCallerBaseName where applicable. > > - fix QemuFwCfgSecLib build errors and simplify SEV support > > - update QemuFwCfgDxeLib to assert when failed to locate IOMMU > > - update comments "host buffer" to " host buffer" > > = > > Changes since v3: > > - update AmdSevDxe driver to produce IOMMU protocol > > - remove BmDmaLib dependency > > - update QemuFwCfgLib to use IOMMU protocol to allocate SEV DMA buffer > > = > > Changes since v2: > > - move memory encryption CPUID and MSR definition into UefiCpuPkg > > - fix the argument order for SUB instruction in ResetVector and add m= ore > > comments > > - update PlatformPei to use BaseMemEncryptSevLib > > - break the overlong comment lines to 79 chars > > - variable aligment and other formating fixes > > - split the SEV DMA support patch for QemuFwCfgLib into multiple patc= hes as > > recommended by Laszlo > > - add AmdSevDxe driver which runs very early in DXE phase and clear t= he C-bit > > from MMIO memory region > > - drop 'QemuVideoDxe: Clear C-bit from framebuffer' patch since AmdSe= vDxe > > driver takes care of clearing the C-bit from MMIO region > > - Verified that Qemu PFLASH works fine with SEV guest, Found a KVM dr= iver issue > > which was causing #PF when PFLASH was enabled. I have submitted pat= ch to > > fix it in upstream http://marc.info/?l=3Dkvm&m=3D149304930814202&w= =3D2 > > = > > Changes since v1: > > - bug fixes in OvmfPkg/ResetVector (pointed by Tom Lendacky) > > - add SEV CPUID and MSR register definition in standard include file > > - remove the MemEncryptLib dependency from PlatformPei. Move AmdSevIn= itialize() > > implementation in local file inside the PlatformPei package > > - rename MemCryptSevLib to MemEncryptSevLib and add functions to set = or > > clear memory encryption attribute on memory region > > - integerate SEV support in BmDmaLib > > - split QemuFwCfgDxePei.c into QemuFwCfgDxe.c and QemuFwCfgPei.c to > > allow building seperate QemuFwCfgLib for Dxe and Pei phase > > (recommended by Laszlo Ersek) > > - add SEV support in QemuFwCfgLib > > - clear the memory encryption attribute from framebuffer memory region > > = > > = > > TODO: > > (Will add these features after basic SEV support patches are accepted i= n upstream) > > - add support for DMA operation in QemuFwCfgS3Lib when SEV is enabled > > - investigate SMM/SMI support > > = > > Cc: Jeff Fan > > Cc: Liming Gao > > Cc: Leo Duran > > Cc: Jordan Justen > > Cc: Laszlo Ersek > > Cc: Leo Duran > > Cc: Jiewen Yao > > Cc: Tom Lendacky > > Contributed-under: TianoCore Contribution Agreement 1.0 > > Signed-off-by: Brijesh Singh > > = > > Brijesh Singh (17): > > UefiCpuPkg: Define AMD Memory Encryption specific CPUID and MSR > > OvmfPkg/ResetVector: Set C-bit when building initial page table > > OvmfPkg: Update dsc to use IoLib from BaseIoLibIntrinsicSev.inf > > OvmfPkg/BaseMemcryptSevLib: Add SEV helper library > > OvmfPkg/PlatformPei: Set memory encryption PCD when SEV is enabled > > OvmfPkg: Add AmdSevDxe driver > > OvmfPkg: Introduce IoMmuAbsent Protocol GUID > > OvmfPkg: Add PlatformHasIoMmuLib > > OvmfPkg: Add IoMmuDxe driver > > OvmfPkg/QemuFwCfgLib: Provide Pei and Dxe specific library > > OvmfPkg/QemuFwCfgLib: Prepare for SEV support > > OvmfPkg/QemuFwCfgLib: Implement SEV internal function for SEC phase > > OvmfPkg/QemuFwCfgLib: Implement SEV internal functions for PEI phase > > OvmfPkg/QemuFwCfgLib: Implement SEV internal function for Dxe phase > > OvmfPkg/QemuFwCfgLib: Add option to dynamic alloc FW_CFG_DMA Access > > OvmfPkg/QemuFwCfgLib: Add SEV support > > OvmfPkg: update PciHostBridgeDxe to use PlatformHasIoMmuLib > > = > > OvmfPkg/OvmfPkg.dec = | 1 + > > OvmfPkg/OvmfPkgIa32.dsc = | 11 +- > > OvmfPkg/OvmfPkgIa32X64.dsc = | 12 +- > > OvmfPkg/OvmfPkgX64.dsc = | 12 +- > > OvmfPkg/OvmfPkgIa32.fdf = | 1 + > > OvmfPkg/OvmfPkgIa32X64.fdf = | 3 + > > OvmfPkg/OvmfPkgX64.fdf = | 3 + > > OvmfPkg/AmdSevDxe/AmdSevDxe.inf = | 43 ++ > > OvmfPkg/IoMmuDxe/IoMmuDxe.inf = | 49 +++ > > OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryptSevLib.inf = | 50 +++ > > OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf = | 37 ++ > > OvmfPkg/Library/QemuFwCfgLib/{QemuFwCfgLib.inf =3D> QemuFwCfgDxeLib.i= nf} | 15 +- > > OvmfPkg/Library/QemuFwCfgLib/{QemuFwCfgLib.inf =3D> QemuFwCfgPeiLib.i= nf} | 9 +- > > OvmfPkg/PlatformPei/PlatformPei.inf = | 3 + > > OvmfPkg/Include/Library/MemEncryptSevLib.h = | 81 ++++ > > OvmfPkg/IoMmuDxe/AmdSevIoMmu.h = | 43 ++ > > OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.h = | 184 ++++++++ > > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h = | 37 ++ > > OvmfPkg/PlatformPei/Platform.h = | 5 + > > UefiCpuPkg/Include/Register/Amd/Cpuid.h = | 162 +++++++ > > UefiCpuPkg/Include/Register/Amd/Fam17Msr.h = | 62 +++ > > UefiCpuPkg/Include/Register/Amd/Msr.h = | 29 ++ > > OvmfPkg/AmdSevDxe/AmdSevDxe.c = | 75 ++++ > > OvmfPkg/IoMmuDxe/AmdSevIoMmu.c = | 459 ++++++++++++++++++++ > > OvmfPkg/IoMmuDxe/IoMmuDxe.c = | 53 +++ > > OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncryptSevLib.c = | 84 ++++ > > OvmfPkg/Library/BaseMemEncryptSevLib/MemEncryptSevLibInternal.c = | 90 ++++ > > OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c = | 84 ++++ > > OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c = | 439 +++++++++++++++++++ > > OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.c = | 32 ++ > > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c = | 230 ++++++++++ > > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c = | 67 ++- > > OvmfPkg/Library/QemuFwCfgLib/{QemuFwCfgPeiDxe.c =3D> QemuFwCfgPei.c} = | 72 ++- > > OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c = | 57 +++ > > OvmfPkg/PlatformPei/AmdSev.c = | 62 +++ > > OvmfPkg/PlatformPei/Platform.c = | 1 + > > OvmfPkg/ResetVector/Ia32/PageTables64.asm = | 70 ++- > > 37 files changed, 2703 insertions(+), 24 deletions(-) > > create mode 100644 OvmfPkg/AmdSevDxe/AmdSevDxe.inf > > create mode 100644 OvmfPkg/IoMmuDxe/IoMmuDxe.inf > > create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/BaseMemEncryp= tSevLib.inf > > create mode 100644 OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoM= muLib.inf > > copy OvmfPkg/Library/QemuFwCfgLib/{QemuFwCfgLib.inf =3D> QemuFwCfgDxe= Lib.inf} (71%) > > rename OvmfPkg/Library/QemuFwCfgLib/{QemuFwCfgLib.inf =3D> QemuFwCfgP= eiLib.inf} (80%) > > create mode 100644 OvmfPkg/Include/Library/MemEncryptSevLib.h > > create mode 100644 OvmfPkg/IoMmuDxe/AmdSevIoMmu.h > > create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMe= mory.h > > create mode 100644 UefiCpuPkg/Include/Register/Amd/Cpuid.h > > create mode 100644 UefiCpuPkg/Include/Register/Amd/Fam17Msr.h > > create mode 100644 UefiCpuPkg/Include/Register/Amd/Msr.h > > create mode 100644 OvmfPkg/AmdSevDxe/AmdSevDxe.c > > create mode 100644 OvmfPkg/IoMmuDxe/AmdSevIoMmu.c > > create mode 100644 OvmfPkg/IoMmuDxe/IoMmuDxe.c > > create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/Ia32/MemEncry= ptSevLib.c > > create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/MemEncryptSev= LibInternal.c > > create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryp= tSevLib.c > > create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMe= mory.c > > create mode 100644 OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoM= muLib.c > > create mode 100644 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c > > rename OvmfPkg/Library/QemuFwCfgLib/{QemuFwCfgPeiDxe.c =3D> QemuFwCfg= Pei.c} (61%) > > create mode 100644 OvmfPkg/PlatformPei/AmdSev.c > >=20