From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-vk0-x229.google.com (mail-vk0-x229.google.com [IPv6:2607:f8b0:400c:c05::229]) (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 8F4AF21BC6ADB for ; Sun, 2 Apr 2017 15:47:28 -0700 (PDT) Received: by mail-vk0-x229.google.com with SMTP id z204so121038210vkd.1 for ; Sun, 02 Apr 2017 15:47:28 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=philjordan-eu.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=C1CZwiwYa/QCA9wt7qw5NX5gULkOi0Y1McJpXRGbaM4=; b=MVqx9FO9Gysy+T4Ot/gn7UoAmDJRXMApJIoZR9lCprG71EoNC+Oi8ns8Ja2BhFV5LO Y0adyJZ748lDnHS0d4nEAJoSaq9uwKwaJd5ZwH3TAqphZD5W7daeslH2Z77xoPti20nC ErCktdzjhiUJSBRcPjBw7s0c/5fLg3NARV+MKBSMg23m886BY5+SN7FcES7Z68sAi3Cl GVTn6lGj8m9YJyPmIjDRpOBxGkLPUy6wy/2FJM0/JYzvjEAGhfKQVpNc6KfklNfBu7Zn PipT+iz8RAXIhVg7geSFCeocbqN/cOSddsahQ39hH25YOQLGrbAV9kmToDEuHt/K8oo1 YEaw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=C1CZwiwYa/QCA9wt7qw5NX5gULkOi0Y1McJpXRGbaM4=; b=h3FjOKIwN1OVf2VsMFJIZw6ux6uobMjiASk5QI3jNj3ISNQlXuH95YHBe0cqpSJ1DL 8Jo4nFCkZKhcwwIjq09vNo4tlmYdJDvD/XsUB+GPti3RxYtyJ5Z97fDN4I/gb1Si7+BD GsAabzJINT3MvVvhaPLNu7U72k3O7f86JGqVKWMpkpmbyMfrjoLkeTSJKInW/z00Eojk 2Jjd76xjzuKdAEyVZMeqKw8HGaw0tN8YlhlQAL5a2xyIy3uutACDmfZHRpGfTyVNVQcC JPqhvfNd4ZQRrTHDilgLSaJbawDAz6o6DzNoH6jli+nI4wUgHAp4AHVHCY4LqWzqaQM3 erPw== X-Gm-Message-State: AFeK/H36u2AUeUChhgs/elCTxFj7Vbd+zZ6K3C+AmmVEG/wjyp6xyJEEgrVjXs0hM4wUFc7DdwnjK3vsL/o9Rw== X-Received: by 10.31.96.198 with SMTP id u189mr3039848vkb.117.1491173247199; Sun, 02 Apr 2017 15:47:27 -0700 (PDT) MIME-Version: 1.0 Received: by 10.31.58.83 with HTTP; Sun, 2 Apr 2017 15:47:06 -0700 (PDT) In-Reply-To: <176dbf0a-8a1e-b18b-461a-39a8330bb0b7@redhat.com> References: <1490870457-27859-1-git-send-email-lists@philjordan.eu> <1490870457-27859-2-git-send-email-lists@philjordan.eu> <176dbf0a-8a1e-b18b-461a-39a8330bb0b7@redhat.com> From: Phil Dennis-Jordan Date: Mon, 3 Apr 2017 10:47:06 +1200 Message-ID: To: Laszlo Ersek Cc: edk2-devel-01 , Jordan Justen , Phil Dennis-Jordan 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: Sun, 02 Apr 2017 22:47:29 -0000 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Fri, Mar 31, 2017 at 9:20 AM, Laszlo Ersek wrote: > 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 proce= ssing, >> not just immediately before table install; updated names & comment= s 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/AcpiPlatf= ormDxe/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) p= ointer >> + 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 =3D=3D 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 abso= lute >> + target addresses that have been pointed-= to by >> + QEMU_LOADER_ADD_POINTER commands thus fa= r. If a >> + target address is encountered for the fi= rst time, >> + and it identifies an ACPI table that is = different >> + from RDST and DSDT, the table is install= ed. > > Argh, this was a brain-fart in my previous review (as I said, I was extre= mely tired at that point). DSDT should be XSDT here. > > Also, this comment adds an overlong line (I think exactly 80 chars are to= o many? I prefer 79 anyway). > >> + If a target address is seen for the seco= nd or >> + later times, it is skipped without takin= g any >> + action. >> + >> @retval EFI_INVALID_PARAMETER NumInstalled was outside the allowed r= ange on >> input. >> >> @@ -564,12 +607,13 @@ UndoCmdWritePointer ( >> table different from RSDT and XSDT, bu= t 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 a= nd >> - NumInstalled), or RSDT or XSDT has bee= n >> - identified but not installed, or the f= w_cfg >> - blob pointed-into by AddPointer has be= en >> + @retval EFI_SUCCESS AddPointer has been processed. Either = its >> + absolute target address has been encou= ntered >> + before, or an ACPI table different fro= m RSDT >> + and XSDT has been installed (reflected= by >> + InstalledKey and NumInstalled), or RSD= T 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_MA= X], >> - 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 +=3D Blob2->Size; >> ASSERT (PointerValue < Blob2Remaining); >> >> + Status =3D OrderedCollectionInsert ( >> + SeenPointers, >> + &SeenPointerEntry, // for reverting insertion in error cas= e >> + (VOID *)(UINTN)PointerValue >> + ); > > The closing paren isn't correctly indented. > >> + if (EFI_ERROR (Status)) { >> + if (Status =3D=3D RETURN_ALREADY_STARTED) { >> + // >> + // Already seen this pointer, don't try to process it again. >> + // >> + DEBUG ((DEBUG_VERBOSE, "%a: AcpiProtocol->InstallAcpiTable report= s table " >> + "already installed, skipping. PointerValue=3D0x%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 n= ew situation. > >> + Status =3D EFI_SUCCESS; >> + } >> + return Status; >> + } >> + >> Blob2Remaining -=3D (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 =3D=3D 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 =3D EFI_OUT_OF_RESOURCES; >> + goto RollbackSeenPointer; >> } >> >> Status =3D 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 =3D QemuFwCfgFindFile ("etc/table-loader", &FwCfgItem, &FwCfgS= ize); >> if (EFI_ERROR (Status)) { >> @@ -827,14 +898,21 @@ InstallQemuFwCfgTables ( >> goto RollbackWritePointersAndFreeTracker; >> } >> >> + SeenPointers =3D OrderedCollectionInit (PointerCompare, PointerCompar= e); >> + if (SeenPointers =3D=3D NULL) { >> + Status =3D EFI_OUT_OF_RESOURCES; >> + goto FreeKeys; >> + } >> + >> // >> // second pass: identify and install ACPI tables >> // >> Installed =3D 0; >> for (LoaderEntry =3D LoaderStart; LoaderEntry < LoaderEnd; ++LoaderEn= try) { >> if (LoaderEntry->Type =3D=3D QemuLoaderCmdAddPointer) { >> - Status =3D Process2ndPassCmdAddPointer (&LoaderEntry->Command.Add= Pointer, >> - Tracker, AcpiProtocol, InstalledKey, &Installed); >> + Status =3D Process2ndPassCmdAddPointer ( >> + &LoaderEntry->Command.AddPointer, Tracker, AcpiProtoco= l, >> + 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__, Inst= alled)); >> } >> >> + for (SeenPointerEntry =3D OrderedCollectionMin (SeenPointers); >> + SeenPointerEntry !=3D NULL; >> + SeenPointerEntry =3D SeenPointerEntry2) { >> + SeenPointerEntry2 =3D 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 aski= ng you politely to send a v3 addressing the above warts, I decided to fix t= hem up myself. This is the diff that I squashed into your patch: > >> diff --git a/OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c b/OvmfPkg/AcpiPlatf= ormDxe/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 abso= lute >> target addresses that have been pointed-= to by >> QEMU_LOADER_ADD_POINTER commands thus fa= r. If a >> - target address is encountered for the fi= rst time, >> - and it identifies an ACPI table that is = different >> - from RDST and DSDT, the table is install= ed. >> - If a target address is seen for the seco= nd or >> - later times, it is skipped without takin= g any >> - action. >> + target address is encountered for the fi= rst >> + time, and it identifies an ACPI table th= at is >> + different from RDST and XSDT, the table = is >> + installed. If a target address is seen f= or the >> + second or later times, it is skipped wit= hout >> + taking any action. >> >> @retval EFI_INVALID_PARAMETER NumInstalled was outside the allowed r= ange 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, be= cause the CRLF in that exact spot tends to break to the next line when disp= layed in a terminal window of exactly 80 chars width.) > >> @@ -611,11 +611,11 @@ UndoCmdWritePointer ( >> absolute target address has been encou= ntered >> before, or an ACPI table different fro= m RSDT >> and XSDT has been installed (reflected= by >> - InstalledKey and NumInstalled), or RSD= T 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 RSD= T or >> + XSDT has been identified but not insta= lled, or >> + the fw_cfg blob pointed-into by AddPoi= nter 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 cas= e >> (VOID *)(UINTN)PointerValue >> - ); >> + ); > > If you look at car= efully, this is what it requires. > >> if (EFI_ERROR (Status)) { >> if (Status =3D=3D RETURN_ALREADY_STARTED) { >> // >> // Already seen this pointer, don't try to process it again. >> // >> - DEBUG ((DEBUG_VERBOSE, "%a: AcpiProtocol->InstallAcpiTable report= s table " >> - "already installed, skipping. PointerValue=3D0x%Lx\n", >> - __FUNCTION__, PointerValue)); >> + DEBUG (( >> + DEBUG_VERBOSE, >> + "%a: PointerValue=3D0x%Lx already processed, skipping.\n", >> + __FUNCTION__, >> + PointerValue >> + )); >> Status =3D 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 =3D LoaderStart; LoaderEntry < LoaderEnd; ++LoaderEn= try) { >> if (LoaderEntry->Type =3D=3D QemuLoaderCmdAddPointer) { >> Status =3D Process2ndPassCmdAddPointer ( >> - &LoaderEntry->Command.AddPointer, Tracker, AcpiProtoco= l, >> - 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 fix= ups] > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D368 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek > > Pushed as commit 072060a6f81b. Thanks for your help with this patch, Laszlo! > Thank you very much for the contribution! > > Laszlo >