On 06/23/21 04:44, Xu, Min M wrote: > On 06/22/2021 9:35 PM, Laszlo wrote: >> >> For example, as I stated earlier, "OvmfPkg/AcpiPlatformDxe" is a >> driver where I'd like to see zero changes, for either SEV or TDX. If >> the TD Mailbox location has to be reported to the OS via the MADT, >> and QEMU cannot (or must not) populate that field in the MADT, then a >> separate, TDX-specific edk2 driver should locate the MADT (installed >> technically by "OvmfPkg/AcpiPlatformDxe", earlier), and update the >> field. >> > We have updated the design of AcpiPlatformDxe. Please see the slides > in below link. > https://edk2.groups.io/g/devel/files/Designs/2021/0611/TDVF_Design_Review-AcpiPlatformDxe.pptx
Thanks, let me mark this with [1]. > > Because MailboxAddress in MADT table is determined in runtime in Tdx, > so we separate the update of the MADT table in TdxDxe driver and keep > AcpiPlatformDxe clean and shim. I've now read 4.3.4 AP information reporting from TDVF to OS from https://software.intel.com/content/dam/develop/external/us/en/documents/tdx-virtual-firmware-design-guide-rev-1.pdf That section does not go into much detail about the expected MADT updates / entries. I've also checked the various MADT subtable types here: https://uefi.org/specs/ACPI/6.4/05_ACPI_Software_Programming_Model/ACPI_Software_Programming_Model.html#interrupt-controller-structure-types It seems that the TDVF spec speaks about subtable type 0 (Processor Local APIC): https://uefi.org/specs/ACPI/6.4/05_ACPI_Software_Programming_Model/ACPI_Software_Programming_Model.html#processor-local-apic-structure I've also checked "acpidump -b; iasl -d" in a normal guest, to remind myself of the actual MADT contents that QEMU currently generates. I see minimally the following subtable types: Subtable Type : 00 [Processor Local APIC] Subtable Type : 01 [I/O APIC] Subtable Type : 02 [Interrupt Source Override] Subtable Type : 04 [Local APIC NMI] Thus, the TDVF spec creates the extremely unfortunate situation where subtables of types different from 0 are expected from QEMU, but subtables of type 0 are expected from the firmware. In this case, QEMU should likely not populate the MADT with any LAPIC (= type 0) subtables. Then, Option-3 from slide#5 in [1] (uninstalling the MADT, *extending* the MADT with LAPIC subtables, installing the new MADT) seems relatively workable. ( Note that uninstalling an ACPI table (with EFI_ACPI_TABLE_PROTOCOL) that was installed by a different driver previously requires the use of EFI_ACPI_SDT_PROTOCOL. That's because the TableKey parameter taken by EFI_ACPI_TABLE_PROTOCOL.UninstallAcpiTable() is only available from EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() -- which the TDX driver will not have called --, or from EFI_ACPI_SDT_PROTOCOL.GetAcpiTable(). EFI_ACPI_SDT_PROTOCOL.RegisterNotify() also exists, but it should be avoided. It should not be used to immediately modify or replace the MADT as soon as the MADT appears. That's because, upon encountering an error, OvmfPkg/AcpiPlatformDxe rolls back all tables it installed up to the error. It wouldn't be great if the rollback attempted to remove a different MADT than what was installed. ) Another approach (which sounds quite horrible) might be for QEMU to pre-populate the MADT with just the right *count* of LAPIC subtables, and then the TDX driver would patch the MADT *in-place* with the proper LAPIC subtable contents (and then re-checksum the table manually). This sounds horrible indeed. Modifying the InstallAcpiTables() function in OvmfPkg/AcpiPlatformDxe, so that it install a custom NULL protocol when InstallQemuFwCfgTables() succeeds, seems tolerable. (In order to trigger the TdxDxe driver's MADT patching.) However, I don't understand the following comment from slide#5: > Open: This method is not practicable if parameters cannot be > transferred when trigger the notify function. What parameters are we talking about here? The TDVF design guide speaks about TD_VCPU_INFO_HOB, regarding the information required for filling in the LAPIC entries in the MADT. But the same HOB should be available to the TDX DXE driver too. It would be much better if TDX specific code were added to QEMU that prevented the generation of the MADT altogether, when running a TDX guest. Then the firmware would fully own the MADT, and the TDX DXE driver would only have to wait for the availability of EFI_ACPI_TABLE_PROTOCOL (simple depex). In this case, of course, the TDX driver would be responsible for all other subtable types too, not just type 0 (LAPIC). If all else fails, you can also copy "OvmfPkg/AcpiPlatformDxe" to "OvmfPkg/TdxAcpiPlatformDxe", and customize it in any way (e.g. as described on slides #3 and #4 [1]; Options 1 and 2). In this case, TdxAcpiPlatformDxe would only be used in "IntelTdx.dsc", not the pre-existent OvmfPkg*.dsc files. Summary: - Options 1 and 2 from [1] are not acceptable for OvmfPkg/AcpiPlatformDxe. - Option 3 from [1] is acceptable for OvmfPkg/AcpiPlatformDxe with a custom NULL protocol instance, but: - I don't understand the "parameter passing problem". - How exactly the TdxDxe driver implements the MADT update (or replacement) remains a question, impacting even QEMU (as QEMU and TdxDxe must not fight for ownership over the LAPIC subtables). Some possibilities are: - stop QEMU from generating the MADT, make TdxDxe own the MADT fully; - stop QEMU from generating LAPIC subtables; - make QEMU generate the right number of dummy MADT entries; - don't touch QEMU, but filter out its LAPIC subtables in TdxDxe. - Options 1 and 2 from [1] are acceptable for a detached / distinct driver called "OvmfPkg/TdxAcpiPlatformDxe", but this driver could only be used in "IntelTdx.dsc". Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#77004): https://edk2.groups.io/g/devel/message/77004 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] -=-=-=-=-=-=-=-=-=-=-=-
