On Tue, Nov 12, 2013 at 7:11 AM, Laszlo Ersek <ler...@redhat.com> 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 <ler...@redhat.com> > --- > 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