From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 3BB8221DFA7AF for ; Thu, 30 Mar 2017 13:21:01 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 6233461D38; Thu, 30 Mar 2017 20:21:00 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 6233461D38 Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 6233461D38 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-143.phx2.redhat.com [10.3.116.143]) by smtp.corp.redhat.com (Postfix) with ESMTP id CAA4517C3F; Thu, 30 Mar 2017 20:20:58 +0000 (UTC) To: Phil Dennis-Jordan , edk2-devel@lists.01.org References: <1490870457-27859-1-git-send-email-lists@philjordan.eu> <1490870457-27859-2-git-send-email-lists@philjordan.eu> Cc: Jordan Justen , Phil Dennis-Jordan From: Laszlo Ersek Message-ID: <176dbf0a-8a1e-b18b-461a-39a8330bb0b7@redhat.com> Date: Thu, 30 Mar 2017 22:20:57 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <1490870457-27859-2-git-send-email-lists@philjordan.eu> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Thu, 30 Mar 2017 20:21:00 +0000 (UTC) Subject: Re: [PATCH v2 1/1] OvmfPkg: Allow multiple add-pointer linker commands to same ACPI table X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 30 Mar 2017 20:21:01 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/30/17 12:40, Phil Dennis-Jordan wrote: > From: Phil Dennis-Jordan > > 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 > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Phil Dennis-Jordan > --- > > 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 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: 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 Pushed as commit 072060a6f81b. Thank you very much for the contribution! Laszlo