[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [edk2 PATCH 1/1] OvmfPkg/AcpiPlatformDxe: download ACPI
From: |
Jordan Justen |
Subject: |
Re: [Qemu-devel] [edk2 PATCH 1/1] OvmfPkg/AcpiPlatformDxe: download ACPI tables from QEMU |
Date: |
Fri, 22 Nov 2013 10:10:34 -0800 |
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