Re: [Qemu-devel] [edk2 PATCH 1/1] OvmfPkg/AcpiPlatformDxe: download ACPI tables from QEMU

[Jordan Justen]

On Tue, Nov 12, 2013 at 7:11 AM, Laszlo Ersek <address@hidden> wrote:
> Qemu v1.7.0-rc0 features an ACPI linker/loader interface, available over
> fw_cfg, written by Michael Tsirkin.
>
> Qemu composes all ACPI tables on the host side, according to the target
> hardware configuration, and makes the tables available to any guest
> firmware over fw_cfg.
>
> The feature moves the burden of keeping ACPI tables up-to-date from boot
> firmware to qemu (which is the source of hardware configuration anyway).
>
> This patch adds client code for this feature.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <address@hidden>
> ---
>  OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h |   7 +-
>  OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c |  12 +-
>  OvmfPkg/AcpiPlatformDxe/Qemu.c         | 204 
> +++++++++++++++++++++++++++++++++
>  3 files changed, 215 insertions(+), 8 deletions(-)
>
> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h 
> b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
> index 21107cd..c643fa1 100644
> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h
> @@ -10,7 +10,7 @@
>    THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>    WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR 
> IMPLIED.
>
> -**/
> +**/
>
>  #ifndef _ACPI_PLATFORM_H_INCLUDED_
>  #define _ACPI_PLATFORM_H_INCLUDED_
> @@ -61,5 +61,10 @@ InstallXenTables (
>    IN   EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
>    );
>
> +EFI_STATUS
> +EFIAPI
> +InstallQemuLinkedTables (
> +  IN   EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
> +  );
>  #endif
>
> diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c 
> b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
> index 6e0b610..084c393 100644
> --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
> +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c
> @@ -256,16 +256,14 @@ AcpiPlatformEntryPoint (
>
>    if (XenDetected ()) {
>      Status = InstallXenTables (AcpiTable);
> -    if (EFI_ERROR (Status)) {
> -      Status = FindAcpiTablesInFv (AcpiTable);
> -    }
>    } else {
> +    Status = InstallQemuLinkedTables (AcpiTable);
> +  }
> +
> +  if (EFI_ERROR (Status)) {
>      Status = FindAcpiTablesInFv (AcpiTable);
>    }
> -  if (EFI_ERROR (Status)) {
> -    return Status;
> -  }
>
> -  return EFI_SUCCESS;
> +  return Status;
>  }
>
> diff --git a/OvmfPkg/AcpiPlatformDxe/Qemu.c b/OvmfPkg/AcpiPlatformDxe/Qemu.c
> index 06bd463..c572f8a 100644
> --- a/OvmfPkg/AcpiPlatformDxe/Qemu.c
> +++ b/OvmfPkg/AcpiPlatformDxe/Qemu.c
> @@ -515,3 +515,207 @@ QemuInstallAcpiTable (
>             );
>  }
>
> +
> +/**
> +  Download the ACPI table data file from QEMU and interpret it.
> +
> +  Starting with v1.7.0-rc0, QEMU provides the following three files for 1.7+
> +  machine types:
> +  - etc/acpi/rsdp
> +  - etc/acpi/tables
> +  - etc/table-loader
> +
> +  "etc/acpi/rsdp" and "etc/acpi/tables" are similar, they are only kept
> +  separate because they have different allocation requirements in SeaBIOS.
> +
> +  Both of these fw_cfg files contain preformatted ACPI payload. 
> "etc/acpi/rsdp"
> +  contains only the RSDP table, while "etc/acpi/tables" contains all other
> +  tables, concatenated.
> +
> +  The tables in these two files have been filled in by qemu, but two kinds of
> +  fields are incomplete in each table: pointers to other tables, and 
> checksums
> +  (which depend on the pointers). The pointers are pre-initialized with
> +  *relative* offsets, but their final values depend on where the actual files
> +  will be placed in memory by the guest. That is, the pointer fields need to 
> be
> +  "relocated" (incremented) by the base addresses of where "/etc/acpi/rsdp" 
> and
> +  "/etc/acpi/tables" will be placed in guest memory.
> +
> +  This is where the third file, "/etc/table-loader" comes into the picture. 
> It
> +  is a linker/loader script that has several command types:
> +
> +    One command type instructs the guest to download the other two files.
> +
> +    Another command type instructs the guest to increment ("absolutize") a
> +    pointer field (having a relative initial value) in some file, with the
> +    dynamic base address of the same (or another) file.
> +
> +    The third command type instructs the guest to compute checksums over
> +    ranges and to store them.
> +
> +  In edk2, EFI_ACPI_TABLE_PROTOCOL knows about table relationships -- it
> +  handles linkage automatically when a table is installed. The protocol takes
> +  care of checksumming too. RSDP is installed automatically. Hence we only 
> need
> +  to care about the "etc/acpi/tables" file, determining the boundaries of 
> each
> +  table and installing it.

I'm wondering if some of the information in this comment might have a
better home in the commit message. Will it help in maintaining the
code to have it here? Or, maybe a more terse version can live here?

Of course, I rarely comment my code enough, which is much worse. So,
feel free to ignore this feedback. :)

> +  @param[in] AcpiProtocol  The ACPI table protocol used to install tables.
> +
> +  @retval  EFI_UNSUPPORTED       Firmware configuration is unavailable.
> +
> +  @retval  EFI_NOT_FOUND         The host doesn't export the 
> "etc/acpi/tables"
> +                                 fw_cfg file.
> +
> +  @retval  EFI_OUT_OF_RESOURCES  Memory allocation failed.
> +
> +  @return                        Status codes returned by
> +                                 AcpiProtocol->InstallAcpiTable().
> +
> +**/
> +
> +//
> +// We'll be saving the keys of installed tables so that we can roll them back
> +// in case of failure. 128 tables should be enough for anyone (TM).
> +//
> +#define INSTALLED_TABLES_MAX 128
> +
> +//
> +// This macro produces three arguments for DEBUG(), for the format string
> +// "%-*.*a" -- left-justify, take field width, take number of characters to
> +// consume, ASCII character string. The argument must be a valid pointer.
> +//
> +#define DBGSTR(ptr) (sizeof *(ptr)), (sizeof *(ptr)), ((CONST CHAR8 *) (ptr))
> +
> +//
> +// Introduce a macro for the format string as well, bracketed by embedded
> +// double quotes.
> +//
> +#define DBGFMT "\"%-*.*a\""

I think that needing these macros is a sign that perhaps there are
excessive debug messages. :)

> +EFI_STATUS
> +EFIAPI
> +InstallQemuLinkedTables (
> +  IN   EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol
> +  )
> +{
> +  STATIC CONST CHAR8   Func[] = "InstallQemuLinkedTables";
> +  EFI_STATUS           Status;
> +  FIRMWARE_CONFIG_ITEM TablesFile;
> +  UINTN                TablesFileSize;
> +  UINT8                *Tables;
> +  UINTN                *InstalledKey;
> +  UINTN                Processed;
> +  INT32                Installed;
> +
> +  Status = QemuFwCfgFindFile ("etc/acpi/tables", &TablesFile, 
> &TablesFileSize);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((EFI_D_INFO, "%a: \"etc/acpi/tables\" interface unavailable: 
> %r\n",
> +      Func, Status));
> +    return Status;
> +  }
> +
> +  Tables = AllocatePool (TablesFileSize);
> +  if (Tables == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  QemuFwCfgSelectItem (TablesFile);
> +  QemuFwCfgReadBytes (TablesFileSize, Tables);
> +
> +  InstalledKey = AllocatePool (INSTALLED_TABLES_MAX * sizeof *InstalledKey);
> +  if (InstalledKey == NULL) {
> +    Status = EFI_OUT_OF_RESOURCES;
> +    goto FreeTables;
> +  }
> +
> +  Processed = 0;
> +  Installed = 0;
> +  while (Processed < TablesFileSize) {
> +    UINTN                       Remaining;
> +    EFI_ACPI_DESCRIPTION_HEADER *Probe;
> +
> +    Remaining = TablesFileSize - Processed;
> +    if (Remaining < sizeof *Probe) {
> +      DEBUG ((EFI_D_VERBOSE, "%a: truncated ACPI table header at offset "
> +        "0x%Lx\n", Func, (UINT64) Processed));
> +      Status = EFI_PROTOCOL_ERROR;
> +      break;
> +    }
> +
> +    Probe = (EFI_ACPI_DESCRIPTION_HEADER *) (Tables + Processed);
> +    DEBUG ((EFI_D_VERBOSE, "%a: offset 0x%016Lx:"
> +      " Signature=" DBGFMT
> +      " Length=0x%08x"
> +      " Revision=0x%02x"
> +      " OemId=" DBGFMT
> +      " OemTableId=" DBGFMT
> +      " OemRevision=0x%08x"
> +      " CreatorId=" DBGFMT
> +      " CreatorRevision=0x%08x\n",
> +      Func, (UINT64) Processed,
> +      DBGSTR (&Probe->Signature),
> +      Probe->Length,
> +      Probe->Revision,
> +      DBGSTR (&Probe->OemId),
> +      DBGSTR (&Probe->OemTableId),
> +      Probe->OemRevision,
> +      DBGSTR (&Probe->CreatorId),
> +      Probe->CreatorRevision));

Yep. :)

Do we need to print all those fields?

Anyway, maybe a better centralized place for this would be:
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c

Also, I think we try to keep debug messages under 80 characters.

> +    if (Remaining < Probe->Length || Probe->Length < sizeof *Probe) {
> +      DEBUG ((EFI_D_VERBOSE, "%a: invalid ACPI table header at offset 
> 0x%Lx\n",
> +        Func, (UINT64) Processed));
> +      Status = EFI_PROTOCOL_ERROR;
> +      break;
> +    }
> +
> +    //
> +    // skip automatically handled "root" tables: RSDT, XSDT
> +    //
> +    if (Probe->Signature !=
> +                        EFI_ACPI_1_0_ROOT_SYSTEM_DESCRIPTION_TABLE_SIGNATURE 
> &&
> +        Probe->Signature !=
> +                    
> EFI_ACPI_2_0_EXTENDED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE) {
> +      if (Installed == INSTALLED_TABLES_MAX) {
> +        DEBUG ((EFI_D_ERROR, "%a: can't install more than %d tables\n", Func,
> +          INSTALLED_TABLES_MAX));
> +        Status = EFI_OUT_OF_RESOURCES;
> +        break;
> +      }
> +
> +      Status = AcpiProtocol->InstallAcpiTable (AcpiProtocol, Probe,
> +                 Probe->Length, &InstalledKey[Installed]);

We do have a local InstallAcpiTable function. (Just remove "AcpiProtocol->")

-Jordan