Re: [edk2-rfc] [edk2-devel] RFC: design review for TDVF in OVMF

["Min Xu" <min.m.xu@intel.com>]

On 06/09/2021 3:33 AM, Laszlo wrote:
> (Min Xu got dropped from the CC list for some reason, at *some* point in this
> sub-thread! Not sure when. Re-adding him.)
> 
> Commenting on excerpts:
> 
> On 06/08/21 18:01, James Bottomley wrote:
> 
> > On TdMailbox and TdHob, we already have two SEV pages in the MEMFD and
> > since TDX and SEV is an either/or, could we simply not rename both
> > pages and use them for either boot depending on what CPU type is
> > detected, so we only have two MEMFD pages, not four?
> 
> Great idea, in my opinion.
> 
Agree, it's a good idea.
> 
> > On your slide 13 Question: "Open: How will the QEMU find the metadata
> > location?" can't you just use the mechanism for SEV that's already
> > upstream in both QEMU and OVMF?
> 
> I think I made the same comment, in different words. (Point (12) at
> <https://listman.redhat.com/archives/edk2-devel-archive/2021-
> June/msg00143.html>.)
> 
So my understanding to this solution is that: 
1) GUID-ed structure chain is started from a fixed GPA in ResetVector. 
2) Append a TDX-specific GUID-ed structure in the chain
3) Qemu search the GUID-ed chain from the fixed GPA and find the TDX-specific
    GUID-ed structure based on TDX-specific GUID.
Is the expected process for QEMU?

> 
> > On slide 19, the mucking with the reset vector really worries me
> > because we don't have that much space to play with.  Given that you're
> > starting in 32 bit mode and can thus enter anywhere in the lower 4GB,
> > why not simply use a different and TDX specific entry point?
>
> What's more, we should use a dedicated ResetVector (through a DSC+FDF
> dedicated solely to TDX).
> 
If TDVF has a separate DSC/FDF, then this is not a problem. Let's wait for a
conclusion to the *one binary* solution.
Thanks for your suggestion.
> 
> > On all the Tcg2 changes: what about installing a vTPM driver that
> > simply translates to your MSRs?  That way we can use all the standard
> > TCG code as is?
> 
> I believe I made the same comment in point (20) (see URL above).
> 
I will answer this comments in my later response. Thanks.
> 
> > Slide 41: IOMMU operation.
> 
> That's more like slides 40 and 42, no?
> 
> 
> > The implication is that you only transition to unencrypted memory for
> > DMA during the actual operation,
> 
> Yes, this is the idea behind EDKII_IOMMU_PROTOCOL, which
> OvmfPkg/IoMmuDxe implements (for SEV only, currently).
> 
> 
> > so do I have it correct that the guest writes DMA to encrypted memory,
> > then the iommu marks the region as unencrypted and transforms the
> > memory to be in the clear and then transforms it back after the DMA
> > operation completes?
> 
> Effectively, yes. (Your summary corresponds to a BusMasterRead
> operation.)
> 
In TDX there are the concepts of Private-Memory and Shared-Memory. 
By default the memory are private. Before the DMA operation, the private
memory should be converted to Shared-Memory by setting the S-Bit. Then
VMM and Guest can do the DMA operation.
(The difference is that in TDX even the Shared-Memory is encrypted with 
a shared-key between VMM and Guest. So we call it Shared-Memory)
So all the changes in IOMMU by TDX is just to customize the page 
decryption / encryption primitives in the driver , and the general logic will not
be touched.
> 
> > Given that SEV operates quite happily with always in the clear DMA
> > buffers,
> 
> I don't understand this comment -- is it a statement about SEV as a technology,
> or about OvmfPkg/IoMmuDxe?
> 
> Specifically in the context of OvmfPkg/IoMmuDxe, there is no "always-in-the-
> clear DMA".
> 
> EDKII_IOMMU_PROTOCOL was designed to fit cleanly into the Map(), Unmap(),
> AllocateBuffer(), FreeBuffer() terminology of the UEFI standard
> EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL and EFI_PCI_IO_PROTOCOL. As far as I
> can tell, the original use case for EDKII_IOMMU_PROTOCOL was VT-d on bare
> metal, but the protocol proved a good match for SEV too.
> 
> VIRTIO_DEVICE_PROTOCOL has similar member functions
> (AllocateSharedPages, FreeSharedPages, MapSharedBuffer,
> UnmapSharedBuffer).
> 
> As long as a PCI device driver (or virtio driver) uses these member functions
> judiciously, only "BusMasterCommonBuffer" operations will be backed by long-
> term plaintext (decrypted) pages. One-shot read and write transactions will be
> backed by plaintext (decrypted) pages only as long as necessary.
> 
> The transitions you outline already happen in any plain SEV guest that uses PCI
> DMA or virtio.
> 
> 
> > this seems to have the potential to be a performance problem, but what
> > security does it gain?
>
As I mentioned above, in TDX DMA operation even the Shared-Memory is encrypted
by a shared-key between VMM and Guest. It is not *plain-text* outside VMM&Guest.
>
> We have not experienced performance problems due to this kind of IOMMU
> protocol usage, when booting SEV guests.
> 
In our test, we don't see the performance problem when booting TDX guests.
We do have the performance issue in "Accept Pages". But we have several solutions
to resolve this performance. See below link.
https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-virtual-firmware-design-guide-rev-1.pdf
Section 7.8 Optimization Consideration.
>
> The basic goal was to keep everything as tightly encrypted as possible (as
> permitted by the individual PCI or Virtio driver, through its conservative usage
> of BusMasterCommonBuffer operations).
> 
> I won't claim that it has zero performance impact, but we should remember
> the purpose that firmware serves (namely, "booting an operating system").
> Really -- I don't recall any performance issues. This applies to such virtio
> devices & drivers too that aren't "bootable", such as virtio-gpu-pci
> (VirtioGpuDxe) and virtio-rng-pci (VirtioRngDxe).
> 
> (
> 
>   If you enable verbose logs, OvmfPkg/IoMmuDxe does produce an immense
>   amount of messages (with the express purpose of a human reading
>   through them, and matching up decryption and re-encryption actions --
>   I've done it, likely with some ad-hoc scripts). *This* does slow down
>   the boot considerably (if you actually enable the QEMU debug console),
>   but for a different reason: producing debug logs through the QEMU
>   debug console (IO Port) is very-very costly in a SEV guest. Not just
>   because an IO port trap may be more expensive in a SEV guest, but
>   because SEV does not support REP OUTSB, so every debug character
>   written traps separately, as opposed to every line written. See
>   the following commits:
> 
>   - b6d11d7c4678 ("MdePkg: BaseIoLibIntrinsic (IoLib class) library",
>     2017-04-13),
> 
>   - 97353a9c914d ("OvmfPkg: Update dsc to use IoLib from
>     BaseIoLibIntrinsicSev.inf", 2017-07-10),
> 
>   - 98a4d04e8fda ("MdePkg/BaseIoLibIntrinsic: fix SEV (=unrolled)
>     variants of IoWriteFifoXX()", 2017-09-11),
> 
>   - c09d9571300a ("OvmfPkg: save on I/O port accesses when the debug
>     port is not in use", 2017-11-17).
> 
> )
> 
> From my perspective, I find the changes proposed for OvmfPkg/IoMmuDxe to
> be among the least intrusive of the whole slide deck (after Min Xu confirmed
> that the intent was really only to customize the page decryption / encryption
> primitives in the driver, and to leave the general logic untouched).
> 
> That's not to say that I'm unhappy about this topic being raised. To the
> contrary!
> 
> Thanks
> Laszlo
> 
Thanks
Min