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

[Laszlo Ersek <lersek@redhat.com>]

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