public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Laszlo Ersek <lersek@redhat.com>
To: Phil Dennis-Jordan <lists@philjordan.eu>, edk2-devel@lists.01.org
Cc: 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: Thu, 30 Mar 2017 22:20:57 +0200	[thread overview]
Message-ID: <176dbf0a-8a1e-b18b-461a-39a8330bb0b7@redhat.com> (raw)
In-Reply-To: <1490870457-27859-2-git-send-email-lists@philjordan.eu>

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.

Thank you very much for the contribution!

Laszlo



  reply	other threads:[~2017-03-30 20:21 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 [this message]
2017-04-02 22:47     ` Phil Dennis-Jordan

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=176dbf0a-8a1e-b18b-461a-39a8330bb0b7@redhat.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