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 8465321A18AAA for ; Mon, 29 May 2017 04:15:21 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id ED52385545; Mon, 29 May 2017 11:16:18 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com ED52385545 Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com ED52385545 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-94.phx2.redhat.com [10.3.116.94]) by smtp.corp.redhat.com (Postfix) with ESMTP id 7E80B18505; Mon, 29 May 2017 11:16:16 +0000 (UTC) To: Jordan Justen , Brijesh Singh , edk2-devel@lists.01.org Cc: Thomas.Lendacky@amd.com, leo.duran@amd.com, Jeff Fan , Liming Gao , Jiewen Yao References: <1495809845-32472-1-git-send-email-brijesh.singh@amd.com> <149583274037.25973.13062338567511386932@jljusten-skl> From: Laszlo Ersek Message-ID: <6ecd0138-454e-6a6e-d034-beaf63466120@redhat.com> Date: Mon, 29 May 2017 13:16:15 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <149583274037.25973.13062338567511386932@jljusten-skl> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Mon, 29 May 2017 11:16:19 +0000 (UTC) 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: Mon, 29 May 2017 11:15:21 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit (looks like I was the one to comment as second reviewer after all :) ) On 05/26/17 23:05, Jordan Justen wrote: > On 2017-05-26 07:43:48, Brijesh Singh wrote: >> Changes since v4: >> - decouple IoMmu protocol implementation from AmdSevDxe into a seperate >> IoMmuDxe driver. And introduce a placeholder protocol to provide the >> dependency support for the dependent modules. > > I think you split IoMmuDxe out from AmdSevDxe based on my feedback > regarding APRIORI, but I don't think this helped. > > Ideally I would like to see one driver named IoMmuDxe that is *not* in > APRIORI. There are two separate goals here: (1) Make sure that any driver that adds MMIO ranges will automatically add those ranges with the C bit cleared in the PTEs, without actually knowing about SEV. (2) Provide an IOMMU protocol for those drivers that explicitly map / unmap memory for DMA(-like) transfers. Goal (2) is covered by the IOMMU protocol, and dependent drivers in category (2) can be nicely ordered against IoMmuDxe with the OR depex (= depend on the IOMMU protocol or the placeholder). Goal (1) is a separate question. For covering goal (1), there are two options: (1a) modify all MMIO-adding drivers individually, to clear the C-bit. (1b) do something general that will affect all MMIO additions at once, without modifying individual drivers. (1a) is ugly and it doesn't scale. For (1b), there are again two possibilities: (1b1) modify the GCD memory space map gDS services, so they handle SEV internally, (1b2) right after we enter the DXE phase (before any MMIO-adding driver gets a chance to be dispatched), clear the C bit for all NonExistent ranges (where MMIO might be added later) and for all currently existing MMIO ranges (which come from the DXE core initialization, according to MMIO memory descriptor HOBs from PEI). I don't think we can express "right after we enter the DXE phase" without an APRIORI entry; DEPEXes don't seem realistic for this. Option (1b1) could still work, but as far as I remember (I could be wrong!), Jiewen didn't like that, and he suggested (1b2) as the alternative. > I think because of QemuFlashFvbServicesRuntimeDxe in APRIORI, we also > need IoMmuDxe in the APRIORI. QemuFlashFvbServicesRuntimeDxe adds the flash range as runtime, uncacheable, system memory to the GCD memory space map (and then allocates it at once). In practice it is used like MMIO, and therefore the driver falls under category (1). Consequently, it should be addressed with: - either (1a) -- clear the C bit manually, - or (1b1) -- modify the gDS services --> automatic C-bit clearing, - or (1b2) -- assign the area with the C-bit pre-cleared. - (or even (2) -- rework the driver to consume the IOMMU protocol explicitly.) Are you suggesting the last option -- rework QemuFlashFvbServicesRuntimeDxe so that it consumes the IOMMU protocol? In my opinion, that option is conceptually not much different from (1a) -- which I don't like --, because it again implements a case-by-case modification of an MMIO driver. Instead, I prefer SEV to be transparent to drivers that don't already do *explicit* DMA. > Hopefully we can figure our how to > remove QemuFlashFvbServicesRuntimeDxe and then be able to remove > IoMmuDxe. The reason why QemuFlashFvbServicesRuntimeDxe is in APRIORI, and the reason why AmdSevDxe is in APRIORI, are different. * QemuFlashFvbServicesRuntimeDxe is in APRIORI -- and that's only when SMM_REQUIRE is FALSE -- because it competes with EmuVariableFvbRuntimeDxe, and QemuFlashFvbServicesRuntimeDxe takes priority. EmuVariableFvbRuntimeDxe is only part of the build when SMM_REQUIRE is false. When SMM_REQUIRE is TRUE, there is no competition, and QemuFlashFvbServicesRuntimeDxe is not in APRIORI. * AmdSevDxe is part of APRIORI because it must pre-clear the C bit for *all* MMIO(-like) drivers. If you move QemuFlashFvbServicesRuntimeDxe out of APRIORI (for example, either by defining SMM_REQUIRE, or by inventing a different serialization vehicle against EmuVariableFvbRuntimeDxe), AmdSevDxe *still* has to remain in APRIORI, because it still must clear the C bit for *all* MMIO(-like) drivers. In my opinion, the current version of the patch set (v6) accurately reflects option (1b2). And option (1b2) requires APRIORI usage. If you dislike that, I think we'll have to go back to another option: (1a), which I dislike; (1b1), which Jiewen dislikes (IIRC); or (2), which I again dislike for all MMIO drivers that don't already use explicit DMA. Thanks Laszlo >> - 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 more >> 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 patches as >> recommended by Laszlo >> - add AmdSevDxe driver which runs very early in DXE phase and clear the C-bit >> from MMIO memory region >> - drop 'QemuVideoDxe: Clear C-bit from framebuffer' patch since AmdSevDxe >> driver takes care of clearing the C-bit from MMIO region >> - Verified that Qemu PFLASH works fine with SEV guest, Found a KVM driver issue >> which was causing #PF when PFLASH was enabled. I have submitted patch to >> fix it in upstream http://marc.info/?l=kvm&m=149304930814202&w=2 >> >> 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 AmdSevInitialize() >> 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 in 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 => QemuFwCfgDxeLib.inf} | 15 +- >> OvmfPkg/Library/QemuFwCfgLib/{QemuFwCfgLib.inf => QemuFwCfgPeiLib.inf} | 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 => 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/BaseMemEncryptSevLib.inf >> create mode 100644 OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.inf >> copy OvmfPkg/Library/QemuFwCfgLib/{QemuFwCfgLib.inf => QemuFwCfgDxeLib.inf} (71%) >> rename OvmfPkg/Library/QemuFwCfgLib/{QemuFwCfgLib.inf => QemuFwCfgPeiLib.inf} (80%) >> create mode 100644 OvmfPkg/Include/Library/MemEncryptSevLib.h >> create mode 100644 OvmfPkg/IoMmuDxe/AmdSevIoMmu.h >> create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.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/MemEncryptSevLib.c >> create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/MemEncryptSevLibInternal.c >> create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/MemEncryptSevLib.c >> create mode 100644 OvmfPkg/Library/BaseMemEncryptSevLib/X64/VirtualMemory.c >> create mode 100644 OvmfPkg/Library/PlatformHasIoMmuLib/PlatformHasIoMmuLib.c >> create mode 100644 OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c >> rename OvmfPkg/Library/QemuFwCfgLib/{QemuFwCfgPeiDxe.c => QemuFwCfgPei.c} (61%) >> create mode 100644 OvmfPkg/PlatformPei/AmdSev.c >> >> -- >> 2.7.4 >>