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.
+ 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)
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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.
Thank you very much for the contribution!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2025 Red Hat, Inc.