Re: [RFC PATCH v1 2/5] OvmfPkg/MemcryptSevLib: Add SEV helper library

[Brijesh Singh <brijesh.ksingh@gmail.com>]

On Fri, Mar 17, 2017 at 5:29 AM, Laszlo Ersek <lersek@redhat.com> wrote:

> On 03/17/17 03:02, Brijesh Singh wrote:
> >
> >
> > On Wed, Mar 8, 2017 at 2:40 AM, Laszlo Ersek <lersek@redhat.com
> > <mailto:lersek@redhat.com>> wrote:
> >
> >     On 03/07/17 23:36, Brijesh Singh wrote:
> >     >
> >     >
> >     > On Tue, Mar 7, 2017 at 4:08 PM, Laszlo Ersek <lersek@redhat.com
> <mailto:lersek@redhat.com>
> >     > <mailto:lersek@redhat.com <mailto:lersek@redhat.com>>> wrote:
> >     >
> >     >     On 03/07/17 20:14, Brijesh Singh wrote:
> >     >     > On Tue, Mar 7, 2017 at 11:06 AM, Laszlo Ersek <
> lersek@redhat.com <mailto:lersek@redhat.com>
> >     >     <mailto:lersek@redhat.com <mailto: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.