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

["Laszlo Ersek" <lersek@redhat.com>]

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