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 891FA8042F for ; Fri, 17 Mar 2017 03:29:26 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EBB2F64DAA; Fri, 17 Mar 2017 10:29:26 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com EBB2F64DAA Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com EBB2F64DAA Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-70.phx2.redhat.com [10.3.116.70]) by smtp.corp.redhat.com (Postfix) with ESMTP id 681B960F88; Fri, 17 Mar 2017 10:29:25 +0000 (UTC) To: Brijesh Singh References: <148884284887.29188.7643544710695103939.stgit@brijesh-build-machine> <148884286215.29188.1084675072356724555.stgit@brijesh-build-machine> Cc: "Justen, Jordan L" , edk2-devel@ml01.01.org, Leo Duran , brijesh.singh@amd.com, Tom Lendacky From: Laszlo Ersek Message-ID: Date: Fri, 17 Mar 2017 11:29:23 +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.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Fri, 17 Mar 2017 10:29:27 +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.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 17 Mar 2017 10:29:26 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 03/17/17 03:02, Brijesh Singh wrote: > > > On Wed, Mar 8, 2017 at 2:40 AM, Laszlo Ersek > wrote: > > On 03/07/17 23:36, Brijesh Singh wrote: > > > > > > On Tue, Mar 7, 2017 at 4:08 PM, Laszlo Ersek > > >> wrote: > > > > On 03/07/17 20:14, Brijesh Singh wrote: > > > On Tue, Mar 7, 2017 at 11:06 AM, Laszlo Ersek > > >> wrote: > > 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. > > > > > > > > I think I should be able to accomate SevInitialize() and SevActive() > > function inside the PlatformPei. I will drop MemcryptSevLib library in > > next rev. > > > > I will go with your idea for adding PTE massaging function directly > > inside the DMA library and will link that into QemuVideoDxe. > > > > Only part which I have not yet figured out, how to deal with Qemu > > FW_CFG DMA support, I believe some of FW_CFG DMA read and write > > happens fairly early (PEI stage). > > That's right, off the top of my head, minimally PlatformPei uses fw_cfg > heavily during PEI. > > > The PTE massaging code may need to > > allocate memory and not sure how to allocate dynamic memory in early stages. > > Any pointers ? > > You can use MemoryAllocationLib functions for that (such as > AllocatePool() and AllocatePages()). The OVMF DSC files resolve the lib > class for the PEI phase like this: > > > MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf > > and the PeiMemoryAllocationLib instance maps those functions to the PEI > services. > > A few important things about this: > > - AllocatePool() works up to only ~64KB in size, and the allocation is > backed by a new HOB. Generally speaking, the HOB may be moved to a > different spot in memory before entering the DXE phase, so pointers > returned by such AllocatePool() calls (in PEI) are not safe to > dereference in the DXE phase. > > - FreePool() does nothing, the allocated memory (the HOB, see above) is > only released when the guest OS starts (and it drops all boot services > data type memory). > > - AllocatePages() works as it says on the tin, and the pointer returned > by it is safe to dereference in DXE. > > > > I took a stab at implementing SEV specific DMA hooks into QemuFwCfgLib. > But I found that QemuFWCfgLib is used in both PEI and Dxe phases. > It makes things interesting, in SEV guest we can perform DMA operation only > when processor is either in 32-bit PAE or long mode (mainly because C-bit > is not accessiable in 32 or 16-bit mode). It limits us to using OvmfX64.dsc. > > Ideally, I would prefer to support both OvmfPkgIa32X64.dsc and > OvmfPkgX64.dsc. > In my code browsing so far I have found that only QemuFwCfgLib does DMA > operations in PEI phase and other packages perform the DMA operation in > DXE phase. > If we can somehow manage to not require the DMA support in PEI phase then we > should be able to support both OvmfPkIa32X64.dsc and OvmfPkgX64.dsc. > How about defaulting to I/O operations in PEI stage for SEV guest ? I suggest the following: (1) Introduce an internal API to "QemuFwCfgLibInternal.h", called InternalQemuFwCfgSevIsEnabled(). (2) Implement InternalQemuFwCfgSevIsEnabled() in "QemuFwCfgSec.c" without using global variables (i.e., with a CPUID on each call, on AMD processors, and return constant FALSE on Intel processors). (3) Create two copies of "QemuFwCfgPeiDxe.c", using the following names: QemuFwCfgPei.c, QemuFwCfgDxe.c, and then delete the original. (4) Accordingly, create two copies of "QemuFwCfgLib.inf", QemuFwCfgPeiLib.inf and QemuFwCfgDxeLib.inf, then delete the original. Don't forget to update the FILE_GUID defines. Additionally, update the library client module type restrictions in each (near LIBRARY_CLASS). (5) In QemuFwCfgDxeLib.inf / QemuFwCfgDxe.c, the constructor function should detect SEV, and store the result in a global variable. (As far as I remember, this detection should only happen on AMD processors, on Intel processors, the CPUID should not even be attempted -- is that right?) The InternalQemuFwCfgSevIsEnabled() function should be implemented to return the global variable. (6) In QemuFwCfgPeiLib.inf / QemuFwCfgPei.c, InternalQemuFwCfgSevIsEnabled() should be implemented the same way, but the constructor function should also avoid setting mQemuFwCfgDmaSupported to TRUE if SEV is detected. (7) In "QemuFwCfgLib.c", the InternalQemuFwCfgDmaBytes() function should use a bounce buffer if InternalQemuFwCfgSevIsEnabled() returns TRUE. (Unfortunately, InternalQemuFwCfgDmaBytes() is not capable of returning an error, so if the bounce buffer allocation fails, you'll have to call ASSERT_EFI_ERROR (Status), and then an explicit CpuDeadLoop() too.) Note that skip operations never need a bounce buffer. The end result is that PEIMs will fall back to "IO only" if SEV is enabled (*), while DXE clients will use bounce buffers with DMA if SEV is enabled. (*) It's OK to use less performant solutions in PEI. > I do > understand > that QEMU prefers us to use DMA interface for FwCfg write. ... It's also OK to use less functional solutions in PEI. Normally, no fw_cfg write occurs in the PEI phase. Unfortunately, during S3 resume, fw_cfg writes (with DMA) do occur in PEI, performed by the stashed BootScriptExecutorDxe driver. And, in the S3 boot script, you cannot allocate memory (for bounce buffering) *dynamically*. However, in the S3 boot script, you cannot allocate memory dynamically for any other purpose either! Which is why QemuFwCfgS3Lib already pre-allocates reserved scratch space anyway, for the boot script opcodes (and QEMU) to operate upon. Thus, it should be possible to customize that too -- find the AllocateReservedPool() call in "QemuFwCfgS3Dxe.c", and make it allocate non-encrypted full pages instead, if SEV is enabled (you can call CPUID right there, on AMD processors). This way, whenever a DXE driver saves fw_cfg DMA writes into the ACPI S3 boot script, to be run on S3 resume, the scratch space that it allocates in advance (for both the DMA control struct and the data to transfer), via QemuFwCfgS3CallWhenBootScriptReady(), will be plain-text. > > Additionally, I found that some FwCfg DMA access happens before > PublishPeiMemory() > hence AllocatePages() was failing to allocate the bounce buffer for SEV DMA. > I was thinking that for SEV guest if we get a request to perform smaller > reads or writes > (maybe < 64 bytes) then silently fallback to IO else perform DMA operations. This should not be necessary with the above approach. (In general, AllocatePages() should not be used for temporary purposes in PEI (for bounce buffering or anything else), because those pages will be leaked for the rest of the firmware (unless something in DXE explicitly frees them up).) Thanks! Laszlo > > Thoughts ? > > -Brijesh > > - FreePages() however is again a no-op, it practically leaks the memory, > > and only the guest OS will be able to release it (see FreePool() above). > One workaround for this could be to stash the address of the PEI-phase > allocation in a GUID HOB or a PCD, and then let some DXE driver in the > DXE phase release the memory with gBS->FreePages(). I'm not sure though > if this complexity is worth it. > > - Note that it is PlatformPei itself that installs the permanent PEI > RAM. Before that happens, PEIMs (including PlatformPei itself) can only > allocate memory from the temporary SEC/PEI heap, which is very very > small, and only AllocatePool() would work at that point (AllocatePages() > wouldn't). However, if you place the AllocatePages() function call after > PublishPeiMemory(), then things should work. > > As far as I can see, you added MemcryptSevInitialize() to the end of > InitializePlatform(); allocating pages at that point should be fine. > > - During S3 resume, a different (pre-reserved) memory area is used as > permanent PEI RAM, which is quite smaller than the one used during > normal boot. It means that, if you need a lot of memory for setting up > SEV during S3 resume, AllocatePages() might run out of memory, and we > might have to resize the pre-reservation mentioned above. > > - If you could reasonably bound the allocation size with a constant, it > might be simplest to use static arrays / variables. Those would be > dropped as soon as the PEI phase was exited. As one quirk however, you > should not rely on such variables being zero-initialized during S3 > resume. > > Thanks > Laszlo > > > > > -- > Confusion is always the most honest response.