[PATCH v2 1/1] OvmfPkg: Allow multiple add-pointer linker commands to same ACPI table

[Phil Dennis-Jordan <lists@philjordan.eu>]

From: Phil Dennis-Jordan <phil@philjordan.eu>

ACPI tables may contain multiple fields which point to the same
destination table. For example, in some revisions, the FADT contains
both DSDT and X_DSDT fields, and they may both point to the DSDT.

Previously, if Qemu created QEMU_LOADER_ADD_POINTER linker commands for
such instances, the linking process would attempt to install the same
pointed-to table repeatedly. For tables of which there must only be one
instance, the call to AcpiProtocol->InstallAcpiTable() would fail during
the second linker command pointing to the same table, thus entirely
aborting the ACPI table linking process. In the case of tables of which
there may be multiple instances, the table would end up duplicated.

This change adds a memoisation data structure which tracks the table
pointers that have already been processed; even if the same pointer is
encountered multiple times, it is only processed once.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---

Notes:
    v2:
    - Improved commit message & doc comments to more accurately explain the
      bug being fixed. [Laszlo]
    - Unsigned pointer ordering logic [Laszlo]
    - Moved memoisation logic before any other add pointer command processing,
      not just immediately before table install; updated names & comments to
      reflect this. [Laszlo]
    - Various style fixes [Laszlo]

 OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 109 ++++++++++++++++++--
 1 file changed, 98 insertions(+), 11 deletions(-)

diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
index 7bb2e3f21821..9e45436bcda3 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
+++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
@@ -100,6 +100,39 @@ BlobCompare (
 
 
 /**
+  Comparator function for two opaque pointers, ordering on (unsigned) pointer
+  value itself.
+  Can be used as both Key and UserStruct comparator.
+
+  @param[in] Pointer1  First pointer.
+
+  @param[in] Pointer2  Second pointer.
+
+  @retval <0  If Pointer1 compares less than Pointer2.
+
+  @retval  0  If Pointer1 compares equal to Pointer2.
+
+  @retval >0  If Pointer1 compares greater than Pointer2.
+**/
+STATIC
+INTN
+EFIAPI
+PointerCompare (
+  IN CONST VOID *Pointer1,
+  IN CONST VOID *Pointer2
+  )
+{
+  if (Pointer1 == Pointer2) {
+    return 0;
+  }
+  if ((UINTN)Pointer1 < (UINTN)Pointer2) {
+    return -1;
+  }
+  return 1;
+}
+
+
+/**
   Process a QEMU_LOADER_ALLOCATE command.
 
   @param[in] Allocate     The QEMU_LOADER_ALLOCATE command to process.
@@ -557,6 +590,16 @@ UndoCmdWritePointer (
                                command identified an ACPI table that is
                                different from RSDT and XSDT.
 
+  @param[in,out] SeenPointers  The ORDERED_COLLECTION tracking the absolute
+                               target addresses that have been pointed-to by
+                               QEMU_LOADER_ADD_POINTER commands thus far. If a
+                               target address is encountered for the first time,
+                               and it identifies an ACPI table that is different
+                               from RDST and DSDT, the table is installed.
+                               If a target address is seen for the second or
+                               later times, it is skipped without taking any
+                               action.
+
   @retval EFI_INVALID_PARAMETER  NumInstalled was outside the allowed range on
                                  input.
 
@@ -564,12 +607,13 @@ UndoCmdWritePointer (
                                  table different from RSDT and XSDT, but there
                                  was no more room in InstalledKey.
 
-  @retval EFI_SUCCESS            AddPointer has been processed. Either an ACPI
-                                 table different from RSDT and XSDT has been
-                                 installed (reflected by InstalledKey and
-                                 NumInstalled), or RSDT or XSDT has been
-                                 identified but not installed, or the fw_cfg
-                                 blob pointed-into by AddPointer has been
+  @retval EFI_SUCCESS            AddPointer has been processed. Either its
+                                 absolute target address has been encountered
+                                 before, or an ACPI table different from RSDT
+                                 and XSDT has been installed (reflected by
+                                 InstalledKey and NumInstalled), or RSDT or XSDT
+                                 has been identified but not installed, or the
+                                 fw_cfg blob pointed-into by AddPointer has been
                                  marked as hosting something else than just
                                  direct ACPI table contents.
 
@@ -584,11 +628,13 @@ Process2ndPassCmdAddPointer (
   IN     CONST ORDERED_COLLECTION      *Tracker,
   IN     EFI_ACPI_TABLE_PROTOCOL       *AcpiProtocol,
   IN OUT UINTN                         InstalledKey[INSTALLED_TABLES_MAX],
-  IN OUT INT32                         *NumInstalled
+  IN OUT INT32                         *NumInstalled,
+  IN OUT ORDERED_COLLECTION            *SeenPointers
   )
 {
   CONST ORDERED_COLLECTION_ENTRY                     *TrackerEntry;
   CONST ORDERED_COLLECTION_ENTRY                     *TrackerEntry2;
+  ORDERED_COLLECTION_ENTRY                           *SeenPointerEntry;
   CONST BLOB                                         *Blob;
   BLOB                                               *Blob2;
   CONST UINT8                                        *PointerField;
@@ -620,6 +666,24 @@ Process2ndPassCmdAddPointer (
   Blob2Remaining += Blob2->Size;
   ASSERT (PointerValue < Blob2Remaining);
 
+  Status = OrderedCollectionInsert (
+             SeenPointers,
+             &SeenPointerEntry, // for reverting insertion in error case
+             (VOID *)(UINTN)PointerValue
+           );
+  if (EFI_ERROR (Status)) {
+    if (Status == RETURN_ALREADY_STARTED) {
+      //
+      // Already seen this pointer, don't try to process it again.
+      //
+      DEBUG ((DEBUG_VERBOSE, "%a: AcpiProtocol->InstallAcpiTable reports table "
+        "already installed, skipping. PointerValue=0x%Lx\n",
+        __FUNCTION__, PointerValue));
+      Status = EFI_SUCCESS;
+    }
+    return Status;
+  }
+
   Blob2Remaining -= (UINTN) PointerValue;
   DEBUG ((EFI_D_VERBOSE, "%a: checking for ACPI header in \"%a\" at 0x%Lx "
     "(remaining: 0x%Lx): ", __FUNCTION__, AddPointer->PointeeFile,
@@ -682,7 +746,8 @@ Process2ndPassCmdAddPointer (
   if (*NumInstalled == INSTALLED_TABLES_MAX) {
     DEBUG ((EFI_D_ERROR, "%a: can't install more than %d tables\n",
       __FUNCTION__, INSTALLED_TABLES_MAX));
-    return EFI_OUT_OF_RESOURCES;
+    Status = EFI_OUT_OF_RESOURCES;
+    goto RollbackSeenPointer;
   }
 
   Status = AcpiProtocol->InstallAcpiTable (AcpiProtocol,
@@ -691,10 +756,14 @@ Process2ndPassCmdAddPointer (
   if (EFI_ERROR (Status)) {
     DEBUG ((EFI_D_ERROR, "%a: InstallAcpiTable(): %r\n", __FUNCTION__,
       Status));
-    return Status;
+    goto RollbackSeenPointer;
   }
   ++*NumInstalled;
   return EFI_SUCCESS;
+
+RollbackSeenPointer:
+  OrderedCollectionDelete (SeenPointers, SeenPointerEntry, NULL);
+  return Status;
 }
 
 
@@ -739,6 +808,8 @@ InstallQemuFwCfgTables (
   UINTN                    *InstalledKey;
   INT32                    Installed;
   ORDERED_COLLECTION_ENTRY *TrackerEntry, *TrackerEntry2;
+  ORDERED_COLLECTION       *SeenPointers;
+  ORDERED_COLLECTION_ENTRY *SeenPointerEntry, *SeenPointerEntry2;
 
   Status = QemuFwCfgFindFile ("etc/table-loader", &FwCfgItem, &FwCfgSize);
   if (EFI_ERROR (Status)) {
@@ -827,14 +898,21 @@ InstallQemuFwCfgTables (
     goto RollbackWritePointersAndFreeTracker;
   }
 
+  SeenPointers = OrderedCollectionInit (PointerCompare, PointerCompare);
+  if (SeenPointers == NULL) {
+    Status = EFI_OUT_OF_RESOURCES;
+    goto FreeKeys;
+  }
+
   //
   // second pass: identify and install ACPI tables
   //
   Installed = 0;
   for (LoaderEntry = LoaderStart; LoaderEntry < LoaderEnd; ++LoaderEntry) {
     if (LoaderEntry->Type == QemuLoaderCmdAddPointer) {
-      Status = Process2ndPassCmdAddPointer (&LoaderEntry->Command.AddPointer,
-                 Tracker, AcpiProtocol, InstalledKey, &Installed);
+      Status = Process2ndPassCmdAddPointer (
+                 &LoaderEntry->Command.AddPointer, Tracker, AcpiProtocol,
+                 InstalledKey, &Installed, SeenPointers);
       if (EFI_ERROR (Status)) {
         goto UninstallAcpiTables;
       }
@@ -870,6 +948,15 @@ UninstallAcpiTables:
     DEBUG ((EFI_D_INFO, "%a: installed %d tables\n", __FUNCTION__, Installed));
   }
 
+  for (SeenPointerEntry = OrderedCollectionMin (SeenPointers);
+       SeenPointerEntry != NULL;
+       SeenPointerEntry = SeenPointerEntry2) {
+    SeenPointerEntry2 = OrderedCollectionNext (SeenPointerEntry);
+    OrderedCollectionDelete (SeenPointers, SeenPointerEntry, NULL);
+  }
+  OrderedCollectionUninit (SeenPointers);
+
+FreeKeys:
   FreePool (InstalledKey);
 
 RollbackWritePointersAndFreeTracker:
-- 
2.3.2 (Apple Git-55)