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