* [PATCH v2 0/1] OvmfPkg/AcpiPlatformDxe: Fix bug 368, multiply pointed-to tables
@ 2017-03-30 10:40 Phil Dennis-Jordan
2017-03-30 10:40 ` [PATCH v2 1/1] OvmfPkg: Allow multiple add-pointer linker commands to same ACPI table Phil Dennis-Jordan
0 siblings, 1 reply; 4+ messages in thread
From: Phil Dennis-Jordan @ 2017-03-30 10:40 UTC (permalink / raw)
To: edk2-devel
This fixes the bug in OVMF's Qemu ACPI table linker which caused it to
fail or misbehave when multiple fields point to the same table.
Previously, each pointer caused the pointed-to table to be installed
via the EFI_ACPI_TABLE_PROTOCOL. However, each table must only be
installed once via this mechanism.
The patch fixes this (as previously suggested by Laszlo) via memoisation
of the pointers. If it encounters the same pointer twice, it will no
longer try to process it again.
I've tested it successfully with Windows 10, a recent Ubuntu
version, and OS X as guest OSes. (further OVMF patches required for
booting the latter)
I found this bug while trying to patch Qemu to generate a Rev3 FADT (as
per ACPI 2.0) instead of a Rev1 FADT (ACPI 1.0). Said patch has missed
the Qemu 2.9 merge window, but has been provisionally accepted for 2.10
(save for some minor tweaks) and will break the ACPI table linker
on unpatched OVMF. (When this bug is triggered, OVMF reverts to its
built-in ACPI tables, ignoring those provided by Qemu.)
v1 -> v2
- The only functional change is that the memoisation happens earlier
in the processing of QEMU_LOADER_ADD_POINTER commands; right at the
start, not immediately before installing the table. [Laszlo]
- Other changes are documentation/naming/style. (see patch 1/1) [Laszlo]
Previous discussion: https://lists.gnu.org/archive/html/qemu-devel/2017-01/msg06679.html
Qemu patch: https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg02837.html
Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=368
Github feature branch: https://github.com/pmj/edk2/tree/bug_368_v2
Phil Dennis-Jordan (1):
OvmfPkg: Allow multiple add-pointer linker commands to same ACPI table
OvmfPkg/AcpiPlatformDxe/QemuFwCfgAcpi.c | 109 ++++++++++++++++++--
1 file changed, 98 insertions(+), 11 deletions(-)
--
2.3.2 (Apple Git-55)
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2 1/1] OvmfPkg: Allow multiple add-pointer linker commands to same ACPI table
2017-03-30 10:40 [PATCH v2 0/1] OvmfPkg/AcpiPlatformDxe: Fix bug 368, multiply pointed-to tables Phil Dennis-Jordan
@ 2017-03-30 10:40 ` Phil Dennis-Jordan
2017-03-30 20:20 ` Laszlo Ersek
0 siblings, 1 reply; 4+ messages in thread
From: Phil Dennis-Jordan @ 2017-03-30 10:40 UTC (permalink / raw)
To: edk2-devel; +Cc: Phil Dennis-Jordan, Jordan Justen, Laszlo Ersek
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)
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/1] OvmfPkg: Allow multiple add-pointer linker commands to same ACPI table
2017-03-30 10:40 ` [PATCH v2 1/1] OvmfPkg: Allow multiple add-pointer linker commands to same ACPI table Phil Dennis-Jordan
@ 2017-03-30 20:20 ` Laszlo Ersek
2017-04-02 22:47 ` Phil Dennis-Jordan
0 siblings, 1 reply; 4+ messages in thread
From: Laszlo Ersek @ 2017-03-30 20:20 UTC (permalink / raw)
To: Phil Dennis-Jordan, edk2-devel; +Cc: Jordan Justen, Phil Dennis-Jordan
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/1] OvmfPkg: Allow multiple add-pointer linker commands to same ACPI table
2017-03-30 20:20 ` Laszlo Ersek
@ 2017-04-02 22:47 ` Phil Dennis-Jordan
0 siblings, 0 replies; 4+ messages in thread
From: Phil Dennis-Jordan @ 2017-04-02 22:47 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Jordan Justen, Phil Dennis-Jordan
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-04-02 22:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-30 10:40 [PATCH v2 0/1] OvmfPkg/AcpiPlatformDxe: Fix bug 368, multiply pointed-to tables Phil Dennis-Jordan
2017-03-30 10:40 ` [PATCH v2 1/1] OvmfPkg: Allow multiple add-pointer linker commands to same ACPI table Phil Dennis-Jordan
2017-03-30 20:20 ` Laszlo Ersek
2017-04-02 22:47 ` Phil Dennis-Jordan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox