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

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

On June 4, 2021 12:12 AM, Laszlo wrote:
> On 06/03/21 15:51, Yao, Jiewen wrote:
> > Hi, All
> > We plan to do a design review for TDVF in OVMF package.
> >
> >
> > The TDVF Design slides for TinaoCore Design Review Meeting (Jun 11) is
> > now available in blow link:
> > https://edk2.groups.io/g/devel/files/Designs/2021/0611.
> >
> > The Bugzilla is https://bugzilla.tianocore.org/show_bug.cgi?id=3429
> >
> >
> >
> > You can have an offline review first. You comments will be warmly
> > welcomed and we will continuously update the slides based on the
> > feedbacks.
> 
> Resending my earlier comments in this mailing list thread, with the feedback
> inserted at the proper spots that has been given in the off-list thread since
> then:
> 

Continue my comments from here.

> 

> *** Slide 11 -- TDVF Image (1)
> 
> (9) CFV -- Configuration Firmware Volume (VarStore.fdf.inc), containing SB
> keys -- how is this firmware volume populated (at build time)? Is this a
> hexdump?
> 
CFV is populated in post build. We can provide such python scripts to do the
SB keys enrollment.

> ... Back from slide 16: it seems like CFV is a raw hexdump indeed; how is that
> managed when keys change (at build time)?
> 
As I mentioned above, SB keys are enrolled in post build phase. We can provide
a python scripts to add/delete/append the keys.

> (10) This slide (slide 11) basically describes an intrusive reorganization of
> "OvmfPkgX64.fdf". I don't think I can agree to that.
> While confidential computing is important, it is not relevant for many users.
> Even if we don't cause outright regressions for existent setups, the
> maintenance cost of the traditional OVMF platform will skyrocket.
> 
> The big bunch of areas that SEV-ES introduced to MEMFD is already a big
> complication. I'd feel much better if we could isolate all that to a dedicated
> "remote attested boot" firmware platform, and not risk the functionality and
> maintenance of the traditional platform. I think this ties in with my comment
> (1).
Actually in our first version of TDVF, it is a separated dsc/fdf. But when I try to
implement the *one binary*, I have to figure out some way to put our mailbox/tdhob.
I checked the OvmfPkgX64.fdf and mimics what SEV-ES does in MEMFD.
I would wait for a conclusion of the *one binary* and then figure out how to
handle the mailbox/tdhob.
> 
> For example, seeing a configuration firmware volume (CFV) with secure boot
> keys embedded, in the "usual" FDF, will confuse lots of people, me included.
> In the traditional OVMF use case, we use a different method:
> namely OvmfPkg/EnrollDefaultKeys, for "priming" a variable store template,
> in the build environment.

As I mentioned above, the SB keys are enrolled in post-build. The standard build
script:
  build -p OvmfPkg/OvmfPkgX64.dsc -a X64 -t GCC5
Its output is a standard OVMF image (with a clean CFV/VarStore)
If the customers want the SB feature configured, it's up to them to enroll the SB
keys. 
CFV is just a concept in TDVF. From the perspective of Standard OVMF, it is still
the VarStore.

> 
> Edk2 (and PI and UEFI) are definitely flexible enough for accommodating TDX,
> but the existent, traditional OVMF platforms are a bad fit. In my opinion.
> 
> 
> *** Slide 12: TDVF Image (2)
> 
> (11) "Page table should support both 4-level and 5-level page table"
> 
> As a general development strategy, I would suggest building TDX support in
> small, well-isolated layers. 5-level paging is not enabled (has never been
> tested, to my knowledge) with OVMF on QEMU/KVM, regardless of
> confidential computing, for starters. If 5-level paging is a strict requirement
> for TDX, then it arguably needs to be implemented independently of TDX, at
> first. So that the common edk2 architecture be at least testable on
> QEMU/KVM with 5-level paging enabled.
> 
Yes, 5-level paging is a strict requirement for TDX. 
I would wait for the conclusion of the *one binary*.
> 
> *** Slide 13:
> 
> (12) My comment is that the GUID-ed structure chain already starts at a fixed
> GPA (the "AMD SEV option"). Ordering between GUID-ed structures is
> irrelevant, so any TDX-specific structures should be eligible for appending to,
> prepending to, or intermixing with, other (possibly
> SEV-specific) structures. There need not be a separate entry point, just
> different GUIDS.
Yes, we prefer a TDX GUID in ResetVector. In that GUID there is a offset which
points to the actual TDX Metadata blob.
> 
> (13) Regarding "4G-0x20[0x10] is OVMF AP reset vector (used in OVMF
> implementation)" -- I think this is a typo: this "AP reset vector" is
> *not* used in OVMF. To my knowledge, it is a vestige from the UefiCpuPkg
> reset vector. In OVMF, APs are booted via MpInitLib (in multiple firmware
> phases), using INIT-SIPI-SIPI, and the reset vector for the APs, posited
> through those IPIs, is prepared in low RAM.
> 
Thanks Laszlo for explanation. 
> 
> *** Slides 14 through 16:
> 
> I consider these TDVF firmware image internals, implementation details
> -- do whatever you need to do, just don't interfere with existing platforms /
> use cases. See my comment (10) above.
> 
Sure. All the TDVF changes will not  interfere with existing platfomrs/use cases.
> 
> *** Slides 17-21:
> 
> (14) Again, a very big difference from traditional OVMF: APs never enter SEC
> in traditional OVMF. I assume this new functionality is part of TdxStartupLib
> (from slide 18) -- will there be a Null instance of that?

Yes, there is a NULL instance of TdxStartupLib.

> 
> Last week I posted a 43-part patch series to edk2-devel, for separating out
> the dynamic Xen enlightenments from the IA32, IA32X64, X64 platforms, in
> favor of the dedicated OvmfXen platform. TDX seems to bring in
> incomparably more complications than Xen, and the OvmfPkg maintainers
> have found even the Xen complications troublesome in the long term.
> 
> If I had had access to all this information when we first discussed "one
> binary" on the mailing list, I'd have never agreed to "one binary". I'm OK with
> attempting one firmware binary for "confidential computing", but that "one
> platform" cannot be "OvmfPkgX64.dsc".
> 
> Even if I make a comparison with just the "technology" (not the remotely-
> attested deployment) of SEV and SEV-ES, as it is included in
> "OvmfPkgX64.dsc", TDX is hugely more complicated and intrusive than that.
> SEV proved possible to integrate into existing modules, into the existing boot
> flow, maybe through the addition of some new drivers (such as a new
> IOMMU protocol implementation, and some "clever" depexes). But we never
> had to restructure the FD layout, eliminate whole firmware phases, or think
> about multiprocessing in the reset vector or the SEC phase.
> 
> In order to bring an example from the ARM world, please note that platforms
> that use a PEI phase, and platforms that don't, are distinct platforms. In
> ArmVirtPkg, two examples are ArmVirtQemu and ArmVirtQemuKernel. The
> latter does not include the PEI Core.
> 

Thanks Laszlo. I will check the example from the ARM world.

> 
> *** Slides 22 through 34:
> 
> (15) All these extra tasks and complications are perfectly fine, as long as they
> exist peacefully *separately* from the traditional ("legacy") OVMF platforms.
> 
> Honestly, in the virtual world, picking your firmware binary is easy.
> The approach here reminds me of a physical firmware binary that includes
> everything possible from "edk2-platforms/Features/Intel", just so it can be
> deployed to any physical board imaginable. That's not how Intel builds
> physical firmware, right? We have "edk2-platforms/Platform/Intel"
> and "edk2-platforms/Silicon/Intel" with many-many separate DSC files.
> 

I will continue my comments in my next mail. 

Thanks!
Min