[PATCH v1 0/1] OvmfPkg/AcpiPlatformDxe: Fix bug 368, multiply pointed-to tables

[PATCH v1 0/1] OvmfPkg/AcpiPlatformDxe: Fix bug 368, multiply pointed-to tables

[Phil Dennis-Jordan]

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

This fixes the bug in OVMF's Qemu ACPI table linker which caused it to
fail when multiple fields point to the same table. Previously, each
pointer caused the pointed-to table to be installed via the
EFI_ACPI_TABLE_PROTOCOL. However, each table must only be installed once
via this mechanism.

The patch fixes this (as previously suggested by Laszlo) via memoisation
of the pointers. If it encounters the same pointer twice, it will no
longer try to re-install it. I hope I got the implementation details
right. I've tested it successfully with Windows 10, a recent Ubuntu
version, and OS X as guest OSes. (further OVMF patches required for 
booting the latter)

I found this bug while trying to patch Qemu to generate a Rev3 FADT (as
per ACPI 2.0) instead of a Rev1 FADT (ACPI 1.0). Said patch has missed
the Qemu 2.9 merge window, but has been provisionally accepted for 2.10
(save for some minor tweaks) and will break the ACPI table linker
on unpatched OVMF. (When this bug is triggered, OVMF reverts to its
built-in ACPI tables, ignoring those provided by Qemu.)

Previous discussion: https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg06679.html

Qemu patch: https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg02837.html

Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=368

Github feature branch: https://github.com/pmj/edk2/tree/bug_368_v1

Phil Dennis-Jordan (1):
  OvmfPkg: Allow multiple add-pointer linker commands to same ACPI table

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

-- 
2.3.2 (Apple Git-55)

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

[Phil Dennis-Jordan]

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

ACPI tables may contain multiple pointer fields 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. Indeed, some operating
systems demand this to be the case.

Previously, if Qemu created "add pointer" linker commands for both fields,
the linking process would fail, as AcpiProtocol->InstallAcpiTable() may
only be called once for each destination table and otherwise returns an
error.

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

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
 OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 109 ++++++++++++++++----
 1 file changed, 89 insertions(+), 20 deletions(-)

diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
index 7bb2e3f21821..cffa838623cc 100644
--- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
+++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
@@ -100,6 +100,39 @@ BlobCompare (
 
 
 /**
+  Comparator function for two opaque pointers, ordering on 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;
+  } else if ((INTN)Pointer1 < (INTN)Pointer2) {
+    return -1;
+  } else {
+    return 1;
+  }
+}
+
+
+/**
   Process a QEMU_LOADER_ALLOCATE command.
 
   @param[in] Allocate     The QEMU_LOADER_ALLOCATE command to process.
@@ -535,27 +568,32 @@ UndoCmdWritePointer (
   This function assumes that the entire QEMU linker/loader command file has
   been processed successfully in a prior first pass.
 
-  @param[in] AddPointer        The QEMU_LOADER_ADD_POINTER command to process.
+  @param[in] AddPointer          The QEMU_LOADER_ADD_POINTER command to process.
 
-  @param[in] Tracker           The ORDERED_COLLECTION tracking the BLOB user
-                               structures.
+  @param[in] Tracker             The ORDERED_COLLECTION tracking the BLOB user
+                                 structures.
 
-  @param[in] AcpiProtocol      The ACPI table protocol used to install tables.
+  @param[in] AcpiProtocol        The ACPI table protocol used to install tables.
 
-  @param[in,out] InstalledKey  On input, an array of INSTALLED_TABLES_MAX UINTN
-                               elements, allocated by the caller. On output,
-                               the function will have stored (appended) the
-                               AcpiProtocol-internal key of the ACPI table that
-                               the function has installed, if the AddPointer
-                               command identified an ACPI table that is
-                               different from RSDT and XSDT.
+  @param[in,out] InstalledKey    On input, an array of INSTALLED_TABLES_MAX UINTN
+                                 elements, allocated by the caller. On output,
+                                 the function will have stored (appended) the
+                                 AcpiProtocol-internal key of the ACPI table that
+                                 the function has installed, if the AddPointer
+                                 command identified an ACPI table that is
+                                 different from RSDT and XSDT.
 
-  @param[in,out] NumInstalled  On input, the number of entries already used in
-                               InstalledKey; it must be in [0,
-                               INSTALLED_TABLES_MAX] inclusive. On output, the
-                               parameter is incremented if the AddPointer
-                               command identified an ACPI table that is
-                               different from RSDT and XSDT.
+  @param[in,out] NumInstalled    On input, the number of entries already used in
+                                 InstalledKey; it must be in [0,
+                                 INSTALLED_TABLES_MAX] inclusive. On output, the
+                                 parameter is incremented if the AddPointer
+                                 command identified an ACPI table that is
+                                 different from RSDT and XSDT.
+
+  @param[in,out] InstalledTables The ORDERED_COLLECTION tracking the ACPI tables
+                                 which have already been installed. If a new
+                                 table is encountered by the function, it is
+                                 added; existing ones will not be installed again.
 
   @retval EFI_INVALID_PARAMETER  NumInstalled was outside the allowed range on
                                  input.
@@ -584,7 +622,8 @@ 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            *InstalledTables
   )
 {
   CONST ORDERED_COLLECTION_ENTRY                     *TrackerEntry;
@@ -679,6 +718,21 @@ Process2ndPassCmdAddPointer (
     return EFI_SUCCESS;
   }
 
+  Status = OrderedCollectionInsert (
+             InstalledTables, NULL, (VOID *)(UINTN)PointerValue);
+  if (EFI_ERROR (Status)) {
+    if (Status == RETURN_ALREADY_STARTED) {
+      //
+      // Already installed this table, don't try to do so again.
+      //
+      DEBUG ((DEBUG_VERBOSE, "%a: AcpiProtocol->InstallAcpiTable reports table "
+        "already installed, skipping. PointerValue=0x%Lx\n",
+        __FUNCTION__, PointerValue));
+      Status = EFI_SUCCESS;
+    }
+    return Status;
+  }
+
   if (*NumInstalled == INSTALLED_TABLES_MAX) {
     DEBUG ((EFI_D_ERROR, "%a: can't install more than %d tables\n",
       __FUNCTION__, INSTALLED_TABLES_MAX));
@@ -739,6 +793,8 @@ InstallQemuFwCfgTables (
   UINTN                    *InstalledKey;
   INT32                    Installed;
   ORDERED_COLLECTION_ENTRY *TrackerEntry, *TrackerEntry2;
+  ORDERED_COLLECTION       *InstalledTables;
+  ORDERED_COLLECTION_ENTRY *InstalledTableEntry;
 
   Status = QemuFwCfgFindFile ("etc/table-loader", &FwCfgItem, &FwCfgSize);
   if (EFI_ERROR (Status)) {
@@ -827,14 +883,21 @@ InstallQemuFwCfgTables (
     goto RollbackWritePointersAndFreeTracker;
   }
 
+  InstalledTables = OrderedCollectionInit (PointerCompare, PointerCompare);
+  if (InstalledTables == 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, InstalledTables);
       if (EFI_ERROR (Status)) {
         goto UninstallAcpiTables;
       }
@@ -870,6 +933,12 @@ UninstallAcpiTables:
     DEBUG ((EFI_D_INFO, "%a: installed %d tables\n", __FUNCTION__, Installed));
   }
 
+  while (((InstalledTableEntry = OrderedCollectionMax(InstalledTables))) != NULL) {
+    OrderedCollectionDelete (InstalledTables, InstalledTableEntry, NULL);
+  }
+  OrderedCollectionUninit (InstalledTables);
+
+FreeKeys:
   FreePool (InstalledKey);
 
 RollbackWritePointersAndFreeTracker:
-- 
2.3.2 (Apple Git-55)

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

[Laszlo Ersek]

This is a very good first patch!

I have a few requests. I'll generally rehash the points from
<https://bugzilla.tianocore.org/show_bug.cgi?id=368#c0>.

On 03/29/17 09:50, Phil Dennis-Jordan wrote:
> From: Phil Dennis-Jordan <phil@philjordan.eu>
> 
> ACPI tables may contain multiple pointer fields 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. Indeed, some operating
> systems demand this to be the case.
> 
> Previously, if Qemu created "add pointer" linker commands for both fields,
> the linking process would fail, as AcpiProtocol->InstallAcpiTable() may
> only be called once for each destination table and otherwise returns an
> error.

(1) Please be a bit more precise here: passing the exact same table
twice to InstallAcpiTable() is wrong, but for two different reasons.

Reason #1 is what you named -- some, but not all, ACPI table types are
not allowed to have multiple instances, and that aborts the linking
process. Reason #2 is that successfully installing the exact same table,
of a type that does allow multiple instances (such as an SSDT, there
could be others) is just as wrong, although it doesn't abort the linking.

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

(2) Please s/installed/processed/g above -- see for more below.

> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
>  OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 109 ++++++++++++++++----
>  1 file changed, 89 insertions(+), 20 deletions(-)
> 
> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> index 7bb2e3f21821..cffa838623cc 100644
> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
> @@ -100,6 +100,39 @@ BlobCompare (
>  
>  
>  /**
> +  Comparator function for two opaque pointers, ordering on 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;
> +  } else if ((INTN)Pointer1 < (INTN)Pointer2) {

(3) Please use UINTN here, not INTN.

Also, I slightly dislike an "else" after a "return", but I guess it's up
to you. :)

> +    return -1;
> +  } else {
> +    return 1;
> +  }
> +}
> +
> +
> +/**
>    Process a QEMU_LOADER_ALLOCATE command.
>  
>    @param[in] Allocate     The QEMU_LOADER_ALLOCATE command to process.
> @@ -535,27 +568,32 @@ UndoCmdWritePointer (
>    This function assumes that the entire QEMU linker/loader command file has
>    been processed successfully in a prior first pass.
>  
> -  @param[in] AddPointer        The QEMU_LOADER_ADD_POINTER command to process.
> +  @param[in] AddPointer          The QEMU_LOADER_ADD_POINTER command to process.
>  
> -  @param[in] Tracker           The ORDERED_COLLECTION tracking the BLOB user
> -                               structures.
> +  @param[in] Tracker             The ORDERED_COLLECTION tracking the BLOB user
> +                                 structures.
>  
> -  @param[in] AcpiProtocol      The ACPI table protocol used to install tables.
> +  @param[in] AcpiProtocol        The ACPI table protocol used to install tables.
>  
> -  @param[in,out] InstalledKey  On input, an array of INSTALLED_TABLES_MAX UINTN
> -                               elements, allocated by the caller. On output,
> -                               the function will have stored (appended) the
> -                               AcpiProtocol-internal key of the ACPI table that
> -                               the function has installed, if the AddPointer
> -                               command identified an ACPI table that is
> -                               different from RSDT and XSDT.
> +  @param[in,out] InstalledKey    On input, an array of INSTALLED_TABLES_MAX UINTN
> +                                 elements, allocated by the caller. On output,
> +                                 the function will have stored (appended) the
> +                                 AcpiProtocol-internal key of the ACPI table that
> +                                 the function has installed, if the AddPointer
> +                                 command identified an ACPI table that is
> +                                 different from RSDT and XSDT.
>  
> -  @param[in,out] NumInstalled  On input, the number of entries already used in
> -                               InstalledKey; it must be in [0,
> -                               INSTALLED_TABLES_MAX] inclusive. On output, the
> -                               parameter is incremented if the AddPointer
> -                               command identified an ACPI table that is
> -                               different from RSDT and XSDT.
> +  @param[in,out] NumInstalled    On input, the number of entries already used in
> +                                 InstalledKey; it must be in [0,
> +                                 INSTALLED_TABLES_MAX] inclusive. On output, the
> +                                 parameter is incremented if the AddPointer
> +                                 command identified an ACPI table that is
> +                                 different from RSDT and XSDT.
> +
> +  @param[in,out] InstalledTables The ORDERED_COLLECTION tracking the ACPI tables
> +                                 which have already been installed. If a new
> +                                 table is encountered by the function, it is
> +                                 added; existing ones will not be installed again.

(4) Please rename "InstalledTables" to "SeenPointers", and update its
comment block accordingly:

"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."

(5) An added benefit of using the name "SeenPointers" is that it is
exactly as long as "NumInstalled", and you won't have to reindent the
other comment blocks.

(6) Please also update the @retval EFI_SUCCESS comment:

"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."

>  
>    @retval EFI_INVALID_PARAMETER  NumInstalled was outside the allowed range on
>                                   input.
> @@ -584,7 +622,8 @@ 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            *InstalledTables
>    )
>  {
>    CONST ORDERED_COLLECTION_ENTRY                     *TrackerEntry;
> @@ -679,6 +718,21 @@ Process2ndPassCmdAddPointer (
>      return EFI_SUCCESS;
>    }
>  
> +  Status = OrderedCollectionInsert (
> +             InstalledTables, NULL, (VOID *)(UINTN)PointerValue);

(7) For function calls (and function-like macro invocations) that don't
fit on a single line, we should now follow the style described in
<https://bugzilla.tianocore.org/show_bug.cgi?id=425>.

(8) The main point of renaming InstalledTables to SeenPointers:

please move this logic higher up in the function,

  ASSERT (PointerValue < Blob2Remaining);
                                          <------- right here
  Blob2Remaining -= (UINTN) PointerValue;

The reason why is explained in
<https://bugzilla.tianocore.org/show_bug.cgi?id=368#c0>, in the part
that starts with "This is justified because all actions after that point
would be..."

(9) Near the two late error exits in the function (when we run out of
INSTALLED_TABLES_MAX, or InstallAcpiTable() fails), please replace the
direct returns with a goto to an error handling label. This error
handling label should remove the just added PointerValue from SeenPointers.

For this, you can use the second parameter of OrderedCollectionInsert().
On success, that parameter is set to the node of the newly inserted
item, and then you can pass that to OrderedCollectionDelete() directly
on the error path.

> +  if (EFI_ERROR (Status)) {
> +    if (Status == RETURN_ALREADY_STARTED) {
> +      //
> +      // Already installed this table, don't try to do so again.
> +      //
> +      DEBUG ((DEBUG_VERBOSE, "%a: AcpiProtocol->InstallAcpiTable reports table "
> +        "already installed, skipping. PointerValue=0x%Lx\n",
> +        __FUNCTION__, PointerValue));
> +      Status = EFI_SUCCESS;
> +    }
> +    return Status;
> +  }
> +
>    if (*NumInstalled == INSTALLED_TABLES_MAX) {
>      DEBUG ((EFI_D_ERROR, "%a: can't install more than %d tables\n",
>        __FUNCTION__, INSTALLED_TABLES_MAX));
> @@ -739,6 +793,8 @@ InstallQemuFwCfgTables (
>    UINTN                    *InstalledKey;
>    INT32                    Installed;
>    ORDERED_COLLECTION_ENTRY *TrackerEntry, *TrackerEntry2;
> +  ORDERED_COLLECTION       *InstalledTables;
> +  ORDERED_COLLECTION_ENTRY *InstalledTableEntry;

(10) please rename InstalledTables here too.

>  
>    Status = QemuFwCfgFindFile ("etc/table-loader", &FwCfgItem, &FwCfgSize);
>    if (EFI_ERROR (Status)) {
> @@ -827,14 +883,21 @@ InstallQemuFwCfgTables (
>      goto RollbackWritePointersAndFreeTracker;
>    }
>  
> +  InstalledTables = OrderedCollectionInit (PointerCompare, PointerCompare);
> +  if (InstalledTables == NULL) {
> +    Status = EFI_OUT_OF_RESOURCES;
> +    goto FreeKeys;
> +  }
> +

Looks valid.

>    //
>    // 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, InstalledTables);
>        if (EFI_ERROR (Status)) {
>          goto UninstallAcpiTables;
>        }
> @@ -870,6 +933,12 @@ UninstallAcpiTables:
>      DEBUG ((EFI_D_INFO, "%a: installed %d tables\n", __FUNCTION__, Installed));
>    }
>  
> +  while (((InstalledTableEntry = OrderedCollectionMax(InstalledTables))) != NULL) {
> +    OrderedCollectionDelete (InstalledTables, InstalledTableEntry, NULL);
> +  }
> +  OrderedCollectionUninit (InstalledTables);
> +

(11) Please implement this loop instead like the one that tears down
"Tracker". Using an assignment in the controlling expression of the
"while" is powerful and I like it too, but it is never used in edk2
code, to my knowledge.

> +FreeKeys:
>    FreePool (InstalledKey);

Yes, this looks correct too.

(I apologize if I made any confusing requests in my review, I'm very tired.)

Thank you,
Laszlo

>  
>  RollbackWritePointersAndFreeTracker:
> 

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

[Phil Dennis-Jordan]

On Thu, Mar 30, 2017 at 2:06 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> This is a very good first patch!
>
> I have a few requests. I'll generally rehash the points from
> <https://bugzilla.tianocore.org/show_bug.cgi?id=368#c0>.
>
> On 03/29/17 09:50, Phil Dennis-Jordan wrote:
>> From: Phil Dennis-Jordan <phil@philjordan.eu>
>>
>> ACPI tables may contain multiple pointer fields 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. Indeed, some operating
>> systems demand this to be the case.
>>
>> Previously, if Qemu created "add pointer" linker commands for both fields,
>> the linking process would fail, as AcpiProtocol->InstallAcpiTable() may
>> only be called once for each destination table and otherwise returns an
>> error.
>
> (1) Please be a bit more precise here: passing the exact same table
> twice to InstallAcpiTable() is wrong, but for two different reasons.
>
> Reason #1 is what you named -- some, but not all, ACPI table types are
> not allowed to have multiple instances, and that aborts the linking
> process. Reason #2 is that successfully installing the exact same table,
> of a type that does allow multiple instances (such as an SSDT, there
> could be others) is just as wrong, although it doesn't abort the linking.

Thanks - I know you've explained this distinction a bunch of times
now, but it's finally clicked with me.

>>
>> This change adds a memoisation data structure which tracks the table
>> pointers that have already been installed; even if the same pointer is
>> encountered multiple times, it is only installed once.
>
> (2) Please s/installed/processed/g above -- see for more below.
>
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
>> ---
>>  OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 109 ++++++++++++++++----
>>  1 file changed, 89 insertions(+), 20 deletions(-)
>>
>> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>> index 7bb2e3f21821..cffa838623cc 100644
>> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>> @@ -100,6 +100,39 @@ BlobCompare (
>>
>>
>>  /**
>> +  Comparator function for two opaque pointers, ordering on 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;
>> +  } else if ((INTN)Pointer1 < (INTN)Pointer2) {
>
> (3) Please use UINTN here, not INTN.
>
> Also, I slightly dislike an "else" after a "return", but I guess it's up
> to you. :)
>
>> +    return -1;
>> +  } else {
>> +    return 1;
>> +  }
>> +}
>> +
>> +
>> +/**
>>    Process a QEMU_LOADER_ALLOCATE command.
>>
>>    @param[in] Allocate     The QEMU_LOADER_ALLOCATE command to process.
>> @@ -535,27 +568,32 @@ UndoCmdWritePointer (
>>    This function assumes that the entire QEMU linker/loader command file has
>>    been processed successfully in a prior first pass.
>>
>> -  @param[in] AddPointer        The QEMU_LOADER_ADD_POINTER command to process.
>> +  @param[in] AddPointer          The QEMU_LOADER_ADD_POINTER command to process.
>>
>> -  @param[in] Tracker           The ORDERED_COLLECTION tracking the BLOB user
>> -                               structures.
>> +  @param[in] Tracker             The ORDERED_COLLECTION tracking the BLOB user
>> +                                 structures.
>>
>> -  @param[in] AcpiProtocol      The ACPI table protocol used to install tables.
>> +  @param[in] AcpiProtocol        The ACPI table protocol used to install tables.
>>
>> -  @param[in,out] InstalledKey  On input, an array of INSTALLED_TABLES_MAX UINTN
>> -                               elements, allocated by the caller. On output,
>> -                               the function will have stored (appended) the
>> -                               AcpiProtocol-internal key of the ACPI table that
>> -                               the function has installed, if the AddPointer
>> -                               command identified an ACPI table that is
>> -                               different from RSDT and XSDT.
>> +  @param[in,out] InstalledKey    On input, an array of INSTALLED_TABLES_MAX UINTN
>> +                                 elements, allocated by the caller. On output,
>> +                                 the function will have stored (appended) the
>> +                                 AcpiProtocol-internal key of the ACPI table that
>> +                                 the function has installed, if the AddPointer
>> +                                 command identified an ACPI table that is
>> +                                 different from RSDT and XSDT.
>>
>> -  @param[in,out] NumInstalled  On input, the number of entries already used in
>> -                               InstalledKey; it must be in [0,
>> -                               INSTALLED_TABLES_MAX] inclusive. On output, the
>> -                               parameter is incremented if the AddPointer
>> -                               command identified an ACPI table that is
>> -                               different from RSDT and XSDT.
>> +  @param[in,out] NumInstalled    On input, the number of entries already used in
>> +                                 InstalledKey; it must be in [0,
>> +                                 INSTALLED_TABLES_MAX] inclusive. On output, the
>> +                                 parameter is incremented if the AddPointer
>> +                                 command identified an ACPI table that is
>> +                                 different from RSDT and XSDT.
>> +
>> +  @param[in,out] InstalledTables The ORDERED_COLLECTION tracking the ACPI tables
>> +                                 which have already been installed. If a new
>> +                                 table is encountered by the function, it is
>> +                                 added; existing ones will not be installed again.
>
> (4) Please rename "InstalledTables" to "SeenPointers", and update its
> comment block accordingly:
>
> "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."
>
> (5) An added benefit of using the name "SeenPointers" is that it is
> exactly as long as "NumInstalled", and you won't have to reindent the
> other comment blocks.
>
> (6) Please also update the @retval EFI_SUCCESS comment:
>
> "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."
>
>>
>>    @retval EFI_INVALID_PARAMETER  NumInstalled was outside the allowed range on
>>                                   input.
>> @@ -584,7 +622,8 @@ 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            *InstalledTables
>>    )
>>  {
>>    CONST ORDERED_COLLECTION_ENTRY                     *TrackerEntry;
>> @@ -679,6 +718,21 @@ Process2ndPassCmdAddPointer (
>>      return EFI_SUCCESS;
>>    }
>>
>> +  Status = OrderedCollectionInsert (
>> +             InstalledTables, NULL, (VOID *)(UINTN)PointerValue);
>
> (7) For function calls (and function-like macro invocations) that don't
> fit on a single line, we should now follow the style described in
> <https://bugzilla.tianocore.org/show_bug.cgi?id=425>.
>
> (8) The main point of renaming InstalledTables to SeenPointers:
>
> please move this logic higher up in the function,
>
>   ASSERT (PointerValue < Blob2Remaining);
>                                           <------- right here
>   Blob2Remaining -= (UINTN) PointerValue;
>
> The reason why is explained in
> <https://bugzilla.tianocore.org/show_bug.cgi?id=368#c0>, in the part
> that starts with "This is justified because all actions after that point
> would be..."

I've now moved it as requested for patch v2, but my reasoning for the
original ordering of the logic was precisely to avoid the complication
of having to  roll back in the error case. It seemed a valid code
transformation as the other early-out paths also return EFI_SUCCESS,
the RSDT/XSDT one has no side effects, and the opaque blob one's side
effect is surely idempotent.

> (9) Near the two late error exits in the function (when we run out of
> INSTALLED_TABLES_MAX, or InstallAcpiTable() fails), please replace the
> direct returns with a goto to an error handling label. This error
> handling label should remove the just added PointerValue from SeenPointers.
>
> For this, you can use the second parameter of OrderedCollectionInsert().
> On success, that parameter is set to the node of the newly inserted
> item, and then you can pass that to OrderedCollectionDelete() directly
> on the error path.
>
>> +  if (EFI_ERROR (Status)) {
>> +    if (Status == RETURN_ALREADY_STARTED) {
>> +      //
>> +      // Already installed this table, don't try to do so again.
>> +      //
>> +      DEBUG ((DEBUG_VERBOSE, "%a: AcpiProtocol->InstallAcpiTable reports table "
>> +        "already installed, skipping. PointerValue=0x%Lx\n",
>> +        __FUNCTION__, PointerValue));
>> +      Status = EFI_SUCCESS;
>> +    }
>> +    return Status;
>> +  }
>> +
>>    if (*NumInstalled == INSTALLED_TABLES_MAX) {
>>      DEBUG ((EFI_D_ERROR, "%a: can't install more than %d tables\n",
>>        __FUNCTION__, INSTALLED_TABLES_MAX));
>> @@ -739,6 +793,8 @@ InstallQemuFwCfgTables (
>>    UINTN                    *InstalledKey;
>>    INT32                    Installed;
>>    ORDERED_COLLECTION_ENTRY *TrackerEntry, *TrackerEntry2;
>> +  ORDERED_COLLECTION       *InstalledTables;
>> +  ORDERED_COLLECTION_ENTRY *InstalledTableEntry;
>
> (10) please rename InstalledTables here too.
>
>>
>>    Status = QemuFwCfgFindFile ("etc/table-loader", &FwCfgItem, &FwCfgSize);
>>    if (EFI_ERROR (Status)) {
>> @@ -827,14 +883,21 @@ InstallQemuFwCfgTables (
>>      goto RollbackWritePointersAndFreeTracker;
>>    }
>>
>> +  InstalledTables = OrderedCollectionInit (PointerCompare, PointerCompare);
>> +  if (InstalledTables == NULL) {
>> +    Status = EFI_OUT_OF_RESOURCES;
>> +    goto FreeKeys;
>> +  }
>> +
>
> Looks valid.
>
>>    //
>>    // 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, InstalledTables);
>>        if (EFI_ERROR (Status)) {
>>          goto UninstallAcpiTables;
>>        }
>> @@ -870,6 +933,12 @@ UninstallAcpiTables:
>>      DEBUG ((EFI_D_INFO, "%a: installed %d tables\n", __FUNCTION__, Installed));
>>    }
>>
>> +  while (((InstalledTableEntry = OrderedCollectionMax(InstalledTables))) != NULL) {
>> +    OrderedCollectionDelete (InstalledTables, InstalledTableEntry, NULL);
>> +  }
>> +  OrderedCollectionUninit (InstalledTables);
>> +
>
> (11) Please implement this loop instead like the one that tears down
> "Tracker". Using an assignment in the controlling expression of the
> "while" is powerful and I like it too, but it is never used in edk2
> code, to my knowledge.

There's a third option, which is to also use a for loop, but avoids
the second running entry variable by calling OrderedCollectionMin()
(or Max) on each iteration. This might even be slightly more efficient
than OrderedCollectionNext(), but only by a constant factor, so I've
gone with the same teardown as for Tracker for the sake of
consistency.

>> +FreeKeys:
>>    FreePool (InstalledKey);
>
> Yes, this looks correct too.
>
> (I apologize if I made any confusing requests in my review, I'm very tired.)
>
> Thank you,
> Laszlo
>
>>
>>  RollbackWritePointersAndFreeTracker:
>>
>

Thanks for the quick review and detailed explanations, Laszlo! I've
submitted v2 of the patch with the changes.

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

[Laszlo Ersek]

On 03/30/17 12:41, Phil Dennis-Jordan wrote:
> On Thu, Mar 30, 2017 at 2:06 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>> This is a very good first patch!
>>
>> I have a few requests. I'll generally rehash the points from
>> <https://bugzilla.tianocore.org/show_bug.cgi?id=368#c0>.
>>
>> On 03/29/17 09:50, Phil Dennis-Jordan wrote:
>>> From: Phil Dennis-Jordan <phil@philjordan.eu>
>>>
>>> ACPI tables may contain multiple pointer fields 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. Indeed, some operating
>>> systems demand this to be the case.
>>>
>>> Previously, if Qemu created "add pointer" linker commands for both fields,
>>> the linking process would fail, as AcpiProtocol->InstallAcpiTable() may
>>> only be called once for each destination table and otherwise returns an
>>> error.
>>
>> (1) Please be a bit more precise here: passing the exact same table
>> twice to InstallAcpiTable() is wrong, but for two different reasons.
>>
>> Reason #1 is what you named -- some, but not all, ACPI table types are
>> not allowed to have multiple instances, and that aborts the linking
>> process. Reason #2 is that successfully installing the exact same table,
>> of a type that does allow multiple instances (such as an SSDT, there
>> could be others) is just as wrong, although it doesn't abort the linking.
> 
> Thanks - I know you've explained this distinction a bunch of times
> now, but it's finally clicked with me.
> 
>>>
>>> This change adds a memoisation data structure which tracks the table
>>> pointers that have already been installed; even if the same pointer is
>>> encountered multiple times, it is only installed once.
>>
>> (2) Please s/installed/processed/g above -- see for more below.
>>
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Phil Dennis-Jordan <phil@philjordan.eu>
>>> ---
>>>  OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 109 ++++++++++++++++----
>>>  1 file changed, 89 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>>> index 7bb2e3f21821..cffa838623cc 100644
>>> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>>> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>>> @@ -100,6 +100,39 @@ BlobCompare (
>>>
>>>
>>>  /**
>>> +  Comparator function for two opaque pointers, ordering on 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;
>>> +  } else if ((INTN)Pointer1 < (INTN)Pointer2) {
>>
>> (3) Please use UINTN here, not INTN.
>>
>> Also, I slightly dislike an "else" after a "return", but I guess it's up
>> to you. :)
>>
>>> +    return -1;
>>> +  } else {
>>> +    return 1;
>>> +  }
>>> +}
>>> +
>>> +
>>> +/**
>>>    Process a QEMU_LOADER_ALLOCATE command.
>>>
>>>    @param[in] Allocate     The QEMU_LOADER_ALLOCATE command to process.
>>> @@ -535,27 +568,32 @@ UndoCmdWritePointer (
>>>    This function assumes that the entire QEMU linker/loader command file has
>>>    been processed successfully in a prior first pass.
>>>
>>> -  @param[in] AddPointer        The QEMU_LOADER_ADD_POINTER command to process.
>>> +  @param[in] AddPointer          The QEMU_LOADER_ADD_POINTER command to process.
>>>
>>> -  @param[in] Tracker           The ORDERED_COLLECTION tracking the BLOB user
>>> -                               structures.
>>> +  @param[in] Tracker             The ORDERED_COLLECTION tracking the BLOB user
>>> +                                 structures.
>>>
>>> -  @param[in] AcpiProtocol      The ACPI table protocol used to install tables.
>>> +  @param[in] AcpiProtocol        The ACPI table protocol used to install tables.
>>>
>>> -  @param[in,out] InstalledKey  On input, an array of INSTALLED_TABLES_MAX UINTN
>>> -                               elements, allocated by the caller. On output,
>>> -                               the function will have stored (appended) the
>>> -                               AcpiProtocol-internal key of the ACPI table that
>>> -                               the function has installed, if the AddPointer
>>> -                               command identified an ACPI table that is
>>> -                               different from RSDT and XSDT.
>>> +  @param[in,out] InstalledKey    On input, an array of INSTALLED_TABLES_MAX UINTN
>>> +                                 elements, allocated by the caller. On output,
>>> +                                 the function will have stored (appended) the
>>> +                                 AcpiProtocol-internal key of the ACPI table that
>>> +                                 the function has installed, if the AddPointer
>>> +                                 command identified an ACPI table that is
>>> +                                 different from RSDT and XSDT.
>>>
>>> -  @param[in,out] NumInstalled  On input, the number of entries already used in
>>> -                               InstalledKey; it must be in [0,
>>> -                               INSTALLED_TABLES_MAX] inclusive. On output, the
>>> -                               parameter is incremented if the AddPointer
>>> -                               command identified an ACPI table that is
>>> -                               different from RSDT and XSDT.
>>> +  @param[in,out] NumInstalled    On input, the number of entries already used in
>>> +                                 InstalledKey; it must be in [0,
>>> +                                 INSTALLED_TABLES_MAX] inclusive. On output, the
>>> +                                 parameter is incremented if the AddPointer
>>> +                                 command identified an ACPI table that is
>>> +                                 different from RSDT and XSDT.
>>> +
>>> +  @param[in,out] InstalledTables The ORDERED_COLLECTION tracking the ACPI tables
>>> +                                 which have already been installed. If a new
>>> +                                 table is encountered by the function, it is
>>> +                                 added; existing ones will not be installed again.
>>
>> (4) Please rename "InstalledTables" to "SeenPointers", and update its
>> comment block accordingly:
>>
>> "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."
>>
>> (5) An added benefit of using the name "SeenPointers" is that it is
>> exactly as long as "NumInstalled", and you won't have to reindent the
>> other comment blocks.
>>
>> (6) Please also update the @retval EFI_SUCCESS comment:
>>
>> "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."
>>
>>>
>>>    @retval EFI_INVALID_PARAMETER  NumInstalled was outside the allowed range on
>>>                                   input.
>>> @@ -584,7 +622,8 @@ 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            *InstalledTables
>>>    )
>>>  {
>>>    CONST ORDERED_COLLECTION_ENTRY                     *TrackerEntry;
>>> @@ -679,6 +718,21 @@ Process2ndPassCmdAddPointer (
>>>      return EFI_SUCCESS;
>>>    }
>>>
>>> +  Status = OrderedCollectionInsert (
>>> +             InstalledTables, NULL, (VOID *)(UINTN)PointerValue);
>>
>> (7) For function calls (and function-like macro invocations) that don't
>> fit on a single line, we should now follow the style described in
>> <https://bugzilla.tianocore.org/show_bug.cgi?id=425>.
>>
>> (8) The main point of renaming InstalledTables to SeenPointers:
>>
>> please move this logic higher up in the function,
>>
>>   ASSERT (PointerValue < Blob2Remaining);
>>                                           <------- right here
>>   Blob2Remaining -= (UINTN) PointerValue;
>>
>> The reason why is explained in
>> <https://bugzilla.tianocore.org/show_bug.cgi?id=368#c0>, in the part
>> that starts with "This is justified because all actions after that point
>> would be..."
> 
> I've now moved it as requested for patch v2, but my reasoning for the
> original ordering of the logic was precisely to avoid the complication
> of having to  roll back in the error case. It seemed a valid code
> transformation as the other early-out paths also return EFI_SUCCESS,
> the RSDT/XSDT one has no side effects, and the opaque blob one's side
> effect is surely idempotent.

Yes, that's a valid argument, but I prefer the proposed (and,
thankfully, v2 :) ) approach for two reasons:

- I would like to benefit from the memoization also in order to save on
the idempotent (but still superfluous) code paths,

- I prefer a clean rollback-on-error because it is more future proof. If
we omit it now, later extensions might forget about it completely, which
at that point might not be so innocent any longer.

> 
>> (9) Near the two late error exits in the function (when we run out of
>> INSTALLED_TABLES_MAX, or InstallAcpiTable() fails), please replace the
>> direct returns with a goto to an error handling label. This error
>> handling label should remove the just added PointerValue from SeenPointers.
>>
>> For this, you can use the second parameter of OrderedCollectionInsert().
>> On success, that parameter is set to the node of the newly inserted
>> item, and then you can pass that to OrderedCollectionDelete() directly
>> on the error path.
>>
>>> +  if (EFI_ERROR (Status)) {
>>> +    if (Status == RETURN_ALREADY_STARTED) {
>>> +      //
>>> +      // Already installed this table, don't try to do so again.
>>> +      //
>>> +      DEBUG ((DEBUG_VERBOSE, "%a: AcpiProtocol->InstallAcpiTable reports table "
>>> +        "already installed, skipping. PointerValue=0x%Lx\n",
>>> +        __FUNCTION__, PointerValue));
>>> +      Status = EFI_SUCCESS;
>>> +    }
>>> +    return Status;
>>> +  }
>>> +
>>>    if (*NumInstalled == INSTALLED_TABLES_MAX) {
>>>      DEBUG ((EFI_D_ERROR, "%a: can't install more than %d tables\n",
>>>        __FUNCTION__, INSTALLED_TABLES_MAX));
>>> @@ -739,6 +793,8 @@ InstallQemuFwCfgTables (
>>>    UINTN                    *InstalledKey;
>>>    INT32                    Installed;
>>>    ORDERED_COLLECTION_ENTRY *TrackerEntry, *TrackerEntry2;
>>> +  ORDERED_COLLECTION       *InstalledTables;
>>> +  ORDERED_COLLECTION_ENTRY *InstalledTableEntry;
>>
>> (10) please rename InstalledTables here too.
>>
>>>
>>>    Status = QemuFwCfgFindFile ("etc/table-loader", &FwCfgItem, &FwCfgSize);
>>>    if (EFI_ERROR (Status)) {
>>> @@ -827,14 +883,21 @@ InstallQemuFwCfgTables (
>>>      goto RollbackWritePointersAndFreeTracker;
>>>    }
>>>
>>> +  InstalledTables = OrderedCollectionInit (PointerCompare, PointerCompare);
>>> +  if (InstalledTables == NULL) {
>>> +    Status = EFI_OUT_OF_RESOURCES;
>>> +    goto FreeKeys;
>>> +  }
>>> +
>>
>> Looks valid.
>>
>>>    //
>>>    // 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, InstalledTables);
>>>        if (EFI_ERROR (Status)) {
>>>          goto UninstallAcpiTables;
>>>        }
>>> @@ -870,6 +933,12 @@ UninstallAcpiTables:
>>>      DEBUG ((EFI_D_INFO, "%a: installed %d tables\n", __FUNCTION__, Installed));
>>>    }
>>>
>>> +  while (((InstalledTableEntry = OrderedCollectionMax(InstalledTables))) != NULL) {
>>> +    OrderedCollectionDelete (InstalledTables, InstalledTableEntry, NULL);
>>> +  }
>>> +  OrderedCollectionUninit (InstalledTables);
>>> +
>>
>> (11) Please implement this loop instead like the one that tears down
>> "Tracker". Using an assignment in the controlling expression of the
>> "while" is powerful and I like it too, but it is never used in edk2
>> code, to my knowledge.
> 
> There's a third option, which is to also use a for loop, but avoids
> the second running entry variable by calling OrderedCollectionMin()
> (or Max) on each iteration. This might even be slightly more efficient
> than OrderedCollectionNext(), but only by a constant factor, so I've
> gone with the same teardown as for Tracker for the sake of
> consistency.

Thanks!

> 
>>> +FreeKeys:
>>>    FreePool (InstalledKey);
>>
>> Yes, this looks correct too.
>>
>> (I apologize if I made any confusing requests in my review, I'm very tired.)
>>
>> Thank you,
>> Laszlo
>>
>>>
>>>  RollbackWritePointersAndFreeTracker:
>>>
>>
> 
> Thanks for the quick review and detailed explanations, Laszlo! I've
> submitted v2 of the patch with the changes.
> 

Thanks, I'll try to look at it sooner or later.

Laszlo