From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg0-x243.google.com (mail-pg0-x243.google.com [IPv6:2607:f8b0:400e:c05::243]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 0537A208F7A02 for ; Thu, 30 Mar 2017 03:41:11 -0700 (PDT) Received: by mail-pg0-x243.google.com with SMTP id 81so9065619pgh.3 for ; Thu, 30 Mar 2017 03:41:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=philjordan-eu.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:date:message-id:in-reply-to:references; bh=hzqsHk12QXnm7zoh3xYGTN/wCmuXYe22K9nDRMLdxBk=; b=wijrvZL6F+W3QSGWO+glabZD1RColZk304T1RgrZYvJ2Sd70aR7Q5kWoNeUE1lYNYG CxgDcqXPV6UTMKNz1voli1OmH4hOIef7gQM2UJSnjlS3mgZfYDADmpR8A09H59XaEUY9 0b9aYg8Br/iPI3tAcZus4sC7LNrrC+Am6p+dtJQ6ltk1lY0xFFEmFF4bbFhRC0Q7w9DM 7aXhDZhtSGdstFORX5zoko0S5zb6Dafax8ev9SeZx+QSqaqQupBxSjiNPKNkNOAR4tw1 LqO55eq5lej7kY/0+SG2Hi+WPRlR/0Wal9cjNM/AVx4t0ZkgMNZ1dscSluKhDKRLBdjL Pgbg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=hzqsHk12QXnm7zoh3xYGTN/wCmuXYe22K9nDRMLdxBk=; b=nEAKqpPYcTywM9YXYd2UEWLfnvIzfKr+MbEaNecRx5fJN3RuJyni0YjeK/4zIi29et q/QcozQDrfYsu1qtxVwvOh8PTJaLZQYpU1Vc9slKhNRGF7YPF37eOWn9Mvgw1tSo/DN+ CViYR8H2bIa/g61Fm0OzN9nTgzZofHVE1YPQzPAu18I9xo/dIU1fG0dRQudPJzhXZknk rgq5eSgp67ZF6jiIWsnWSb7/pNeVCImspw47ILQaDysqFgAnbC12o5X8gwdyyxTriuX/ F/r8sLdFNVyded4GlF2ykK439ESiXWO5yJldGYYRx6xiBv6GEOdOhC+92YjLzQB2PwIg dFWA== X-Gm-Message-State: AFeK/H14J8/dyQ1CU1MdhD2uYSVhCAIbc2NmeChVHii1KPNUPWZbidfC7iXr3GRUPoRe3g== X-Received: by 10.98.71.149 with SMTP id p21mr5513465pfi.94.1490870470597; Thu, 30 Mar 2017 03:41:10 -0700 (PDT) Received: from localhost.localdomain ([118.82.182.58]) by smtp.gmail.com with ESMTPSA id t187sm3691181pfb.116.2017.03.30.03.41.07 (version=TLS1 cipher=AES128-SHA bits=128/128); Thu, 30 Mar 2017 03:41:10 -0700 (PDT) From: Phil Dennis-Jordan To: edk2-devel@lists.01.org Cc: Phil Dennis-Jordan , Jordan Justen , Laszlo Ersek Date: Thu, 30 Mar 2017 23:40:57 +1300 Message-Id: <1490870457-27859-2-git-send-email-lists@philjordan.eu> X-Mailer: git-send-email 2.3.2 (Apple Git-55) In-Reply-To: <1490870457-27859-1-git-send-email-lists@philjordan.eu> References: <1490870457-27859-1-git-send-email-lists@philjordan.eu> Subject: [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 10:41:11 -0000 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. + 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 + 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 + ); + 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)); + 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); 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: -- 2.3.2 (Apple Git-55)