From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qk0-x233.google.com (mail-qk0-x233.google.com [IPv6:2607:f8b0:400d:c09::233]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 5155D80429 for ; Fri, 17 Mar 2017 07:08:29 -0700 (PDT) Received: by mail-qk0-x233.google.com with SMTP id p64so65146889qke.1 for ; Fri, 17 Mar 2017 07:08:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=YMuistiZWYgDfKu6ncv7W/2x7GKdrMTpxd66kOINxRg=; b=sGXYD1603OcoOdMUiV99/riMPTpRX2flFpM58vTmUMhOlpkjEd6nb27maN8XYjVDHX Mo+qiwhix9vkYeo/eSait9IezfCmrUQKgXs0r6XkyXOhaNbvKOg5xubIqWghwHBA4cP0 AbXIN7OL8/zLcUOF6Nfz52cMqTjciUe9PoSC4VRqBWLw6/oqXNNrMltPxENioNzfRSzY AkX3WAyzcZx9bzWyCxOniMiHJl72aDs1tHqyCW3EVrYkaBnTe7hKUZXK+4NjaNBSDG4h sR4DBFqZf2l8HFAQh+PIfqn8q98dziNSWclgq+pKSGOIl2zV8HuvOUfdKc46POorF/Mi GMpA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=YMuistiZWYgDfKu6ncv7W/2x7GKdrMTpxd66kOINxRg=; b=cYCKUzA22kygDCiA4aBNE/2YlvX1946LnSvjoCb8gDkApF3OcYnsMt1oF/2KUL2naR A1paPx144BlNkbZ0X1Exm75m4+qLwoVPIu4mJ66AF69gNKjuVJzk65olBpY/HQmxuXfH cqbVXYU2j6uphd/GPxmOGdJHFWV5WJfOrI+c4zb0Dkn1GJhtXqJ7OSs01yWNQbblC7AF bk9S4xHRqdOSD7ECzOAJ5+9iYiqnIMADA6TAbPx3E3GR1ugpon9LeBLen95ib5W/xAb0 Zck1RC1NzmbYWm4v1MjxGobNtAxJUISGQ8uLQwsrhWb/PeXkk03a/S1WuNcMzSfs+/8r NMMQ== X-Gm-Message-State: AFeK/H0x2Hk/DiZsh/kpeuOUEGG8uxtnuCu4ggt63vWUxCPz6Zl4JH9exbFVeppYPh13/GBwIZ/PqmnmOSjJXA== X-Received: by 10.55.60.137 with SMTP id j131mr11117268qka.19.1489759708306; Fri, 17 Mar 2017 07:08:28 -0700 (PDT) MIME-Version: 1.0 Received: by 10.12.182.65 with HTTP; Fri, 17 Mar 2017 07:08:27 -0700 (PDT) In-Reply-To: References: <148884284887.29188.7643544710695103939.stgit@brijesh-build-machine> <148884286215.29188.1084675072356724555.stgit@brijesh-build-machine> From: Brijesh Singh Date: Fri, 17 Mar 2017 09:08:27 -0500 Message-ID: To: Laszlo Ersek Cc: "Justen, Jordan L" , edk2-devel@ml01.01.org, Leo Duran , brijesh.singh@amd.com, Tom Lendacky X-Content-Filtered-By: Mailman/MimeDel 2.1.22 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 14:08:29 -0000 Content-Type: text/plain; charset=UTF-8 On Fri, Mar 17, 2017 at 5:29 AM, Laszlo Ersek wrote: > 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 < > lersek@redhat.com > > > >> 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?) > > Yes that is right. The CPUID detection function will return TRUE only when SEV is enabled on AMD processor. On Intel it will return FALSE. > 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. > > -- Confusion is always the most honest response.