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
next prev parent 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