thank you Laszlo. Your feedback is received. I am waiting for comment from other people.
thank you! Yao, Jiewen > 在 2021年6月4日，下午6:11，Laszlo Ersek <ler...@redhat.com> 写道： > > On 06/04/21 01:19, Yao, Jiewen wrote: >> Hi Laszlo. >> >> To clarify your "one binary" feedback below, do you mean you suggest A) >> create a separate DSC (for example OvmfPkg/ConfidentialComputing.dsc) for a >> full solution including AMD SEC + Intel TDX + NonConfidentialComputing? >> Or B) to create a standalone DSC for Intel TDX (for example, create a >> OvmfPkg/IntelTdx/IntelTdxXS64.dsc) ? >> >> To me, A) does not change too much, we just create another DSC file - that >> is it. >> Then the original OvmfPkgX64.dsc will only be used for POC/Testing purpose. >> It does not provide any security guarantee. >> (The threat model is: we don't trust VMM. Without attestation, you don't >> know if VMM modified the OVMF.) >> >> I don't know how "simply" it means. To enable TDX to make it work is not a >> simple work. >> Some architecture changes are mandatory, such as reset vector, IO/MMIO >> access, #VE handler, IOMMU based shared memory access, etc. I think AMD SEV >> already did those. > > I mean option (B). Create a completely separate DSC+FDF for Intel TDX. > > In my mind, there are two (very high level) stages for developing the > "Confidential Computing with TDX" feature in edk2. > > Stage 1: allow a guest to run in a TDX domain. "Guest owner" and "Cloud > owner" are *not* separate concepts in this stage. > > Stage 2: enable such a guest to be deployed remotely to an untrusted > cloud, and ensure its integrity via remote attestation. > > > Stage 1 is *hugely different* between AMD SEV* technologies and Intel > TDX. That's why we need, in my opinion, a separate DSC+FDF for Intel TDX > right from the start. This does not necessarily mean that we need to > duplicate every single module (library, PEIM, DXE driver, etc) for Intel > TDX. Wherever it makes sense, and the changes are not very intrusive or > wasteful (considering binary code that becomes "dead" on in a TDX > guest), we can modify (extend) existent modules in-place. > > Stage 1 is complete for AMD SEV and AMD SEV-ES. AMD SEV-SNP is in > progress. These AMD SEV* technologies have been possible to integrate > (thus far) into the existing "OvmfPkg/OvmfPkgX64.dsc" platform. But > Intel TDX is very different from that, even in Stage 1. > > > Stage 2 is far out in the future, for Intel TDX. I have no idea about > it, but whatever it's going to look like, it will be based on Stage 1. > > The "OvmfPkg/AmdSev/AmdSevX64.dsc" platform is *one approach* for > implementing Stage 2. And this platform utilizes AMD SEV* technologies > only (thus far). *If* and *how much* the approach of > "OvmfPkg/AmdSev/AmdSevX64.dsc" will apply to Intel TDX -- once Stage 1 > of Intel TDX is complete --, I cannot predict. > > > The underlying physical hardware features are completely different > between AMD SEV* and Intel TDX, as much as I can tell. It makes zero > sense to me to start from a unified perspective, and shoehorn everything > possible (and impossible) into a common blob and framework. That > approach has never *ever* worked for me, not even when I started working > on the virtio drivers for OVMF 9 years ago. I extracted VirtioLib only > when I worked on the second virtio driver -- that is, when I was about > to have *working code* for two *distinct* virtio devices, and it was > possible to identify commonalities between the drivers, and to extract > those commonalities. The fact that I knew, in advance, that my "end > goal" was the same with these devices, namely to "boot from them", made > no difference whatsoever. I still I had to start implementing them in > separate sandboxes. Soon enough, VirtioLib emerged, and later > VIRTIO_DEVICE_PROTOCOL emerged even. > > I'm 100% incapable of dealing with a top-down approach here. Only > bottom-up works for me. > > > Most importantly, the OvmfPkgIa32, OvmfPkgIa32X64, OvmfPkgX64 platforms > must be kept regression-free, and preferably their complexity should be > kept manageable for maintenance too. These platforms are *entirely > irrelevant* for Stage 2 (regardless of underlying security technology). > They *happen* to be relevant for Stage 1 of AMD SEV*, purely because SEV > proved possible to integrate into them, in very small, well isolated, > surgical advances, without upsetting everything. But I can tell upfront > that Intel TDX is way more intrusive than anything I've seen thus far in > AMD SEV*. So I want Intel TDX to exist in a brand new platform, even as > Stage 1. And, to reiterate, just because Confidential Computing is the > new hot thing, the use cases for OvmfPkgIa32, OvmfPkgIa32X64, OvmfPkgX64 > do not disappear. Regressing them, or making them unmaintainable due to > skyrocketing complexity, is not acceptable. > > Thanks > Laszlo > >> >> =================== >> My point is that the "one binary" that the slide deck refers to (i.e., >> OvmfPkgX64.dsc) might prove OK for utilizing the Intel TDX *technology* >> in itself. Simply enabling OVMF + a guest OS to boot in a TDX domain. >> >> But "OvmfPkgX64.dsc" is *not* the "one binary" that is suitable for >> remote attestation, which has a much broader scope, involving multiple >> computers, networking, deployment, guest-owner/host-owner separation, >> whatever. For the latter, we needed a separate platform anyway, even >> with only SEV in mind; that's why "OvmfPkg/AmdSev/AmdSevX64.dsc" exists. >> =================== >> >> Thank you >> Yao Jiewen >> >>> -----Original Message----- >>> From: r...@edk2.groups.io <r...@edk2.groups.io> On Behalf Of Laszlo Ersek >>> Sent: Friday, June 4, 2021 12:12 AM >>> To: Yao, Jiewen <jiewen....@intel.com>; r...@edk2.groups.io; >>> devel@edk2.groups.io >>> Cc: j...@linux.ibm.com; Brijesh Singh <brijesh.si...@amd.com>; Tom >>> Lendacky <thomas.lenda...@amd.com>; erdemak...@google.com; >>> c...@microsoft.com; bret.barke...@microsoft.com; Jon Lange >>> <jla...@microsoft.com>; Karen Noel <kn...@redhat.com>; Paolo Bonzini >>> <pbonz...@redhat.com>; Nathaniel McCallum <npmccal...@redhat.com>; >>> Dr. David Alan Gilbert <dgilb...@redhat.com>; Ademar de Souza Reis Jr. >>> <ar...@redhat.com> >>> Subject: Re: [edk2-rfc] [edk2-devel] RFC: design review for TDVF in OVMF >>> >>>> 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: >>> >>> >>> *** Slides 4, 6, 7: the "one binary requirement". >>> >>> (1) The presentation refers to "OvmfPkgX64.dsc" as "the one" on slide#4, >>> but then the explanation for the requirement, given on slide 7, speaks >>> about "common attestation interface". >>> >>> I think we have a misunderstanding here. The "OvmfPkgX64.dsc" platform >>> indeed contains SEV, SEV-ES, and (in the future, will contain) SEV-SNP >>> support. In that sense, adding TDX support to the same platform should >>> be (hopefully) possible, at the cost of ugly gymnastics in the reset >>> vector. >>> >>> But "OvmfPkgX64.dsc" is *already* different from the remotely attested >>> OVMF platform, namely "OvmfPkg/AmdSev/AmdSevX64.dsc". >>> >>> The latter has some additional modules (secret PEIM and DXE driver), it >>> has the Grub binary built in, and -- probably most importantly -- it >>> trusts host-originated information less than "OvmfPkgX64.dsc". >>> >>> For example, "OvmfPkg/AmdSev/AmdSevX64.dsc" has a dedicated >>> PlatformBootManagerLib instance, one that ignores the non-volatile UEFI >>> variables Boot#### and BootOrder, and ignores (thus far) the fw_cfg >>> originated kernel/initrd/cmdline as well. >>> >>> It remains an "area of research" to see what else should be removed from >>> the traditional host-guest integration (which integration is usually >>> desirable for management and convenience), in the remotely-attested boot >>> scenario. See e.g. >>> <https://bugzilla.tianocore.org/show_bug.cgi?id=3180>. >>> >>> My point is that the "one binary" that the slide deck refers to (i.e., >>> OvmfPkgX64.dsc) might prove OK for utilizing the Intel TDX *technology* >>> in itself. Simply enabling OVMF + a guest OS to boot in a TDX domain. >>> >>> But "OvmfPkgX64.dsc" is *not* the "one binary" that is suitable for >>> remote attestation, which has a much broader scope, involving multiple >>> computers, networking, deployment, guest-owner/host-owner separation, >>> whatever. For the latter, we needed a separate platform anyway, even >>> with only SEV in mind; that's why "OvmfPkg/AmdSev/AmdSevX64.dsc" >>> exists. >>> >>> >>> *** Slides 8-9: general boot flow -- TDVF; TDVF Flow >>> >>> I'm likely missing a big bunch of background here, so just a few >>> questions: >>> >>> (2) Not sure what RTMR is, but it's associated with "Enable TrustedBoot" >>> -- so is a virtual TPM a hard requirement? >>> >>> ... Back from slide 10: "TCG measurement and event log framework w/o >>> TPM" -- that's curious. >>> >>> [response from Dave Gilbert:] >>>> My reading of this is that the RTMR (and another set of similar >>>> registers) are a TDX thing that is like the PCRs from a TPM but >>>> without the rest of the TPM features; so you can do the one-way >>>> measurement into the RTMRs just like you do into a TPM PCR, and the >>>> measurements pop out somewhere in the TDX quote. Just like a TPM you >>>> need the event log to make any sense of how the final hashed value >>>> supposedly got to where it did. >>> >>> [response from Erdem Aktas:] >>>> +1 to David on this. TDX provides 2 kinds of measurement registers: >>>> MRTDs and RTMRs >>>> >>> (https://software.intel.com/content/dam/develop/external/us/en/docume >>> nts/tdx-module-1eas-v0.85.039.pdf >>>> section 10.1.2) . MRTDs are build-time measurement registers which are >>>> updated when TDX is being created. Once TDX is finalized (before the >>>> first run), the MRTDs are finalized and cannot be updated anymore. On >>>> the other hand, while the TD is running, TD can extend RTMRs through >>>> TDCALLs which will provide TPM PCR kind of capabilities. >>> >>> ... Back from slide 43: feel free to skip this now; I will comment in >>> more detail below. >>> >>> (3) Prepare AcpiTable -- OVMF fetches ACPI tables from QEMU; is this a >>> new (firmware originated) table that is installed in addition, or does >>> it replace QEMU's tables? >>> >>> ... Ignore for now, will comment on the MADT stuff later. >>> >>> (4) Does DMA management mean a different IOMMU protocol? That is >>> going >>> to conflict with the SEV IOMMU protocol. Depexes in OVMF expect one or >>> zero IOMMU protocols to be present. >>> >>> ... Back from slide 40: feel free to skip this now; I'll comment on this >>> separately, below. >>> >>> (5) Enumerate VirtIO -- virtio enumeration is PCI based on x86. But I >>> see no mention of PCI. If you mean VirtioMmioDeviceLib, that's no good, >>> because it does not support virtio-1.0, only virtio-0.9.5. >>> >>> ... Back from slide 42: I got my answer to this on slide 42, so don't >>> worry about this point. >>> >>> (6) The PEI phase is skipped as a whole. I don't see how that can be >>> reasonably brought together with either "OvmfPkgX64.dsc" or >>> "OvmfPkg/AmdSev/AmdSevX64.dsc". I guess you can always modify SEC to >>> jump into DXE directly, but then why keep the PEI core and a bunch of >>> PEIMs in the firmware binary? >>> >>> Also touched on by slide 9, TDVF Flow -- PEI is eliminated but SEC >>> becomes more heavy-weight. >>> >>> Wouldn't this deserve a dedicated, separate platform DSC? The >>> 8-bit/32-bit branching at the front of the reset vector is a smaller >>> complication in comparison. >>> >>> Slide 6 references the mailing list archive: >>> >>> https://edk2.groups.io/g/devel/topic/81969494#74319 >>> >>> and in that message, I wrote: >>> >>> I'm doubtful that this is a unique problem ("just fix the reset >>> vector") the likes of which will supposedly never return during the >>> integration of SEV and TDX >>> >>> See also: >>> >>> https://listman.redhat.com/archives/edk2-devel-archive/2021- >>> April/msg00784.html >>> >>> where I said: >>> >>> It's not lost on me that we're talking about ~3 instructions. >>> >>> Let's keep a close eye on further "polymorphisms" that would require >>> hacks. >>> >>> The first 9 slides in the presentation introduce much-much more >>> intrusive problems than the "polymorphism" of the reset vector. Would I >>> be correct to say that my concern in the above messages was right? I >>> think I was only given a fraction of the necessary information when I >>> was asked "do you agree 'one binary' is viable". >>> >>> [response from Erdem Aktas:] >>>> Let's not worry about this for now. We want the one binary solution >>>> for practical reasons and also for simplicity. In the end, we want to >>>> do what is right and good for everyone. >>>> Those are legit concerns and I think what Intel is trying to do (sorry >>>> for mind reading) is to discuss all those concerns and questions to >>>> make the right decision. I really appreciate their effort on preparing >>>> those slides and bringing it to the community to review. >>>> >>>> I will also read your comments more carefully and provide my thoughts >>>> on them. >>>> Sorry for being a little slow on this. >>> >>> >>> *** Slide 10 -- Key impact to firmware >>> >>> (7) "CPUs are left running in long mode after exiting firmware" -- what >>> kind of "pen" are we talking about? Does a HLT loop still work? >>> >>> (8) "I/O, MMIO, MSR accesses are different" -- those are implemented by >>> low-level libraries in edk2; how can they be configured dynamically? >>> >>> ... Back from slide 53: I'll comment on slide 53 separately; ignore >>> this. >>> >>> >>> *** 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? >>> >>> ... Back from slide 16: it seems like CFV is a raw hexdump indeed; how >>> is that managed when keys change (at build time)? >>> >>> (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). >>> >>> 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. >>> >>> 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. >>> >>> >>> *** 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. >>> >>> (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. >>> >>> >>> *** 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. >>> >>> >>> *** 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? >>> >>> 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. >>> >>> >>> *** 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. >>> >>> >>> *** Slide 35-36: DXE phase >>> >>> (16) "Some DXE Drivers not allowed to load/start in Td guest -- Network >>> stack, RNG, ..." >>> >>> Same comment as (several times) above. The Linuxboot project is a good >>> example for eliminating cruft from DXEFV (in fact, for eliminating most >>> of the DXE phase). In a TDX environment, why include drivers in the >>> firmware binary that are never used? Meanwhile, DXEFV in OVMF grows by >>> a >>> MB every 1.5 years or so. Again, remove these drivers from the DSC/FDF >>> then, and it needs to be a separate platform. >>> >>> (17) "Other DXE Phase drivers -- [...] AcpiPlatformDxe" >>> >>> I'm not sure what this section is supposed to mean. Other DXE phase >>> drivers included, or excluded? Without AcpiPlatformDxe, the guest OS >>> will not see QEMU's ACPI content, and will almost certainly malfunction. >>> >>> ... Back from slide 48: ignore this for now, I'll comment in more detail >>> later. >>> >>> >>> *** Slide 37: DXE Core >>> >>> (18) says "SMM is not supported in Td guest" -- how is the variable >>> store protected from direct hardware (pflash) access from the guest OS? >>> Without SMM, the guest OS need not go through gRT->SetVariable() to >>> update authenticated non-volatile UEFI variables, and that undermines >>> Secure Boot. >>> >>> Note that, while SEV-ES has the same limitation wrt. SMM, the >>> "OvmfPkg/AmdSev/AmdSevX64.dsc" platform doesn't even include the >>> Secure >>> Boot firmware feature. For another example, the OVMF firmware binary in >>> RHEL that's going to support SEV-ES is such a build of "OvmfPkgX64.dsc" >>> that does not include the Secure Boot feature either. >>> >>> But in this TDX slide deck, Secure Boot certificates are embedded in the >>> CFV (configuration firmware volume) -- see slide 11 and slide 16 --, >>> which suggests that this platform does want secure boot. >>> >>> ... Back from slide 48: I'm going to make *additional* comments on this, >>> when I'm at slide 48, too. >>> >>> The rest of this slide (slide 37) looks reasonable (generic DXE Core >>> changes -- possibly PI spec changes too). >>> >>> >>> *** Slides 38 through 39: >>> >>> These seem reasonable (TdxDxe assumes some responsibilities of >>> OvmfPkg/PlatformPei) >>> >>> >>> *** Slides 40 through 42: >>> >>> *If* you really can implement TDX support for the IOMMU driver *this* >>> surgically, then I'm OK with it. The general logic in the IOMMU driver >>> was truly difficult to write, and I'd be seriously concerned if those >>> parts would have to be modified. Customizing just the page >>> encryption/decryption primitives for TDX vs. SEV is OK. >>> >>> >>> *** Slides 43 through 47: >>> >>> (19) Slide 46 and slide 47 are almost identical. Please consolidate them >>> into a single slide. >>> >>> (20) the TPM2 infrastructure in edk2 is baroque (over-engineered), in my >>> opinion. It has so many layers that I can never keep them in mind. When >>> we added TPM support to OVMF, I required commit messages that would >>> help >>> us recall the layering. In particular, please refer to commit >>> 0c0a50d6b3ff ("OvmfPkg: include Tcg2Dxe module", 2018-03-09). Here's an >>> excerpt: >>> >>> TPM 2 consumer driver >>> | >>> v >>> Tpm2DeviceLib class: Tpm2DeviceLibTcg2 instance >>> | >>> v >>> TCG2 protocol interface >>> | >>> v >>> TCG2 protocol provider: Tcg2Dxe.inf driver >>> | >>> v >>> Tpm2DeviceLib class: Tpm2DeviceLibRouter instance >>> | >>> v >>> NULL class: Tpm2InstanceLibDTpm instance >>> (via earlier registration) >>> | >>> v >>> TPM2 chip (actual hardware) >>> >>> The slide deck says that EFI_TD_PROTOCOL is supposed to reuse the >>> EFI_TCG2_PROTOCOL definition. If that's the case, then why don't we push >>> the TDX specifics (more or less: the replacement of PCRs with RTMR) down >>> to the lowest possible level? >>> >>> Can we have "Tpm2InstanceLibTdxRtmr", plugged into the same Tcg2Dxe.inf >>> driver? >>> >>> If not, can we have a new TdTcg2Dxe.inf driver, but make it so that it >>> install the same protocol as before (EFI_TCG2_PROTOCOL -- same old >>> protocol GUID)? Then DxeTpmMeasurementLib doesn't have to change. >>> >>> As long as there is *at most* one EFI_TCG2_PROTOCOL instance published >>> in the protocol database, DxeTpmMeasurementLib should be fine. In SEV* >>> guests, the standard Tcg2Dxe driver provides that protocol. In TDX >>> guests, TdTcg2Dxe.inf should provide the protocol. Arbitration between >>> the two can be implemented with the pattern seen in the following >>> commits: >>> >>> 1 05db0948cc60 EmbeddedPkg: introduce EDKII Platform Has ACPI GUID >>> 2 786f476323a6 EmbeddedPkg: introduce PlatformHasAcpiLib >>> 3 65a69b214840 EmbeddedPkg: introduce EDKII Platform Has Device Tree >>> GUID >>> 4 2558bfe3e907 ArmVirtPkg: add PlatformHasAcpiDtDxe >>> >>> The basic idea is that Tcg2Dxe can have a depex on a new "running in >>> SEV" protocol GUID, and TdTcg2Dxe can have a depex on a new "running in >>> TDX" protocol GUID. A separate platform driver can install the proper >>> GUID -- possibly *neither* of those GUIDs. >>> >>> And, we don't have to change the depex section of >>> "SecurityPkg/Tcg/Tcg2Dxe/Tcg2Dxe.inf" for this; we can implement a >>> library instance with an empty constructor, but a non-empty depex, and >>> then hook this lib instance into Tcg2Dxe.inf via module scope NULL lib >>> class override in the DSC file. Basically we could forcibly restrict >>> Tcg2Dxe's DEPEX by making it inherit the new DEPEX from the library. >>> >>> >>> *** Slide 48: DXE Phase -- Other Modules >>> >>> Regarding IncompatiblePciDeviceSupportDxe: the proposed update sounds >>> plausible and simple enough. >>> >>> (21) "AcpiPlatformDxe: Support for MADT/ACPI addition to report Td >>> Mailbox entry" >>> >>> Firmware-owned tables must not be installed from this driver. >>> >>> Please refer to my "Xen removal" patch set again, for >>> <https://bugzilla.tianocore.org/show_bug.cgi?id=2122>, which I mention >>> above in point (14). As part of the Xen removal, the AcpiPlatformDxe >>> driver in OvmfPkg is significantly trimmed: all unused (dead) cruft is >>> removed, including any ACPI table templates that are built into the >>> firmware. >>> >>> OvmfPkg/AcpiPlatformDxe is responsible *solely* for implementing the >>> client side of the QEMU ACPI linker/loader. >>> >>> If you need to prepare & install different ACPI tables, please do it >>> elsewhere, in another DXE driver. A bunch of other firmware modules do >>> that (NFIT, IBFT, BGRT, ...). >>> >>> For example, the OvmfPkg/TdxDxe DXE_DRIVER is supposed to be launched >>> early in the DXE phase, via APRIORI section -- please consider >>> registering a protocol notify in that driver, for >>> EFI_ACPI_TABLE_PROTOCOL, and when it becomes available, install >>> whatever >>> *extra* tables you need. >>> >>> Note that if you need to *customize* an ACPI table that QEMU already >>> provides, then you will have to modify the ACPI generator on the QEMU >>> side. It is a design tenet between QEMU and OVMF that OVMF include no >>> business logic for either parsing or fixing up ACPI tables that QEMU >>> provides. AcpiPlatformDxe contains the minimum (which is already a whole >>> lot, unfortunately) that's necessary for implementing the QEMU ACPI >>> linker/loader client in the UEFI environment. >>> >>> The slide deck mentions MADT, which is also known as the "APIC" table -- >>> and indeed, QEMU does provide that. (See acpi_build_madt() >>> [hw/i386/acpi-common.c].) So if TDX needs MADT customizations, that >>> should go into QEMU. >>> >>> (22) EmuVariableFvbRuntimeDxe >>> >>> Ouch, this is an unpleasant surprise. >>> >>> First, if you know for a fact that pflash is not part of the *board* in >>> any TDX setup, then pulling >>> >>> OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf >>> >>> into the firmware platform is useless, as it is mutually exclusive with >>> >>> OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.inf >>> >>> (via dynamic means -- a dynamic PCD). >>> >>> Note that the FDF file places QemuFlashFvbServicesRuntimeDxe in APRIORI >>> DXE when SMM_REQUIRE is FALSE. This driver checks for pflash presence, >>> and lets EmuVariableFvbRuntimeDxe perform its in-RAM flash emulation >>> only in case pflash is not found. >>> >>> So this is again in favor of a separate platform -- if we know pflash is >>> never part of the board, then QemuFlashFvbServicesRuntimeDxe is never >>> needed, but you cannot remove it from the traditional DSC/FDF files. >>> >>> Second, EmuVariableFvbRuntimeDxe consumes the PlatformFvbLib class, >>> for >>> the PlatformFvbDataWritten() API (among other things). This lib class is >>> implemented by two instances in OvmfPkg, PlatformFvbLibNull and >>> EmuVariableFvbLib. The latter instance allows Platform BDS to hook an >>> event (for signaling) via "PcdEmuVariableEvent" into the >>> EmuVariableFvbRuntimeDxe driver. >>> >>> In old (very old) QEMU board configurations, namely those without >>> pflash, this (mis)feature is used by OVMF's PlatformBootManagerLib to >>> write out all variables to the EFI system partition in a regular file >>> called \NvVars, with the help of NvVarsFileLib, whenever >>> EmuVariableFvbRuntimeDxe writes out an emulated "flash" block. For this >>> purpose, the traditional OVMF DSC files link EmuVariableFvbLib into >>> EmuVariableFvbRuntimeDxe. >>> >>> But it counts as an absolute disaster nowadays, and should not be >>> revived in any platform. If you don't have pflash in TDX guests, just >>> accept that you won't have non-volatile variables. And link >>> PlatformFvbLibNull into EmuVariableFvbRuntimeDxe. You're going to need >>> a >>> separate PlatformBootManagerLib instance anyway. >>> >>> (We should have removed EmuVariableFvbRuntimeDxe a long time ago >>> from >>> the traditional OVMF platforms, i.e. made pflash a hard requirement, >>> even when SMM is not built into the platform -- but whenever I tried >>> that, Jordan always shot me down.) >>> >>> My point is: using EmuVariableFvbRuntimeDxe in the TDX platform may be >>> defensible per se, but we must be very clear that it will never provide >>> a standards-conformant service for non-volatile UEFI variables, and we >>> must keep as much of the \NvVars mess out of EmuVariableFvbRuntimeDxe >>> as >>> possible. This will require a separate PlatformBootManagerLib instance >>> for TDX anyway (or maybe share PlatformBootManagerLibGrub with >>> "OvmfPkg/AmdSev/AmdSevX64.dsc"). >>> >>> Apart from the volatility aspect, let's assume we have this in-RAM >>> emulated "flash device", storing authenticated UEFI variables for Secure >>> Boot purposes. And we don't have SMM. >>> >>> What protects this in-RAM variable store from tampering by the guest OS? >>> It's all guest RAM only, after all. What provides the privilege barrier >>> between the guest firmware and the guest OS? >>> >>> >>> *** Slide 50: Library >>> >>> (23) Should we introduce Null instances for all (or most) new lib >>> classes here? Code size is a concern (static linking). If we extend a >>> common OvmfPkg module with new hook points, it's one thing to return >>> from that hook point early *dynamically*, but it's even better (given >>> separate platforms) to allow the traditional platform firmware to use a >>> Null lib instance, to cut out all the dead code statically. >>> >>> >>> *** Slides 51 through 52 >>> >>> Seems OK. >>> >>> >>> *** Slide 53: >>> >>> (24) It might be worth noting that BaseIoLibIntrinsic already has some >>> SEV enlightenment, as the FIFO IO port operations (which normally use >>> the REP prefix) are not handled on SEV. I don't have an immediate idea >>> why this might matter, we should just minimize code duplication if >>> possible. >>> >>> >>> *** Slides 54-56: >>> >>> No comments, this stuff seems reasonable. >>> >>> >>> *** Slide 57: MpInitLib >>> >>> I don't know enough to give a summary judgement. >>> >>> >>> All in all, I see the controversial / messy parts in the platform >>> bringup, and how all that differs from the traditional ("legacy") OVMF >>> platforms. I admit I *may* be biased in favor of SEV, possibly because >>> SEV landed first -- if you find signs of such a bias in my comments, >>> please don't hesitate to debunk those points. Yet my general impression >>> is that the early bringup stuff is significantly different from >>> everything before, and because of this, a separate platform is >>> justified. >>> >>> Definitely separate from the traditional OVMF IA32, IA32X64, and X64 >>> platforms, and *possibly* separate from the "remote attestation" >>> AmdSevX64.dsc platform. I would approach the TDX feature-set in complete >>> isolation (exactly how Intel commenced the work, if I understand >>> correctly), modulo obviously shareable / reusable parts, and then slowly >>> & gradually work on extracting / refactoring commonalities. >>> >>> (But, given my stance on Xen for example, I could disagree even with the >>> latter, retroactive kind of unification -- it all boils down to shared >>> developer and user base. Component sharing should reflect the community >>> structure, otherwise maintenance will be a nightmare.) >>> >>> Thanks >>> Laszlo >>> >>> >>> >>> >>> >> > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#76066): https://edk2.groups.io/g/devel/message/76066 Mute This Topic: https://groups.io/mt/83283616/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
