public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Phil Dennis-Jordan <lists@philjordan.eu>
To: Laszlo Ersek <lersek@redhat.com>
Cc: edk2-devel-01 <edk2-devel@lists.01.org>,
	Jordan Justen <jordan.l.justen@intel.com>,
	 Phil Dennis-Jordan <phil@philjordan.eu>
Subject: Re: [PATCH v2 1/1] OvmfPkg: Allow multiple add-pointer linker commands to same ACPI table
Date: Mon, 3 Apr 2017 10:47:06 +1200	[thread overview]
Message-ID: <CAGCz3vs3+LbdTHVvEyzT1rdNCi-_RqhaWQ8S6cSuCvgqAK4HYg@mail.gmail.com> (raw)
In-Reply-To: <176dbf0a-8a1e-b18b-461a-39a8330bb0b7@redhat.com>

On Fri, Mar 31, 2017 at 9:20 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/30/17 12:40, Phil Dennis-Jordan wrote:
>> 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.
>
> Argh, this was a brain-fart in my previous review (as I said, I was extremely tired at that point). DSDT should be XSDT here.
>
> Also, this comment adds an overlong line (I think exactly 80 chars are too many? I prefer 79 anyway).
>
>> +                               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
>
> Same line length problem.
>
>> +                                 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
>> +           );
>
> The closing paren isn't correctly indented.
>
>> +  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));
>
> First, this is a multi-line function-like macro invocation, so it should be broken up; second, you forgot to update the debug message to match the new situation.
>
>> +      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);
>
> Since you are touching this multi-line function call, it should be broken up.
>
>>        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:
>>
>
> Apologies for the terse remarks above. In reality I'm super pleased with this patch; all of my v1 comments have been addressed. So, rather than asking you politely to send a v3 addressing the above warts, I decided to fix them up myself. This is the diff that I squashed into your patch:
>
>> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>> index 9e45436bcda3..1bc5fe297a96 100644
>> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c
>> @@ -593,12 +593,12 @@ UndoCmdWritePointer (
>>    @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.
>> +                               target address is encountered for the first
>> +                               time, and it identifies an ACPI table that is
>> +                               different from RDST and XSDT, 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.
>
> Two updates here: fixing the DSDT<->XSDT typo (which was my fault in the v1 review), and rewrapping to 79 chars. (Again, I consider 80 exclusive, because the CRLF in that exact spot tends to break to the next line when displayed in a terminal window of exactly 80 chars width.)
>
>> @@ -611,11 +611,11 @@ UndoCmdWritePointer (
>>                                   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.
>> +                                 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.
>>
>>    @return                        Error codes returned by
>>                                   AcpiProtocol->InstallAcpiTable().
>
> Rewrapping.
>
>> @@ -670,15 +670,18 @@ Process2ndPassCmdAddPointer (
>>               SeenPointers,
>>               &SeenPointerEntry, // for reverting insertion in error case
>>               (VOID *)(UINTN)PointerValue
>> -           );
>> +             );
>
> If you look at <https://bugzilla.tianocore.org/show_bug.cgi?id=425> carefully, this is what it requires.
>
>>    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));
>> +      DEBUG ((
>> +        DEBUG_VERBOSE,
>> +        "%a: PointerValue=0x%Lx already processed, skipping.\n",
>> +        __FUNCTION__,
>> +        PointerValue
>> +        ));
>>        Status = EFI_SUCCESS;
>>      }
>>      return Status;
>
> Two updates: adapt the debug message to the situation, and use the right multi-line style.
>
>> @@ -911,8 +914,13 @@ InstallQemuFwCfgTables (
>>    for (LoaderEntry = LoaderStart; LoaderEntry < LoaderEnd; ++LoaderEntry) {
>>      if (LoaderEntry->Type == QemuLoaderCmdAddPointer) {
>>        Status = Process2ndPassCmdAddPointer (
>> -                 &LoaderEntry->Command.AddPointer, Tracker, AcpiProtocol,
>> -                 InstalledKey, &Installed, SeenPointers);
>> +                 &LoaderEntry->Command.AddPointer,
>> +                 Tracker,
>> +                 AcpiProtocol,
>> +                 InstalledKey,
>> +                 &Installed,
>> +                 SeenPointers
>> +                 );
>>        if (EFI_ERROR (Status)) {
>>          goto UninstallAcpiTables;
>>        }
>
> Use the right multi-line style.
>
> Tags I appended:
>
>     Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>     [lersek@redhat.com: DSDT<->XSDT typo, debug msg, and coding style fixups]
>     Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=368
>     Contributed-under: TianoCore Contribution Agreement 1.0
>     Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>
> Pushed as commit 072060a6f81b.

Thanks for your help with this patch, Laszlo!

> Thank you very much for the contribution!
>
> Laszlo
>


      reply	other threads:[~2017-04-02 22:47 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-30 10:40 [PATCH v2 0/1] OvmfPkg/AcpiPlatformDxe: Fix bug 368, multiply pointed-to tables Phil Dennis-Jordan
2017-03-30 10:40 ` [PATCH v2 1/1] OvmfPkg: Allow multiple add-pointer linker commands to same ACPI table Phil Dennis-Jordan
2017-03-30 20:20   ` Laszlo Ersek
2017-04-02 22:47     ` Phil Dennis-Jordan [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGCz3vs3+LbdTHVvEyzT1rdNCi-_RqhaWQ8S6cSuCvgqAK4HYg@mail.gmail.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox