On Thu, Mar 27, 2014 at 2:00 PM, Laszlo Ersek <[email protected]> wrote: > On 03/27/14 19:05, Jordan Justen wrote: >> 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.) > > Yep. > > >>> - 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? > > I hope this one's clear enough :) : > > http://thread.gmane.org/gmane.comp.emulators.qemu/257366/focus=257529 > > Dependent on guest, it may cause a nonfunctional display (blank screen), > but (as linked above) some Linux guests like to crash too.
So, that's what I'd like to make clear in the commit message: * After this change, using QEMU 1.7.0 with OVMF might cause your OS boot to crash. Don't use QEMU 1.7.0! Right now it looks like: * Hey we support something added in QEMU 1.7.0! Oh, yeah there's some kind of regression related to QEMU 1.7.0. >> Depending on the symptom, we might want to update the README. > > That's a good idea. Can you suggest a wording? I can include it as a > separate patch with your S-o-b. I'll try to get something to you, but we probably can add it sometime after this patch. >>> - 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? > > You are right of course. Probably a leftover from one of the earlier > versions. Can't recall any longer why I did it like this. > >> >>> + 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? > > Well this is kinda messy. The fw_cfg file in question is padded with a > bunch of NULs. The reason being something about forward compat for VM > migration -- the fw_cfg file is also mapped as a ROM, and optimally (for > migratability) its size shouldn't grow even if the ACPI payload grows. > > Whatever the reason for NUL-padding, it means that when parsing the > fw_cfg file, at one point the probe probes into the trailing set of > zeros, and catches that as invalid table header, with the > > (Probe->Length < sizeof *Probe) > > sub-condition: > > InstallQemuLinkedTables: invalid ACPI table header at offset 0x1B38 > > It really means EOF, but it is undistinguishable from genuinely > nonsensical blob contents. (Just because Probe->Length ends up zero it > doesn't imply it's not the result of some garbled qemu code; so I don't > feel convenient treating Probe->Length==0 as a clear-cut EOF situation.) > > Hence, undetectable EOF and data corruption can't be really told apart. You could check that the remaining data is all zero assuming QEMU is consistent about this like you mentioned above. > If I reject EFI_PROTOCOL_ERROR, then in (current) practice the parsing > will always fail, because right now there's a quite sizeable trailing > portion of NULs. > > If you have a suggestion I'm glad to listen. How about up above doing: // // QEMU adds some junk data to fw_cfg file etc/acpi/tables. // // We just skip the remaining data once we encounter this. // Processed = TablesFileSize; 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 >> > ------------------------------------------------------------------------------ _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
