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 7E7F721DFA7B0 for ; Thu, 30 Mar 2017 08:58:29 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id AC7EB19CF90; Thu, 30 Mar 2017 15:58:28 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com AC7EB19CF90 Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com AC7EB19CF90 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 5782E84709; Thu, 30 Mar 2017 15:58:27 +0000 (UTC) To: Phil Dennis-Jordan References: <1490773812-23839-1-git-send-email-lists@philjordan.eu> <1490773812-23839-2-git-send-email-lists@philjordan.eu> Cc: edk2-devel-01 , Phil Dennis-Jordan From: Laszlo Ersek Message-ID: Date: Thu, 30 Mar 2017 17:58:26 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Thu, 30 Mar 2017 15:58:29 +0000 (UTC) Subject: Re: [PATCH v1 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 15:58:29 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 03/30/17 12:41, Phil Dennis-Jordan wrote: > On Thu, Mar 30, 2017 at 2:06 PM, Laszlo Ersek wrote: >> This is a very good first patch! >> >> I have a few requests. I'll generally rehash the points from >> . >> >> On 03/29/17 09:50, Phil Dennis-Jordan wrote: >>> From: Phil Dennis-Jordan >>> >>> ACPI tables may contain multiple pointer fields 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. Indeed, some operating >>> systems demand this to be the case. >>> >>> Previously, if Qemu created "add pointer" linker commands for both fields, >>> the linking process would fail, as AcpiProtocol->InstallAcpiTable() may >>> only be called once for each destination table and otherwise returns an >>> error. >> >> (1) Please be a bit more precise here: passing the exact same table >> twice to InstallAcpiTable() is wrong, but for two different reasons. >> >> Reason #1 is what you named -- some, but not all, ACPI table types are >> not allowed to have multiple instances, and that aborts the linking >> process. Reason #2 is that successfully installing the exact same table, >> of a type that does allow multiple instances (such as an SSDT, there >> could be others) is just as wrong, although it doesn't abort the linking. > > Thanks - I know you've explained this distinction a bunch of times > now, but it's finally clicked with me. > >>> >>> This change adds a memoisation data structure which tracks the table >>> pointers that have already been installed; even if the same pointer is >>> encountered multiple times, it is only installed once. >> >> (2) Please s/installed/processed/g above -- see for more below. >> >>> >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Signed-off-by: Phil Dennis-Jordan >>> --- >>> OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 109 ++++++++++++++++---- >>> 1 file changed, 89 insertions(+), 20 deletions(-) >>> >>> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c >>> index 7bb2e3f21821..cffa838623cc 100644 >>> --- a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c >>> +++ b/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c >>> @@ -100,6 +100,39 @@ BlobCompare ( >>> >>> >>> /** >>> + Comparator function for two opaque pointers, ordering on 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; >>> + } else if ((INTN)Pointer1 < (INTN)Pointer2) { >> >> (3) Please use UINTN here, not INTN. >> >> Also, I slightly dislike an "else" after a "return", but I guess it's up >> to you. :) >> >>> + return -1; >>> + } else { >>> + return 1; >>> + } >>> +} >>> + >>> + >>> +/** >>> Process a QEMU_LOADER_ALLOCATE command. >>> >>> @param[in] Allocate The QEMU_LOADER_ALLOCATE command to process. >>> @@ -535,27 +568,32 @@ UndoCmdWritePointer ( >>> This function assumes that the entire QEMU linker/loader command file has >>> been processed successfully in a prior first pass. >>> >>> - @param[in] AddPointer The QEMU_LOADER_ADD_POINTER command to process. >>> + @param[in] AddPointer The QEMU_LOADER_ADD_POINTER command to process. >>> >>> - @param[in] Tracker The ORDERED_COLLECTION tracking the BLOB user >>> - structures. >>> + @param[in] Tracker The ORDERED_COLLECTION tracking the BLOB user >>> + structures. >>> >>> - @param[in] AcpiProtocol The ACPI table protocol used to install tables. >>> + @param[in] AcpiProtocol The ACPI table protocol used to install tables. >>> >>> - @param[in,out] InstalledKey On input, an array of INSTALLED_TABLES_MAX UINTN >>> - elements, allocated by the caller. On output, >>> - the function will have stored (appended) the >>> - AcpiProtocol-internal key of the ACPI table that >>> - the function has installed, if the AddPointer >>> - command identified an ACPI table that is >>> - different from RSDT and XSDT. >>> + @param[in,out] InstalledKey On input, an array of INSTALLED_TABLES_MAX UINTN >>> + elements, allocated by the caller. On output, >>> + the function will have stored (appended) the >>> + AcpiProtocol-internal key of the ACPI table that >>> + the function has installed, if the AddPointer >>> + command identified an ACPI table that is >>> + different from RSDT and XSDT. >>> >>> - @param[in,out] NumInstalled On input, the number of entries already used in >>> - InstalledKey; it must be in [0, >>> - INSTALLED_TABLES_MAX] inclusive. On output, the >>> - parameter is incremented if the AddPointer >>> - command identified an ACPI table that is >>> - different from RSDT and XSDT. >>> + @param[in,out] NumInstalled On input, the number of entries already used in >>> + InstalledKey; it must be in [0, >>> + INSTALLED_TABLES_MAX] inclusive. On output, the >>> + parameter is incremented if the AddPointer >>> + command identified an ACPI table that is >>> + different from RSDT and XSDT. >>> + >>> + @param[in,out] InstalledTables The ORDERED_COLLECTION tracking the ACPI tables >>> + which have already been installed. If a new >>> + table is encountered by the function, it is >>> + added; existing ones will not be installed again. >> >> (4) Please rename "InstalledTables" to "SeenPointers", and update its >> comment block accordingly: >> >> "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." >> >> (5) An added benefit of using the name "SeenPointers" is that it is >> exactly as long as "NumInstalled", and you won't have to reindent the >> other comment blocks. >> >> (6) Please also update the @retval EFI_SUCCESS comment: >> >> "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 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." >> >>> >>> @retval EFI_INVALID_PARAMETER NumInstalled was outside the allowed range on >>> input. >>> @@ -584,7 +622,8 @@ 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 *InstalledTables >>> ) >>> { >>> CONST ORDERED_COLLECTION_ENTRY *TrackerEntry; >>> @@ -679,6 +718,21 @@ Process2ndPassCmdAddPointer ( >>> return EFI_SUCCESS; >>> } >>> >>> + Status = OrderedCollectionInsert ( >>> + InstalledTables, NULL, (VOID *)(UINTN)PointerValue); >> >> (7) For function calls (and function-like macro invocations) that don't >> fit on a single line, we should now follow the style described in >> . >> >> (8) The main point of renaming InstalledTables to SeenPointers: >> >> please move this logic higher up in the function, >> >> ASSERT (PointerValue < Blob2Remaining); >> <------- right here >> Blob2Remaining -= (UINTN) PointerValue; >> >> The reason why is explained in >> , in the part >> that starts with "This is justified because all actions after that point >> would be..." > > I've now moved it as requested for patch v2, but my reasoning for the > original ordering of the logic was precisely to avoid the complication > of having to roll back in the error case. It seemed a valid code > transformation as the other early-out paths also return EFI_SUCCESS, > the RSDT/XSDT one has no side effects, and the opaque blob one's side > effect is surely idempotent. Yes, that's a valid argument, but I prefer the proposed (and, thankfully, v2 :) ) approach for two reasons: - I would like to benefit from the memoization also in order to save on the idempotent (but still superfluous) code paths, - I prefer a clean rollback-on-error because it is more future proof. If we omit it now, later extensions might forget about it completely, which at that point might not be so innocent any longer. > >> (9) Near the two late error exits in the function (when we run out of >> INSTALLED_TABLES_MAX, or InstallAcpiTable() fails), please replace the >> direct returns with a goto to an error handling label. This error >> handling label should remove the just added PointerValue from SeenPointers. >> >> For this, you can use the second parameter of OrderedCollectionInsert(). >> On success, that parameter is set to the node of the newly inserted >> item, and then you can pass that to OrderedCollectionDelete() directly >> on the error path. >> >>> + if (EFI_ERROR (Status)) { >>> + if (Status == RETURN_ALREADY_STARTED) { >>> + // >>> + // Already installed this table, don't try to do so again. >>> + // >>> + DEBUG ((DEBUG_VERBOSE, "%a: AcpiProtocol->InstallAcpiTable reports table " >>> + "already installed, skipping. PointerValue=0x%Lx\n", >>> + __FUNCTION__, PointerValue)); >>> + Status = EFI_SUCCESS; >>> + } >>> + return Status; >>> + } >>> + >>> if (*NumInstalled == INSTALLED_TABLES_MAX) { >>> DEBUG ((EFI_D_ERROR, "%a: can't install more than %d tables\n", >>> __FUNCTION__, INSTALLED_TABLES_MAX)); >>> @@ -739,6 +793,8 @@ InstallQemuFwCfgTables ( >>> UINTN *InstalledKey; >>> INT32 Installed; >>> ORDERED_COLLECTION_ENTRY *TrackerEntry, *TrackerEntry2; >>> + ORDERED_COLLECTION *InstalledTables; >>> + ORDERED_COLLECTION_ENTRY *InstalledTableEntry; >> >> (10) please rename InstalledTables here too. >> >>> >>> Status = QemuFwCfgFindFile ("etc/table-loader", &FwCfgItem, &FwCfgSize); >>> if (EFI_ERROR (Status)) { >>> @@ -827,14 +883,21 @@ InstallQemuFwCfgTables ( >>> goto RollbackWritePointersAndFreeTracker; >>> } >>> >>> + InstalledTables = OrderedCollectionInit (PointerCompare, PointerCompare); >>> + if (InstalledTables == NULL) { >>> + Status = EFI_OUT_OF_RESOURCES; >>> + goto FreeKeys; >>> + } >>> + >> >> Looks valid. >> >>> // >>> // 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, InstalledTables); >>> if (EFI_ERROR (Status)) { >>> goto UninstallAcpiTables; >>> } >>> @@ -870,6 +933,12 @@ UninstallAcpiTables: >>> DEBUG ((EFI_D_INFO, "%a: installed %d tables\n", __FUNCTION__, Installed)); >>> } >>> >>> + while (((InstalledTableEntry = OrderedCollectionMax(InstalledTables))) != NULL) { >>> + OrderedCollectionDelete (InstalledTables, InstalledTableEntry, NULL); >>> + } >>> + OrderedCollectionUninit (InstalledTables); >>> + >> >> (11) Please implement this loop instead like the one that tears down >> "Tracker". Using an assignment in the controlling expression of the >> "while" is powerful and I like it too, but it is never used in edk2 >> code, to my knowledge. > > There's a third option, which is to also use a for loop, but avoids > the second running entry variable by calling OrderedCollectionMin() > (or Max) on each iteration. This might even be slightly more efficient > than OrderedCollectionNext(), but only by a constant factor, so I've > gone with the same teardown as for Tracker for the sake of > consistency. Thanks! > >>> +FreeKeys: >>> FreePool (InstalledKey); >> >> Yes, this looks correct too. >> >> (I apologize if I made any confusing requests in my review, I'm very tired.) >> >> Thank you, >> Laszlo >> >>> >>> RollbackWritePointersAndFreeTracker: >>> >> > > Thanks for the quick review and detailed explanations, Laszlo! I've > submitted v2 of the patch with the changes. > Thanks, I'll try to look at it sooner or later. Laszlo