| Message ID | 1407550139-25313-5-git-send-email-lersek@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Aug 8, 2014 at 7:08 PM, Laszlo Ersek <lersek@redhat.com> wrote: > Recent changes in the QEMU ACPI table generator have shown that our > limited client for that interface is insufficient and/or brittle. > > Implement the full interface utilizing OrderedCollectionLib for addressing > fw_cfg blobs by name. > > In addition, we stop using EFI_ACPI_TABLE_PROTOCOL in the QEMU loader > client, because: > - splitting the blobs for EFI_ACPI_TABLE_PROTOCOL is just untenable short > of a complete ACPI parser, > - and it is fully sufficient to install the RSD PTR as an EFI > configuration table for the guest OS to see everything. > > In short, for this interface, EFI_ACPI_TABLE_PROTOCOL is an unwanted and > restrictive convenience; let's stop using it. Hmm. Been through a few rounds of this for Xen. It's QEMU's turn now? :) Is this required? What happens if another driver uses EFI_ACPI_TABLE_PROTOCOL? We now have two drivers that want to install the pointer. It seems the AcpiTableDxe driver has had to fix things on occasion to cover some corner cases of table publishing. Did you capture all these? What about future bugs? How did Xen manage to end up using EFI_ACPI_TABLE_PROTOCOL if it is not possible or practical for QEMU? What is it about EFI_ACPI_TABLE_PROTOCOL that is such a bad fit for VMs? -Jordan > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf | 3 + > OvmfPkg/AcpiPlatformDxe/Qemu.c | 415 ++++++++++++++++++++++++++-- > 2 files changed, 398 insertions(+), 20 deletions(-) > > diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf > index 90178e0..02eaf23 100644 > --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf > +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf > @@ -51,12 +51,15 @@ > MemoryAllocationLib > BaseLib > DxeServicesTableLib > + OrderedCollectionLib > > [Protocols] > gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED > > [Guids] > gEfiXenInfoGuid > + gEfiAcpi10TableGuid > + gEfiAcpi20TableGuid > > [Pcd] > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile > diff --git a/OvmfPkg/AcpiPlatformDxe/Qemu.c b/OvmfPkg/AcpiPlatformDxe/Qemu.c > index 333766e..4663ecb 100644 > --- a/OvmfPkg/AcpiPlatformDxe/Qemu.c > +++ b/OvmfPkg/AcpiPlatformDxe/Qemu.c > @@ -17,12 +17,15 @@ > > #include "AcpiPlatform.h" > #include "QemuLoader.h" > -#include <Library/BaseMemoryLib.h> > -#include <Library/MemoryAllocationLib.h> > -#include <Library/QemuFwCfgLib.h> > -#include <Library/DxeServicesTableLib.h> > -#include <Library/PcdLib.h> > -#include <IndustryStandard/Acpi.h> > +#include <Library/BaseMemoryLib.h> // CopyMem() > +#include <Library/MemoryAllocationLib.h> // AllocatePool() > +#include <Library/QemuFwCfgLib.h> // QemuFwCfgFindFile() > +#include <Library/DxeServicesTableLib.h> // gDS > +#include <Library/PcdLib.h> // PcdGet16() > +#include <Library/UefiLib.h> // EfiGetSystemConfigurationTable() > +#include <Library/OrderedCollectionLib.h> // OrderedCollectionInit() > +#include <IndustryStandard/Acpi.h> // EFI_ACPI_DESCRIPTION_HEADER > +#include <Guid/Acpi.h> // gEfiAcpi10TableGuid > > BOOLEAN > QemuDetected ( > @@ -518,7 +521,8 @@ QemuInstallAcpiTable ( > > > /** > - Check if an array of bytes starts with an RSD PTR structure. > + Check if an array of bytes starts with an RSD PTR structure, and if so, > + return the EFI ACPI table GUID that corresponds to its version. > > Checksum is ignored. > > @@ -526,8 +530,9 @@ QemuInstallAcpiTable ( > > @param[in] Size Number of bytes in Buffer. > > - @param[out] RsdpSize If the function returns EFI_SUCCESS, this parameter > - contains the size of the detected RSD PTR structure. > + @param[out] AcpiTableGuid On successful exit, pointer to the EFI ACPI table > + GUID in statically allocated storage that > + corresponds to the detected RSD PTR version. > > @retval EFI_SUCCESS RSD PTR structure detected at the beginning of > Buffer, and its advertised size does not exceed > @@ -545,7 +550,7 @@ EFI_STATUS > CheckRsdp ( > IN CONST VOID *Buffer, > IN UINTN Size, > - OUT UINTN *RsdpSize > + OUT EFI_GUID **AcpiTableGuid > ) > { > CONST UINT64 *Signature; > @@ -574,7 +579,7 @@ CheckRsdp ( > // > // ACPI 1.0 doesn't include the Length field > // > - *RsdpSize = sizeof *Rsdp1; > + *AcpiTableGuid = &gEfiAcpi10TableGuid; > return EFI_SUCCESS; > } > > @@ -587,27 +592,99 @@ CheckRsdp ( > return EFI_PROTOCOL_ERROR; > } > > - *RsdpSize = Rsdp2->Length; > + *AcpiTableGuid = &gEfiAcpi20TableGuid; > return EFI_SUCCESS; > } > > +// > +// The user structure for the ordered collection that will track the fw_cfg > +// blobs under processing. > +// > +typedef struct { > + UINT8 File[QEMU_LOADER_FNAME_SIZE]; // NUL-terminated name of the fw_cfg > + // blob. This is the ordering / search > + // key. > + UINTN Size; // The number of bytes in this blob. > + UINT8 *Base; // Pointer to the blob data. > +} BLOB; > + > +/** > + Compare a standalone key against a user structure containing an embedded key. > + > + @param[in] StandaloneKey Pointer to the bare key. > + > + @param[in] UserStruct Pointer to the user structure with the embedded > + key. > + > + @retval <0 If StandaloneKey compares less than UserStruct's key. > + > + @retval 0 If StandaloneKey compares equal to UserStruct's key. > + > + @retval >0 If StandaloneKey compares greater than UserStruct's key. > +**/ > +STATIC > +INTN > +EFIAPI > +BlobKeyCompare ( > + IN CONST VOID *StandaloneKey, > + IN CONST VOID *UserStruct > + ) > +{ > + CONST BLOB *Blob; > + > + Blob = UserStruct; > + return AsciiStrCmp (StandaloneKey, (CONST CHAR8 *)Blob->File); > +} > + > /** > - Download all ACPI table data files from QEMU and interpret them. > + Comparator function for two user structures. > + > + @param[in] UserStruct1 Pointer to the first user structure. > + > + @param[in] UserStruct2 Pointer to the second user structure. > + > + @retval <0 If UserStruct1 compares less than UserStruct2. > > - @param[in] AcpiProtocol The ACPI table protocol used to install tables. > + @retval 0 If UserStruct1 compares equal to UserStruct2. > + > + @retval >0 If UserStruct1 compares greater than UserStruct2. > +**/ > +STATIC > +INTN > +EFIAPI > +BlobCompare ( > + IN CONST VOID *UserStruct1, > + IN CONST VOID *UserStruct2 > + ) > +{ > + CONST BLOB *Blob1; > + > + Blob1 = UserStruct1; > + return BlobKeyCompare (Blob1->File, UserStruct2); > +} > + > +/** > + Download, process, and install ACPI table data from the QEMU loader > + interface. > > - @retval EFI_UNSUPPORTED Firmware configuration is unavailable. > + @retval EFI_UNSUPPORTED Firmware configuration is unavailable, or QEMU > + loader command with unsupported parameters > + has been found. > > @retval EFI_NOT_FOUND The host doesn't export the required fw_cfg > files. > > - @retval EFI_OUT_OF_RESOURCES Memory allocation failed, or more than > - INSTALLED_TABLES_MAX tables found. > + @retval EFI_OUT_OF_RESOURCES Memory allocation failed. > > @retval EFI_PROTOCOL_ERROR Found invalid fw_cfg contents. > > - @return Status codes returned by > - AcpiProtocol->InstallAcpiTable(). > + @retval EFI_ALREADY_STARTED One of the ACPI TABLE GUIDs has been found in > + the EFI Configuration Table, indicating the > + presence of a preexistent RSD PTR table, and > + therefore that of another module installing > + ACPI tables. > + > + @retval EFI_SUCCESS Installation of ACPI tables succeeded. > > **/ > > @@ -617,5 +694,303 @@ InstallAllQemuLinkedTables ( > VOID > ) > { > - return CheckRsdp (NULL, 0, NULL); > + EFI_STATUS Status; > + FIRMWARE_CONFIG_ITEM FwCfgItem; > + UINTN FwCfgSize; > + VOID *Rsdp; > + UINTN RsdpBufferSize; > + UINT8 *Loader; > + CONST QEMU_LOADER_ENTRY *LoaderEntry, *LoaderEnd; > + ORDERED_COLLECTION *Tracker; > + ORDERED_COLLECTION_ENTRY *TrackerEntry, *TrackerEntry2; > + BLOB *Blob; > + EFI_GUID *AcpiTableGuid; > + > + // > + // This function allocates memory on four levels. From lowest to highest: > + // - Areas consisting of whole pages, of type EfiACPIMemoryNVS, for > + // (processed) ACPI payload, > + // - BLOB structures referencing the above, tracking their names, sizes, and > + // addresses, > + // - ORDERED_COLLECTION_ENTRY objects internal to OrderedCollectionLib, > + // linking the BLOB structures, > + // - an ORDERED_COLLECTION organizing the ORDERED_COLLECTION_ENTRY entries. > + // > + // On exit, the last three levels are torn down unconditionally. If we exit > + // with success, then the first (lowest) level is left in place, constituting > + // the ACPI tables for the guest. If we exit with error, then even the first > + // (lowest) level is torn down. > + // > + > + Status = QemuFwCfgFindFile ("etc/table-loader", &FwCfgItem, &FwCfgSize); > + if (EFI_ERROR (Status)) { > + return Status; > + } > + if (FwCfgSize % sizeof *LoaderEntry != 0) { > + DEBUG ((EFI_D_ERROR, "%a: \"etc/table-loader\" has invalid size 0x%Lx\n", > + __FUNCTION__, (UINT64)FwCfgSize)); > + return EFI_PROTOCOL_ERROR; > + } > + > + if (!EFI_ERROR (EfiGetSystemConfigurationTable ( > + &gEfiAcpi10TableGuid, &Rsdp)) || > + !EFI_ERROR (EfiGetSystemConfigurationTable ( > + &gEfiAcpi20TableGuid, &Rsdp))) { > + DEBUG ((EFI_D_ERROR, "%a: RSD PTR already present\n", __FUNCTION__)); > + return EFI_ALREADY_STARTED; > + } > + > + Loader = AllocatePool (FwCfgSize); > + if (Loader == NULL) { > + return EFI_OUT_OF_RESOURCES; > + } > + > + QemuFwCfgSelectItem (FwCfgItem); > + QemuFwCfgReadBytes (FwCfgSize, Loader); > + > + Tracker = OrderedCollectionInit (BlobCompare, BlobKeyCompare); > + if (Tracker == NULL) { > + Status = EFI_OUT_OF_RESOURCES; > + goto FreeLoader; > + } > + > + Rsdp = NULL; > + RsdpBufferSize = 0; > + > + LoaderEntry = (QEMU_LOADER_ENTRY *)Loader; > + LoaderEnd = (QEMU_LOADER_ENTRY *)(Loader + FwCfgSize); > + while (LoaderEntry < LoaderEnd) { > + switch (LoaderEntry->Type) { > + case QemuLoaderCmdAllocate: { > + CONST QEMU_LOADER_ALLOCATE *Allocate; > + UINTN NumPages; > + EFI_PHYSICAL_ADDRESS Address; > + > + Allocate = &LoaderEntry->Command.Allocate; > + if (Allocate->File[QEMU_LOADER_FNAME_SIZE - 1] != '\0') { > + DEBUG ((EFI_D_ERROR, "%a: malformed file name in Allocate command\n", > + __FUNCTION__)); > + Status = EFI_PROTOCOL_ERROR; > + goto FreeTracker; > + } > + if (Allocate->Alignment > EFI_PAGE_SIZE) { > + DEBUG ((EFI_D_ERROR, "%a: unsupported alignment 0x%x in Allocate " > + "command\n", __FUNCTION__, Allocate->Alignment)); > + Status = EFI_UNSUPPORTED; > + goto FreeTracker; > + } > + Status = QemuFwCfgFindFile ((CHAR8 *)Allocate->File, &FwCfgItem, > + &FwCfgSize); > + if (EFI_ERROR (Status)) { > + DEBUG ((EFI_D_ERROR, "%a: nonexistent file \"%a\" in Allocate " > + "command\n", __FUNCTION__, Allocate->File)); > + goto FreeTracker; > + } > + > + NumPages = EFI_SIZE_TO_PAGES (FwCfgSize); > + Address = 0xFFFFFFFF; > + Status = gBS->AllocatePages (AllocateMaxAddress, EfiACPIMemoryNVS, > + NumPages, &Address); > + if (EFI_ERROR (Status)) { > + goto FreeTracker; > + } > + > + Blob = AllocatePool (sizeof *Blob); > + if (Blob == NULL) { > + Status = EFI_OUT_OF_RESOURCES; > + goto FreePages; > + } > + CopyMem (Blob->File, Allocate->File, QEMU_LOADER_FNAME_SIZE); > + Blob->Size = FwCfgSize; > + Blob->Base = (VOID *)(UINTN)Address; > + > + if (Allocate->Zone == QemuLoaderAllocFSeg) { > + if (Rsdp == NULL) { > + Rsdp = Blob->Base; > + RsdpBufferSize = Blob->Size; > + } else { > + DEBUG ((EFI_D_ERROR, "%a: duplicate RSD PTR candidate in Allocate " > + "command\n", __FUNCTION__)); > + Status = EFI_PROTOCOL_ERROR; > + goto FreeBlob; > + } > + } > + > + Status = OrderedCollectionInsert (Tracker, NULL, Blob); > + if (Status == RETURN_ALREADY_STARTED) { > + DEBUG ((EFI_D_ERROR, "%a: duplicated file \"%a\" in Allocate " > + "command\n", __FUNCTION__, Allocate->File)); > + Status = EFI_PROTOCOL_ERROR; > + goto FreeBlob; > + } > + if (EFI_ERROR (Status)) { > + goto FreeBlob; > + } > + > + QemuFwCfgSelectItem (FwCfgItem); > + QemuFwCfgReadBytes (FwCfgSize, Blob->Base); > + ZeroMem (Blob->Base + Blob->Size, > + EFI_PAGES_TO_SIZE (NumPages) - Blob->Size); > + > + DEBUG ((EFI_D_VERBOSE, "%a: Allocate: File=\"%a\" Alignment=0x%x " > + "Zone=%d Size=0x%Lx Address=0x%Lx\n", __FUNCTION__, Allocate->File, > + Allocate->Alignment, Allocate->Zone, (UINT64)Blob->Size, > + (UINT64)(UINTN)Blob->Base)); > + break; > + > +FreeBlob: > + FreePool (Blob); > +FreePages: > + gBS->FreePages (Address, NumPages); > + goto FreeTracker; > + } > + > + case QemuLoaderCmdAddPointer: { > + CONST QEMU_LOADER_ADD_POINTER *AddPointer; > + CONST BLOB *Blob2; > + UINT8 *PointerField; > + > + AddPointer = &LoaderEntry->Command.AddPointer; > + if (AddPointer->PointerFile[QEMU_LOADER_FNAME_SIZE - 1] != '\0' || > + AddPointer->PointeeFile[QEMU_LOADER_FNAME_SIZE - 1] != '\0') { > + DEBUG ((EFI_D_ERROR, "%a: malformed file name in AddPointer command\n", > + __FUNCTION__)); > + Status = EFI_PROTOCOL_ERROR; > + goto FreeTracker; > + } > + > + TrackerEntry = OrderedCollectionFind (Tracker, AddPointer->PointerFile); > + TrackerEntry2 = OrderedCollectionFind (Tracker, AddPointer->PointeeFile); > + if (TrackerEntry == NULL || TrackerEntry2 == NULL) { > + DEBUG ((EFI_D_ERROR, "%a: invalid blob reference(s) \"%a\" / \"%a\" " > + "in AddPointer command\n", __FUNCTION__, AddPointer->PointerFile, > + AddPointer->PointeeFile)); > + Status = EFI_PROTOCOL_ERROR; > + goto FreeTracker; > + } > + > + Blob = OrderedCollectionUserStruct (TrackerEntry); > + Blob2 = OrderedCollectionUserStruct (TrackerEntry2); > + if ((AddPointer->PointerSize != 1 && AddPointer->PointerSize != 2 && > + AddPointer->PointerSize != 4 && AddPointer->PointerSize != 8) || > + Blob->Size < AddPointer->PointerSize || > + Blob->Size - AddPointer->PointerSize < AddPointer->PointerOffset) { > + DEBUG ((EFI_D_ERROR, "%a: invalid pointer location in \"%a\" in " > + "AddPointer command\n", __FUNCTION__, AddPointer->PointerFile)); > + Status = EFI_PROTOCOL_ERROR; > + goto FreeTracker; > + } > + > + PointerField = Blob->Base + AddPointer->PointerOffset; > + switch (AddPointer->PointerSize) { > + case 1: > + *PointerField += (UINT8)(UINTN)Blob2->Base; > + break; > + > + case 2: > + *(UINT16 *)PointerField += (UINT16)(UINTN)Blob2->Base; > + break; > + > + case 4: > + *(UINT32 *)PointerField += (UINT32)(UINTN)Blob2->Base; > + break; > + > + case 8: > + *(UINT64 *)PointerField += (UINT64)(UINTN)Blob2->Base; > + break; > + } > + > + DEBUG ((EFI_D_VERBOSE, "%a: AddPointer: PointerFile=\"%a\" " > + "PointeeFile=\"%a\" PointerOffset=0x%x PointerSize=%d\n", __FUNCTION__, > + AddPointer->PointerFile, AddPointer->PointeeFile, > + AddPointer->PointerOffset, AddPointer->PointerSize)); > + break; > + } > + > + case QemuLoaderCmdAddChecksum: { > + CONST QEMU_LOADER_ADD_CHECKSUM *AddChecksum; > + > + AddChecksum = &LoaderEntry->Command.AddChecksum; > + if (AddChecksum->File[QEMU_LOADER_FNAME_SIZE - 1] != '\0') { > + DEBUG ((EFI_D_ERROR, "%a: malformed file name in AddChecksum " > + "command\n", __FUNCTION__)); > + Status = EFI_PROTOCOL_ERROR; > + goto FreeTracker; > + } > + > + TrackerEntry = OrderedCollectionFind (Tracker, AddChecksum->File); > + if (TrackerEntry == NULL) { > + DEBUG ((EFI_D_ERROR, "%a: invalid blob reference \"%a\" in " > + "AddChecksum command\n", __FUNCTION__, AddChecksum->File)); > + Status = EFI_PROTOCOL_ERROR; > + goto FreeTracker; > + } > + > + Blob = OrderedCollectionUserStruct (TrackerEntry); > + if (Blob->Size <= AddChecksum->ResultOffset || > + Blob->Size < AddChecksum->Length || > + Blob->Size - AddChecksum->Length < AddChecksum->Start) { > + DEBUG ((EFI_D_ERROR, "%a: invalid checksum location or range in " > + "\"%a\" in AddChecksum command\n", __FUNCTION__, AddChecksum->File)); > + Status = EFI_PROTOCOL_ERROR; > + goto FreeTracker; > + } > + > + Blob->Base[AddChecksum->ResultOffset] = CalculateCheckSum8 ( > + Blob->Base + AddChecksum->Start, > + AddChecksum->Length > + ); > + DEBUG ((EFI_D_VERBOSE, "%a: AddChecksum: File=\"%a\" ResultOffset=0x%x " > + "Start=0x%x Length=0x%x\n", __FUNCTION__, AddChecksum->File, > + AddChecksum->ResultOffset, AddChecksum->Start, AddChecksum->Length)); > + break; > + } > + > + default: > + DEBUG ((EFI_D_VERBOSE, "%a: unknown loader command: 0x%x\n", > + __FUNCTION__, LoaderEntry->Type)); > + break; > + } > + > + ++LoaderEntry; > + } > + > + if (Rsdp == NULL) { > + DEBUG ((EFI_D_ERROR, "%a: no RSD PTR candidate\n", __FUNCTION__)); > + Status = EFI_PROTOCOL_ERROR; > + goto FreeTracker; > + } > + > + AcpiTableGuid = NULL; > + if (EFI_ERROR (CheckRsdp (Rsdp, RsdpBufferSize, &AcpiTableGuid))) { > + DEBUG ((EFI_D_ERROR, "%a: RSD PTR not found in candidate\n", > + __FUNCTION__)); > + Status = EFI_PROTOCOL_ERROR; > + goto FreeTracker; > + } > + > + Status = gBS->InstallConfigurationTable (AcpiTableGuid, Rsdp); > + > +FreeTracker: > + // > + // Tear down the tracker structure, and if we're exiting with an error, the > + // pages holding the blob data (ie. the processed ACPI payload) as well. > + // > + for (TrackerEntry = OrderedCollectionMin (Tracker); TrackerEntry != NULL; > + TrackerEntry = TrackerEntry2) { > + VOID *UserStruct; > + > + TrackerEntry2 = OrderedCollectionNext (TrackerEntry); > + OrderedCollectionDelete (Tracker, TrackerEntry, &UserStruct); > + if (EFI_ERROR (Status)) { > + Blob = UserStruct; > + gBS->FreePages ((UINTN)Blob->Base, EFI_SIZE_TO_PAGES (Blob->Size)); > + } > + FreePool (UserStruct); > + } > + OrderedCollectionUninit (Tracker); > + > +FreeLoader: > + FreePool (Loader); > + return Status; > } > -- > 1.8.3.1 > > > ------------------------------------------------------------------------------ > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
On 08/26/14 00:24, Jordan Justen wrote: > On Fri, Aug 8, 2014 at 7:08 PM, Laszlo Ersek <lersek@redhat.com> wrote: >> Recent changes in the QEMU ACPI table generator have shown that our >> limited client for that interface is insufficient and/or brittle. >> >> Implement the full interface utilizing OrderedCollectionLib for addressing >> fw_cfg blobs by name. >> >> In addition, we stop using EFI_ACPI_TABLE_PROTOCOL in the QEMU loader >> client, because: >> - splitting the blobs for EFI_ACPI_TABLE_PROTOCOL is just untenable short >> of a complete ACPI parser, >> - and it is fully sufficient to install the RSD PTR as an EFI >> configuration table for the guest OS to see everything. >> >> In short, for this interface, EFI_ACPI_TABLE_PROTOCOL is an unwanted and >> restrictive convenience; let's stop using it. > > Hmm. Been through a few rounds of this for Xen. It's QEMU's turn now? :) > > Is this required? Depends on how good (how accurate) ACPI tables you want to have in your VMs. For a few qemu releases now, qemu has been the "master" of ACPI payload generation. SeaBIOS too has kept its own builtin tables, yes, but when the QEMU generated payload is available, it interprets this linker/loader script just the same. If you want PCI hotplug, pvpanic support; or, going forward, memory hotplug, then yes, those things need ACPI support, and the ACPI stuff for them is now developed in QEMU (and dynamically generated by it, dependent on the actual hardware config). > What happens if another driver uses EFI_ACPI_TABLE_PROTOCOL? We now > have two drivers that want to install the pointer. Yes, I checked that. The UEFI spec says that whenever you install a configuration table that's already present (by GUID), the reference is updated. InstallConfigurationTable() [...] * If Guid is present in the System Table, and Table is not NULL, then the (Guid, Table) pair is updated with the new Table value. [...] For this reason, the first thing InstallAllQemuLinkedTables() does is check, with EfiGetSystemConfigurationTable(), for the presence of any of the ACPI GUIDs in the config table. There's no "absolute" cure against another driver in OVMF just grabbing EFI_ACPI_TABLE_PROTOCOL and installing its own stuff (which would then completely hide the tables installed by this patchset, because "MdeModulePkg/Universal/Acpi/AcpiTableDxe" would simply replace QEMU's RSD PTR's address in the UEFI config table with its own). But ACPI tables are not randomly installed in OVMF, we keep it centralized in AcpiTableDxe. > It seems the AcpiTableDxe driver has had to fix things on occasion to > cover some corner cases of table publishing. Did you capture all > these? What about future bugs? The point of this series, and more importantly, qemu's linker/loader interface, is that the firmware can remain dumb. It doesn't need to know about inter-table relationships, it doesn't need to know about tables. It only needs to recognize the RSD PTR (which we can easily do now, in a way that was suggested by Michael). Bugs in ACPI payload, including inter-table relationships, will now be caused by QEMU, and will have to be fixed in QEMU; they can remain transparent to OVMF. > How did Xen manage to end up using EFI_ACPI_TABLE_PROTOCOL if it is > not possible or practical for QEMU? > > What is it about EFI_ACPI_TABLE_PROTOCOL that is such a bad fit for VMs? The difference is simply in how the different hypervisors provide the ACPI stuff to the firmware. Xen elected for a per-table export, which OVMF can super-easily pass to EFI_ACPI_TABLE_PROTOCOL, because the latter expects individual tables as well. QEMU chose a different path. The interface does not expose ACPI tables as such, it exposes blobs that may contain binary data that, once correctly cross-linked and checksummed, will be understood by a full-blown ACPI interpreter (the ones you can find in OS kernels). This is *very* convenient for SeaBIOS. We tried to split the blobs into individual tables before, so that we could pass each table to EFI_ACPI_TABLE_PROTOCOL individually. This splitting logic turned out to be brittle. Although there wasn't (and still isn't) any semi-official, written contract for the linker/loader interface, our splitting logic did not meet the *intent* of the qemu interface; it abused it. And, in practice, please see this recent qemu-devel discussion -- an example where our current, splitter approach would break down. It's about TPM / TCPA. http://thread.gmane.org/gmane.comp.emulators.qemu/287985 This thread was what finally convinced me that I'd have to implement the full thing. BTW you don't have to look at Xen for a per-table approach. If you check QEMU's new SMBIOS (as opposed to ACPI) generator (for which Gabriel has contributed the OVMF client code too), that's table-based as well, and uses EFI_SMBIOS_PROTOCOL. Now, if you want to know why the QEMU ACPI interface was *designed* like this, ie. with such a mismatch for EFI_ACPI_TABLE_PROTOCOL, I'm partially to blame for that. First, I was unusually overloaded at that time. Second, moving the ACPI generator from SeaBIOS to QEMU was like a small civil war in the qemu developer community. By the time this conflict progressed to a phase where actual, technical specifics were possible to discuss, the social traits of that discussion had made me want to have nothing to do with it. As a thin-skinned person I was incapable of collaborating on the design, and provide hints about UEFI compatibility in a timely manner. Ultimately, the interface was specialized for the QEMU and SeaBIOS, and largely ignored OVMF. TL;DR: I should have insisted on an interface that would match EFI_ACPI_TABLE_PROTOCOL better, but I had very little time, plus I could not bear participating in that ACPI debate and the later design. I failed to "steer" the design closer to OVMF. Sorry. Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
On Mon, Aug 25, 2014 at 4:27 PM, Laszlo Ersek <lersek@redhat.com> wrote: > On 08/26/14 00:24, Jordan Justen wrote: >> On Fri, Aug 8, 2014 at 7:08 PM, Laszlo Ersek <lersek@redhat.com> wrote: >>> Recent changes in the QEMU ACPI table generator have shown that our >>> limited client for that interface is insufficient and/or brittle. >>> >>> Implement the full interface utilizing OrderedCollectionLib for addressing >>> fw_cfg blobs by name. >>> >>> In addition, we stop using EFI_ACPI_TABLE_PROTOCOL in the QEMU loader >>> client, because: >>> - splitting the blobs for EFI_ACPI_TABLE_PROTOCOL is just untenable short >>> of a complete ACPI parser, >>> - and it is fully sufficient to install the RSD PTR as an EFI >>> configuration table for the guest OS to see everything. >>> >>> In short, for this interface, EFI_ACPI_TABLE_PROTOCOL is an unwanted and >>> restrictive convenience; let's stop using it. >> >> Hmm. Been through a few rounds of this for Xen. It's QEMU's turn now? :) >> >> Is this required? > > Depends on how good (how accurate) ACPI tables you want to have in your > VMs. For a few qemu releases now, qemu has been the "master" of ACPI > payload generation. SeaBIOS too has kept its own builtin tables, yes, > but when the QEMU generated payload is available, it interprets this > linker/loader script just the same. > > If you want PCI hotplug, pvpanic support; or, going forward, memory > hotplug, then yes, those things need ACPI support, and the ACPI stuff > for them is now developed in QEMU (and dynamically generated by it, > dependent on the actual hardware config). > >> What happens if another driver uses EFI_ACPI_TABLE_PROTOCOL? We now >> have two drivers that want to install the pointer. > > Yes, I checked that. The UEFI spec says that whenever you install a > configuration table that's already present (by GUID), the reference is > updated. > > InstallConfigurationTable() > > [...] > * If Guid is present in the System Table, and Table is not NULL, then > the (Guid, Table) pair is updated with the new Table value. > [...] > > For this reason, the first thing InstallAllQemuLinkedTables() does is > check, with EfiGetSystemConfigurationTable(), for the presence of any of > the ACPI GUIDs in the config table. > > There's no "absolute" cure against another driver in OVMF just grabbing > EFI_ACPI_TABLE_PROTOCOL and installing its own stuff (which would then > completely hide the tables installed by this patchset, because > "MdeModulePkg/Universal/Acpi/AcpiTableDxe" would simply replace QEMU's > RSD PTR's address in the UEFI config table with its own). > > But ACPI tables are not randomly installed in OVMF, we keep it > centralized in AcpiTableDxe. I don't agree with this statement. Rather, I would say that OVMF follows the EDK II method of publishing tables, which means EFI_ACPI_TABLE_PROTOCOL. In the past we were a good little sample platform, and provided the ACPI tables directly. Well, that is less and less the case. But, is it a good idea to stop using EFI_ACPI_TABLE_PROTOCOL? What about MdeModulePkg/Universal/Network/IScsiDxe/IScsiIbft.c, which uses EFI_ACPI_TABLE_PROTOCOL? Do we need to keep monitoring which EDK II drivers decide to use EFI_ACPI_TABLE_PROTOCOL, or can we find a way to make these work together? If the answer is no, then I wonder if this is something that UEFI or EDK II needs to consider. -Jordan >> It seems the AcpiTableDxe driver has had to fix things on occasion to >> cover some corner cases of table publishing. Did you capture all >> these? What about future bugs? > > The point of this series, and more importantly, qemu's linker/loader > interface, is that the firmware can remain dumb. It doesn't need to know > about inter-table relationships, it doesn't need to know about tables. > It only needs to recognize the RSD PTR (which we can easily do now, in a > way that was suggested by Michael). > > Bugs in ACPI payload, including inter-table relationships, will now be > caused by QEMU, and will have to be fixed in QEMU; they can remain > transparent to OVMF. > >> How did Xen manage to end up using EFI_ACPI_TABLE_PROTOCOL if it is >> not possible or practical for QEMU? >> >> What is it about EFI_ACPI_TABLE_PROTOCOL that is such a bad fit for VMs? > > The difference is simply in how the different hypervisors provide the > ACPI stuff to the firmware. Xen elected for a per-table export, which > OVMF can super-easily pass to EFI_ACPI_TABLE_PROTOCOL, because the > latter expects individual tables as well. > > QEMU chose a different path. The interface does not expose ACPI tables > as such, it exposes blobs that may contain binary data that, once > correctly cross-linked and checksummed, will be understood by a > full-blown ACPI interpreter (the ones you can find in OS kernels). This > is *very* convenient for SeaBIOS. > > We tried to split the blobs into individual tables before, so that we > could pass each table to EFI_ACPI_TABLE_PROTOCOL individually. > > This splitting logic turned out to be brittle. Although there wasn't > (and still isn't) any semi-official, written contract for the > linker/loader interface, our splitting logic did not meet the *intent* > of the qemu interface; it abused it. > > And, in practice, please see this recent qemu-devel discussion -- an > example where our current, splitter approach would break down. It's > about TPM / TCPA. > > http://thread.gmane.org/gmane.comp.emulators.qemu/287985 > > This thread was what finally convinced me that I'd have to implement the > full thing. > > BTW you don't have to look at Xen for a per-table approach. If you check > QEMU's new SMBIOS (as opposed to ACPI) generator (for which Gabriel has > contributed the OVMF client code too), that's table-based as well, and > uses EFI_SMBIOS_PROTOCOL. > > Now, if you want to know why the QEMU ACPI interface was *designed* like > this, ie. with such a mismatch for EFI_ACPI_TABLE_PROTOCOL, I'm > partially to blame for that. > > First, I was unusually overloaded at that time. > > Second, moving the ACPI generator from SeaBIOS to QEMU was like a small > civil war in the qemu developer community. By the time this conflict > progressed to a phase where actual, technical specifics were possible to > discuss, the social traits of that discussion had made me want to have > nothing to do with it. As a thin-skinned person I was incapable of > collaborating on the design, and provide hints about UEFI compatibility > in a timely manner. Ultimately, the interface was specialized for the > QEMU and SeaBIOS, and largely ignored OVMF. > > TL;DR: I should have insisted on an interface that would match > EFI_ACPI_TABLE_PROTOCOL better, but I had very little time, plus I could > not bear participating in that ACPI debate and the later design. I > failed to "steer" the design closer to OVMF. Sorry. > > Laszlo > > ------------------------------------------------------------------------------ > Slashdot TV. > Video for Nerds. Stuff that matters. > http://tv.slashdot.org/ > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
On 08/26/14 02:12, Jordan Justen wrote: > On Mon, Aug 25, 2014 at 4:27 PM, Laszlo Ersek <lersek@redhat.com> wrote: >> On 08/26/14 00:24, Jordan Justen wrote: >>> On Fri, Aug 8, 2014 at 7:08 PM, Laszlo Ersek <lersek@redhat.com> wrote: >>>> Recent changes in the QEMU ACPI table generator have shown that our >>>> limited client for that interface is insufficient and/or brittle. >>>> >>>> Implement the full interface utilizing OrderedCollectionLib for addressing >>>> fw_cfg blobs by name. >>>> >>>> In addition, we stop using EFI_ACPI_TABLE_PROTOCOL in the QEMU loader >>>> client, because: >>>> - splitting the blobs for EFI_ACPI_TABLE_PROTOCOL is just untenable short >>>> of a complete ACPI parser, >>>> - and it is fully sufficient to install the RSD PTR as an EFI >>>> configuration table for the guest OS to see everything. >>>> >>>> In short, for this interface, EFI_ACPI_TABLE_PROTOCOL is an unwanted and >>>> restrictive convenience; let's stop using it. >>> >>> Hmm. Been through a few rounds of this for Xen. It's QEMU's turn now? :) >>> >>> Is this required? >> >> Depends on how good (how accurate) ACPI tables you want to have in your >> VMs. For a few qemu releases now, qemu has been the "master" of ACPI >> payload generation. SeaBIOS too has kept its own builtin tables, yes, >> but when the QEMU generated payload is available, it interprets this >> linker/loader script just the same. >> >> If you want PCI hotplug, pvpanic support; or, going forward, memory >> hotplug, then yes, those things need ACPI support, and the ACPI stuff >> for them is now developed in QEMU (and dynamically generated by it, >> dependent on the actual hardware config). >> >>> What happens if another driver uses EFI_ACPI_TABLE_PROTOCOL? We now >>> have two drivers that want to install the pointer. >> >> Yes, I checked that. The UEFI spec says that whenever you install a >> configuration table that's already present (by GUID), the reference is >> updated. >> >> InstallConfigurationTable() >> >> [...] >> * If Guid is present in the System Table, and Table is not NULL, then >> the (Guid, Table) pair is updated with the new Table value. >> [...] >> >> For this reason, the first thing InstallAllQemuLinkedTables() does is >> check, with EfiGetSystemConfigurationTable(), for the presence of any of >> the ACPI GUIDs in the config table. >> >> There's no "absolute" cure against another driver in OVMF just grabbing >> EFI_ACPI_TABLE_PROTOCOL and installing its own stuff (which would then >> completely hide the tables installed by this patchset, because >> "MdeModulePkg/Universal/Acpi/AcpiTableDxe" would simply replace QEMU's >> RSD PTR's address in the UEFI config table with its own). >> >> But ACPI tables are not randomly installed in OVMF, we keep it >> centralized in AcpiTableDxe. > > I don't agree with this statement. Rather, I would say that OVMF > follows the EDK II method of publishing tables, which means > EFI_ACPI_TABLE_PROTOCOL. > > In the past we were a good little sample platform, and provided the > ACPI tables directly. Well, that is less and less the case. But, is it > a good idea to stop using EFI_ACPI_TABLE_PROTOCOL? No. It's not a good idea. But if we want to support the qemu interface, in the manner that interface has been intended, then I can see no way around avoiding EFI_ACPI_TABLE_PROTOCOL. The only way you could reliably fish out tables, operation regions etc. from the qemu payload would be to write a near-full ACPI interpreter. The goal of the interface is the polar opposite, ie. to require the firmware to know the least possible about ACPI table specifics. > What about MdeModulePkg/Universal/Network/IScsiDxe/IScsiIbft.c, which > uses EFI_ACPI_TABLE_PROTOCOL? Good find. > Do we need to keep monitoring which EDK II drivers decide to use > EFI_ACPI_TABLE_PROTOCOL, I guess so. > or can we find a way to make these work > together? I doubt it. The current in-tree solution is probably as good as it gets with EFI_ACPI_TABLE_PROTOCOL. It would co-operate with IScsiIbft.c, but (if you read the TPM thread I linked from qemu-devel) it would install a broken TCPA table from QEMU, for example. > If the answer is no, then I wonder if this is something that > UEFI or EDK II needs to consider. Maybe it is. As far as the UEFI spec goes, EFI_ACPI_TABLE_PROTOCOL looks like the canonical way. RSD PTR being referenced in the config table is not even part of the UEFI spec (if I searched it right); it's described in the ACPI spec. The UEFI spec only names the relevant GUIDs, but doesn't explain how to use them. In any case, under the IBFT example you name, we have a conflict because ACPI tables have two genuine, distinct sources. IBFT comes from the firmware, while the rest would come from QEMU. (I don't know if IBFT could be provided by QEMU. The ACPI 5.1 spec doesn't describe the table. Googling led me to a license acceptance window at Microsoft's site, which I closed.) In practice, IBFT seems less important than the tables that QEMU does offer -- embedded in blobs -- eg. I forgot to mention VCPU hotplug. Thanks, Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
On Aug 25, 2014, at 6:03 PM, Laszlo Ersek <lersek@redhat.com> wrote: > On 08/26/14 02:12, Jordan Justen wrote: >> On Mon, Aug 25, 2014 at 4:27 PM, Laszlo Ersek <lersek@redhat.com> wrote: >>> On 08/26/14 00:24, Jordan Justen wrote: >>>> On Fri, Aug 8, 2014 at 7:08 PM, Laszlo Ersek <lersek@redhat.com> wrote: >>>>> Recent changes in the QEMU ACPI table generator have shown that our >>>>> limited client for that interface is insufficient and/or brittle. >>>>> >>>>> Implement the full interface utilizing OrderedCollectionLib for addressing >>>>> fw_cfg blobs by name. >>>>> >>>>> In addition, we stop using EFI_ACPI_TABLE_PROTOCOL in the QEMU loader >>>>> client, because: >>>>> - splitting the blobs for EFI_ACPI_TABLE_PROTOCOL is just untenable short >>>>> of a complete ACPI parser, >>>>> - and it is fully sufficient to install the RSD PTR as an EFI >>>>> configuration table for the guest OS to see everything. >>>>> >>>>> In short, for this interface, EFI_ACPI_TABLE_PROTOCOL is an unwanted and >>>>> restrictive convenience; let's stop using it. >>>> >>>> Hmm. Been through a few rounds of this for Xen. It's QEMU's turn now? :) >>>> >>>> Is this required? >>> >>> Depends on how good (how accurate) ACPI tables you want to have in your >>> VMs. For a few qemu releases now, qemu has been the "master" of ACPI >>> payload generation. SeaBIOS too has kept its own builtin tables, yes, >>> but when the QEMU generated payload is available, it interprets this >>> linker/loader script just the same. >>> >>> If you want PCI hotplug, pvpanic support; or, going forward, memory >>> hotplug, then yes, those things need ACPI support, and the ACPI stuff >>> for them is now developed in QEMU (and dynamically generated by it, >>> dependent on the actual hardware config). >>> >>>> What happens if another driver uses EFI_ACPI_TABLE_PROTOCOL? We now >>>> have two drivers that want to install the pointer. >>> >>> Yes, I checked that. The UEFI spec says that whenever you install a >>> configuration table that's already present (by GUID), the reference is >>> updated. >>> >>> InstallConfigurationTable() >>> >>> [...] >>> * If Guid is present in the System Table, and Table is not NULL, then >>> the (Guid, Table) pair is updated with the new Table value. >>> [...] >>> >>> For this reason, the first thing InstallAllQemuLinkedTables() does is >>> check, with EfiGetSystemConfigurationTable(), for the presence of any of >>> the ACPI GUIDs in the config table. >>> >>> There's no "absolute" cure against another driver in OVMF just grabbing >>> EFI_ACPI_TABLE_PROTOCOL and installing its own stuff (which would then >>> completely hide the tables installed by this patchset, because >>> "MdeModulePkg/Universal/Acpi/AcpiTableDxe" would simply replace QEMU's >>> RSD PTR's address in the UEFI config table with its own). >>> >>> But ACPI tables are not randomly installed in OVMF, we keep it >>> centralized in AcpiTableDxe. >> >> I don't agree with this statement. Rather, I would say that OVMF >> follows the EDK II method of publishing tables, which means >> EFI_ACPI_TABLE_PROTOCOL. >> >> In the past we were a good little sample platform, and provided the >> ACPI tables directly. Well, that is less and less the case. But, is it >> a good idea to stop using EFI_ACPI_TABLE_PROTOCOL? > > No. It's not a good idea. > > But if we want to support the qemu interface, in the manner that > interface has been intended, then I can see no way around avoiding > EFI_ACPI_TABLE_PROTOCOL. > > The only way you could reliably fish out tables, operation regions etc. > from the qemu payload would be to write a near-full ACPI interpreter. > The goal of the interface is the polar opposite, ie. to require the > firmware to know the least possible about ACPI table specifics. > I wonder if there are other cases when the platform “knows more” than the edk2 driver producing the ACPI tables? Maybe having a PCD feature flag to turn off ACPI table production in the modules would make sense? Thanks, Andrew Fish >> What about MdeModulePkg/Universal/Network/IScsiDxe/IScsiIbft.c, which >> uses EFI_ACPI_TABLE_PROTOCOL? > > Good find. > >> Do we need to keep monitoring which EDK II drivers decide to use >> EFI_ACPI_TABLE_PROTOCOL, > > I guess so. > >> or can we find a way to make these work >> together? > > I doubt it. > > The current in-tree solution is probably as good as it gets with > EFI_ACPI_TABLE_PROTOCOL. It would co-operate with IScsiIbft.c, but (if > you read the TPM thread I linked from qemu-devel) it would install a > broken TCPA table from QEMU, for example. > >> If the answer is no, then I wonder if this is something that >> UEFI or EDK II needs to consider. > > Maybe it is. > > As far as the UEFI spec goes, EFI_ACPI_TABLE_PROTOCOL looks like the > canonical way. > > RSD PTR being referenced in the config table is not even part of the > UEFI spec (if I searched it right); it's described in the ACPI spec. The > UEFI spec only names the relevant GUIDs, but doesn't explain how to use > them. > > In any case, under the IBFT example you name, we have a conflict because > ACPI tables have two genuine, distinct sources. IBFT comes from the > firmware, while the rest would come from QEMU. > > (I don't know if IBFT could be provided by QEMU. The ACPI 5.1 spec > doesn't describe the table. Googling led me to a license acceptance > window at Microsoft's site, which I closed.) > > In practice, IBFT seems less important than the tables that QEMU does > offer -- embedded in blobs -- eg. I forgot to mention VCPU hotplug. > > Thanks, > Laszlo > > ------------------------------------------------------------------------------ > Slashdot TV. > Video for Nerds. Stuff that matters. > http://tv.slashdot.org/ > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
Il 26/08/2014 03:03, Laszlo Ersek ha scritto: > > The only way you could reliably fish out tables, operation regions etc. > from the qemu payload would be to write a near-full ACPI interpreter. > The goal of the interface is the polar opposite, ie. to require the > firmware to know the least possible about ACPI table specifics. Could OvmfPkg include its own minimal implementation of AcpiTableProtocol? You can check that InstallAcpiTable calls do not override QEMU-provided tables (which is currently the case), and abort if that happens. OvmfPkg does not need to expose AcpiSdtProtocol---which I'm not even sure who uses; AcpiTableProtocol is small(ish) and a lot of code could go away as far as OVMF is concerned if you drop support for three ACPI versions. If you do that, you can parse only the QEMU-provided RSD PTR and RSDT (to find out which tables QEMU provides and avoid overriding them). In the meanwhile, UEFI could work on a new v2 AcpiTableProtocol that has an InstallAcpiTableNoRelocate function. With it, the QEMU ACPI table linker can, again, parse the RSDT, pass each table to InstallAcpiTableNoRelocate, and then discard the QEMU-provided RSDT. > RSD PTR being referenced in the config table is not even part of the > UEFI spec (if I searched it right); it's described in the ACPI spec. The > UEFI spec only names the relevant GUIDs, but doesn't explain how to use > them. > > In any case, under the IBFT example you name, we have a conflict because > ACPI tables have two genuine, distinct sources. IBFT comes from the > firmware, while the rest would come from QEMU. > > (I don't know if IBFT could be provided by QEMU. The ACPI 5.1 spec > doesn't describe the table. Googling led me to a license acceptance > window at Microsoft's site, which I closed.) No, it couldn't. The iBFT information depends on where the system boots from. Basically it's how the firmware tells the OS "I booted you from here, that's where you can get any other files you need". Basically it lets you configure your network and iSCSI initiator before you have a DHCP client. ftp://ftp.software.ibm.com/systems/support/bladecenter/iscsi_boot_firmware_table_v1.03.pdf Paolo > In practice, IBFT seems less important than the tables that QEMU does > offer -- embedded in blobs -- eg. I forgot to mention VCPU hotplug. ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
On 08/26/14 16:33, Paolo Bonzini wrote: > Il 26/08/2014 03:03, Laszlo Ersek ha scritto: >> >> The only way you could reliably fish out tables, operation regions etc. >> from the qemu payload would be to write a near-full ACPI interpreter. >> The goal of the interface is the polar opposite, ie. to require the >> firmware to know the least possible about ACPI table specifics. > > Could OvmfPkg include its own minimal implementation of > AcpiTableProtocol? You can check that InstallAcpiTable calls do not > override QEMU-provided tables (which is currently the case), and abort > if that happens. The problem is that for this check I need to determine the set of ACPI tables that QEMU provides. (Otherwise I can't detect a conflict in the InstallAcpiTable function().) In order to determine the QEMU provided set of tables, I need to locate all of the ACPI table headers in the fw_cfg blobs. Which is exactly what the current, in-tree code does (not in order to look for conflicts, but to pass those tables to EFI_ACPI_TABLE_PROTOCOL, one by one); and that approach is too brittle. > OvmfPkg does not need to expose AcpiSdtProtocol---which I'm not even > sure who uses; SDT is used in code that has intimate knowledge of some AML (binary) code, and needs to navigate the nodes to a specific node, to read or write some scalar value. It is quite similar to an absolutely minimal XPath implementation, conceptually. > AcpiTableProtocol is small(ish) and a lot of code could > go away as far as OVMF is concerned if you drop support for three ACPI > versions. > > If you do that, you can parse only the QEMU-provided RSD PTR and RSDT > (to find out which tables QEMU provides and avoid overriding them). The problem is that a number of tables are not linked into the RSDT, they are referenced by other tables. Basically, all the pointer fields in all tables that are updated (relocated / absolutized) by AddPointer commands, point *potentially* to further ACPI tables. (Not necessarily though. For example, the LASA field in TCPA is a pointer, but its pointee is not an ACPI table header. Another example is BGRT, whose ImageAddress field is a pointer, but doesn't point to another table. The current linker-loader *can* export an ACPI payload that includes some placeholder area pointed-to by TCPA.LASA. Similarly it could export an fw_cfg blob that contains the boot logo (the image data) itself, and a BGRT table whose ImageAddress field gets relocated to the image data with an AddPointer command.) So, in order to determine the full set of QEMU-exported ACPI tables, it doesn't suffice to scan the RSDT. We'd have to follow all pointer fields, recursively (eg. RSDT->FACP->DSDT), and even know if *under* each pointer we'll find another table, or just some header-less stuff. The above would mean two things at the same time: - precisely the ACPI knowledge that the linker/loader interface is supposed to obviate in the firmware, - precisely the ACPI knowledge that the current implementation of EFI_ACPI_TABLE_PROTOCOL encodes, and updates from time to time, when some new table is standardized (or when something already standardized breaks simply due to bugs). > In the meanwhile, UEFI could work on a new v2 AcpiTableProtocol that has > an InstallAcpiTableNoRelocate function. With it, the QEMU ACPI table > linker can, again, parse the RSDT, pass each table to > InstallAcpiTableNoRelocate, and then discard the QEMU-provided RSDT. > >> RSD PTR being referenced in the config table is not even part of the >> UEFI spec (if I searched it right); it's described in the ACPI spec. The >> UEFI spec only names the relevant GUIDs, but doesn't explain how to use >> them. >> >> In any case, under the IBFT example you name, we have a conflict because >> ACPI tables have two genuine, distinct sources. IBFT comes from the >> firmware, while the rest would come from QEMU. >> >> (I don't know if IBFT could be provided by QEMU. The ACPI 5.1 spec >> doesn't describe the table. Googling led me to a license acceptance >> window at Microsoft's site, which I closed.) > > No, it couldn't. The iBFT information depends on where the system boots > from. Basically it's how the firmware tells the OS "I booted you from > here, that's where you can get any other files you need". Basically it > lets you configure your network and iSCSI initiator before you have a > DHCP client. > > ftp://ftp.software.ibm.com/systems/support/bladecenter/iscsi_boot_firmware_table_v1.03.pdf Sigh. I suspected that iBFT would store stuff like UEFI device paths or some such (similarly to EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL), which qemu has no clue at all about. Thanks for the info though. Cheers Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
Il 26/08/2014 18:49, Laszlo Ersek ha scritto: > The problem is that a number of tables are not linked into the RSDT, > they are referenced by other tables. Basically, all the pointer fields > in all tables that are updated (relocated / absolutized) by AddPointer > commands, point *potentially* to further ACPI tables. Right, the FADT's pointers to FACS and DSDT are an exception. But are there others? And you don't really care about _where_ the tables are, just if they are there, and you know that FACS+DSDT are always provided by QEMU. > > The above would mean two things at the same time: > - precisely the ACPI knowledge that the linker/loader interface is > supposed to obviate in the firmware, > - precisely the ACPI knowledge that the current implementation of > EFI_ACPI_TABLE_PROTOCOL encodes, and updates from time to time, when > some new table is standardized (or when something already standardized > breaks simply due to bugs). Yes, this is unfortunate. But I still believe that all you need to know is the format of the RSDT/XSDT. Is it possible to at least provide a *dummy* implementation of the ACPI table protocol, that returns an error? This would avoid weird situations where booting from iSCSI breaks CPU hotplug or something like that. Paolo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
On 08/27/14 11:47, Paolo Bonzini wrote: > Il 26/08/2014 18:49, Laszlo Ersek ha scritto: >> The problem is that a number of tables are not linked into the RSDT, >> they are referenced by other tables. Basically, all the pointer fields >> in all tables that are updated (relocated / absolutized) by AddPointer >> commands, point *potentially* to further ACPI tables. > > Right, the FADT's pointers to FACS and DSDT are an exception. But are > there others? And you don't really care about _where_ the tables are, > just if they are there, and you know that FACS+DSDT are always provided > by QEMU. This is probably implementable, if someone can come up with a clear list of reasonably future-proof rules. It would be good to avoid revisiting this OVMF code every now and then, whenever QEMU's ACPI generator undergoes a bit more intrusive changes. >> The above would mean two things at the same time: >> - precisely the ACPI knowledge that the linker/loader interface is >> supposed to obviate in the firmware, >> - precisely the ACPI knowledge that the current implementation of >> EFI_ACPI_TABLE_PROTOCOL encodes, and updates from time to time, when >> some new table is standardized (or when something already standardized >> breaks simply due to bugs). > > Yes, this is unfortunate. But I still believe that all you need to know > is the format of the RSDT/XSDT. > > Is it possible to at least provide a *dummy* implementation of the ACPI > table protocol, that returns an error? This would avoid weird > situations where booting from iSCSI breaks CPU hotplug or something like > that. Sure, that's certainly possible; it crossed my mind too. (We could also consistently lie and return EFI_SUCCESS -- who knows which one would do less damage. Drivers that depend on EFI_ACPI_TABLE_PROTOCOL *and* have sensible error handling would cope with a persistent error just fine (at worst the service wouldn't be available). But some drivers are lazy and just say ASSERT_EFI_ERROR(), and then an error would halt the firmware.) Anyway, implementing such a dummy driver would take a build time flag (-D ...), and when it was present, we'd build and include this dummy protocol implementation in the firmware, rather than the one from MdeModulePkg. (Because, although EFI_ACPI_TABLE_PROTOCOL is not an "architectural" protocol, it's definitely a "singleton" one, so clients just grab the first instance they find.) Not sure how Jordan would like this though :) Thanks Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
On 08/26/14 02:12, Jordan Justen wrote: > On Mon, Aug 25, 2014 at 4:27 PM, Laszlo Ersek <lersek@redhat.com> wrote: >> On 08/26/14 00:24, Jordan Justen wrote: >>> On Fri, Aug 8, 2014 at 7:08 PM, Laszlo Ersek <lersek@redhat.com> wrote: >>>> Recent changes in the QEMU ACPI table generator have shown that our >>>> limited client for that interface is insufficient and/or brittle. >>>> >>>> Implement the full interface utilizing OrderedCollectionLib for addressing >>>> fw_cfg blobs by name. >>>> >>>> In addition, we stop using EFI_ACPI_TABLE_PROTOCOL in the QEMU loader >>>> client, because: >>>> - splitting the blobs for EFI_ACPI_TABLE_PROTOCOL is just untenable short >>>> of a complete ACPI parser, >>>> - and it is fully sufficient to install the RSD PTR as an EFI >>>> configuration table for the guest OS to see everything. >>>> >>>> In short, for this interface, EFI_ACPI_TABLE_PROTOCOL is an unwanted and >>>> restrictive convenience; let's stop using it. >>> >>> Hmm. Been through a few rounds of this for Xen. It's QEMU's turn now? :) >>> >>> Is this required? >> >> Depends on how good (how accurate) ACPI tables you want to have in your >> VMs. For a few qemu releases now, qemu has been the "master" of ACPI >> payload generation. SeaBIOS too has kept its own builtin tables, yes, >> but when the QEMU generated payload is available, it interprets this >> linker/loader script just the same. >> >> If you want PCI hotplug, pvpanic support; or, going forward, memory >> hotplug, then yes, those things need ACPI support, and the ACPI stuff >> for them is now developed in QEMU (and dynamically generated by it, >> dependent on the actual hardware config). >> >>> What happens if another driver uses EFI_ACPI_TABLE_PROTOCOL? We now >>> have two drivers that want to install the pointer. >> >> Yes, I checked that. The UEFI spec says that whenever you install a >> configuration table that's already present (by GUID), the reference is >> updated. >> >> InstallConfigurationTable() >> >> [...] >> * If Guid is present in the System Table, and Table is not NULL, then >> the (Guid, Table) pair is updated with the new Table value. >> [...] >> >> For this reason, the first thing InstallAllQemuLinkedTables() does is >> check, with EfiGetSystemConfigurationTable(), for the presence of any of >> the ACPI GUIDs in the config table. >> >> There's no "absolute" cure against another driver in OVMF just grabbing >> EFI_ACPI_TABLE_PROTOCOL and installing its own stuff (which would then >> completely hide the tables installed by this patchset, because >> "MdeModulePkg/Universal/Acpi/AcpiTableDxe" would simply replace QEMU's >> RSD PTR's address in the UEFI config table with its own). >> >> But ACPI tables are not randomly installed in OVMF, we keep it >> centralized in AcpiTableDxe. > > I don't agree with this statement. Rather, I would say that OVMF > follows the EDK II method of publishing tables, which means > EFI_ACPI_TABLE_PROTOCOL. > > In the past we were a good little sample platform, and provided the > ACPI tables directly. Well, that is less and less the case. But, is it > a good idea to stop using EFI_ACPI_TABLE_PROTOCOL? > > What about MdeModulePkg/Universal/Network/IScsiDxe/IScsiIbft.c, which > uses EFI_ACPI_TABLE_PROTOCOL? > > Do we need to keep monitoring which EDK II drivers decide to use > EFI_ACPI_TABLE_PROTOCOL, or can we find a way to make these work > together? If the answer is no, then I wonder if this is something that > UEFI or EDK II needs to consider. I think I might have found a solution -- at this point it's just an idea, but I want to run it by you guys first before I start implementing it. The idea is based on this patchset, in an incremental fashion. Where the current code calls gBS->InstallConfigurationTable(), we shall replace that call with further processing. Michael has proposed before to check the *targets* of all the pointer commands. Namely, a pointed-to "something" in the blob that hosts it is with a good chance an ACPI table. Not guaranteed, but likely. Now, after all is done by the current patchset *except* linking the RSDP into the UEFI config table, the payload we have in AcpiNVS memory is a nicely cross-linked, checksummed forest of ACPI stuff. We re-scan the QEMU commands. We ignore everything different from AddPointer. When an AddPointer call is found, the following statements are true: (a) the pointer field in the blob that the AddPointer call would originally patch *now* contains a valid, absolute physical address. (b) the pointed-to byte-array may or may not be an ACPI table. It could be an ACPI table, or it could be some funny area that QEMU has preallocated, like the target of the TCPA.LASA field, or the target of the BGRT.ImageAddress field, or something else. (c) *if* the pointed-to byte-array is an ACPI table, then we'll have it already checksummed, since we're past the first complete pass of the processing. So the idea is, look at the target area, - determine if the remaining size in that blob (the pointed-to blob) could still contain an ACPI table header, - if so, check the presumed "length" field in that header, and see if it's self-consistent (ie. >= sizeof(header), and <= remaining size in blob) - if so, run a checksum on the bytes that presumably constitute the ACPI table - if it sums to zero, we attempt to install the target area with EFI_ACPI_TABLE_PROTOCOL, otherwise we leave it alone. Now, EFI_ACPI_TABLE_PROTOCOL makes deep copies of the tables it gets, but we'll leave the entire forest that we've build during the first processing in place. Why? Because this way all the stuff that *didn't* look like an ACPI table during the procedure above will remain in place, and the pointers *to* them will remain valid, in those ACPI table copies that EFI_ACPI_TABLE_PROTOCOL creates. For example, consider a BGRT table, where QEMU places both the BGRT in the blob, and the image data right after it (and sets BGRT.ImageAddress to the relative address inside the blob where the image data starts). The above procedure will result in: - the BGRT table itself being allocated twice, once by our first pass, and once by EFI_ACPI_TABLE_PROTOCOL itself (from the second pass). The first instance will be leaked (it won't be reachable from the system table that EFI_ACPI_TABLE_PROTOCOL now manages), but it's not grave. - the BGRT image data will be allocated only once, from our first pass, and it will be referenced *both* from our first-pass BGRT table (which is irrelevant) *and* from the BGRT copy that EFI_ACPI_TABLE_PROTOCOL installs (which is relevant). This approach would leak a few pages of AcpiNVS memory, and it has a slight chance to call EFI_ACPI_TABLE_PROTOCOL.InstallTable() with garbage (that has a valid-looking Length field and checksums to zero). But it would remain compatible with EFI_ACPI_TABLE_PROTOCOL, and it wouldn't miss anything from QEMU's payload that in fact *is* an ACPI table. Deal? Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
On 09/05/14 10:52, Laszlo Ersek wrote: > On 08/26/14 02:12, Jordan Justen wrote: >> On Mon, Aug 25, 2014 at 4:27 PM, Laszlo Ersek <lersek@redhat.com> wrote: >>> On 08/26/14 00:24, Jordan Justen wrote: >>>> On Fri, Aug 8, 2014 at 7:08 PM, Laszlo Ersek <lersek@redhat.com> wrote: >>>>> Recent changes in the QEMU ACPI table generator have shown that our >>>>> limited client for that interface is insufficient and/or brittle. >>>>> >>>>> Implement the full interface utilizing OrderedCollectionLib for addressing >>>>> fw_cfg blobs by name. >>>>> >>>>> In addition, we stop using EFI_ACPI_TABLE_PROTOCOL in the QEMU loader >>>>> client, because: >>>>> - splitting the blobs for EFI_ACPI_TABLE_PROTOCOL is just untenable short >>>>> of a complete ACPI parser, >>>>> - and it is fully sufficient to install the RSD PTR as an EFI >>>>> configuration table for the guest OS to see everything. >>>>> >>>>> In short, for this interface, EFI_ACPI_TABLE_PROTOCOL is an unwanted and >>>>> restrictive convenience; let's stop using it. >>>> >>>> Hmm. Been through a few rounds of this for Xen. It's QEMU's turn now? :) >>>> >>>> Is this required? >>> >>> Depends on how good (how accurate) ACPI tables you want to have in your >>> VMs. For a few qemu releases now, qemu has been the "master" of ACPI >>> payload generation. SeaBIOS too has kept its own builtin tables, yes, >>> but when the QEMU generated payload is available, it interprets this >>> linker/loader script just the same. >>> >>> If you want PCI hotplug, pvpanic support; or, going forward, memory >>> hotplug, then yes, those things need ACPI support, and the ACPI stuff >>> for them is now developed in QEMU (and dynamically generated by it, >>> dependent on the actual hardware config). >>> >>>> What happens if another driver uses EFI_ACPI_TABLE_PROTOCOL? We now >>>> have two drivers that want to install the pointer. >>> >>> Yes, I checked that. The UEFI spec says that whenever you install a >>> configuration table that's already present (by GUID), the reference is >>> updated. >>> >>> InstallConfigurationTable() >>> >>> [...] >>> * If Guid is present in the System Table, and Table is not NULL, then >>> the (Guid, Table) pair is updated with the new Table value. >>> [...] >>> >>> For this reason, the first thing InstallAllQemuLinkedTables() does is >>> check, with EfiGetSystemConfigurationTable(), for the presence of any of >>> the ACPI GUIDs in the config table. >>> >>> There's no "absolute" cure against another driver in OVMF just grabbing >>> EFI_ACPI_TABLE_PROTOCOL and installing its own stuff (which would then >>> completely hide the tables installed by this patchset, because >>> "MdeModulePkg/Universal/Acpi/AcpiTableDxe" would simply replace QEMU's >>> RSD PTR's address in the UEFI config table with its own). >>> >>> But ACPI tables are not randomly installed in OVMF, we keep it >>> centralized in AcpiTableDxe. >> >> I don't agree with this statement. Rather, I would say that OVMF >> follows the EDK II method of publishing tables, which means >> EFI_ACPI_TABLE_PROTOCOL. >> >> In the past we were a good little sample platform, and provided the >> ACPI tables directly. Well, that is less and less the case. But, is it >> a good idea to stop using EFI_ACPI_TABLE_PROTOCOL? >> >> What about MdeModulePkg/Universal/Network/IScsiDxe/IScsiIbft.c, which >> uses EFI_ACPI_TABLE_PROTOCOL? >> >> Do we need to keep monitoring which EDK II drivers decide to use >> EFI_ACPI_TABLE_PROTOCOL, or can we find a way to make these work >> together? If the answer is no, then I wonder if this is something that >> UEFI or EDK II needs to consider. > > I think I might have found a solution -- at this point it's just an > idea, but I want to run it by you guys first before I start implementing > it. The idea is based on this patchset, in an incremental fashion. > > Where the current code calls gBS->InstallConfigurationTable(), we shall > replace that call with further processing. > > Michael has proposed before to check the *targets* of all the pointer > commands. Namely, a pointed-to "something" in the blob that hosts it is > with a good chance an ACPI table. Not guaranteed, but likely. > > Now, after all is done by the current patchset *except* linking the RSDP > into the UEFI config table, the payload we have in AcpiNVS memory is a > nicely cross-linked, checksummed forest of ACPI stuff. We re-scan the > QEMU commands. We ignore everything different from AddPointer. When an > AddPointer call is found, the following statements are true: > > (a) the pointer field in the blob that the AddPointer call would > originally patch *now* contains a valid, absolute physical address. > > (b) the pointed-to byte-array may or may not be an ACPI table. It could > be an ACPI table, or it could be some funny area that QEMU has > preallocated, like the target of the TCPA.LASA field, or the target of > the BGRT.ImageAddress field, or something else. > > (c) *if* the pointed-to byte-array is an ACPI table, then we'll have it > already checksummed, since we're past the first complete pass of the > processing. > > So the idea is, look at the target area, > - determine if the remaining size in that blob (the pointed-to blob) > could still contain an ACPI table header, > - if so, check the presumed "length" field in that header, and see if > it's self-consistent (ie. >= sizeof(header), and <= remaining size in > blob) > - if so, run a checksum on the bytes that presumably constitute the > ACPI table > - if it sums to zero, we attempt to install the target area with > EFI_ACPI_TABLE_PROTOCOL, otherwise we leave it alone. > > Now, EFI_ACPI_TABLE_PROTOCOL makes deep copies of the tables it gets, > but we'll leave the entire forest that we've build during the first > processing in place. Why? Because this way all the stuff that *didn't* > look like an ACPI table during the procedure above will remain in place, > and the pointers *to* them will remain valid, in those ACPI table copies > that EFI_ACPI_TABLE_PROTOCOL creates. > > For example, consider a BGRT table, where QEMU places both the BGRT in > the blob, and the image data right after it (and sets BGRT.ImageAddress > to the relative address inside the blob where the image data starts). > The above procedure will result in: > > - the BGRT table itself being allocated twice, once by our first pass, > and once by EFI_ACPI_TABLE_PROTOCOL itself (from the second pass). The > first instance will be leaked (it won't be reachable from the system > table that EFI_ACPI_TABLE_PROTOCOL now manages), but it's not grave. > > - the BGRT image data will be allocated only once, from our first pass, > and it will be referenced *both* from our first-pass BGRT table (which > is irrelevant) *and* from the BGRT copy that EFI_ACPI_TABLE_PROTOCOL > installs (which is relevant). > > This approach would leak a few pages of AcpiNVS memory, and it has a > slight chance to call EFI_ACPI_TABLE_PROTOCOL.InstallTable() with > garbage (that has a valid-looking Length field and checksums to zero). > But it would remain compatible with EFI_ACPI_TABLE_PROTOCOL, and it > wouldn't miss anything from QEMU's payload that in fact *is* an ACPI table. In addition, I could tighten the Length + checksum validation with ACPI_BUILD_APPNAME6 and ACPI_BUILD_APPNAME4 checks, according to qemu's build_header() function -- if Michael agrees that these are stable. IOW, the OEMID would have to be "BOCHS ", and the first four bytes of OEMTableID would have to be "BXPC". I think these four checks together are pretty strong: a static check for a *10-byte* signature (in effect), and a dynamic check for length + checksum. I think this should be acceptable. Thanks, Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
> So the idea is, look at the target area, > - determine if the remaining size in that blob (the pointed-to blob) > could still contain an ACPI table header, > - if so, check the presumed "length" field in that header, and see if > it's self-consistent (ie. >= sizeof(header), and <= remaining size in > blob) > - if so, run a checksum on the bytes that presumably constitute the > ACPI table > - if it sums to zero, we attempt to install the target area with > EFI_ACPI_TABLE_PROTOCOL, otherwise we leave it alone. This is clever. But maybe it is even simpler to do look at the few tables that contain pointers to other tables (RSDP -> RSDT, RSDT -> FADT and more, FADT -> DSDT, FADT -> FACS) and pass those to EFI_ACPI_TABLE_PROTOCOL. Of course the _other_ clever, and IMHO more important, part of your scheme... > Now, EFI_ACPI_TABLE_PROTOCOL makes deep copies of the tables it gets, > but we'll leave the entire forest that we've build during the first > processing in place. Why? Because this way all the stuff that *didn't* > look like an ACPI table during the procedure above will remain in place, > and the pointers *to* them will remain valid, in those ACPI table copies > that EFI_ACPI_TABLE_PROTOCOL creates. ... i.e. this one, remains unchanged. > This approach would leak a few pages of AcpiNVS memory, and it has a > slight chance to call EFI_ACPI_TABLE_PROTOCOL.InstallTable() with > garbage (that has a valid-looking Length field and checksums to zero). > But it would remain compatible with EFI_ACPI_TABLE_PROTOCOL, and it > wouldn't miss anything from QEMU's payload that in fact *is* an ACPI table. > > Deal? As far as the leak is concerned, definitely. For the other part, whatever you think is simpler will do for me. Paolo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
On Fri, Sep 5, 2014 at 2:03 AM, Laszlo Ersek <lersek@redhat.com> wrote: > On 09/05/14 10:52, Laszlo Ersek wrote: >> On 08/26/14 02:12, Jordan Justen wrote: >>> On Mon, Aug 25, 2014 at 4:27 PM, Laszlo Ersek <lersek@redhat.com> wrote: >>>> On 08/26/14 00:24, Jordan Justen wrote: >>>>> On Fri, Aug 8, 2014 at 7:08 PM, Laszlo Ersek <lersek@redhat.com> wrote: >>>>>> Recent changes in the QEMU ACPI table generator have shown that our >>>>>> limited client for that interface is insufficient and/or brittle. >>>>>> >>>>>> Implement the full interface utilizing OrderedCollectionLib for addressing >>>>>> fw_cfg blobs by name. >>>>>> >>>>>> In addition, we stop using EFI_ACPI_TABLE_PROTOCOL in the QEMU loader >>>>>> client, because: >>>>>> - splitting the blobs for EFI_ACPI_TABLE_PROTOCOL is just untenable short >>>>>> of a complete ACPI parser, >>>>>> - and it is fully sufficient to install the RSD PTR as an EFI >>>>>> configuration table for the guest OS to see everything. >>>>>> >>>>>> In short, for this interface, EFI_ACPI_TABLE_PROTOCOL is an unwanted and >>>>>> restrictive convenience; let's stop using it. >>>>> >>>>> Hmm. Been through a few rounds of this for Xen. It's QEMU's turn now? :) >>>>> >>>>> Is this required? >>>> >>>> Depends on how good (how accurate) ACPI tables you want to have in your >>>> VMs. For a few qemu releases now, qemu has been the "master" of ACPI >>>> payload generation. SeaBIOS too has kept its own builtin tables, yes, >>>> but when the QEMU generated payload is available, it interprets this >>>> linker/loader script just the same. >>>> >>>> If you want PCI hotplug, pvpanic support; or, going forward, memory >>>> hotplug, then yes, those things need ACPI support, and the ACPI stuff >>>> for them is now developed in QEMU (and dynamically generated by it, >>>> dependent on the actual hardware config). >>>> >>>>> What happens if another driver uses EFI_ACPI_TABLE_PROTOCOL? We now >>>>> have two drivers that want to install the pointer. >>>> >>>> Yes, I checked that. The UEFI spec says that whenever you install a >>>> configuration table that's already present (by GUID), the reference is >>>> updated. >>>> >>>> InstallConfigurationTable() >>>> >>>> [...] >>>> * If Guid is present in the System Table, and Table is not NULL, then >>>> the (Guid, Table) pair is updated with the new Table value. >>>> [...] >>>> >>>> For this reason, the first thing InstallAllQemuLinkedTables() does is >>>> check, with EfiGetSystemConfigurationTable(), for the presence of any of >>>> the ACPI GUIDs in the config table. >>>> >>>> There's no "absolute" cure against another driver in OVMF just grabbing >>>> EFI_ACPI_TABLE_PROTOCOL and installing its own stuff (which would then >>>> completely hide the tables installed by this patchset, because >>>> "MdeModulePkg/Universal/Acpi/AcpiTableDxe" would simply replace QEMU's >>>> RSD PTR's address in the UEFI config table with its own). >>>> >>>> But ACPI tables are not randomly installed in OVMF, we keep it >>>> centralized in AcpiTableDxe. >>> >>> I don't agree with this statement. Rather, I would say that OVMF >>> follows the EDK II method of publishing tables, which means >>> EFI_ACPI_TABLE_PROTOCOL. >>> >>> In the past we were a good little sample platform, and provided the >>> ACPI tables directly. Well, that is less and less the case. But, is it >>> a good idea to stop using EFI_ACPI_TABLE_PROTOCOL? >>> >>> What about MdeModulePkg/Universal/Network/IScsiDxe/IScsiIbft.c, which >>> uses EFI_ACPI_TABLE_PROTOCOL? >>> >>> Do we need to keep monitoring which EDK II drivers decide to use >>> EFI_ACPI_TABLE_PROTOCOL, or can we find a way to make these work >>> together? If the answer is no, then I wonder if this is something that >>> UEFI or EDK II needs to consider. >> >> I think I might have found a solution -- at this point it's just an >> idea, but I want to run it by you guys first before I start implementing >> it. The idea is based on this patchset, in an incremental fashion. >> >> Where the current code calls gBS->InstallConfigurationTable(), we shall >> replace that call with further processing. >> >> Michael has proposed before to check the *targets* of all the pointer >> commands. Namely, a pointed-to "something" in the blob that hosts it is >> with a good chance an ACPI table. Not guaranteed, but likely. >> >> Now, after all is done by the current patchset *except* linking the RSDP >> into the UEFI config table, the payload we have in AcpiNVS memory is a >> nicely cross-linked, checksummed forest of ACPI stuff. We re-scan the >> QEMU commands. We ignore everything different from AddPointer. When an >> AddPointer call is found, the following statements are true: >> >> (a) the pointer field in the blob that the AddPointer call would >> originally patch *now* contains a valid, absolute physical address. >> >> (b) the pointed-to byte-array may or may not be an ACPI table. It could >> be an ACPI table, or it could be some funny area that QEMU has >> preallocated, like the target of the TCPA.LASA field, or the target of >> the BGRT.ImageAddress field, or something else. >> >> (c) *if* the pointed-to byte-array is an ACPI table, then we'll have it >> already checksummed, since we're past the first complete pass of the >> processing. >> >> So the idea is, look at the target area, >> - determine if the remaining size in that blob (the pointed-to blob) >> could still contain an ACPI table header, >> - if so, check the presumed "length" field in that header, and see if >> it's self-consistent (ie. >= sizeof(header), and <= remaining size in >> blob) >> - if so, run a checksum on the bytes that presumably constitute the >> ACPI table >> - if it sums to zero, we attempt to install the target area with >> EFI_ACPI_TABLE_PROTOCOL, otherwise we leave it alone. >> >> Now, EFI_ACPI_TABLE_PROTOCOL makes deep copies of the tables it gets, >> but we'll leave the entire forest that we've build during the first >> processing in place. Why? Because this way all the stuff that *didn't* >> look like an ACPI table during the procedure above will remain in place, >> and the pointers *to* them will remain valid, in those ACPI table copies >> that EFI_ACPI_TABLE_PROTOCOL creates. >> >> For example, consider a BGRT table, where QEMU places both the BGRT in >> the blob, and the image data right after it (and sets BGRT.ImageAddress >> to the relative address inside the blob where the image data starts). >> The above procedure will result in: >> >> - the BGRT table itself being allocated twice, once by our first pass, >> and once by EFI_ACPI_TABLE_PROTOCOL itself (from the second pass). The >> first instance will be leaked (it won't be reachable from the system >> table that EFI_ACPI_TABLE_PROTOCOL now manages), but it's not grave. >> >> - the BGRT image data will be allocated only once, from our first pass, >> and it will be referenced *both* from our first-pass BGRT table (which >> is irrelevant) *and* from the BGRT copy that EFI_ACPI_TABLE_PROTOCOL >> installs (which is relevant). >> >> This approach would leak a few pages of AcpiNVS memory, and it has a >> slight chance to call EFI_ACPI_TABLE_PROTOCOL.InstallTable() with >> garbage (that has a valid-looking Length field and checksums to zero). >> But it would remain compatible with EFI_ACPI_TABLE_PROTOCOL, and it >> wouldn't miss anything from QEMU's payload that in fact *is* an ACPI table. > > In addition, I could tighten the Length + checksum validation with > ACPI_BUILD_APPNAME6 and ACPI_BUILD_APPNAME4 checks, according to qemu's > build_header() function -- if Michael agrees that these are stable. IOW, > the OEMID would have to be "BOCHS ", and the first four bytes of > OEMTableID would have to be "BXPC". I think these four checks together > are pretty strong: a static check for a *10-byte* signature (in effect), > and a dynamic check for length + checksum. > > I think this should be acceptable. It sounds like it would solve my concern. It doesn't sound like it would be a huge amount of extra effort or code. The part about having to identify tables from the blob is not great, but it does seem like your plan is unlikely to mistakenly identify a non-table as a table. Is it possible to get more confirmation that the few wasted pages would remain small. I guess maybe the easiest argument to this would be that QEMU is not going to have a large AcpiNVS blob in the first place because it would always mean taking some RAM from the OS. -Jordan ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
On 09/05/14 17:42, Jordan Justen wrote: > On Fri, Sep 5, 2014 at 2:03 AM, Laszlo Ersek <lersek@redhat.com> wrote: >> On 09/05/14 10:52, Laszlo Ersek wrote: >>> On 08/26/14 02:12, Jordan Justen wrote: >>>> On Mon, Aug 25, 2014 at 4:27 PM, Laszlo Ersek <lersek@redhat.com> wrote: >>>>> On 08/26/14 00:24, Jordan Justen wrote: >>>>>> On Fri, Aug 8, 2014 at 7:08 PM, Laszlo Ersek <lersek@redhat.com> wrote: >>>>>>> Recent changes in the QEMU ACPI table generator have shown that our >>>>>>> limited client for that interface is insufficient and/or brittle. >>>>>>> >>>>>>> Implement the full interface utilizing OrderedCollectionLib for addressing >>>>>>> fw_cfg blobs by name. >>>>>>> >>>>>>> In addition, we stop using EFI_ACPI_TABLE_PROTOCOL in the QEMU loader >>>>>>> client, because: >>>>>>> - splitting the blobs for EFI_ACPI_TABLE_PROTOCOL is just untenable short >>>>>>> of a complete ACPI parser, >>>>>>> - and it is fully sufficient to install the RSD PTR as an EFI >>>>>>> configuration table for the guest OS to see everything. >>>>>>> >>>>>>> In short, for this interface, EFI_ACPI_TABLE_PROTOCOL is an unwanted and >>>>>>> restrictive convenience; let's stop using it. >>>>>> >>>>>> Hmm. Been through a few rounds of this for Xen. It's QEMU's turn now? :) >>>>>> >>>>>> Is this required? >>>>> >>>>> Depends on how good (how accurate) ACPI tables you want to have in your >>>>> VMs. For a few qemu releases now, qemu has been the "master" of ACPI >>>>> payload generation. SeaBIOS too has kept its own builtin tables, yes, >>>>> but when the QEMU generated payload is available, it interprets this >>>>> linker/loader script just the same. >>>>> >>>>> If you want PCI hotplug, pvpanic support; or, going forward, memory >>>>> hotplug, then yes, those things need ACPI support, and the ACPI stuff >>>>> for them is now developed in QEMU (and dynamically generated by it, >>>>> dependent on the actual hardware config). >>>>> >>>>>> What happens if another driver uses EFI_ACPI_TABLE_PROTOCOL? We now >>>>>> have two drivers that want to install the pointer. >>>>> >>>>> Yes, I checked that. The UEFI spec says that whenever you install a >>>>> configuration table that's already present (by GUID), the reference is >>>>> updated. >>>>> >>>>> InstallConfigurationTable() >>>>> >>>>> [...] >>>>> * If Guid is present in the System Table, and Table is not NULL, then >>>>> the (Guid, Table) pair is updated with the new Table value. >>>>> [...] >>>>> >>>>> For this reason, the first thing InstallAllQemuLinkedTables() does is >>>>> check, with EfiGetSystemConfigurationTable(), for the presence of any of >>>>> the ACPI GUIDs in the config table. >>>>> >>>>> There's no "absolute" cure against another driver in OVMF just grabbing >>>>> EFI_ACPI_TABLE_PROTOCOL and installing its own stuff (which would then >>>>> completely hide the tables installed by this patchset, because >>>>> "MdeModulePkg/Universal/Acpi/AcpiTableDxe" would simply replace QEMU's >>>>> RSD PTR's address in the UEFI config table with its own). >>>>> >>>>> But ACPI tables are not randomly installed in OVMF, we keep it >>>>> centralized in AcpiTableDxe. >>>> >>>> I don't agree with this statement. Rather, I would say that OVMF >>>> follows the EDK II method of publishing tables, which means >>>> EFI_ACPI_TABLE_PROTOCOL. >>>> >>>> In the past we were a good little sample platform, and provided the >>>> ACPI tables directly. Well, that is less and less the case. But, is it >>>> a good idea to stop using EFI_ACPI_TABLE_PROTOCOL? >>>> >>>> What about MdeModulePkg/Universal/Network/IScsiDxe/IScsiIbft.c, which >>>> uses EFI_ACPI_TABLE_PROTOCOL? >>>> >>>> Do we need to keep monitoring which EDK II drivers decide to use >>>> EFI_ACPI_TABLE_PROTOCOL, or can we find a way to make these work >>>> together? If the answer is no, then I wonder if this is something that >>>> UEFI or EDK II needs to consider. >>> >>> I think I might have found a solution -- at this point it's just an >>> idea, but I want to run it by you guys first before I start implementing >>> it. The idea is based on this patchset, in an incremental fashion. >>> >>> Where the current code calls gBS->InstallConfigurationTable(), we shall >>> replace that call with further processing. >>> >>> Michael has proposed before to check the *targets* of all the pointer >>> commands. Namely, a pointed-to "something" in the blob that hosts it is >>> with a good chance an ACPI table. Not guaranteed, but likely. >>> >>> Now, after all is done by the current patchset *except* linking the RSDP >>> into the UEFI config table, the payload we have in AcpiNVS memory is a >>> nicely cross-linked, checksummed forest of ACPI stuff. We re-scan the >>> QEMU commands. We ignore everything different from AddPointer. When an >>> AddPointer call is found, the following statements are true: >>> >>> (a) the pointer field in the blob that the AddPointer call would >>> originally patch *now* contains a valid, absolute physical address. >>> >>> (b) the pointed-to byte-array may or may not be an ACPI table. It could >>> be an ACPI table, or it could be some funny area that QEMU has >>> preallocated, like the target of the TCPA.LASA field, or the target of >>> the BGRT.ImageAddress field, or something else. >>> >>> (c) *if* the pointed-to byte-array is an ACPI table, then we'll have it >>> already checksummed, since we're past the first complete pass of the >>> processing. >>> >>> So the idea is, look at the target area, >>> - determine if the remaining size in that blob (the pointed-to blob) >>> could still contain an ACPI table header, >>> - if so, check the presumed "length" field in that header, and see if >>> it's self-consistent (ie. >= sizeof(header), and <= remaining size in >>> blob) >>> - if so, run a checksum on the bytes that presumably constitute the >>> ACPI table >>> - if it sums to zero, we attempt to install the target area with >>> EFI_ACPI_TABLE_PROTOCOL, otherwise we leave it alone. >>> >>> Now, EFI_ACPI_TABLE_PROTOCOL makes deep copies of the tables it gets, >>> but we'll leave the entire forest that we've build during the first >>> processing in place. Why? Because this way all the stuff that *didn't* >>> look like an ACPI table during the procedure above will remain in place, >>> and the pointers *to* them will remain valid, in those ACPI table copies >>> that EFI_ACPI_TABLE_PROTOCOL creates. >>> >>> For example, consider a BGRT table, where QEMU places both the BGRT in >>> the blob, and the image data right after it (and sets BGRT.ImageAddress >>> to the relative address inside the blob where the image data starts). >>> The above procedure will result in: >>> >>> - the BGRT table itself being allocated twice, once by our first pass, >>> and once by EFI_ACPI_TABLE_PROTOCOL itself (from the second pass). The >>> first instance will be leaked (it won't be reachable from the system >>> table that EFI_ACPI_TABLE_PROTOCOL now manages), but it's not grave. >>> >>> - the BGRT image data will be allocated only once, from our first pass, >>> and it will be referenced *both* from our first-pass BGRT table (which >>> is irrelevant) *and* from the BGRT copy that EFI_ACPI_TABLE_PROTOCOL >>> installs (which is relevant). >>> >>> This approach would leak a few pages of AcpiNVS memory, and it has a >>> slight chance to call EFI_ACPI_TABLE_PROTOCOL.InstallTable() with >>> garbage (that has a valid-looking Length field and checksums to zero). >>> But it would remain compatible with EFI_ACPI_TABLE_PROTOCOL, and it >>> wouldn't miss anything from QEMU's payload that in fact *is* an ACPI table. >> >> In addition, I could tighten the Length + checksum validation with >> ACPI_BUILD_APPNAME6 and ACPI_BUILD_APPNAME4 checks, according to qemu's >> build_header() function -- if Michael agrees that these are stable. IOW, >> the OEMID would have to be "BOCHS ", and the first four bytes of >> OEMTableID would have to be "BXPC". I think these four checks together >> are pretty strong: a static check for a *10-byte* signature (in effect), >> and a dynamic check for length + checksum. >> >> I think this should be acceptable. > > It sounds like it would solve my concern. > > It doesn't sound like it would be a huge amount of extra effort or code. > > The part about having to identify tables from the blob is not great, > but it does seem like your plan is unlikely to mistakenly identify a > non-table as a table. > > Is it possible to get more confirmation that the few wasted pages > would remain small. I guess maybe the easiest argument to this would > be that QEMU is not going to have a large AcpiNVS blob in the first > place because it would always mean taking some RAM from the OS. I've written the code such that it tracks, per fw_cfg blob, if the blob had any pointers into it that turned out to be non-tables. If it didn't, then the blob is released in the end. In my current testing, there are two blobs in total, /etc/acpi/rsdp and /etc/acpi/tables, and both are released. Hence no pages are wasted at all. In general, as far as I understand, QEMU developers don't intend to push any blobs > 64K, and an idea was raised to keep a minimal number of tables per blob. So this should limit the leakage even if a few blobs remain with "opaque" contents in them. I'll go through the patches again and post them soon. Thanks Laszlo ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
On Fri, Sep 5, 2014 at 2:03 AM, Laszlo Ersek <lersek@redhat.com> wrote: > In addition, I could tighten the Length + checksum validation with > ACPI_BUILD_APPNAME6 and ACPI_BUILD_APPNAME4 checks, according to qemu's > build_header() function -- if Michael agrees that these are stable. IOW, > the OEMID would have to be "BOCHS ", and the first four bytes of > OEMTableID would have to be "BXPC". I think these four checks together > are pretty strong: a static check for a *10-byte* signature (in effect), > and a dynamic check for length + checksum. Michael, what do you think about Laszlo's idea to verify "BOCHS" and "BXPC" in the tables? Can we assume that these won't be changing anytime soon? You also suggested a new flag to indicate that a blob is acpi data. I guess if we ever see that we can skip the extra ACPI table checks, such as OEMID/OEMTableID. -Jordan ------------------------------------------------------------------------------ Slashdot TV. Video for Nerds. Stuff that matters. http://tv.slashdot.org/
On Sun, Sep 7, 2014 at 11:17 AM, Jordan Justen <jljusten@gmail.com> wrote: > On Fri, Sep 5, 2014 at 2:03 AM, Laszlo Ersek <lersek@redhat.com> wrote: >> In addition, I could tighten the Length + checksum validation with >> ACPI_BUILD_APPNAME6 and ACPI_BUILD_APPNAME4 checks, according to qemu's >> build_header() function -- if Michael agrees that these are stable. IOW, >> the OEMID would have to be "BOCHS ", and the first four bytes of >> OEMTableID would have to be "BXPC". I think these four checks together >> are pretty strong: a static check for a *10-byte* signature (in effect), >> and a dynamic check for length + checksum. > > Michael, what do you think about Laszlo's idea to verify "BOCHS" and > "BXPC" in the tables? Can we assume that these won't be changing > anytime soon? > > You also suggested a new flag to indicate that a blob is acpi data. I > guess if we ever see that we can skip the extra ACPI table checks, > such as OEMID/OEMTableID. Laszlo, For your v2 series, Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> contingent on Michael agreeing that checking OEMID/OEMTableID is okay. Regarding the new flag, it seems like we can add support for that separately if it is implemented. -Jordan ------------------------------------------------------------------------------ Want excitement? Manually upgrade your production database. When you want reliability, choose Perforce. Perforce version control. Predictably reliable. http://pubads.g.doubleclick.net/gampad/clk?id=157508191&iu=/4140/ostg.clktrk
diff --git a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf index 90178e0..02eaf23 100644 --- a/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf +++ b/OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf @@ -51,12 +51,15 @@ MemoryAllocationLib BaseLib DxeServicesTableLib + OrderedCollectionLib [Protocols] gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED [Guids] gEfiXenInfoGuid + gEfiAcpi10TableGuid + gEfiAcpi20TableGuid [Pcd] gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiTableStorageFile diff --git a/OvmfPkg/AcpiPlatformDxe/Qemu.c b/OvmfPkg/AcpiPlatformDxe/Qemu.c index 333766e..4663ecb 100644 --- a/OvmfPkg/AcpiPlatformDxe/Qemu.c +++ b/OvmfPkg/AcpiPlatformDxe/Qemu.c @@ -17,12 +17,15 @@ #include "AcpiPlatform.h" #include "QemuLoader.h" -#include <Library/BaseMemoryLib.h> -#include <Library/MemoryAllocationLib.h> -#include <Library/QemuFwCfgLib.h> -#include <Library/DxeServicesTableLib.h> -#include <Library/PcdLib.h> -#include <IndustryStandard/Acpi.h> +#include <Library/BaseMemoryLib.h> // CopyMem() +#include <Library/MemoryAllocationLib.h> // AllocatePool() +#include <Library/QemuFwCfgLib.h> // QemuFwCfgFindFile() +#include <Library/DxeServicesTableLib.h> // gDS +#include <Library/PcdLib.h> // PcdGet16() +#include <Library/UefiLib.h> // EfiGetSystemConfigurationTable() +#include <Library/OrderedCollectionLib.h> // OrderedCollectionInit() +#include <IndustryStandard/Acpi.h> // EFI_ACPI_DESCRIPTION_HEADER +#include <Guid/Acpi.h> // gEfiAcpi10TableGuid BOOLEAN QemuDetected ( @@ -518,7 +521,8 @@ QemuInstallAcpiTable ( /** - Check if an array of bytes starts with an RSD PTR structure. + Check if an array of bytes starts with an RSD PTR structure, and if so, + return the EFI ACPI table GUID that corresponds to its version. Checksum is ignored. @@ -526,8 +530,9 @@ QemuInstallAcpiTable ( @param[in] Size Number of bytes in Buffer. - @param[out] RsdpSize If the function returns EFI_SUCCESS, this parameter - contains the size of the detected RSD PTR structure. + @param[out] AcpiTableGuid On successful exit, pointer to the EFI ACPI table + GUID in statically allocated storage that + corresponds to the detected RSD PTR version. @retval EFI_SUCCESS RSD PTR structure detected at the beginning of Buffer, and its advertised size does not exceed @@ -545,7 +550,7 @@ EFI_STATUS CheckRsdp ( IN CONST VOID *Buffer, IN UINTN Size, - OUT UINTN *RsdpSize + OUT EFI_GUID **AcpiTableGuid ) { CONST UINT64 *Signature; @@ -574,7 +579,7 @@ CheckRsdp ( // // ACPI 1.0 doesn't include the Length field // - *RsdpSize = sizeof *Rsdp1; + *AcpiTableGuid = &gEfiAcpi10TableGuid; return EFI_SUCCESS; } @@ -587,27 +592,99 @@ CheckRsdp ( return EFI_PROTOCOL_ERROR; } - *RsdpSize = Rsdp2->Length; + *AcpiTableGuid = &gEfiAcpi20TableGuid; return EFI_SUCCESS; } +// +// The user structure for the ordered collection that will track the fw_cfg +// blobs under processing. +// +typedef struct { + UINT8 File[QEMU_LOADER_FNAME_SIZE]; // NUL-terminated name of the fw_cfg + // blob. This is the ordering / search + // key. + UINTN Size; // The number of bytes in this blob. + UINT8 *Base; // Pointer to the blob data. +} BLOB; + +/** + Compare a standalone key against a user structure containing an embedded key. + + @param[in] StandaloneKey Pointer to the bare key. + + @param[in] UserStruct Pointer to the user structure with the embedded + key. + + @retval <0 If StandaloneKey compares less than UserStruct's key. + + @retval 0 If StandaloneKey compares equal to UserStruct's key. + + @retval >0 If StandaloneKey compares greater than UserStruct's key. +**/ +STATIC +INTN +EFIAPI +BlobKeyCompare ( + IN CONST VOID *StandaloneKey, + IN CONST VOID *UserStruct + ) +{ + CONST BLOB *Blob; + + Blob = UserStruct; + return AsciiStrCmp (StandaloneKey, (CONST CHAR8 *)Blob->File); +} + /** - Download all ACPI table data files from QEMU and interpret them. + Comparator function for two user structures. + + @param[in] UserStruct1 Pointer to the first user structure. + + @param[in] UserStruct2 Pointer to the second user structure. + + @retval <0 If UserStruct1 compares less than UserStruct2. - @param[in] AcpiProtocol The ACPI table protocol used to install tables. + @retval 0 If UserStruct1 compares equal to UserStruct2. + + @retval >0 If UserStruct1 compares greater than UserStruct2. +**/ +STATIC +INTN +EFIAPI +BlobCompare ( + IN CONST VOID *UserStruct1, + IN CONST VOID *UserStruct2 + ) +{ + CONST BLOB *Blob1; + + Blob1 = UserStruct1; + return BlobKeyCompare (Blob1->File, UserStruct2); +} + +/** + Download, process, and install ACPI table data from the QEMU loader + interface. - @retval EFI_UNSUPPORTED Firmware configuration is unavailable. + @retval EFI_UNSUPPORTED Firmware configuration is unavailable, or QEMU + loader command with unsupported parameters + has been found. @retval EFI_NOT_FOUND The host doesn't export the required fw_cfg files. - @retval EFI_OUT_OF_RESOURCES Memory allocation failed, or more than - INSTALLED_TABLES_MAX tables found. + @retval EFI_OUT_OF_RESOURCES Memory allocation failed. @retval EFI_PROTOCOL_ERROR Found invalid fw_cfg contents. - @return Status codes returned by - AcpiProtocol->InstallAcpiTable(). + @retval EFI_ALREADY_STARTED One of the ACPI TABLE GUIDs has been found in + the EFI Configuration Table, indicating the + presence of a preexistent RSD PTR table, and + therefore that of another module installing + ACPI tables. + + @retval EFI_SUCCESS Installation of ACPI tables succeeded. **/ @@ -617,5 +694,303 @@ InstallAllQemuLinkedTables ( VOID ) { - return CheckRsdp (NULL, 0, NULL); + EFI_STATUS Status; + FIRMWARE_CONFIG_ITEM FwCfgItem; + UINTN FwCfgSize; + VOID *Rsdp; + UINTN RsdpBufferSize; + UINT8 *Loader; + CONST QEMU_LOADER_ENTRY *LoaderEntry, *LoaderEnd; + ORDERED_COLLECTION *Tracker; + ORDERED_COLLECTION_ENTRY *TrackerEntry, *TrackerEntry2; + BLOB *Blob; + EFI_GUID *AcpiTableGuid; + + // + // This function allocates memory on four levels. From lowest to highest: + // - Areas consisting of whole pages, of type EfiACPIMemoryNVS, for + // (processed) ACPI payload, + // - BLOB structures referencing the above, tracking their names, sizes, and + // addresses, + // - ORDERED_COLLECTION_ENTRY objects internal to OrderedCollectionLib, + // linking the BLOB structures, + // - an ORDERED_COLLECTION organizing the ORDERED_COLLECTION_ENTRY entries. + // + // On exit, the last three levels are torn down unconditionally. If we exit + // with success, then the first (lowest) level is left in place, constituting + // the ACPI tables for the guest. If we exit with error, then even the first + // (lowest) level is torn down. + // + + Status = QemuFwCfgFindFile ("etc/table-loader", &FwCfgItem, &FwCfgSize); + if (EFI_ERROR (Status)) { + return Status; + } + if (FwCfgSize % sizeof *LoaderEntry != 0) { + DEBUG ((EFI_D_ERROR, "%a: \"etc/table-loader\" has invalid size 0x%Lx\n", + __FUNCTION__, (UINT64)FwCfgSize)); + return EFI_PROTOCOL_ERROR; + } + + if (!EFI_ERROR (EfiGetSystemConfigurationTable ( + &gEfiAcpi10TableGuid, &Rsdp)) || + !EFI_ERROR (EfiGetSystemConfigurationTable ( + &gEfiAcpi20TableGuid, &Rsdp))) { + DEBUG ((EFI_D_ERROR, "%a: RSD PTR already present\n", __FUNCTION__)); + return EFI_ALREADY_STARTED; + } + + Loader = AllocatePool (FwCfgSize); + if (Loader == NULL) { + return EFI_OUT_OF_RESOURCES; + } + + QemuFwCfgSelectItem (FwCfgItem); + QemuFwCfgReadBytes (FwCfgSize, Loader); + + Tracker = OrderedCollectionInit (BlobCompare, BlobKeyCompare); + if (Tracker == NULL) { + Status = EFI_OUT_OF_RESOURCES; + goto FreeLoader; + } + + Rsdp = NULL; + RsdpBufferSize = 0; + + LoaderEntry = (QEMU_LOADER_ENTRY *)Loader; + LoaderEnd = (QEMU_LOADER_ENTRY *)(Loader + FwCfgSize); + while (LoaderEntry < LoaderEnd) { + switch (LoaderEntry->Type) { + case QemuLoaderCmdAllocate: { + CONST QEMU_LOADER_ALLOCATE *Allocate; + UINTN NumPages; + EFI_PHYSICAL_ADDRESS Address; + + Allocate = &LoaderEntry->Command.Allocate; + if (Allocate->File[QEMU_LOADER_FNAME_SIZE - 1] != '\0') { + DEBUG ((EFI_D_ERROR, "%a: malformed file name in Allocate command\n", + __FUNCTION__)); + Status = EFI_PROTOCOL_ERROR; + goto FreeTracker; + } + if (Allocate->Alignment > EFI_PAGE_SIZE) { + DEBUG ((EFI_D_ERROR, "%a: unsupported alignment 0x%x in Allocate " + "command\n", __FUNCTION__, Allocate->Alignment)); + Status = EFI_UNSUPPORTED; + goto FreeTracker; + } + Status = QemuFwCfgFindFile ((CHAR8 *)Allocate->File, &FwCfgItem, + &FwCfgSize); + if (EFI_ERROR (Status)) { + DEBUG ((EFI_D_ERROR, "%a: nonexistent file \"%a\" in Allocate " + "command\n", __FUNCTION__, Allocate->File)); + goto FreeTracker; + } + + NumPages = EFI_SIZE_TO_PAGES (FwCfgSize); + Address = 0xFFFFFFFF; + Status = gBS->AllocatePages (AllocateMaxAddress, EfiACPIMemoryNVS, + NumPages, &Address); + if (EFI_ERROR (Status)) { + goto FreeTracker; + } + + Blob = AllocatePool (sizeof *Blob); + if (Blob == NULL) { + Status = EFI_OUT_OF_RESOURCES; + goto FreePages; + } + CopyMem (Blob->File, Allocate->File, QEMU_LOADER_FNAME_SIZE); + Blob->Size = FwCfgSize; + Blob->Base = (VOID *)(UINTN)Address; + + if (Allocate->Zone == QemuLoaderAllocFSeg) { + if (Rsdp == NULL) { + Rsdp = Blob->Base; + RsdpBufferSize = Blob->Size; + } else { + DEBUG ((EFI_D_ERROR, "%a: duplicate RSD PTR candidate in Allocate " + "command\n", __FUNCTION__)); + Status = EFI_PROTOCOL_ERROR; + goto FreeBlob; + } + } + + Status = OrderedCollectionInsert (Tracker, NULL, Blob); + if (Status == RETURN_ALREADY_STARTED) { + DEBUG ((EFI_D_ERROR, "%a: duplicated file \"%a\" in Allocate " + "command\n", __FUNCTION__, Allocate->File)); + Status = EFI_PROTOCOL_ERROR; + goto FreeBlob; + } + if (EFI_ERROR (Status)) { + goto FreeBlob; + } + + QemuFwCfgSelectItem (FwCfgItem); + QemuFwCfgReadBytes (FwCfgSize, Blob->Base); + ZeroMem (Blob->Base + Blob->Size, + EFI_PAGES_TO_SIZE (NumPages) - Blob->Size); + + DEBUG ((EFI_D_VERBOSE, "%a: Allocate: File=\"%a\" Alignment=0x%x " + "Zone=%d Size=0x%Lx Address=0x%Lx\n", __FUNCTION__, Allocate->File, + Allocate->Alignment, Allocate->Zone, (UINT64)Blob->Size, + (UINT64)(UINTN)Blob->Base)); + break; + +FreeBlob: + FreePool (Blob); +FreePages: + gBS->FreePages (Address, NumPages); + goto FreeTracker; + } + + case QemuLoaderCmdAddPointer: { + CONST QEMU_LOADER_ADD_POINTER *AddPointer; + CONST BLOB *Blob2; + UINT8 *PointerField; + + AddPointer = &LoaderEntry->Command.AddPointer; + if (AddPointer->PointerFile[QEMU_LOADER_FNAME_SIZE - 1] != '\0' || + AddPointer->PointeeFile[QEMU_LOADER_FNAME_SIZE - 1] != '\0') { + DEBUG ((EFI_D_ERROR, "%a: malformed file name in AddPointer command\n", + __FUNCTION__)); + Status = EFI_PROTOCOL_ERROR; + goto FreeTracker; + } + + TrackerEntry = OrderedCollectionFind (Tracker, AddPointer->PointerFile); + TrackerEntry2 = OrderedCollectionFind (Tracker, AddPointer->PointeeFile); + if (TrackerEntry == NULL || TrackerEntry2 == NULL) { + DEBUG ((EFI_D_ERROR, "%a: invalid blob reference(s) \"%a\" / \"%a\" " + "in AddPointer command\n", __FUNCTION__, AddPointer->PointerFile, + AddPointer->PointeeFile)); + Status = EFI_PROTOCOL_ERROR; + goto FreeTracker; + } + + Blob = OrderedCollectionUserStruct (TrackerEntry); + Blob2 = OrderedCollectionUserStruct (TrackerEntry2); + if ((AddPointer->PointerSize != 1 && AddPointer->PointerSize != 2 && + AddPointer->PointerSize != 4 && AddPointer->PointerSize != 8) || + Blob->Size < AddPointer->PointerSize || + Blob->Size - AddPointer->PointerSize < AddPointer->PointerOffset) { + DEBUG ((EFI_D_ERROR, "%a: invalid pointer location in \"%a\" in " + "AddPointer command\n", __FUNCTION__, AddPointer->PointerFile)); + Status = EFI_PROTOCOL_ERROR; + goto FreeTracker; + } + + PointerField = Blob->Base + AddPointer->PointerOffset; + switch (AddPointer->PointerSize) { + case 1: + *PointerField += (UINT8)(UINTN)Blob2->Base; + break; + + case 2: + *(UINT16 *)PointerField += (UINT16)(UINTN)Blob2->Base; + break; + + case 4: + *(UINT32 *)PointerField += (UINT32)(UINTN)Blob2->Base; + break; + + case 8: + *(UINT64 *)PointerField += (UINT64)(UINTN)Blob2->Base; + break; + } + + DEBUG ((EFI_D_VERBOSE, "%a: AddPointer: PointerFile=\"%a\" " + "PointeeFile=\"%a\" PointerOffset=0x%x PointerSize=%d\n", __FUNCTION__, + AddPointer->PointerFile, AddPointer->PointeeFile, + AddPointer->PointerOffset, AddPointer->PointerSize)); + break; + } + + case QemuLoaderCmdAddChecksum: { + CONST QEMU_LOADER_ADD_CHECKSUM *AddChecksum; + + AddChecksum = &LoaderEntry->Command.AddChecksum; + if (AddChecksum->File[QEMU_LOADER_FNAME_SIZE - 1] != '\0') { + DEBUG ((EFI_D_ERROR, "%a: malformed file name in AddChecksum " + "command\n", __FUNCTION__)); + Status = EFI_PROTOCOL_ERROR; + goto FreeTracker; + } + + TrackerEntry = OrderedCollectionFind (Tracker, AddChecksum->File); + if (TrackerEntry == NULL) { + DEBUG ((EFI_D_ERROR, "%a: invalid blob reference \"%a\" in " + "AddChecksum command\n", __FUNCTION__, AddChecksum->File)); + Status = EFI_PROTOCOL_ERROR; + goto FreeTracker; + } + + Blob = OrderedCollectionUserStruct (TrackerEntry); + if (Blob->Size <= AddChecksum->ResultOffset || + Blob->Size < AddChecksum->Length || + Blob->Size - AddChecksum->Length < AddChecksum->Start) { + DEBUG ((EFI_D_ERROR, "%a: invalid checksum location or range in " + "\"%a\" in AddChecksum command\n", __FUNCTION__, AddChecksum->File)); + Status = EFI_PROTOCOL_ERROR; + goto FreeTracker; + } + + Blob->Base[AddChecksum->ResultOffset] = CalculateCheckSum8 ( + Blob->Base + AddChecksum->Start, + AddChecksum->Length + ); + DEBUG ((EFI_D_VERBOSE, "%a: AddChecksum: File=\"%a\" ResultOffset=0x%x " + "Start=0x%x Length=0x%x\n", __FUNCTION__, AddChecksum->File, + AddChecksum->ResultOffset, AddChecksum->Start, AddChecksum->Length)); + break; + } + + default: + DEBUG ((EFI_D_VERBOSE, "%a: unknown loader command: 0x%x\n", + __FUNCTION__, LoaderEntry->Type)); + break; + } + + ++LoaderEntry; + } + + if (Rsdp == NULL) { + DEBUG ((EFI_D_ERROR, "%a: no RSD PTR candidate\n", __FUNCTION__)); + Status = EFI_PROTOCOL_ERROR; + goto FreeTracker; + } + + AcpiTableGuid = NULL; + if (EFI_ERROR (CheckRsdp (Rsdp, RsdpBufferSize, &AcpiTableGuid))) { + DEBUG ((EFI_D_ERROR, "%a: RSD PTR not found in candidate\n", + __FUNCTION__)); + Status = EFI_PROTOCOL_ERROR; + goto FreeTracker; + } + + Status = gBS->InstallConfigurationTable (AcpiTableGuid, Rsdp); + +FreeTracker: + // + // Tear down the tracker structure, and if we're exiting with an error, the + // pages holding the blob data (ie. the processed ACPI payload) as well. + // + for (TrackerEntry = OrderedCollectionMin (Tracker); TrackerEntry != NULL; + TrackerEntry = TrackerEntry2) { + VOID *UserStruct; + + TrackerEntry2 = OrderedCollectionNext (TrackerEntry); + OrderedCollectionDelete (Tracker, TrackerEntry, &UserStruct); + if (EFI_ERROR (Status)) { + Blob = UserStruct; + gBS->FreePages ((UINTN)Blob->Base, EFI_SIZE_TO_PAGES (Blob->Size)); + } + FreePool (UserStruct); + } + OrderedCollectionUninit (Tracker); + +FreeLoader: + FreePool (Loader); + return Status; }
Recent changes in the QEMU ACPI table generator have shown that our limited client for that interface is insufficient and/or brittle. Implement the full interface utilizing OrderedCollectionLib for addressing fw_cfg blobs by name. In addition, we stop using EFI_ACPI_TABLE_PROTOCOL in the QEMU loader client, because: - splitting the blobs for EFI_ACPI_TABLE_PROTOCOL is just untenable short of a complete ACPI parser, - and it is fully sufficient to install the RSD PTR as an EFI configuration table for the guest OS to see everything. In short, for this interface, EFI_ACPI_TABLE_PROTOCOL is an unwanted and restrictive convenience; let's stop using it. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- OvmfPkg/AcpiPlatformDxe/AcpiPlatformDxe.inf | 3 + OvmfPkg/AcpiPlatformDxe/Qemu.c | 415 ++++++++++++++++++++++++++-- 2 files changed, 398 insertions(+), 20 deletions(-)