On Tue, Mar 25, 2014 at 4:58 PM, Laszlo Ersek <[email protected]> wrote:
> Qemu v1.7.0 features an ACPI linker/loader interface, available over
> fw_cfg, written by Michael Tsirkin.
Can we add something like:
(See 1.7.0 compatibility information below.)
> 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. Benefits of the
> qemu-provided ACPI tables include PCI hotplug for example.
>
> 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 fw_cfg files are filled in by qemu, but two kinds
> of fields are left incomplete in each table: pointers to other tables, and
> checksums (which depend on the pointers).
>
> Qemu initializes each pointer with a relative offset into the fw_cfg file
> that contains the pointed-to ACPI table. The final pointer values depend
> on where the fw_cfg files, holding the pointed-to ACPI tables, 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 in 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 the pointing ACPI
> table, present in some fw_cfg file, with the dynamic base address of the
> same (or another) fw_cfg file, holding the pointed-to ACPI table.
>
> 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" fw_cfg file, determining the
> boundaries of each ACPI table inside it, and installing those tables.
>
> Compatibility information:
>
> - The patch has no effect when OVMF runs on qemu versions up to v1.6.x.
> OVMF's built-in ACPI tables are used.
>
> - The patch has no effect when OVMF runs on any qemu version, and a
> machine type up to "pc-i440fx-1.6" is selected. OVMF's built-in ACPI
> tables are used.
>
> - The patch causes a regression in PCI resource allocation for some guest
> RAM sizes (eg. 2560MB) when OVMF runs on qemu v1.7.0 precisely, and the
> "pc-i440fx-1.7" (ie. default) machine type is selected. This bug has
> been fixed in qemu v1.7.1 (commit 03bc4f6 -- "piix: fix 32bit pci
> hole").
Doesn't this cause a hang in certain situations? Could you more
clearly indicate the symptom seen on 1.7.0?
Depending on the symptom, we might want to update the README.
> - The patch has the intended effect when OVMF runs on qemu versions
> v1.7.1+ (including the current development tree, v2.0.0-rc0) and a
> pc-i440fx-1.7+ machine type is selected.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <[email protected]>
> ---
>
> Notes:
> v1: http://thread.gmane.org/gmane.comp.bios.tianocore.devel/4881
> v2: http://thread.gmane.org/gmane.comp.bios.tianocore.devel/5103
> v3: this posting
>
> v1->v2 changes:
> - move large comment block from "OvmfPkg/AcpiPlatformDxe/Qemu.c" to
> commit message
> - fall back to builtin tables if 32-bit PCI window has not been found in
> the PEI phase (and consequently could not cause PCI resource
> allocation to match qemu's ACPI tables)
> - trim excessive logging
>
> v2->v3:
> - Support for the "etc/pci-info" fw_cfg file has been revoked in qemu
> v1.7.0 (hence there is no official qemu release that provdies it). The
> bottom of the 32-bit PCI hole that is exported in qemu's DSDT/SSDTs
> has been fixed in qemu v1.7.1 (qemu commit 03bc4f6), thus we don't
> need "etc/pci-info" any longer.
>
> Testing: been running my Linux and Windows guests with this patch for
> months.
>
> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.h | 7 +-
> OvmfPkg/AcpiPlatformDxe/AcpiPlatform.c | 12 ++-
> OvmfPkg/AcpiPlatformDxe/Qemu.c | 138
> +++++++++++++++++++++++++++++++++
> 3 files changed, 149 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..91f6db5 100644
> --- a/OvmfPkg/AcpiPlatformDxe/Qemu.c
> +++ b/OvmfPkg/AcpiPlatformDxe/Qemu.c
> @@ -515,3 +515,141 @@ QemuInstallAcpiTable (
> );
> }
>
> +
> +/**
> + Download the ACPI table data file from QEMU and interpret it.
> +
> + @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 required fw_cfg
> + files.
> +
> + @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
> +
> +EFI_STATUS
> +EFIAPI
> +InstallQemuLinkedTables (
> + IN EFI_ACPI_TABLE_PROTOCOL *AcpiProtocol
> + )
> +{
> + STATIC CONST CHAR8 Func[] = "InstallQemuLinkedTables";
The name Func makes me think this might be a function pointer.
What about just using __FUNCTION__ in place of Func?
> + 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=\"%-4.4a\" Length=0x%08x\n",
> + Func, (UINT64) Processed,
> + (CONST CHAR8 *) &Probe->Signature, Probe->Length));
> +
> + 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]);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((EFI_D_ERROR,
> + "%a: failed to install table \"%-4.4a\" at offset 0x%Lx: %r\n",
> Func,
> + (CONST CHAR8 *) &Probe->Signature, (UINT64) Processed, Status));
> + break;
> + }
> +
> + ++Installed;
> + }
> +
> + Processed += Probe->Length;
> + }
> +
> + if (Processed == TablesFileSize || Status == EFI_PROTOCOL_ERROR) {
PROTOCOL_ERROR seems odd here. Why does this situation get turned into
'success'?
Maybe just set Processed = TablesFileSize (with a comment) up above
before the break?
-Jordan
> + DEBUG ((EFI_D_INFO, "%a: installed %d tables\n", Func, Installed));
> + Status = EFI_SUCCESS;
> + } else {
> + ASSERT (EFI_ERROR (Status));
> +
> + //
> + // Roll back partial installation.
> + //
> + while (Installed > 0) {
> + --Installed;
> + AcpiProtocol->UninstallAcpiTable (AcpiProtocol,
> InstalledKey[Installed]);
> + }
> + }
> +
> + FreePool (InstalledKey);
> +
> +FreeTables:
> + FreePool (Tables);
> +
> + return Status;
> +}
> --
> 1.8.3.1
------------------------------------------------------------------------------
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/edk2-devel
