* [PATCH v2 0/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
@ 2017-03-08 19:58 Laszlo Ersek
2017-03-08 19:58 ` [PATCH v2 1/2] MdeModulePkg/AcpiTableDxe: condense whitespace around FADT.{DSDT, X_DSDT} Laszlo Ersek
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-03-08 19:58 UTC (permalink / raw)
To: edk2-devel-01
Cc: Al Stone, Ard Biesheuvel, Feng Tian, Igor Mammedov, Jiewen Yao,
Leo Duran, Michael Tsirkin, Phil Dennis-Jordan, Star Zeng
This is version 2 of the series originally posted at
<https://lists.01.org/pipermail/edk2-devel/2017-February/007004.html>
and further discussed at
<https://lists.01.org/pipermail/edk2-devel/2017-March/008297.html>.
This version contains minor updates (noted on each patch) that don't
affect functionality.
Repo: https://github.com/lersek/edk2.git
Branch: fadt_dsdt_v2
Cc: Al Stone <ahs3@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Feng Tian <feng.tian@intel.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Leo Duran <leo.duran@amd.com>
Cc: Michael Tsirkin <mtsirkin@redhat.com>
Cc: Phil Dennis-Jordan <phil@philjordan.eu>
Cc: Star Zeng <star.zeng@intel.com>
Thanks
Laszlo
Laszlo Ersek (2):
MdeModulePkg/AcpiTableDxe: condense whitespace around
FADT.{DSDT,X_DSDT}
MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT,X_DSDT} mutual exclusion
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 78 ++++++++++++++++----
1 file changed, 63 insertions(+), 15 deletions(-)
--
2.9.3
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/2] MdeModulePkg/AcpiTableDxe: condense whitespace around FADT.{DSDT, X_DSDT}
2017-03-08 19:58 [PATCH v2 0/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion Laszlo Ersek
@ 2017-03-08 19:58 ` Laszlo Ersek
2017-03-09 0:47 ` Yao, Jiewen
2017-03-08 19:58 ` [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion Laszlo Ersek
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Laszlo Ersek @ 2017-03-08 19:58 UTC (permalink / raw)
To: edk2-devel-01
Cc: Al Stone, Ard Biesheuvel, Feng Tian, Igor Mammedov, Jiewen Yao,
Leo Duran, Michael Tsirkin, Phil Dennis-Jordan, Star Zeng
This patch incurs no functional changes, it just removes some whitespace,
and also makes sure we always assign
AcpiTableInstance->Fadt3->Dsdt
first, and
AcpiTableInstance->Fadt3->XDsdt
second. The goal is to separate the syntactic changes from the functional
changes implemented by the next patch.
Cc: Al Stone <ahs3@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Feng Tian <feng.tian@intel.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Leo Duran <leo.duran@amd.com>
Cc: Michael Tsirkin <mtsirkin@redhat.com>
Cc: Phil Dennis-Jordan <phil@philjordan.eu>
Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Feng Tian <feng.tian@intel.com>
---
Notes:
v2:
- no changes
- pick up R-b's from Thomas and Feng
v1:
NOTE for people on the CC list:
If you are not presently subscribed to edk2-devel and wish to comment on
this patch publicly, you need to subscribe first, and wait for the
subscription request to *complete* (see your inbox), *before* sending
your followup. This is not ideal, but edk2-devel requires subscription
before reflecting messages from someone.
Subscribe at <https://lists.01.org/mailman/listinfo/edk2-devel>. Thanks.
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 20 ++++++--------------
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
index 7f95b9dc709d..7795ff7269ca 100644
--- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
+++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
@@ -646,16 +646,12 @@ AddTableToList (
AcpiTableInstance->Fadt3->FirmwareCtrl = 0;
}
if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
- AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN) AcpiTableInstance->Dsdt3;
+ AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN) AcpiTableInstance->Dsdt3;
ZeroMem (&AcpiTableInstance->Fadt3->XDsdt, sizeof (UINT64));
} else {
- Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
- CopyMem (
- &AcpiTableInstance->Fadt3->XDsdt,
- &Buffer64,
- sizeof (UINT64)
- );
AcpiTableInstance->Fadt3->Dsdt = 0;
+ Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
+ CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof (UINT64));
}
//
@@ -850,14 +846,10 @@ AddTableToList (
//
if (AcpiTableInstance->Fadt3 != NULL) {
if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
- AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN) AcpiTableInstance->Dsdt3;
+ AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN) AcpiTableInstance->Dsdt3;
}
- Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
- CopyMem (
- &AcpiTableInstance->Fadt3->XDsdt,
- &Buffer64,
- sizeof (UINT64)
- );
+ Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
+ CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof (UINT64));
//
// Checksum FADT table
--
2.9.3
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
2017-03-08 19:58 [PATCH v2 0/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion Laszlo Ersek
2017-03-08 19:58 ` [PATCH v2 1/2] MdeModulePkg/AcpiTableDxe: condense whitespace around FADT.{DSDT, X_DSDT} Laszlo Ersek
@ 2017-03-08 19:58 ` Laszlo Ersek
2017-03-09 0:47 ` Yao, Jiewen
2017-03-13 3:07 ` Fan, Jeff
2017-03-09 1:59 ` [PATCH v2 0/2] " Zeng, Star
2017-03-09 14:06 ` Laszlo Ersek
3 siblings, 2 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-03-08 19:58 UTC (permalink / raw)
To: edk2-devel-01
Cc: Al Stone, Ard Biesheuvel, Feng Tian, Igor Mammedov, Jiewen Yao,
Leo Duran, Michael Tsirkin, Phil Dennis-Jordan, Star Zeng
The ACPI specification, up to and including revision 5.1 Errata A, allows
the DSDT and X_DSDT fields to be both set in the FADT. (Obviously, this
only makes sense if the DSDT address is representable in 4 bytes.)
Starting with 5.1 Errata B, specifically for Mantis 1393
<https://mantis.uefi.org/mantis/view.php?id=1393>, the spec requires at
most one of DSDT and X_DSDT to be set to a nonzero value.
MdeModulePkg/AcpiTableDxe handles this mutual exclusion somewhat
inconsistently.
- If the caller of EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() installs the
tables in "DSDT, FADT" order, then we enforce the exclusion between the
DSDT and X_DSDT fields:
DSDT under 4GB FADT.DSDT FADT.X_DSDT [VARIANT B]
-------------- --------- -----------
yes set clear
no clear set
This behavior conforms to 5.1 Errata B. (And it's not required by
earlier versions of the spec.)
- If the caller passes in the tables in "FADT, DSDT" relative order, then
we do not enforce the exclusion:
DSDT under 4GB FADT.DSDT FADT.X_DSDT [VARIANT A]
-------------- --------- -----------
yes set set
no clear set
This satisfies 5.1 Errata A and earlier, but breaks 5.1 Errata B and
later.
Unify the handling of both relative orders. In particular, check the major
and minor version numbers in the FADT. If the FADT version is strictly
before 5.1, then implement [VARIANT A]. If the FADT version is equal to or
larger than 5.1, then implement [VARIANT B].
We make three observations:
- We can't check the FADT table version precisely against "5.1 Errata B";
erratum levels are not captured in the table. We err in the safe
direction, namely we enforce the exclusion for "5.1" and "5.1 Errata A".
- The same applies to "6.0" versus "6.0 Errata A". Because we cannot
distinguish these two, we consider "6.0" to be "equal to or larger than
5.1", and apply [VARIANT B], enforcing the exclusion.
- While a blanket [VARIANT B] would be simpler, there is a significant
benefit to [VARIANT A], under the spec versions that permit it:
compatibility with a wider range of OSPMs (typically, older ones).
For example, Igor reported about a "DELL R430 system with rev4 FADT
where DSDT and X_DSDT are pointing to the same address". Michael also
reported about several systems that exhibit the same.
Regression tested with the following KVM guests (QEMU built at
ata0def594286d, "Merge remote-tracking branch
'remotes/bonzini/tags/for-upstream' into staging", 2017-01-30):
- OVMF: boot and S3 suspend/resume
- Ia32, Q35, SMM
- Fedlet 20141209
- Ia32X64, Q35, SMM
- Fedora 22
- Windows 7
- Windows 8.1
- Windows 10
- Windows Server 2008 R2
- Windows Server 2012 R2
- Windows Server 2016 Tech Preview 4
- X64, I440FX, no SMM
- Fedora 24
- RHEL-6.7
- RHEL-7.2-ish
- ArmVirtQemu: boot test with virtio-gpu
- AARCH64
- Fedora 24
- RHELSA-7.3
- openSUSE Tumbleweed (4.8.4-based)
This change is connected to ASWG ticket
<https://mantis.uefi.org/mantis/view.php?id=1757>, which is now
closed/fixed.
Cc: Al Stone <ahs3@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Feng Tian <feng.tian@intel.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Leo Duran <leo.duran@amd.com>
Cc: Michael Tsirkin <mtsirkin@redhat.com>
Cc: Phil Dennis-Jordan <phil@philjordan.eu>
Cc: Star Zeng <star.zeng@intel.com>
Reported-by: Phil Dennis-Jordan <phil@philjordan.eu>
Suggested-by: Igor Mammedov <imammedo@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
Notes:
v2:
- simplify logic in RequireDsdtXDsdtExclusion() [Jiewen]
- pick up Phil's R-b nonetheless (the above change is a minimal
reformulation of code, with no behavioral difference)
- add reference to Mantis#1757 to the commit message
v1:
NOTE for people on the CC list:
If you are not presently subscribed to edk2-devel and wish to comment on
this patch publicly, you need to subscribe first, and wait for the
subscription request to *complete* (see your inbox), *before* sending
your followup. This is not ideal, but edk2-devel requires subscription
before reflecting messages from someone.
Subscribe at <https://lists.01.org/mailman/listinfo/edk2-devel>. Thanks.
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 62 +++++++++++++++++++-
1 file changed, 59 insertions(+), 3 deletions(-)
diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
index 7795ff7269ca..4bb848df5203 100644
--- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
+++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
@@ -430,6 +430,51 @@ ReallocateAcpiTableBuffer (
mEfiAcpiMaxNumTables = NewMaxTableNumber;
return EFI_SUCCESS;
}
+
+/**
+ Determine whether the FADT table passed in as parameter requires mutual
+ exclusion between the DSDT and X_DSDT fields. (That is, whether there exists
+ an explicit requirement that at most one of those fields is permitted to be
+ nonzero.)
+
+ @param[in] Fadt The EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE object to
+ check.
+
+ @retval TRUE Fadt requires mutual exclusion between DSDT and X_DSDT.
+ @retval FALSE Otherwise.
+**/
+BOOLEAN
+RequireDsdtXDsdtExclusion (
+ IN EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *Fadt
+ )
+{
+ //
+ // Mantis ticket #1393 was addressed in ACPI 5.1 Errata B. Unfortunately, we
+ // can't tell apart 5.1 Errata A and 5.1 Errata B just from looking at the
+ // FADT table. Therefore let's require exclusion for table versions >= 5.1.
+ //
+ // While this needlessly covers 5.1 and 5.1A too, it is safer to require
+ // DSDT<->X_DSDT exclusion for lax (5.1, 5.1A) versions of the spec than to
+ // permit DSDT<->X_DSDT duplication for strict (5.1B) versions of the spec.
+ //
+ // The same applies to 6.0 vs. 6.0A. While 6.0 does not require the
+ // exclusion, 6.0A and 6.1 do. Since we cannot distinguish 6.0 from 6.0A
+ // based on just the FADT, we lump 6.0 in with the rest of >= 5.1.
+ //
+ if ((Fadt->Header.Revision < 5) ||
+ ((Fadt->Header.Revision == 5) &&
+ (((EFI_ACPI_5_1_FIXED_ACPI_DESCRIPTION_TABLE *)Fadt)->MinorVersion == 0))) {
+ //
+ // version <= 5.0
+ //
+ return FALSE;
+ }
+ //
+ // version >= 5.1
+ //
+ return TRUE;
+}
+
/**
This function adds an ACPI table to the table list. It will detect FACS and
allocate the correct type of memory and properly align the table.
@@ -647,12 +692,16 @@ AddTableToList (
}
if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN) AcpiTableInstance->Dsdt3;
- ZeroMem (&AcpiTableInstance->Fadt3->XDsdt, sizeof (UINT64));
+ if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) {
+ Buffer64 = 0;
+ } else {
+ Buffer64 = AcpiTableInstance->Fadt3->Dsdt;
+ }
} else {
AcpiTableInstance->Fadt3->Dsdt = 0;
Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
- CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof (UINT64));
}
+ CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof (UINT64));
//
// RSDP OEM information is updated to match the FADT OEM information
@@ -847,8 +896,15 @@ AddTableToList (
if (AcpiTableInstance->Fadt3 != NULL) {
if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN) AcpiTableInstance->Dsdt3;
+ if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) {
+ Buffer64 = 0;
+ } else {
+ Buffer64 = AcpiTableInstance->Fadt3->Dsdt;
+ }
+ } else {
+ AcpiTableInstance->Fadt3->Dsdt = 0;
+ Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
}
- Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof (UINT64));
//
--
2.9.3
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
2017-03-08 19:58 ` [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion Laszlo Ersek
@ 2017-03-09 0:47 ` Yao, Jiewen
2017-03-13 3:07 ` Fan, Jeff
1 sibling, 0 replies; 14+ messages in thread
From: Yao, Jiewen @ 2017-03-09 0:47 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Al Stone, Ard Biesheuvel, Tian, Feng, Igor Mammedov, Leo Duran,
Michael Tsirkin, Phil Dennis-Jordan, Zeng, Star
Reviewed-by: jiewen.yao@Intel.com
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, March 9, 2017 3:59 AM
> To: edk2-devel-01 <edk2-devel@ml01.01.org>
> Cc: Al Stone <ahs3@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> Tian, Feng <feng.tian@intel.com>; Igor Mammedov <imammedo@redhat.com>;
> Yao, Jiewen <jiewen.yao@intel.com>; Leo Duran <leo.duran@amd.com>;
> Michael Tsirkin <mtsirkin@redhat.com>; Phil Dennis-Jordan
> <phil@philjordan.eu>; Zeng, Star <star.zeng@intel.com>
> Subject: [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve
> FADT.{DSDT,X_DSDT} mutual exclusion
>
> The ACPI specification, up to and including revision 5.1 Errata A, allows
> the DSDT and X_DSDT fields to be both set in the FADT. (Obviously, this
> only makes sense if the DSDT address is representable in 4 bytes.)
>
> Starting with 5.1 Errata B, specifically for Mantis 1393
> <https://mantis.uefi.org/mantis/view.php?id=1393>, the spec requires at
> most one of DSDT and X_DSDT to be set to a nonzero value.
>
> MdeModulePkg/AcpiTableDxe handles this mutual exclusion somewhat
> inconsistently.
>
> - If the caller of EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() installs the
> tables in "DSDT, FADT" order, then we enforce the exclusion between the
> DSDT and X_DSDT fields:
>
> DSDT under 4GB FADT.DSDT FADT.X_DSDT [VARIANT B]
> -------------- --------- -----------
> yes set clear
> no clear set
>
> This behavior conforms to 5.1 Errata B. (And it's not required by
> earlier versions of the spec.)
>
> - If the caller passes in the tables in "FADT, DSDT" relative order, then
> we do not enforce the exclusion:
>
> DSDT under 4GB FADT.DSDT FADT.X_DSDT [VARIANT A]
> -------------- --------- -----------
> yes set set
> no clear set
>
> This satisfies 5.1 Errata A and earlier, but breaks 5.1 Errata B and
> later.
>
> Unify the handling of both relative orders. In particular, check the major
> and minor version numbers in the FADT. If the FADT version is strictly
> before 5.1, then implement [VARIANT A]. If the FADT version is equal to or
> larger than 5.1, then implement [VARIANT B].
>
> We make three observations:
>
> - We can't check the FADT table version precisely against "5.1 Errata B";
> erratum levels are not captured in the table. We err in the safe
> direction, namely we enforce the exclusion for "5.1" and "5.1 Errata A".
>
> - The same applies to "6.0" versus "6.0 Errata A". Because we cannot
> distinguish these two, we consider "6.0" to be "equal to or larger than
> 5.1", and apply [VARIANT B], enforcing the exclusion.
>
> - While a blanket [VARIANT B] would be simpler, there is a significant
> benefit to [VARIANT A], under the spec versions that permit it:
> compatibility with a wider range of OSPMs (typically, older ones).
>
> For example, Igor reported about a "DELL R430 system with rev4 FADT
> where DSDT and X_DSDT are pointing to the same address". Michael also
> reported about several systems that exhibit the same.
>
> Regression tested with the following KVM guests (QEMU built at
> ata0def594286d, "Merge remote-tracking branch
> 'remotes/bonzini/tags/for-upstream' into staging", 2017-01-30):
>
> - OVMF: boot and S3 suspend/resume
> - Ia32, Q35, SMM
> - Fedlet 20141209
> - Ia32X64, Q35, SMM
> - Fedora 22
> - Windows 7
> - Windows 8.1
> - Windows 10
> - Windows Server 2008 R2
> - Windows Server 2012 R2
> - Windows Server 2016 Tech Preview 4
> - X64, I440FX, no SMM
> - Fedora 24
> - RHEL-6.7
> - RHEL-7.2-ish
> - ArmVirtQemu: boot test with virtio-gpu
> - AARCH64
> - Fedora 24
> - RHELSA-7.3
> - openSUSE Tumbleweed (4.8.4-based)
>
> This change is connected to ASWG ticket
> <https://mantis.uefi.org/mantis/view.php?id=1757>, which is now
> closed/fixed.
>
> Cc: Al Stone <ahs3@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Leo Duran <leo.duran@amd.com>
> Cc: Michael Tsirkin <mtsirkin@redhat.com>
> Cc: Phil Dennis-Jordan <phil@philjordan.eu>
> Cc: Star Zeng <star.zeng@intel.com>
> Reported-by: Phil Dennis-Jordan <phil@philjordan.eu>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
>
> Notes:
> v2:
> - simplify logic in RequireDsdtXDsdtExclusion() [Jiewen]
> - pick up Phil's R-b nonetheless (the above change is a minimal
> reformulation of code, with no behavioral difference)
> - add reference to Mantis#1757 to the commit message
>
> v1:
> NOTE for people on the CC list:
>
> If you are not presently subscribed to edk2-devel and wish to comment on
> this patch publicly, you need to subscribe first, and wait for the
> subscription request to *complete* (see your inbox), *before* sending
> your followup. This is not ideal, but edk2-devel requires subscription
> before reflecting messages from someone.
>
> Subscribe at <https://lists.01.org/mailman/listinfo/edk2-devel>. Thanks.
>
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 62
> +++++++++++++++++++-
> 1 file changed, 59 insertions(+), 3 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> index 7795ff7269ca..4bb848df5203 100644
> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> @@ -430,6 +430,51 @@ ReallocateAcpiTableBuffer (
> mEfiAcpiMaxNumTables = NewMaxTableNumber;
> return EFI_SUCCESS;
> }
> +
> +/**
> + Determine whether the FADT table passed in as parameter requires mutual
> + exclusion between the DSDT and X_DSDT fields. (That is, whether there
> exists
> + an explicit requirement that at most one of those fields is permitted to be
> + nonzero.)
> +
> + @param[in] Fadt The EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE
> object to
> + check.
> +
> + @retval TRUE Fadt requires mutual exclusion between DSDT and
> X_DSDT.
> + @retval FALSE Otherwise.
> +**/
> +BOOLEAN
> +RequireDsdtXDsdtExclusion (
> + IN EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *Fadt
> + )
> +{
> + //
> + // Mantis ticket #1393 was addressed in ACPI 5.1 Errata B. Unfortunately, we
> + // can't tell apart 5.1 Errata A and 5.1 Errata B just from looking at the
> + // FADT table. Therefore let's require exclusion for table versions >= 5.1.
> + //
> + // While this needlessly covers 5.1 and 5.1A too, it is safer to require
> + // DSDT<->X_DSDT exclusion for lax (5.1, 5.1A) versions of the spec than to
> + // permit DSDT<->X_DSDT duplication for strict (5.1B) versions of the spec.
> + //
> + // The same applies to 6.0 vs. 6.0A. While 6.0 does not require the
> + // exclusion, 6.0A and 6.1 do. Since we cannot distinguish 6.0 from 6.0A
> + // based on just the FADT, we lump 6.0 in with the rest of >= 5.1.
> + //
> + if ((Fadt->Header.Revision < 5) ||
> + ((Fadt->Header.Revision == 5) &&
> + (((EFI_ACPI_5_1_FIXED_ACPI_DESCRIPTION_TABLE
> *)Fadt)->MinorVersion == 0))) {
> + //
> + // version <= 5.0
> + //
> + return FALSE;
> + }
> + //
> + // version >= 5.1
> + //
> + return TRUE;
> +}
> +
> /**
> This function adds an ACPI table to the table list. It will detect FACS and
> allocate the correct type of memory and properly align the table.
> @@ -647,12 +692,16 @@ AddTableToList (
> }
> if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
> AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN)
> AcpiTableInstance->Dsdt3;
> - ZeroMem (&AcpiTableInstance->Fadt3->XDsdt, sizeof (UINT64));
> + if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) {
> + Buffer64 = 0;
> + } else {
> + Buffer64 = AcpiTableInstance->Fadt3->Dsdt;
> + }
> } else {
> AcpiTableInstance->Fadt3->Dsdt = 0;
> Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
> - CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof
> (UINT64));
> }
> + CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof
> (UINT64));
>
> //
> // RSDP OEM information is updated to match the FADT OEM information
> @@ -847,8 +896,15 @@ AddTableToList (
> if (AcpiTableInstance->Fadt3 != NULL) {
> if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
> AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN)
> AcpiTableInstance->Dsdt3;
> + if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) {
> + Buffer64 = 0;
> + } else {
> + Buffer64 = AcpiTableInstance->Fadt3->Dsdt;
> + }
> + } else {
> + AcpiTableInstance->Fadt3->Dsdt = 0;
> + Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
> }
> - Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
> CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof
> (UINT64));
>
> //
> --
> 2.9.3
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/2] MdeModulePkg/AcpiTableDxe: condense whitespace around FADT.{DSDT, X_DSDT}
2017-03-08 19:58 ` [PATCH v2 1/2] MdeModulePkg/AcpiTableDxe: condense whitespace around FADT.{DSDT, X_DSDT} Laszlo Ersek
@ 2017-03-09 0:47 ` Yao, Jiewen
0 siblings, 0 replies; 14+ messages in thread
From: Yao, Jiewen @ 2017-03-09 0:47 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Tian, Feng, Michael Tsirkin, Ard Biesheuvel, Phil Dennis-Jordan,
Leo Duran, Al Stone, Zeng, Star
Reviewed-by: jiewen.yao@Intel.com
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: Thursday, March 9, 2017 3:59 AM
> To: edk2-devel-01 <edk2-devel@ml01.01.org>
> Cc: Tian, Feng <feng.tian@intel.com>; Michael Tsirkin <mtsirkin@redhat.com>;
> Ard Biesheuvel <ard.biesheuvel@linaro.org>; Phil Dennis-Jordan
> <phil@philjordan.eu>; Leo Duran <leo.duran@amd.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Al Stone <ahs3@redhat.com>; Zeng, Star
> <star.zeng@intel.com>
> Subject: [edk2] [PATCH v2 1/2] MdeModulePkg/AcpiTableDxe: condense
> whitespace around FADT.{DSDT, X_DSDT}
>
> This patch incurs no functional changes, it just removes some whitespace,
> and also makes sure we always assign
>
> AcpiTableInstance->Fadt3->Dsdt
>
> first, and
>
> AcpiTableInstance->Fadt3->XDsdt
>
> second. The goal is to separate the syntactic changes from the functional
> changes implemented by the next patch.
>
> Cc: Al Stone <ahs3@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Leo Duran <leo.duran@amd.com>
> Cc: Michael Tsirkin <mtsirkin@redhat.com>
> Cc: Phil Dennis-Jordan <phil@philjordan.eu>
> Cc: Star Zeng <star.zeng@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> Reviewed-by: Feng Tian <feng.tian@intel.com>
> ---
>
> Notes:
> v2:
> - no changes
> - pick up R-b's from Thomas and Feng
>
> v1:
> NOTE for people on the CC list:
>
> If you are not presently subscribed to edk2-devel and wish to comment on
> this patch publicly, you need to subscribe first, and wait for the
> subscription request to *complete* (see your inbox), *before* sending
> your followup. This is not ideal, but edk2-devel requires subscription
> before reflecting messages from someone.
>
> Subscribe at <https://lists.01.org/mailman/listinfo/edk2-devel>. Thanks.
>
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 20
> ++++++--------------
> 1 file changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> index 7f95b9dc709d..7795ff7269ca 100644
> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> @@ -646,16 +646,12 @@ AddTableToList (
> AcpiTableInstance->Fadt3->FirmwareCtrl = 0;
> }
> if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
> - AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN)
> AcpiTableInstance->Dsdt3;
> + AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN)
> AcpiTableInstance->Dsdt3;
> ZeroMem (&AcpiTableInstance->Fadt3->XDsdt, sizeof (UINT64));
> } else {
> - Buffer64 = (UINT64) (UINTN)
> AcpiTableInstance->Dsdt3;
> - CopyMem (
> - &AcpiTableInstance->Fadt3->XDsdt,
> - &Buffer64,
> - sizeof (UINT64)
> - );
> AcpiTableInstance->Fadt3->Dsdt = 0;
> + Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
> + CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof
> (UINT64));
> }
>
> //
> @@ -850,14 +846,10 @@ AddTableToList (
> //
> if (AcpiTableInstance->Fadt3 != NULL) {
> if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
> - AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN)
> AcpiTableInstance->Dsdt3;
> + AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN)
> AcpiTableInstance->Dsdt3;
> }
> - Buffer64 = (UINT64) (UINTN)
> AcpiTableInstance->Dsdt3;
> - CopyMem (
> - &AcpiTableInstance->Fadt3->XDsdt,
> - &Buffer64,
> - sizeof (UINT64)
> - );
> + Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
> + CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof
> (UINT64));
>
> //
> // Checksum FADT table
> --
> 2.9.3
>
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 0/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
2017-03-08 19:58 [PATCH v2 0/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion Laszlo Ersek
2017-03-08 19:58 ` [PATCH v2 1/2] MdeModulePkg/AcpiTableDxe: condense whitespace around FADT.{DSDT, X_DSDT} Laszlo Ersek
2017-03-08 19:58 ` [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion Laszlo Ersek
@ 2017-03-09 1:59 ` Zeng, Star
2017-03-09 14:06 ` Laszlo Ersek
3 siblings, 0 replies; 14+ messages in thread
From: Zeng, Star @ 2017-03-09 1:59 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Al Stone, Ard Biesheuvel, Tian, Feng, Igor Mammedov, Yao, Jiewen,
Leo Duran, Michael Tsirkin, Phil Dennis-Jordan, Zeng, Star
Reviewed-by: Star Zeng <star.zeng@intel.com>
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Thursday, March 9, 2017 3:59 AM
To: edk2-devel-01 <edk2-devel@ml01.01.org>
Cc: Al Stone <ahs3@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Tian, Feng <feng.tian@intel.com>; Igor Mammedov <imammedo@redhat.com>; Yao, Jiewen <jiewen.yao@intel.com>; Leo Duran <leo.duran@amd.com>; Michael Tsirkin <mtsirkin@redhat.com>; Phil Dennis-Jordan <phil@philjordan.eu>; Zeng, Star <star.zeng@intel.com>
Subject: [PATCH v2 0/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT,X_DSDT} mutual exclusion
This is version 2 of the series originally posted at <https://lists.01.org/pipermail/edk2-devel/2017-February/007004.html>
and further discussed at
<https://lists.01.org/pipermail/edk2-devel/2017-March/008297.html>.
This version contains minor updates (noted on each patch) that don't affect functionality.
Repo: https://github.com/lersek/edk2.git
Branch: fadt_dsdt_v2
Cc: Al Stone <ahs3@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Feng Tian <feng.tian@intel.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Leo Duran <leo.duran@amd.com>
Cc: Michael Tsirkin <mtsirkin@redhat.com>
Cc: Phil Dennis-Jordan <phil@philjordan.eu>
Cc: Star Zeng <star.zeng@intel.com>
Thanks
Laszlo
Laszlo Ersek (2):
MdeModulePkg/AcpiTableDxe: condense whitespace around
FADT.{DSDT,X_DSDT}
MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT,X_DSDT} mutual exclusion
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 78 ++++++++++++++++----
1 file changed, 63 insertions(+), 15 deletions(-)
--
2.9.3
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 0/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
2017-03-08 19:58 [PATCH v2 0/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion Laszlo Ersek
` (2 preceding siblings ...)
2017-03-09 1:59 ` [PATCH v2 0/2] " Zeng, Star
@ 2017-03-09 14:06 ` Laszlo Ersek
3 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-03-09 14:06 UTC (permalink / raw)
To: edk2-devel-01
Cc: Feng Tian, Michael Tsirkin, Ard Biesheuvel, Phil Dennis-Jordan,
Leo Duran, Jiewen Yao, Al Stone, Star Zeng
On 03/08/17 20:58, Laszlo Ersek wrote:
> This is version 2 of the series originally posted at
> <https://lists.01.org/pipermail/edk2-devel/2017-February/007004.html>
> and further discussed at
> <https://lists.01.org/pipermail/edk2-devel/2017-March/008297.html>.
>
> This version contains minor updates (noted on each patch) that don't
> affect functionality.
>
> Repo: https://github.com/lersek/edk2.git
> Branch: fadt_dsdt_v2
>
> Cc: Al Stone <ahs3@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Leo Duran <leo.duran@amd.com>
> Cc: Michael Tsirkin <mtsirkin@redhat.com>
> Cc: Phil Dennis-Jordan <phil@philjordan.eu>
> Cc: Star Zeng <star.zeng@intel.com>
>
> Thanks
> Laszlo
>
> Laszlo Ersek (2):
> MdeModulePkg/AcpiTableDxe: condense whitespace around
> FADT.{DSDT,X_DSDT}
> MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT,X_DSDT} mutual exclusion
>
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 78 ++++++++++++++++----
> 1 file changed, 63 insertions(+), 15 deletions(-)
>
Thanks All, this is now pushed (commit range e0e1cfcbbb24..198a46d768fb).
Laszlo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
2017-03-08 19:58 ` [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion Laszlo Ersek
2017-03-09 0:47 ` Yao, Jiewen
@ 2017-03-13 3:07 ` Fan, Jeff
2017-03-13 14:44 ` Laszlo Ersek
1 sibling, 1 reply; 14+ messages in thread
From: Fan, Jeff @ 2017-03-13 3:07 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Tian, Feng, Michael Tsirkin, Ard Biesheuvel, Phil Dennis-Jordan,
Leo Duran, Yao, Jiewen, Al Stone, Zeng, Star
Laszlo,
We found one Windows Server 2012 R2 blue screen issue with ACPI 6.1 FADT table.
We did the following configuration test with DSDT under 4GB.
.DSDT .X_DSDT Window Server 2012 R2
---------- ------------ -------------------------------
set clear Failed // current implementation
clear set Succeed
set set Succeed
Thanks!
Jeff
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Thursday, March 09, 2017 3:59 AM
To: edk2-devel-01
Cc: Tian, Feng; Michael Tsirkin; Ard Biesheuvel; Phil Dennis-Jordan; Leo Duran; Yao, Jiewen; Al Stone; Zeng, Star
Subject: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
The ACPI specification, up to and including revision 5.1 Errata A, allows the DSDT and X_DSDT fields to be both set in the FADT. (Obviously, this only makes sense if the DSDT address is representable in 4 bytes.)
Starting with 5.1 Errata B, specifically for Mantis 1393 <https://mantis.uefi.org/mantis/view.php?id=1393>, the spec requires at most one of DSDT and X_DSDT to be set to a nonzero value.
MdeModulePkg/AcpiTableDxe handles this mutual exclusion somewhat inconsistently.
- If the caller of EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() installs the
tables in "DSDT, FADT" order, then we enforce the exclusion between the
DSDT and X_DSDT fields:
DSDT under 4GB FADT.DSDT FADT.X_DSDT [VARIANT B]
-------------- --------- -----------
yes set clear
no clear set
This behavior conforms to 5.1 Errata B. (And it's not required by
earlier versions of the spec.)
- If the caller passes in the tables in "FADT, DSDT" relative order, then
we do not enforce the exclusion:
DSDT under 4GB FADT.DSDT FADT.X_DSDT [VARIANT A]
-------------- --------- -----------
yes set set
no clear set
This satisfies 5.1 Errata A and earlier, but breaks 5.1 Errata B and
later.
Unify the handling of both relative orders. In particular, check the major and minor version numbers in the FADT. If the FADT version is strictly before 5.1, then implement [VARIANT A]. If the FADT version is equal to or larger than 5.1, then implement [VARIANT B].
We make three observations:
- We can't check the FADT table version precisely against "5.1 Errata B";
erratum levels are not captured in the table. We err in the safe
direction, namely we enforce the exclusion for "5.1" and "5.1 Errata A".
- The same applies to "6.0" versus "6.0 Errata A". Because we cannot
distinguish these two, we consider "6.0" to be "equal to or larger than
5.1", and apply [VARIANT B], enforcing the exclusion.
- While a blanket [VARIANT B] would be simpler, there is a significant
benefit to [VARIANT A], under the spec versions that permit it:
compatibility with a wider range of OSPMs (typically, older ones).
For example, Igor reported about a "DELL R430 system with rev4 FADT
where DSDT and X_DSDT are pointing to the same address". Michael also
reported about several systems that exhibit the same.
Regression tested with the following KVM guests (QEMU built at ata0def594286d, "Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging", 2017-01-30):
- OVMF: boot and S3 suspend/resume
- Ia32, Q35, SMM
- Fedlet 20141209
- Ia32X64, Q35, SMM
- Fedora 22
- Windows 7
- Windows 8.1
- Windows 10
- Windows Server 2008 R2
- Windows Server 2012 R2
- Windows Server 2016 Tech Preview 4
- X64, I440FX, no SMM
- Fedora 24
- RHEL-6.7
- RHEL-7.2-ish
- ArmVirtQemu: boot test with virtio-gpu
- AARCH64
- Fedora 24
- RHELSA-7.3
- openSUSE Tumbleweed (4.8.4-based)
This change is connected to ASWG ticket
<https://mantis.uefi.org/mantis/view.php?id=1757>, which is now closed/fixed.
Cc: Al Stone <ahs3@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Feng Tian <feng.tian@intel.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Leo Duran <leo.duran@amd.com>
Cc: Michael Tsirkin <mtsirkin@redhat.com>
Cc: Phil Dennis-Jordan <phil@philjordan.eu>
Cc: Star Zeng <star.zeng@intel.com>
Reported-by: Phil Dennis-Jordan <phil@philjordan.eu>
Suggested-by: Igor Mammedov <imammedo@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
---
Notes:
v2:
- simplify logic in RequireDsdtXDsdtExclusion() [Jiewen]
- pick up Phil's R-b nonetheless (the above change is a minimal
reformulation of code, with no behavioral difference)
- add reference to Mantis#1757 to the commit message
v1:
NOTE for people on the CC list:
If you are not presently subscribed to edk2-devel and wish to comment on
this patch publicly, you need to subscribe first, and wait for the
subscription request to *complete* (see your inbox), *before* sending
your followup. This is not ideal, but edk2-devel requires subscription
before reflecting messages from someone.
Subscribe at <https://lists.01.org/mailman/listinfo/edk2-devel>. Thanks.
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 62 +++++++++++++++++++-
1 file changed, 59 insertions(+), 3 deletions(-)
diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
index 7795ff7269ca..4bb848df5203 100644
--- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
+++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
@@ -430,6 +430,51 @@ ReallocateAcpiTableBuffer (
mEfiAcpiMaxNumTables = NewMaxTableNumber;
return EFI_SUCCESS;
}
+
+/**
+ Determine whether the FADT table passed in as parameter requires
+mutual
+ exclusion between the DSDT and X_DSDT fields. (That is, whether there
+exists
+ an explicit requirement that at most one of those fields is permitted
+to be
+ nonzero.)
+
+ @param[in] Fadt The EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE object to
+ check.
+
+ @retval TRUE Fadt requires mutual exclusion between DSDT and X_DSDT.
+ @retval FALSE Otherwise.
+**/
+BOOLEAN
+RequireDsdtXDsdtExclusion (
+ IN EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *Fadt
+ )
+{
+ //
+ // Mantis ticket #1393 was addressed in ACPI 5.1 Errata B.
+Unfortunately, we
+ // can't tell apart 5.1 Errata A and 5.1 Errata B just from looking
+at the
+ // FADT table. Therefore let's require exclusion for table versions >= 5.1.
+ //
+ // While this needlessly covers 5.1 and 5.1A too, it is safer to
+require
+ // DSDT<->X_DSDT exclusion for lax (5.1, 5.1A) versions of the spec
+than to
+ // permit DSDT<->X_DSDT duplication for strict (5.1B) versions of the spec.
+ //
+ // The same applies to 6.0 vs. 6.0A. While 6.0 does not require the
+ // exclusion, 6.0A and 6.1 do. Since we cannot distinguish 6.0 from
+6.0A
+ // based on just the FADT, we lump 6.0 in with the rest of >= 5.1.
+ //
+ if ((Fadt->Header.Revision < 5) ||
+ ((Fadt->Header.Revision == 5) &&
+ (((EFI_ACPI_5_1_FIXED_ACPI_DESCRIPTION_TABLE *)Fadt)->MinorVersion == 0))) {
+ //
+ // version <= 5.0
+ //
+ return FALSE;
+ }
+ //
+ // version >= 5.1
+ //
+ return TRUE;
+}
+
/**
This function adds an ACPI table to the table list. It will detect FACS and
allocate the correct type of memory and properly align the table.
@@ -647,12 +692,16 @@ AddTableToList (
}
if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN) AcpiTableInstance->Dsdt3;
- ZeroMem (&AcpiTableInstance->Fadt3->XDsdt, sizeof (UINT64));
+ if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) {
+ Buffer64 = 0;
+ } else {
+ Buffer64 = AcpiTableInstance->Fadt3->Dsdt;
+ }
} else {
AcpiTableInstance->Fadt3->Dsdt = 0;
Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
- CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof (UINT64));
}
+ CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof
+ (UINT64));
//
// RSDP OEM information is updated to match the FADT OEM information @@ -847,8 +896,15 @@ AddTableToList (
if (AcpiTableInstance->Fadt3 != NULL) {
if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN) AcpiTableInstance->Dsdt3;
+ if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) {
+ Buffer64 = 0;
+ } else {
+ Buffer64 = AcpiTableInstance->Fadt3->Dsdt;
+ }
+ } else {
+ AcpiTableInstance->Fadt3->Dsdt = 0;
+ Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
}
- Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof (UINT64));
//
--
2.9.3
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
2017-03-13 3:07 ` Fan, Jeff
@ 2017-03-13 14:44 ` Laszlo Ersek
2017-03-14 7:56 ` Fan, Jeff
0 siblings, 1 reply; 14+ messages in thread
From: Laszlo Ersek @ 2017-03-13 14:44 UTC (permalink / raw)
To: Fan, Jeff, edk2-devel-01
Cc: Tian, Feng, Michael Tsirkin, Ard Biesheuvel, Phil Dennis-Jordan,
Leo Duran, Yao, Jiewen, Al Stone, Zeng, Star
On 03/13/17 04:07, Fan, Jeff wrote:
> Laszlo,
>
> We found one Windows Server 2012 R2 blue screen issue with ACPI 6.1 FADT table.
>
> We did the following configuration test with DSDT under 4GB.
> .DSDT .X_DSDT Window Server 2012 R2
> ---------- ------------ -------------------------------
> set clear Failed // current implementation
> clear set Succeed
> set set Succeed
That looks like a Windows bug. The above configuration satisfies ACPI 6.1:
DSDT -- Physical memory address of the DSDT. If the X_DSDT field
contains a non-zero value then this field must be zero.
X_DSDT -- Extended physical address of the DSDT. If the DSDT field
contains a non-zero value then this field must be zero.
Michael told me that "6.1 errata will specify X_DSDT takes preference
over DSDT but both can be present legaly", however, here X_DSDT cannot
take precedence because it is zero.
Based on past experience, I don't expect that Microsoft will ever fix
this ACPI bug in Windows Server 2012 R2. I don't even expect that they
would share with us a list of ACPI spec versions that should be exempted
from RequireDsdtXDsdtExclusion() -- despite the spec clearly requiring
DSDT / X_DSDT exclusion --, for bug compatibility.
That leaves us with trial and error, to see what works and what doesn't.
Unfortunately, I don't have ACPI tables for several ACPI spec versions;
I don't think I can experiment with this. If you find a workaround, that
would be great, but if we can't, I guess the patch should be reverted.
(Note however that the BSOD will remain possible to trigger, with the
DSDT, FADT installation order.)
Thanks
Laszlo
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
> Sent: Thursday, March 09, 2017 3:59 AM
> To: edk2-devel-01
> Cc: Tian, Feng; Michael Tsirkin; Ard Biesheuvel; Phil Dennis-Jordan; Leo Duran; Yao, Jiewen; Al Stone; Zeng, Star
> Subject: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
>
> The ACPI specification, up to and including revision 5.1 Errata A, allows the DSDT and X_DSDT fields to be both set in the FADT. (Obviously, this only makes sense if the DSDT address is representable in 4 bytes.)
>
> Starting with 5.1 Errata B, specifically for Mantis 1393 <https://mantis.uefi.org/mantis/view.php?id=1393>, the spec requires at most one of DSDT and X_DSDT to be set to a nonzero value.
>
> MdeModulePkg/AcpiTableDxe handles this mutual exclusion somewhat inconsistently.
>
> - If the caller of EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() installs the
> tables in "DSDT, FADT" order, then we enforce the exclusion between the
> DSDT and X_DSDT fields:
>
> DSDT under 4GB FADT.DSDT FADT.X_DSDT [VARIANT B]
> -------------- --------- -----------
> yes set clear
> no clear set
>
> This behavior conforms to 5.1 Errata B. (And it's not required by
> earlier versions of the spec.)
>
> - If the caller passes in the tables in "FADT, DSDT" relative order, then
> we do not enforce the exclusion:
>
> DSDT under 4GB FADT.DSDT FADT.X_DSDT [VARIANT A]
> -------------- --------- -----------
> yes set set
> no clear set
>
> This satisfies 5.1 Errata A and earlier, but breaks 5.1 Errata B and
> later.
>
> Unify the handling of both relative orders. In particular, check the major and minor version numbers in the FADT. If the FADT version is strictly before 5.1, then implement [VARIANT A]. If the FADT version is equal to or larger than 5.1, then implement [VARIANT B].
>
> We make three observations:
>
> - We can't check the FADT table version precisely against "5.1 Errata B";
> erratum levels are not captured in the table. We err in the safe
> direction, namely we enforce the exclusion for "5.1" and "5.1 Errata A".
>
> - The same applies to "6.0" versus "6.0 Errata A". Because we cannot
> distinguish these two, we consider "6.0" to be "equal to or larger than
> 5.1", and apply [VARIANT B], enforcing the exclusion.
>
> - While a blanket [VARIANT B] would be simpler, there is a significant
> benefit to [VARIANT A], under the spec versions that permit it:
> compatibility with a wider range of OSPMs (typically, older ones).
>
> For example, Igor reported about a "DELL R430 system with rev4 FADT
> where DSDT and X_DSDT are pointing to the same address". Michael also
> reported about several systems that exhibit the same.
>
> Regression tested with the following KVM guests (QEMU built at ata0def594286d, "Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging", 2017-01-30):
>
> - OVMF: boot and S3 suspend/resume
> - Ia32, Q35, SMM
> - Fedlet 20141209
> - Ia32X64, Q35, SMM
> - Fedora 22
> - Windows 7
> - Windows 8.1
> - Windows 10
> - Windows Server 2008 R2
> - Windows Server 2012 R2
> - Windows Server 2016 Tech Preview 4
> - X64, I440FX, no SMM
> - Fedora 24
> - RHEL-6.7
> - RHEL-7.2-ish
> - ArmVirtQemu: boot test with virtio-gpu
> - AARCH64
> - Fedora 24
> - RHELSA-7.3
> - openSUSE Tumbleweed (4.8.4-based)
>
> This change is connected to ASWG ticket
> <https://mantis.uefi.org/mantis/view.php?id=1757>, which is now closed/fixed.
>
> Cc: Al Stone <ahs3@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Leo Duran <leo.duran@amd.com>
> Cc: Michael Tsirkin <mtsirkin@redhat.com>
> Cc: Phil Dennis-Jordan <phil@philjordan.eu>
> Cc: Star Zeng <star.zeng@intel.com>
> Reported-by: Phil Dennis-Jordan <phil@philjordan.eu>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
>
> Notes:
> v2:
> - simplify logic in RequireDsdtXDsdtExclusion() [Jiewen]
> - pick up Phil's R-b nonetheless (the above change is a minimal
> reformulation of code, with no behavioral difference)
> - add reference to Mantis#1757 to the commit message
>
> v1:
> NOTE for people on the CC list:
>
> If you are not presently subscribed to edk2-devel and wish to comment on
> this patch publicly, you need to subscribe first, and wait for the
> subscription request to *complete* (see your inbox), *before* sending
> your followup. This is not ideal, but edk2-devel requires subscription
> before reflecting messages from someone.
>
> Subscribe at <https://lists.01.org/mailman/listinfo/edk2-devel>. Thanks.
>
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 62 +++++++++++++++++++-
> 1 file changed, 59 insertions(+), 3 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> index 7795ff7269ca..4bb848df5203 100644
> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> @@ -430,6 +430,51 @@ ReallocateAcpiTableBuffer (
> mEfiAcpiMaxNumTables = NewMaxTableNumber;
> return EFI_SUCCESS;
> }
> +
> +/**
> + Determine whether the FADT table passed in as parameter requires
> +mutual
> + exclusion between the DSDT and X_DSDT fields. (That is, whether there
> +exists
> + an explicit requirement that at most one of those fields is permitted
> +to be
> + nonzero.)
> +
> + @param[in] Fadt The EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE object to
> + check.
> +
> + @retval TRUE Fadt requires mutual exclusion between DSDT and X_DSDT.
> + @retval FALSE Otherwise.
> +**/
> +BOOLEAN
> +RequireDsdtXDsdtExclusion (
> + IN EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *Fadt
> + )
> +{
> + //
> + // Mantis ticket #1393 was addressed in ACPI 5.1 Errata B.
> +Unfortunately, we
> + // can't tell apart 5.1 Errata A and 5.1 Errata B just from looking
> +at the
> + // FADT table. Therefore let's require exclusion for table versions >= 5.1.
> + //
> + // While this needlessly covers 5.1 and 5.1A too, it is safer to
> +require
> + // DSDT<->X_DSDT exclusion for lax (5.1, 5.1A) versions of the spec
> +than to
> + // permit DSDT<->X_DSDT duplication for strict (5.1B) versions of the spec.
> + //
> + // The same applies to 6.0 vs. 6.0A. While 6.0 does not require the
> + // exclusion, 6.0A and 6.1 do. Since we cannot distinguish 6.0 from
> +6.0A
> + // based on just the FADT, we lump 6.0 in with the rest of >= 5.1.
> + //
> + if ((Fadt->Header.Revision < 5) ||
> + ((Fadt->Header.Revision == 5) &&
> + (((EFI_ACPI_5_1_FIXED_ACPI_DESCRIPTION_TABLE *)Fadt)->MinorVersion == 0))) {
> + //
> + // version <= 5.0
> + //
> + return FALSE;
> + }
> + //
> + // version >= 5.1
> + //
> + return TRUE;
> +}
> +
> /**
> This function adds an ACPI table to the table list. It will detect FACS and
> allocate the correct type of memory and properly align the table.
> @@ -647,12 +692,16 @@ AddTableToList (
> }
> if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
> AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN) AcpiTableInstance->Dsdt3;
> - ZeroMem (&AcpiTableInstance->Fadt3->XDsdt, sizeof (UINT64));
> + if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) {
> + Buffer64 = 0;
> + } else {
> + Buffer64 = AcpiTableInstance->Fadt3->Dsdt;
> + }
> } else {
> AcpiTableInstance->Fadt3->Dsdt = 0;
> Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
> - CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof (UINT64));
> }
> + CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof
> + (UINT64));
>
> //
> // RSDP OEM information is updated to match the FADT OEM information @@ -847,8 +896,15 @@ AddTableToList (
> if (AcpiTableInstance->Fadt3 != NULL) {
> if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
> AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN) AcpiTableInstance->Dsdt3;
> + if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) {
> + Buffer64 = 0;
> + } else {
> + Buffer64 = AcpiTableInstance->Fadt3->Dsdt;
> + }
> + } else {
> + AcpiTableInstance->Fadt3->Dsdt = 0;
> + Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
> }
> - Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
> CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof (UINT64));
>
> //
> --
> 2.9.3
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
2017-03-13 14:44 ` Laszlo Ersek
@ 2017-03-14 7:56 ` Fan, Jeff
2017-03-14 8:33 ` Zeng, Star
0 siblings, 1 reply; 14+ messages in thread
From: Fan, Jeff @ 2017-03-14 7:56 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
Cc: Tian, Feng, Michael Tsirkin, Ard Biesheuvel, Phil Dennis-Jordan,
Leo Duran, Yao, Jiewen, Al Stone, Zeng, Star
Laszlo,
Basically, I agree with this is OS assumption. I did not find better fix to handle such compatibility issue.
I agree to revert this patch 2/2 to fix Windows 2012 R2 boot issue.
I don't know if the other guys have other suggestions. :-)
Thanks!
Jeff
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Monday, March 13, 2017 10:44 PM
To: Fan, Jeff; edk2-devel-01
Cc: Tian, Feng; Michael Tsirkin; Ard Biesheuvel; Phil Dennis-Jordan; Leo Duran; Yao, Jiewen; Al Stone; Zeng, Star
Subject: Re: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
On 03/13/17 04:07, Fan, Jeff wrote:
> Laszlo,
>
> We found one Windows Server 2012 R2 blue screen issue with ACPI 6.1 FADT table.
>
> We did the following configuration test with DSDT under 4GB.
> .DSDT .X_DSDT Window Server 2012 R2
> ---------- ------------ -------------------------------
> set clear Failed // current implementation
> clear set Succeed
> set set Succeed
That looks like a Windows bug. The above configuration satisfies ACPI 6.1:
DSDT -- Physical memory address of the DSDT. If the X_DSDT field contains a non-zero value then this field must be zero.
X_DSDT -- Extended physical address of the DSDT. If the DSDT field contains a non-zero value then this field must be zero.
Michael told me that "6.1 errata will specify X_DSDT takes preference over DSDT but both can be present legaly", however, here X_DSDT cannot take precedence because it is zero.
Based on past experience, I don't expect that Microsoft will ever fix this ACPI bug in Windows Server 2012 R2. I don't even expect that they would share with us a list of ACPI spec versions that should be exempted from RequireDsdtXDsdtExclusion() -- despite the spec clearly requiring DSDT / X_DSDT exclusion --, for bug compatibility.
That leaves us with trial and error, to see what works and what doesn't.
Unfortunately, I don't have ACPI tables for several ACPI spec versions; I don't think I can experiment with this. If you find a workaround, that would be great, but if we can't, I guess the patch should be reverted.
(Note however that the BSOD will remain possible to trigger, with the DSDT, FADT installation order.)
Thanks
Laszlo
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, March 09, 2017 3:59 AM
> To: edk2-devel-01
> Cc: Tian, Feng; Michael Tsirkin; Ard Biesheuvel; Phil Dennis-Jordan;
> Leo Duran; Yao, Jiewen; Al Stone; Zeng, Star
> Subject: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve
> FADT.{DSDT, X_DSDT} mutual exclusion
>
> The ACPI specification, up to and including revision 5.1 Errata A,
> allows the DSDT and X_DSDT fields to be both set in the FADT.
> (Obviously, this only makes sense if the DSDT address is representable
> in 4 bytes.)
>
> Starting with 5.1 Errata B, specifically for Mantis 1393 <https://mantis.uefi.org/mantis/view.php?id=1393>, the spec requires at most one of DSDT and X_DSDT to be set to a nonzero value.
>
> MdeModulePkg/AcpiTableDxe handles this mutual exclusion somewhat inconsistently.
>
> - If the caller of EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() installs the
> tables in "DSDT, FADT" order, then we enforce the exclusion between the
> DSDT and X_DSDT fields:
>
> DSDT under 4GB FADT.DSDT FADT.X_DSDT [VARIANT B]
> -------------- --------- -----------
> yes set clear
> no clear set
>
> This behavior conforms to 5.1 Errata B. (And it's not required by
> earlier versions of the spec.)
>
> - If the caller passes in the tables in "FADT, DSDT" relative order, then
> we do not enforce the exclusion:
>
> DSDT under 4GB FADT.DSDT FADT.X_DSDT [VARIANT A]
> -------------- --------- -----------
> yes set set
> no clear set
>
> This satisfies 5.1 Errata A and earlier, but breaks 5.1 Errata B and
> later.
>
> Unify the handling of both relative orders. In particular, check the major and minor version numbers in the FADT. If the FADT version is strictly before 5.1, then implement [VARIANT A]. If the FADT version is equal to or larger than 5.1, then implement [VARIANT B].
>
> We make three observations:
>
> - We can't check the FADT table version precisely against "5.1 Errata B";
> erratum levels are not captured in the table. We err in the safe
> direction, namely we enforce the exclusion for "5.1" and "5.1 Errata A".
>
> - The same applies to "6.0" versus "6.0 Errata A". Because we cannot
> distinguish these two, we consider "6.0" to be "equal to or larger than
> 5.1", and apply [VARIANT B], enforcing the exclusion.
>
> - While a blanket [VARIANT B] would be simpler, there is a significant
> benefit to [VARIANT A], under the spec versions that permit it:
> compatibility with a wider range of OSPMs (typically, older ones).
>
> For example, Igor reported about a "DELL R430 system with rev4 FADT
> where DSDT and X_DSDT are pointing to the same address". Michael also
> reported about several systems that exhibit the same.
>
> Regression tested with the following KVM guests (QEMU built at ata0def594286d, "Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging", 2017-01-30):
>
> - OVMF: boot and S3 suspend/resume
> - Ia32, Q35, SMM
> - Fedlet 20141209
> - Ia32X64, Q35, SMM
> - Fedora 22
> - Windows 7
> - Windows 8.1
> - Windows 10
> - Windows Server 2008 R2
> - Windows Server 2012 R2
> - Windows Server 2016 Tech Preview 4
> - X64, I440FX, no SMM
> - Fedora 24
> - RHEL-6.7
> - RHEL-7.2-ish
> - ArmVirtQemu: boot test with virtio-gpu
> - AARCH64
> - Fedora 24
> - RHELSA-7.3
> - openSUSE Tumbleweed (4.8.4-based)
>
> This change is connected to ASWG ticket
> <https://mantis.uefi.org/mantis/view.php?id=1757>, which is now closed/fixed.
>
> Cc: Al Stone <ahs3@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Leo Duran <leo.duran@amd.com>
> Cc: Michael Tsirkin <mtsirkin@redhat.com>
> Cc: Phil Dennis-Jordan <phil@philjordan.eu>
> Cc: Star Zeng <star.zeng@intel.com>
> Reported-by: Phil Dennis-Jordan <phil@philjordan.eu>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
>
> Notes:
> v2:
> - simplify logic in RequireDsdtXDsdtExclusion() [Jiewen]
> - pick up Phil's R-b nonetheless (the above change is a minimal
> reformulation of code, with no behavioral difference)
> - add reference to Mantis#1757 to the commit message
>
> v1:
> NOTE for people on the CC list:
>
> If you are not presently subscribed to edk2-devel and wish to comment on
> this patch publicly, you need to subscribe first, and wait for the
> subscription request to *complete* (see your inbox), *before* sending
> your followup. This is not ideal, but edk2-devel requires subscription
> before reflecting messages from someone.
>
> Subscribe at <https://lists.01.org/mailman/listinfo/edk2-devel>. Thanks.
>
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 62
> +++++++++++++++++++-
> 1 file changed, 59 insertions(+), 3 deletions(-)
>
> diff --git
> a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> index 7795ff7269ca..4bb848df5203 100644
> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> @@ -430,6 +430,51 @@ ReallocateAcpiTableBuffer (
> mEfiAcpiMaxNumTables = NewMaxTableNumber;
> return EFI_SUCCESS;
> }
> +
> +/**
> + Determine whether the FADT table passed in as parameter requires
> +mutual
> + exclusion between the DSDT and X_DSDT fields. (That is, whether
> +there exists
> + an explicit requirement that at most one of those fields is
> +permitted to be
> + nonzero.)
> +
> + @param[in] Fadt The EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE object to
> + check.
> +
> + @retval TRUE Fadt requires mutual exclusion between DSDT and X_DSDT.
> + @retval FALSE Otherwise.
> +**/
> +BOOLEAN
> +RequireDsdtXDsdtExclusion (
> + IN EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *Fadt
> + )
> +{
> + //
> + // Mantis ticket #1393 was addressed in ACPI 5.1 Errata B.
> +Unfortunately, we
> + // can't tell apart 5.1 Errata A and 5.1 Errata B just from looking
> +at the
> + // FADT table. Therefore let's require exclusion for table versions >= 5.1.
> + //
> + // While this needlessly covers 5.1 and 5.1A too, it is safer to
> +require
> + // DSDT<->X_DSDT exclusion for lax (5.1, 5.1A) versions of the spec
> +than to
> + // permit DSDT<->X_DSDT duplication for strict (5.1B) versions of the spec.
> + //
> + // The same applies to 6.0 vs. 6.0A. While 6.0 does not require the
> + // exclusion, 6.0A and 6.1 do. Since we cannot distinguish 6.0 from
> +6.0A
> + // based on just the FADT, we lump 6.0 in with the rest of >= 5.1.
> + //
> + if ((Fadt->Header.Revision < 5) ||
> + ((Fadt->Header.Revision == 5) &&
> + (((EFI_ACPI_5_1_FIXED_ACPI_DESCRIPTION_TABLE *)Fadt)->MinorVersion == 0))) {
> + //
> + // version <= 5.0
> + //
> + return FALSE;
> + }
> + //
> + // version >= 5.1
> + //
> + return TRUE;
> +}
> +
> /**
> This function adds an ACPI table to the table list. It will detect FACS and
> allocate the correct type of memory and properly align the table.
> @@ -647,12 +692,16 @@ AddTableToList (
> }
> if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
> AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN) AcpiTableInstance->Dsdt3;
> - ZeroMem (&AcpiTableInstance->Fadt3->XDsdt, sizeof (UINT64));
> + if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) {
> + Buffer64 = 0;
> + } else {
> + Buffer64 = AcpiTableInstance->Fadt3->Dsdt;
> + }
> } else {
> AcpiTableInstance->Fadt3->Dsdt = 0;
> Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
> - CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof (UINT64));
> }
> + CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof
> + (UINT64));
>
> //
> // RSDP OEM information is updated to match the FADT OEM information @@ -847,8 +896,15 @@ AddTableToList (
> if (AcpiTableInstance->Fadt3 != NULL) {
> if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
> AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN)
> AcpiTableInstance->Dsdt3;
> + if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) {
> + Buffer64 = 0;
> + } else {
> + Buffer64 = AcpiTableInstance->Fadt3->Dsdt;
> + }
> + } else {
> + AcpiTableInstance->Fadt3->Dsdt = 0;
> + Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
> }
> - Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
> CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof
> (UINT64));
>
> //
> --
> 2.9.3
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
2017-03-14 7:56 ` Fan, Jeff
@ 2017-03-14 8:33 ` Zeng, Star
2017-03-14 13:13 ` Laszlo Ersek
0 siblings, 1 reply; 14+ messages in thread
From: Zeng, Star @ 2017-03-14 8:33 UTC (permalink / raw)
To: Fan, Jeff, Laszlo Ersek, edk2-devel-01
Cc: Tian, Feng, Michael Tsirkin, Ard Biesheuvel, Phil Dennis-Jordan,
Leo Duran, Yao, Jiewen, Al Stone, Zeng, Star
In original code for < 4G table,
Dsdt and XDsdt will be both assigned if FADT is installed before DSDT, but
Dsdt and XDsdt will have mutual exclusion if FADT is installed after DSDT.
They are inconsistent.
Is there any negative impact found to assign both Dsdt and XDsdt for < 4G table except the spec volation?
Thanks,
Star
-----Original Message-----
From: Fan, Jeff
Sent: Tuesday, March 14, 2017 3:57 PM
To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2-devel@ml01.01.org>
Cc: Tian, Feng <feng.tian@intel.com>; Michael Tsirkin <mtsirkin@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Phil Dennis-Jordan <phil@philjordan.eu>; Leo Duran <leo.duran@amd.com>; Yao, Jiewen <jiewen.yao@intel.com>; Al Stone <ahs3@redhat.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
Laszlo,
Basically, I agree with this is OS assumption. I did not find better fix to handle such compatibility issue.
I agree to revert this patch 2/2 to fix Windows 2012 R2 boot issue.
I don't know if the other guys have other suggestions. :-)
Thanks!
Jeff
-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Monday, March 13, 2017 10:44 PM
To: Fan, Jeff; edk2-devel-01
Cc: Tian, Feng; Michael Tsirkin; Ard Biesheuvel; Phil Dennis-Jordan; Leo Duran; Yao, Jiewen; Al Stone; Zeng, Star
Subject: Re: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
On 03/13/17 04:07, Fan, Jeff wrote:
> Laszlo,
>
> We found one Windows Server 2012 R2 blue screen issue with ACPI 6.1 FADT table.
>
> We did the following configuration test with DSDT under 4GB.
> .DSDT .X_DSDT Window Server 2012 R2
> ---------- ------------ -------------------------------
> set clear Failed // current implementation
> clear set Succeed
> set set Succeed
That looks like a Windows bug. The above configuration satisfies ACPI 6.1:
DSDT -- Physical memory address of the DSDT. If the X_DSDT field contains a non-zero value then this field must be zero.
X_DSDT -- Extended physical address of the DSDT. If the DSDT field contains a non-zero value then this field must be zero.
Michael told me that "6.1 errata will specify X_DSDT takes preference over DSDT but both can be present legaly", however, here X_DSDT cannot take precedence because it is zero.
Based on past experience, I don't expect that Microsoft will ever fix this ACPI bug in Windows Server 2012 R2. I don't even expect that they would share with us a list of ACPI spec versions that should be exempted from RequireDsdtXDsdtExclusion() -- despite the spec clearly requiring DSDT / X_DSDT exclusion --, for bug compatibility.
That leaves us with trial and error, to see what works and what doesn't.
Unfortunately, I don't have ACPI tables for several ACPI spec versions; I don't think I can experiment with this. If you find a workaround, that would be great, but if we can't, I guess the patch should be reverted.
(Note however that the BSOD will remain possible to trigger, with the DSDT, FADT installation order.)
Thanks
Laszlo
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Laszlo Ersek
> Sent: Thursday, March 09, 2017 3:59 AM
> To: edk2-devel-01
> Cc: Tian, Feng; Michael Tsirkin; Ard Biesheuvel; Phil Dennis-Jordan;
> Leo Duran; Yao, Jiewen; Al Stone; Zeng, Star
> Subject: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve
> FADT.{DSDT, X_DSDT} mutual exclusion
>
> The ACPI specification, up to and including revision 5.1 Errata A,
> allows the DSDT and X_DSDT fields to be both set in the FADT.
> (Obviously, this only makes sense if the DSDT address is representable
> in 4 bytes.)
>
> Starting with 5.1 Errata B, specifically for Mantis 1393 <https://mantis.uefi.org/mantis/view.php?id=1393>, the spec requires at most one of DSDT and X_DSDT to be set to a nonzero value.
>
> MdeModulePkg/AcpiTableDxe handles this mutual exclusion somewhat inconsistently.
>
> - If the caller of EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() installs the
> tables in "DSDT, FADT" order, then we enforce the exclusion between the
> DSDT and X_DSDT fields:
>
> DSDT under 4GB FADT.DSDT FADT.X_DSDT [VARIANT B]
> -------------- --------- -----------
> yes set clear
> no clear set
>
> This behavior conforms to 5.1 Errata B. (And it's not required by
> earlier versions of the spec.)
>
> - If the caller passes in the tables in "FADT, DSDT" relative order, then
> we do not enforce the exclusion:
>
> DSDT under 4GB FADT.DSDT FADT.X_DSDT [VARIANT A]
> -------------- --------- -----------
> yes set set
> no clear set
>
> This satisfies 5.1 Errata A and earlier, but breaks 5.1 Errata B and
> later.
>
> Unify the handling of both relative orders. In particular, check the major and minor version numbers in the FADT. If the FADT version is strictly before 5.1, then implement [VARIANT A]. If the FADT version is equal to or larger than 5.1, then implement [VARIANT B].
>
> We make three observations:
>
> - We can't check the FADT table version precisely against "5.1 Errata B";
> erratum levels are not captured in the table. We err in the safe
> direction, namely we enforce the exclusion for "5.1" and "5.1 Errata A".
>
> - The same applies to "6.0" versus "6.0 Errata A". Because we cannot
> distinguish these two, we consider "6.0" to be "equal to or larger than
> 5.1", and apply [VARIANT B], enforcing the exclusion.
>
> - While a blanket [VARIANT B] would be simpler, there is a significant
> benefit to [VARIANT A], under the spec versions that permit it:
> compatibility with a wider range of OSPMs (typically, older ones).
>
> For example, Igor reported about a "DELL R430 system with rev4 FADT
> where DSDT and X_DSDT are pointing to the same address". Michael also
> reported about several systems that exhibit the same.
>
> Regression tested with the following KVM guests (QEMU built at ata0def594286d, "Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging", 2017-01-30):
>
> - OVMF: boot and S3 suspend/resume
> - Ia32, Q35, SMM
> - Fedlet 20141209
> - Ia32X64, Q35, SMM
> - Fedora 22
> - Windows 7
> - Windows 8.1
> - Windows 10
> - Windows Server 2008 R2
> - Windows Server 2012 R2
> - Windows Server 2016 Tech Preview 4
> - X64, I440FX, no SMM
> - Fedora 24
> - RHEL-6.7
> - RHEL-7.2-ish
> - ArmVirtQemu: boot test with virtio-gpu
> - AARCH64
> - Fedora 24
> - RHELSA-7.3
> - openSUSE Tumbleweed (4.8.4-based)
>
> This change is connected to ASWG ticket
> <https://mantis.uefi.org/mantis/view.php?id=1757>, which is now closed/fixed.
>
> Cc: Al Stone <ahs3@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Feng Tian <feng.tian@intel.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Leo Duran <leo.duran@amd.com>
> Cc: Michael Tsirkin <mtsirkin@redhat.com>
> Cc: Phil Dennis-Jordan <phil@philjordan.eu>
> Cc: Star Zeng <star.zeng@intel.com>
> Reported-by: Phil Dennis-Jordan <phil@philjordan.eu>
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
> ---
>
> Notes:
> v2:
> - simplify logic in RequireDsdtXDsdtExclusion() [Jiewen]
> - pick up Phil's R-b nonetheless (the above change is a minimal
> reformulation of code, with no behavioral difference)
> - add reference to Mantis#1757 to the commit message
>
> v1:
> NOTE for people on the CC list:
>
> If you are not presently subscribed to edk2-devel and wish to comment on
> this patch publicly, you need to subscribe first, and wait for the
> subscription request to *complete* (see your inbox), *before* sending
> your followup. This is not ideal, but edk2-devel requires subscription
> before reflecting messages from someone.
>
> Subscribe at <https://lists.01.org/mailman/listinfo/edk2-devel>. Thanks.
>
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 62
> +++++++++++++++++++-
> 1 file changed, 59 insertions(+), 3 deletions(-)
>
> diff --git
> a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> index 7795ff7269ca..4bb848df5203 100644
> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> @@ -430,6 +430,51 @@ ReallocateAcpiTableBuffer (
> mEfiAcpiMaxNumTables = NewMaxTableNumber;
> return EFI_SUCCESS;
> }
> +
> +/**
> + Determine whether the FADT table passed in as parameter requires
> +mutual
> + exclusion between the DSDT and X_DSDT fields. (That is, whether
> +there exists
> + an explicit requirement that at most one of those fields is
> +permitted to be
> + nonzero.)
> +
> + @param[in] Fadt The EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE object to
> + check.
> +
> + @retval TRUE Fadt requires mutual exclusion between DSDT and X_DSDT.
> + @retval FALSE Otherwise.
> +**/
> +BOOLEAN
> +RequireDsdtXDsdtExclusion (
> + IN EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *Fadt
> + )
> +{
> + //
> + // Mantis ticket #1393 was addressed in ACPI 5.1 Errata B.
> +Unfortunately, we
> + // can't tell apart 5.1 Errata A and 5.1 Errata B just from looking
> +at the
> + // FADT table. Therefore let's require exclusion for table versions >= 5.1.
> + //
> + // While this needlessly covers 5.1 and 5.1A too, it is safer to
> +require
> + // DSDT<->X_DSDT exclusion for lax (5.1, 5.1A) versions of the spec
> +than to
> + // permit DSDT<->X_DSDT duplication for strict (5.1B) versions of the spec.
> + //
> + // The same applies to 6.0 vs. 6.0A. While 6.0 does not require the
> + // exclusion, 6.0A and 6.1 do. Since we cannot distinguish 6.0 from
> +6.0A
> + // based on just the FADT, we lump 6.0 in with the rest of >= 5.1.
> + //
> + if ((Fadt->Header.Revision < 5) ||
> + ((Fadt->Header.Revision == 5) &&
> + (((EFI_ACPI_5_1_FIXED_ACPI_DESCRIPTION_TABLE *)Fadt)->MinorVersion == 0))) {
> + //
> + // version <= 5.0
> + //
> + return FALSE;
> + }
> + //
> + // version >= 5.1
> + //
> + return TRUE;
> +}
> +
> /**
> This function adds an ACPI table to the table list. It will detect FACS and
> allocate the correct type of memory and properly align the table.
> @@ -647,12 +692,16 @@ AddTableToList (
> }
> if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
> AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN) AcpiTableInstance->Dsdt3;
> - ZeroMem (&AcpiTableInstance->Fadt3->XDsdt, sizeof (UINT64));
> + if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) {
> + Buffer64 = 0;
> + } else {
> + Buffer64 = AcpiTableInstance->Fadt3->Dsdt;
> + }
> } else {
> AcpiTableInstance->Fadt3->Dsdt = 0;
> Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
> - CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof (UINT64));
> }
> + CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof
> + (UINT64));
>
> //
> // RSDP OEM information is updated to match the FADT OEM information @@ -847,8 +896,15 @@ AddTableToList (
> if (AcpiTableInstance->Fadt3 != NULL) {
> if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
> AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN)
> AcpiTableInstance->Dsdt3;
> + if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) {
> + Buffer64 = 0;
> + } else {
> + Buffer64 = AcpiTableInstance->Fadt3->Dsdt;
> + }
> + } else {
> + AcpiTableInstance->Fadt3->Dsdt = 0;
> + Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
> }
> - Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
> CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof
> (UINT64));
>
> //
> --
> 2.9.3
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
2017-03-14 8:33 ` Zeng, Star
@ 2017-03-14 13:13 ` Laszlo Ersek
2017-03-15 1:22 ` Zeng, Star
0 siblings, 1 reply; 14+ messages in thread
From: Laszlo Ersek @ 2017-03-14 13:13 UTC (permalink / raw)
To: Zeng, Star, Fan, Jeff, edk2-devel-01
Cc: Tian, Feng, Michael Tsirkin, Ard Biesheuvel, Phil Dennis-Jordan,
Leo Duran, Yao, Jiewen, Al Stone
On 03/14/17 09:33, Zeng, Star wrote:
> In original code for < 4G table,
> Dsdt and XDsdt will be both assigned if FADT is installed before DSDT, but
> Dsdt and XDsdt will have mutual exclusion if FADT is installed after DSDT.
>
> They are inconsistent.
That's right. The revert would not solve this original problem.
>
> Is there any negative impact found to assign both Dsdt and XDsdt for < 4G table except the spec volation?
No, I don't think so; the spec violation is the only impact to my knowledge.
Are you suggesting that we:
- delete RequireDsdtXDsdtExclusion(),
- replace all invocations of RequireDsdtXDsdtExclusion() with FALSE,
- and then simplify the resultant code (that is, eliminate the
always-FALSE branches, and un-indent the always-TRUE branches)?
This would be [VARIANT A], regardless of ACPI spec version.
I think that could work (as long as we don't mind breaking the ACPI spec
versions that require mutual exclusion).
Thanks
Laszlo
>
> Thanks,
> Star
> -----Original Message-----
> From: Fan, Jeff
> Sent: Tuesday, March 14, 2017 3:57 PM
> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2-devel@ml01.01.org>
> Cc: Tian, Feng <feng.tian@intel.com>; Michael Tsirkin <mtsirkin@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Phil Dennis-Jordan <phil@philjordan.eu>; Leo Duran <leo.duran@amd.com>; Yao, Jiewen <jiewen.yao@intel.com>; Al Stone <ahs3@redhat.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
>
> Laszlo,
>
> Basically, I agree with this is OS assumption. I did not find better fix to handle such compatibility issue.
>
> I agree to revert this patch 2/2 to fix Windows 2012 R2 boot issue.
>
> I don't know if the other guys have other suggestions. :-)
>
> Thanks!
> Jeff
>
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, March 13, 2017 10:44 PM
> To: Fan, Jeff; edk2-devel-01
> Cc: Tian, Feng; Michael Tsirkin; Ard Biesheuvel; Phil Dennis-Jordan; Leo Duran; Yao, Jiewen; Al Stone; Zeng, Star
> Subject: Re: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
>
> On 03/13/17 04:07, Fan, Jeff wrote:
>> Laszlo,
>>
>> We found one Windows Server 2012 R2 blue screen issue with ACPI 6.1 FADT table.
>>
>> We did the following configuration test with DSDT under 4GB.
>> .DSDT .X_DSDT Window Server 2012 R2
>> ---------- ------------ -------------------------------
>> set clear Failed // current implementation
>> clear set Succeed
>> set set Succeed
>
> That looks like a Windows bug. The above configuration satisfies ACPI 6.1:
>
> DSDT -- Physical memory address of the DSDT. If the X_DSDT field contains a non-zero value then this field must be zero.
>
> X_DSDT -- Extended physical address of the DSDT. If the DSDT field contains a non-zero value then this field must be zero.
>
> Michael told me that "6.1 errata will specify X_DSDT takes preference over DSDT but both can be present legaly", however, here X_DSDT cannot take precedence because it is zero.
>
> Based on past experience, I don't expect that Microsoft will ever fix this ACPI bug in Windows Server 2012 R2. I don't even expect that they would share with us a list of ACPI spec versions that should be exempted from RequireDsdtXDsdtExclusion() -- despite the spec clearly requiring DSDT / X_DSDT exclusion --, for bug compatibility.
>
> That leaves us with trial and error, to see what works and what doesn't.
> Unfortunately, I don't have ACPI tables for several ACPI spec versions; I don't think I can experiment with this. If you find a workaround, that would be great, but if we can't, I guess the patch should be reverted.
> (Note however that the BSOD will remain possible to trigger, with the DSDT, FADT installation order.)
>
> Thanks
> Laszlo
>
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>> Laszlo Ersek
>> Sent: Thursday, March 09, 2017 3:59 AM
>> To: edk2-devel-01
>> Cc: Tian, Feng; Michael Tsirkin; Ard Biesheuvel; Phil Dennis-Jordan;
>> Leo Duran; Yao, Jiewen; Al Stone; Zeng, Star
>> Subject: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve
>> FADT.{DSDT, X_DSDT} mutual exclusion
>>
>> The ACPI specification, up to and including revision 5.1 Errata A,
>> allows the DSDT and X_DSDT fields to be both set in the FADT.
>> (Obviously, this only makes sense if the DSDT address is representable
>> in 4 bytes.)
>>
>> Starting with 5.1 Errata B, specifically for Mantis 1393 <https://mantis.uefi.org/mantis/view.php?id=1393>, the spec requires at most one of DSDT and X_DSDT to be set to a nonzero value.
>>
>> MdeModulePkg/AcpiTableDxe handles this mutual exclusion somewhat inconsistently.
>>
>> - If the caller of EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() installs the
>> tables in "DSDT, FADT" order, then we enforce the exclusion between the
>> DSDT and X_DSDT fields:
>>
>> DSDT under 4GB FADT.DSDT FADT.X_DSDT [VARIANT B]
>> -------------- --------- -----------
>> yes set clear
>> no clear set
>>
>> This behavior conforms to 5.1 Errata B. (And it's not required by
>> earlier versions of the spec.)
>>
>> - If the caller passes in the tables in "FADT, DSDT" relative order, then
>> we do not enforce the exclusion:
>>
>> DSDT under 4GB FADT.DSDT FADT.X_DSDT [VARIANT A]
>> -------------- --------- -----------
>> yes set set
>> no clear set
>>
>> This satisfies 5.1 Errata A and earlier, but breaks 5.1 Errata B and
>> later.
>>
>> Unify the handling of both relative orders. In particular, check the major and minor version numbers in the FADT. If the FADT version is strictly before 5.1, then implement [VARIANT A]. If the FADT version is equal to or larger than 5.1, then implement [VARIANT B].
>>
>> We make three observations:
>>
>> - We can't check the FADT table version precisely against "5.1 Errata B";
>> erratum levels are not captured in the table. We err in the safe
>> direction, namely we enforce the exclusion for "5.1" and "5.1 Errata A".
>>
>> - The same applies to "6.0" versus "6.0 Errata A". Because we cannot
>> distinguish these two, we consider "6.0" to be "equal to or larger than
>> 5.1", and apply [VARIANT B], enforcing the exclusion.
>>
>> - While a blanket [VARIANT B] would be simpler, there is a significant
>> benefit to [VARIANT A], under the spec versions that permit it:
>> compatibility with a wider range of OSPMs (typically, older ones).
>>
>> For example, Igor reported about a "DELL R430 system with rev4 FADT
>> where DSDT and X_DSDT are pointing to the same address". Michael also
>> reported about several systems that exhibit the same.
>>
>> Regression tested with the following KVM guests (QEMU built at ata0def594286d, "Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging", 2017-01-30):
>>
>> - OVMF: boot and S3 suspend/resume
>> - Ia32, Q35, SMM
>> - Fedlet 20141209
>> - Ia32X64, Q35, SMM
>> - Fedora 22
>> - Windows 7
>> - Windows 8.1
>> - Windows 10
>> - Windows Server 2008 R2
>> - Windows Server 2012 R2
>> - Windows Server 2016 Tech Preview 4
>> - X64, I440FX, no SMM
>> - Fedora 24
>> - RHEL-6.7
>> - RHEL-7.2-ish
>> - ArmVirtQemu: boot test with virtio-gpu
>> - AARCH64
>> - Fedora 24
>> - RHELSA-7.3
>> - openSUSE Tumbleweed (4.8.4-based)
>>
>> This change is connected to ASWG ticket
>> <https://mantis.uefi.org/mantis/view.php?id=1757>, which is now closed/fixed.
>>
>> Cc: Al Stone <ahs3@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Feng Tian <feng.tian@intel.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Leo Duran <leo.duran@amd.com>
>> Cc: Michael Tsirkin <mtsirkin@redhat.com>
>> Cc: Phil Dennis-Jordan <phil@philjordan.eu>
>> Cc: Star Zeng <star.zeng@intel.com>
>> Reported-by: Phil Dennis-Jordan <phil@philjordan.eu>
>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
>> ---
>>
>> Notes:
>> v2:
>> - simplify logic in RequireDsdtXDsdtExclusion() [Jiewen]
>> - pick up Phil's R-b nonetheless (the above change is a minimal
>> reformulation of code, with no behavioral difference)
>> - add reference to Mantis#1757 to the commit message
>>
>> v1:
>> NOTE for people on the CC list:
>>
>> If you are not presently subscribed to edk2-devel and wish to comment on
>> this patch publicly, you need to subscribe first, and wait for the
>> subscription request to *complete* (see your inbox), *before* sending
>> your followup. This is not ideal, but edk2-devel requires subscription
>> before reflecting messages from someone.
>>
>> Subscribe at <https://lists.01.org/mailman/listinfo/edk2-devel>. Thanks.
>>
>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 62
>> +++++++++++++++++++-
>> 1 file changed, 59 insertions(+), 3 deletions(-)
>>
>> diff --git
>> a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>> index 7795ff7269ca..4bb848df5203 100644
>> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>> @@ -430,6 +430,51 @@ ReallocateAcpiTableBuffer (
>> mEfiAcpiMaxNumTables = NewMaxTableNumber;
>> return EFI_SUCCESS;
>> }
>> +
>> +/**
>> + Determine whether the FADT table passed in as parameter requires
>> +mutual
>> + exclusion between the DSDT and X_DSDT fields. (That is, whether
>> +there exists
>> + an explicit requirement that at most one of those fields is
>> +permitted to be
>> + nonzero.)
>> +
>> + @param[in] Fadt The EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE object to
>> + check.
>> +
>> + @retval TRUE Fadt requires mutual exclusion between DSDT and X_DSDT.
>> + @retval FALSE Otherwise.
>> +**/
>> +BOOLEAN
>> +RequireDsdtXDsdtExclusion (
>> + IN EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *Fadt
>> + )
>> +{
>> + //
>> + // Mantis ticket #1393 was addressed in ACPI 5.1 Errata B.
>> +Unfortunately, we
>> + // can't tell apart 5.1 Errata A and 5.1 Errata B just from looking
>> +at the
>> + // FADT table. Therefore let's require exclusion for table versions >= 5.1.
>> + //
>> + // While this needlessly covers 5.1 and 5.1A too, it is safer to
>> +require
>> + // DSDT<->X_DSDT exclusion for lax (5.1, 5.1A) versions of the spec
>> +than to
>> + // permit DSDT<->X_DSDT duplication for strict (5.1B) versions of the spec.
>> + //
>> + // The same applies to 6.0 vs. 6.0A. While 6.0 does not require the
>> + // exclusion, 6.0A and 6.1 do. Since we cannot distinguish 6.0 from
>> +6.0A
>> + // based on just the FADT, we lump 6.0 in with the rest of >= 5.1.
>> + //
>> + if ((Fadt->Header.Revision < 5) ||
>> + ((Fadt->Header.Revision == 5) &&
>> + (((EFI_ACPI_5_1_FIXED_ACPI_DESCRIPTION_TABLE *)Fadt)->MinorVersion == 0))) {
>> + //
>> + // version <= 5.0
>> + //
>> + return FALSE;
>> + }
>> + //
>> + // version >= 5.1
>> + //
>> + return TRUE;
>> +}
>> +
>> /**
>> This function adds an ACPI table to the table list. It will detect FACS and
>> allocate the correct type of memory and properly align the table.
>> @@ -647,12 +692,16 @@ AddTableToList (
>> }
>> if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
>> AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN) AcpiTableInstance->Dsdt3;
>> - ZeroMem (&AcpiTableInstance->Fadt3->XDsdt, sizeof (UINT64));
>> + if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) {
>> + Buffer64 = 0;
>> + } else {
>> + Buffer64 = AcpiTableInstance->Fadt3->Dsdt;
>> + }
>> } else {
>> AcpiTableInstance->Fadt3->Dsdt = 0;
>> Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
>> - CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof (UINT64));
>> }
>> + CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof
>> + (UINT64));
>>
>> //
>> // RSDP OEM information is updated to match the FADT OEM information @@ -847,8 +896,15 @@ AddTableToList (
>> if (AcpiTableInstance->Fadt3 != NULL) {
>> if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
>> AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN)
>> AcpiTableInstance->Dsdt3;
>> + if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) {
>> + Buffer64 = 0;
>> + } else {
>> + Buffer64 = AcpiTableInstance->Fadt3->Dsdt;
>> + }
>> + } else {
>> + AcpiTableInstance->Fadt3->Dsdt = 0;
>> + Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
>> }
>> - Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
>> CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof
>> (UINT64));
>>
>> //
>> --
>> 2.9.3
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
>>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
2017-03-14 13:13 ` Laszlo Ersek
@ 2017-03-15 1:22 ` Zeng, Star
2017-03-15 15:10 ` Laszlo Ersek
0 siblings, 1 reply; 14+ messages in thread
From: Zeng, Star @ 2017-03-15 1:22 UTC (permalink / raw)
To: Laszlo Ersek, Fan, Jeff, edk2-devel-01
Cc: Tian, Feng, Michael Tsirkin, Ard Biesheuvel, Phil Dennis-Jordan,
Yao, Jiewen, Leo Duran, Al Stone, star.zeng
On 2017/3/14 21:13, Laszlo Ersek wrote:
> On 03/14/17 09:33, Zeng, Star wrote:
>> In original code for < 4G table,
>> Dsdt and XDsdt will be both assigned if FADT is installed before DSDT, but
>> Dsdt and XDsdt will have mutual exclusion if FADT is installed after DSDT.
>>
>> They are inconsistent.
>
> That's right. The revert would not solve this original problem.
>
>>
>> Is there any negative impact found to assign both Dsdt and XDsdt for < 4G table except the spec volation?
>
> No, I don't think so; the spec violation is the only impact to my knowledge.
>
> Are you suggesting that we:
> - delete RequireDsdtXDsdtExclusion(),
> - replace all invocations of RequireDsdtXDsdtExclusion() with FALSE,
> - and then simplify the resultant code (that is, eliminate the
> always-FALSE branches, and un-indent the always-TRUE branches)?
>
> This would be [VARIANT A], regardless of ACPI spec version.
>
> I think that could work (as long as we don't mind breaking the ACPI spec
> versions that require mutual exclusion).
Yes. Then the code could have maximum compatibility and the behavior
could be consistent, sure more comments about code behavior and ACPI
spec could be added.
Thanks,
Star
>
> Thanks
> Laszlo
>
>>
>> Thanks,
>> Star
>> -----Original Message-----
>> From: Fan, Jeff
>> Sent: Tuesday, March 14, 2017 3:57 PM
>> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2-devel@ml01.01.org>
>> Cc: Tian, Feng <feng.tian@intel.com>; Michael Tsirkin <mtsirkin@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; Phil Dennis-Jordan <phil@philjordan.eu>; Leo Duran <leo.duran@amd.com>; Yao, Jiewen <jiewen.yao@intel.com>; Al Stone <ahs3@redhat.com>; Zeng, Star <star.zeng@intel.com>
>> Subject: RE: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
>>
>> Laszlo,
>>
>> Basically, I agree with this is OS assumption. I did not find better fix to handle such compatibility issue.
>>
>> I agree to revert this patch 2/2 to fix Windows 2012 R2 boot issue.
>>
>> I don't know if the other guys have other suggestions. :-)
>>
>> Thanks!
>> Jeff
>>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, March 13, 2017 10:44 PM
>> To: Fan, Jeff; edk2-devel-01
>> Cc: Tian, Feng; Michael Tsirkin; Ard Biesheuvel; Phil Dennis-Jordan; Leo Duran; Yao, Jiewen; Al Stone; Zeng, Star
>> Subject: Re: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
>>
>> On 03/13/17 04:07, Fan, Jeff wrote:
>>> Laszlo,
>>>
>>> We found one Windows Server 2012 R2 blue screen issue with ACPI 6.1 FADT table.
>>>
>>> We did the following configuration test with DSDT under 4GB.
>>> .DSDT .X_DSDT Window Server 2012 R2
>>> ---------- ------------ -------------------------------
>>> set clear Failed // current implementation
>>> clear set Succeed
>>> set set Succeed
>>
>> That looks like a Windows bug. The above configuration satisfies ACPI 6.1:
>>
>> DSDT -- Physical memory address of the DSDT. If the X_DSDT field contains a non-zero value then this field must be zero.
>>
>> X_DSDT -- Extended physical address of the DSDT. If the DSDT field contains a non-zero value then this field must be zero.
>>
>> Michael told me that "6.1 errata will specify X_DSDT takes preference over DSDT but both can be present legaly", however, here X_DSDT cannot take precedence because it is zero.
>>
>> Based on past experience, I don't expect that Microsoft will ever fix this ACPI bug in Windows Server 2012 R2. I don't even expect that they would share with us a list of ACPI spec versions that should be exempted from RequireDsdtXDsdtExclusion() -- despite the spec clearly requiring DSDT / X_DSDT exclusion --, for bug compatibility.
>>
>> That leaves us with trial and error, to see what works and what doesn't.
>> Unfortunately, I don't have ACPI tables for several ACPI spec versions; I don't think I can experiment with this. If you find a workaround, that would be great, but if we can't, I guess the patch should be reverted.
>> (Note however that the BSOD will remain possible to trigger, with the DSDT, FADT installation order.)
>>
>> Thanks
>> Laszlo
>>
>>> -----Original Message-----
>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>> Laszlo Ersek
>>> Sent: Thursday, March 09, 2017 3:59 AM
>>> To: edk2-devel-01
>>> Cc: Tian, Feng; Michael Tsirkin; Ard Biesheuvel; Phil Dennis-Jordan;
>>> Leo Duran; Yao, Jiewen; Al Stone; Zeng, Star
>>> Subject: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve
>>> FADT.{DSDT, X_DSDT} mutual exclusion
>>>
>>> The ACPI specification, up to and including revision 5.1 Errata A,
>>> allows the DSDT and X_DSDT fields to be both set in the FADT.
>>> (Obviously, this only makes sense if the DSDT address is representable
>>> in 4 bytes.)
>>>
>>> Starting with 5.1 Errata B, specifically for Mantis 1393 <https://mantis.uefi.org/mantis/view.php?id=1393>, the spec requires at most one of DSDT and X_DSDT to be set to a nonzero value.
>>>
>>> MdeModulePkg/AcpiTableDxe handles this mutual exclusion somewhat inconsistently.
>>>
>>> - If the caller of EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() installs the
>>> tables in "DSDT, FADT" order, then we enforce the exclusion between the
>>> DSDT and X_DSDT fields:
>>>
>>> DSDT under 4GB FADT.DSDT FADT.X_DSDT [VARIANT B]
>>> -------------- --------- -----------
>>> yes set clear
>>> no clear set
>>>
>>> This behavior conforms to 5.1 Errata B. (And it's not required by
>>> earlier versions of the spec.)
>>>
>>> - If the caller passes in the tables in "FADT, DSDT" relative order, then
>>> we do not enforce the exclusion:
>>>
>>> DSDT under 4GB FADT.DSDT FADT.X_DSDT [VARIANT A]
>>> -------------- --------- -----------
>>> yes set set
>>> no clear set
>>>
>>> This satisfies 5.1 Errata A and earlier, but breaks 5.1 Errata B and
>>> later.
>>>
>>> Unify the handling of both relative orders. In particular, check the major and minor version numbers in the FADT. If the FADT version is strictly before 5.1, then implement [VARIANT A]. If the FADT version is equal to or larger than 5.1, then implement [VARIANT B].
>>>
>>> We make three observations:
>>>
>>> - We can't check the FADT table version precisely against "5.1 Errata B";
>>> erratum levels are not captured in the table. We err in the safe
>>> direction, namely we enforce the exclusion for "5.1" and "5.1 Errata A".
>>>
>>> - The same applies to "6.0" versus "6.0 Errata A". Because we cannot
>>> distinguish these two, we consider "6.0" to be "equal to or larger than
>>> 5.1", and apply [VARIANT B], enforcing the exclusion.
>>>
>>> - While a blanket [VARIANT B] would be simpler, there is a significant
>>> benefit to [VARIANT A], under the spec versions that permit it:
>>> compatibility with a wider range of OSPMs (typically, older ones).
>>>
>>> For example, Igor reported about a "DELL R430 system with rev4 FADT
>>> where DSDT and X_DSDT are pointing to the same address". Michael also
>>> reported about several systems that exhibit the same.
>>>
>>> Regression tested with the following KVM guests (QEMU built at ata0def594286d, "Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging", 2017-01-30):
>>>
>>> - OVMF: boot and S3 suspend/resume
>>> - Ia32, Q35, SMM
>>> - Fedlet 20141209
>>> - Ia32X64, Q35, SMM
>>> - Fedora 22
>>> - Windows 7
>>> - Windows 8.1
>>> - Windows 10
>>> - Windows Server 2008 R2
>>> - Windows Server 2012 R2
>>> - Windows Server 2016 Tech Preview 4
>>> - X64, I440FX, no SMM
>>> - Fedora 24
>>> - RHEL-6.7
>>> - RHEL-7.2-ish
>>> - ArmVirtQemu: boot test with virtio-gpu
>>> - AARCH64
>>> - Fedora 24
>>> - RHELSA-7.3
>>> - openSUSE Tumbleweed (4.8.4-based)
>>>
>>> This change is connected to ASWG ticket
>>> <https://mantis.uefi.org/mantis/view.php?id=1757>, which is now closed/fixed.
>>>
>>> Cc: Al Stone <ahs3@redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Feng Tian <feng.tian@intel.com>
>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Cc: Leo Duran <leo.duran@amd.com>
>>> Cc: Michael Tsirkin <mtsirkin@redhat.com>
>>> Cc: Phil Dennis-Jordan <phil@philjordan.eu>
>>> Cc: Star Zeng <star.zeng@intel.com>
>>> Reported-by: Phil Dennis-Jordan <phil@philjordan.eu>
>>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
>>> ---
>>>
>>> Notes:
>>> v2:
>>> - simplify logic in RequireDsdtXDsdtExclusion() [Jiewen]
>>> - pick up Phil's R-b nonetheless (the above change is a minimal
>>> reformulation of code, with no behavioral difference)
>>> - add reference to Mantis#1757 to the commit message
>>>
>>> v1:
>>> NOTE for people on the CC list:
>>>
>>> If you are not presently subscribed to edk2-devel and wish to comment on
>>> this patch publicly, you need to subscribe first, and wait for the
>>> subscription request to *complete* (see your inbox), *before* sending
>>> your followup. This is not ideal, but edk2-devel requires subscription
>>> before reflecting messages from someone.
>>>
>>> Subscribe at <https://lists.01.org/mailman/listinfo/edk2-devel>. Thanks.
>>>
>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 62
>>> +++++++++++++++++++-
>>> 1 file changed, 59 insertions(+), 3 deletions(-)
>>>
>>> diff --git
>>> a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>>> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>>> index 7795ff7269ca..4bb848df5203 100644
>>> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>>> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>>> @@ -430,6 +430,51 @@ ReallocateAcpiTableBuffer (
>>> mEfiAcpiMaxNumTables = NewMaxTableNumber;
>>> return EFI_SUCCESS;
>>> }
>>> +
>>> +/**
>>> + Determine whether the FADT table passed in as parameter requires
>>> +mutual
>>> + exclusion between the DSDT and X_DSDT fields. (That is, whether
>>> +there exists
>>> + an explicit requirement that at most one of those fields is
>>> +permitted to be
>>> + nonzero.)
>>> +
>>> + @param[in] Fadt The EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE object to
>>> + check.
>>> +
>>> + @retval TRUE Fadt requires mutual exclusion between DSDT and X_DSDT.
>>> + @retval FALSE Otherwise.
>>> +**/
>>> +BOOLEAN
>>> +RequireDsdtXDsdtExclusion (
>>> + IN EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *Fadt
>>> + )
>>> +{
>>> + //
>>> + // Mantis ticket #1393 was addressed in ACPI 5.1 Errata B.
>>> +Unfortunately, we
>>> + // can't tell apart 5.1 Errata A and 5.1 Errata B just from looking
>>> +at the
>>> + // FADT table. Therefore let's require exclusion for table versions >= 5.1.
>>> + //
>>> + // While this needlessly covers 5.1 and 5.1A too, it is safer to
>>> +require
>>> + // DSDT<->X_DSDT exclusion for lax (5.1, 5.1A) versions of the spec
>>> +than to
>>> + // permit DSDT<->X_DSDT duplication for strict (5.1B) versions of the spec.
>>> + //
>>> + // The same applies to 6.0 vs. 6.0A. While 6.0 does not require the
>>> + // exclusion, 6.0A and 6.1 do. Since we cannot distinguish 6.0 from
>>> +6.0A
>>> + // based on just the FADT, we lump 6.0 in with the rest of >= 5.1.
>>> + //
>>> + if ((Fadt->Header.Revision < 5) ||
>>> + ((Fadt->Header.Revision == 5) &&
>>> + (((EFI_ACPI_5_1_FIXED_ACPI_DESCRIPTION_TABLE *)Fadt)->MinorVersion == 0))) {
>>> + //
>>> + // version <= 5.0
>>> + //
>>> + return FALSE;
>>> + }
>>> + //
>>> + // version >= 5.1
>>> + //
>>> + return TRUE;
>>> +}
>>> +
>>> /**
>>> This function adds an ACPI table to the table list. It will detect FACS and
>>> allocate the correct type of memory and properly align the table.
>>> @@ -647,12 +692,16 @@ AddTableToList (
>>> }
>>> if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
>>> AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN) AcpiTableInstance->Dsdt3;
>>> - ZeroMem (&AcpiTableInstance->Fadt3->XDsdt, sizeof (UINT64));
>>> + if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) {
>>> + Buffer64 = 0;
>>> + } else {
>>> + Buffer64 = AcpiTableInstance->Fadt3->Dsdt;
>>> + }
>>> } else {
>>> AcpiTableInstance->Fadt3->Dsdt = 0;
>>> Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
>>> - CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof (UINT64));
>>> }
>>> + CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof
>>> + (UINT64));
>>>
>>> //
>>> // RSDP OEM information is updated to match the FADT OEM information @@ -847,8 +896,15 @@ AddTableToList (
>>> if (AcpiTableInstance->Fadt3 != NULL) {
>>> if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
>>> AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN)
>>> AcpiTableInstance->Dsdt3;
>>> + if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) {
>>> + Buffer64 = 0;
>>> + } else {
>>> + Buffer64 = AcpiTableInstance->Fadt3->Dsdt;
>>> + }
>>> + } else {
>>> + AcpiTableInstance->Fadt3->Dsdt = 0;
>>> + Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
>>> }
>>> - Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
>>> CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof
>>> (UINT64));
>>>
>>> //
>>> --
>>> 2.9.3
>>>
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion
2017-03-15 1:22 ` Zeng, Star
@ 2017-03-15 15:10 ` Laszlo Ersek
0 siblings, 0 replies; 14+ messages in thread
From: Laszlo Ersek @ 2017-03-15 15:10 UTC (permalink / raw)
To: Zeng, Star, Fan, Jeff, edk2-devel-01
Cc: Tian, Feng, Michael Tsirkin, Ard Biesheuvel, Phil Dennis-Jordan,
Yao, Jiewen, Leo Duran, Al Stone
On 03/15/17 02:22, Zeng, Star wrote:
> On 2017/3/14 21:13, Laszlo Ersek wrote:
>> On 03/14/17 09:33, Zeng, Star wrote:
>>> In original code for < 4G table,
>>> Dsdt and XDsdt will be both assigned if FADT is installed before
>>> DSDT, but
>>> Dsdt and XDsdt will have mutual exclusion if FADT is installed after
>>> DSDT.
>>>
>>> They are inconsistent.
>>
>> That's right. The revert would not solve this original problem.
>>
>>>
>>> Is there any negative impact found to assign both Dsdt and XDsdt for
>>> < 4G table except the spec volation?
>>
>> No, I don't think so; the spec violation is the only impact to my
>> knowledge.
>>
>> Are you suggesting that we:
>> - delete RequireDsdtXDsdtExclusion(),
>> - replace all invocations of RequireDsdtXDsdtExclusion() with FALSE,
>> - and then simplify the resultant code (that is, eliminate the
>> always-FALSE branches, and un-indent the always-TRUE branches)?
>>
>> This would be [VARIANT A], regardless of ACPI spec version.
>>
>> I think that could work (as long as we don't mind breaking the ACPI spec
>> versions that require mutual exclusion).
>
> Yes. Then the code could have maximum compatibility and the behavior
> could be consistent, sure more comments about code behavior and ACPI
> spec could be added.
Do you want me to submit the patch, or can one of you guys do it? I'm
happy to review the patch, but I would be slower to post it.
Thanks!
Laszlo
>
> Thanks,
> Star
>
>>
>> Thanks
>> Laszlo
>>
>>>
>>> Thanks,
>>> Star
>>> -----Original Message-----
>>> From: Fan, Jeff
>>> Sent: Tuesday, March 14, 2017 3:57 PM
>>> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01
>>> <edk2-devel@ml01.01.org>
>>> Cc: Tian, Feng <feng.tian@intel.com>; Michael Tsirkin
>>> <mtsirkin@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
>>> Phil Dennis-Jordan <phil@philjordan.eu>; Leo Duran
>>> <leo.duran@amd.com>; Yao, Jiewen <jiewen.yao@intel.com>; Al Stone
>>> <ahs3@redhat.com>; Zeng, Star <star.zeng@intel.com>
>>> Subject: RE: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve
>>> FADT.{DSDT, X_DSDT} mutual exclusion
>>>
>>> Laszlo,
>>>
>>> Basically, I agree with this is OS assumption. I did not find better
>>> fix to handle such compatibility issue.
>>>
>>> I agree to revert this patch 2/2 to fix Windows 2012 R2 boot issue.
>>>
>>> I don't know if the other guys have other suggestions. :-)
>>>
>>> Thanks!
>>> Jeff
>>>
>>> -----Original Message-----
>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>> Sent: Monday, March 13, 2017 10:44 PM
>>> To: Fan, Jeff; edk2-devel-01
>>> Cc: Tian, Feng; Michael Tsirkin; Ard Biesheuvel; Phil Dennis-Jordan;
>>> Leo Duran; Yao, Jiewen; Al Stone; Zeng, Star
>>> Subject: Re: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve
>>> FADT.{DSDT, X_DSDT} mutual exclusion
>>>
>>> On 03/13/17 04:07, Fan, Jeff wrote:
>>>> Laszlo,
>>>>
>>>> We found one Windows Server 2012 R2 blue screen issue with ACPI 6.1
>>>> FADT table.
>>>>
>>>> We did the following configuration test with DSDT under 4GB.
>>>> .DSDT .X_DSDT Window Server 2012 R2
>>>> ---------- ------------ -------------------------------
>>>> set clear Failed // current
>>>> implementation
>>>> clear set Succeed
>>>> set set Succeed
>>>
>>> That looks like a Windows bug. The above configuration satisfies ACPI
>>> 6.1:
>>>
>>> DSDT -- Physical memory address of the DSDT. If the X_DSDT field
>>> contains a non-zero value then this field must be zero.
>>>
>>> X_DSDT -- Extended physical address of the DSDT. If the DSDT field
>>> contains a non-zero value then this field must be zero.
>>>
>>> Michael told me that "6.1 errata will specify X_DSDT takes preference
>>> over DSDT but both can be present legaly", however, here X_DSDT
>>> cannot take precedence because it is zero.
>>>
>>> Based on past experience, I don't expect that Microsoft will ever fix
>>> this ACPI bug in Windows Server 2012 R2. I don't even expect that
>>> they would share with us a list of ACPI spec versions that should be
>>> exempted from RequireDsdtXDsdtExclusion() -- despite the spec clearly
>>> requiring DSDT / X_DSDT exclusion --, for bug compatibility.
>>>
>>> That leaves us with trial and error, to see what works and what doesn't.
>>> Unfortunately, I don't have ACPI tables for several ACPI spec
>>> versions; I don't think I can experiment with this. If you find a
>>> workaround, that would be great, but if we can't, I guess the patch
>>> should be reverted.
>>> (Note however that the BSOD will remain possible to trigger, with the
>>> DSDT, FADT installation order.)
>>>
>>> Thanks
>>> Laszlo
>>>
>>>> -----Original Message-----
>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
>>>> Laszlo Ersek
>>>> Sent: Thursday, March 09, 2017 3:59 AM
>>>> To: edk2-devel-01
>>>> Cc: Tian, Feng; Michael Tsirkin; Ard Biesheuvel; Phil Dennis-Jordan;
>>>> Leo Duran; Yao, Jiewen; Al Stone; Zeng, Star
>>>> Subject: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve
>>>> FADT.{DSDT, X_DSDT} mutual exclusion
>>>>
>>>> The ACPI specification, up to and including revision 5.1 Errata A,
>>>> allows the DSDT and X_DSDT fields to be both set in the FADT.
>>>> (Obviously, this only makes sense if the DSDT address is representable
>>>> in 4 bytes.)
>>>>
>>>> Starting with 5.1 Errata B, specifically for Mantis 1393
>>>> <https://mantis.uefi.org/mantis/view.php?id=1393>, the spec requires
>>>> at most one of DSDT and X_DSDT to be set to a nonzero value.
>>>>
>>>> MdeModulePkg/AcpiTableDxe handles this mutual exclusion somewhat
>>>> inconsistently.
>>>>
>>>> - If the caller of EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable()
>>>> installs the
>>>> tables in "DSDT, FADT" order, then we enforce the exclusion
>>>> between the
>>>> DSDT and X_DSDT fields:
>>>>
>>>> DSDT under 4GB FADT.DSDT FADT.X_DSDT [VARIANT B]
>>>> -------------- --------- -----------
>>>> yes set clear
>>>> no clear set
>>>>
>>>> This behavior conforms to 5.1 Errata B. (And it's not required by
>>>> earlier versions of the spec.)
>>>>
>>>> - If the caller passes in the tables in "FADT, DSDT" relative order,
>>>> then
>>>> we do not enforce the exclusion:
>>>>
>>>> DSDT under 4GB FADT.DSDT FADT.X_DSDT [VARIANT A]
>>>> -------------- --------- -----------
>>>> yes set set
>>>> no clear set
>>>>
>>>> This satisfies 5.1 Errata A and earlier, but breaks 5.1 Errata B and
>>>> later.
>>>>
>>>> Unify the handling of both relative orders. In particular, check the
>>>> major and minor version numbers in the FADT. If the FADT version is
>>>> strictly before 5.1, then implement [VARIANT A]. If the FADT version
>>>> is equal to or larger than 5.1, then implement [VARIANT B].
>>>>
>>>> We make three observations:
>>>>
>>>> - We can't check the FADT table version precisely against "5.1
>>>> Errata B";
>>>> erratum levels are not captured in the table. We err in the safe
>>>> direction, namely we enforce the exclusion for "5.1" and "5.1
>>>> Errata A".
>>>>
>>>> - The same applies to "6.0" versus "6.0 Errata A". Because we cannot
>>>> distinguish these two, we consider "6.0" to be "equal to or larger
>>>> than
>>>> 5.1", and apply [VARIANT B], enforcing the exclusion.
>>>>
>>>> - While a blanket [VARIANT B] would be simpler, there is a significant
>>>> benefit to [VARIANT A], under the spec versions that permit it:
>>>> compatibility with a wider range of OSPMs (typically, older ones).
>>>>
>>>> For example, Igor reported about a "DELL R430 system with rev4 FADT
>>>> where DSDT and X_DSDT are pointing to the same address". Michael also
>>>> reported about several systems that exhibit the same.
>>>>
>>>> Regression tested with the following KVM guests (QEMU built at
>>>> ata0def594286d, "Merge remote-tracking branch
>>>> 'remotes/bonzini/tags/for-upstream' into staging", 2017-01-30):
>>>>
>>>> - OVMF: boot and S3 suspend/resume
>>>> - Ia32, Q35, SMM
>>>> - Fedlet 20141209
>>>> - Ia32X64, Q35, SMM
>>>> - Fedora 22
>>>> - Windows 7
>>>> - Windows 8.1
>>>> - Windows 10
>>>> - Windows Server 2008 R2
>>>> - Windows Server 2012 R2
>>>> - Windows Server 2016 Tech Preview 4
>>>> - X64, I440FX, no SMM
>>>> - Fedora 24
>>>> - RHEL-6.7
>>>> - RHEL-7.2-ish
>>>> - ArmVirtQemu: boot test with virtio-gpu
>>>> - AARCH64
>>>> - Fedora 24
>>>> - RHELSA-7.3
>>>> - openSUSE Tumbleweed (4.8.4-based)
>>>>
>>>> This change is connected to ASWG ticket
>>>> <https://mantis.uefi.org/mantis/view.php?id=1757>, which is now
>>>> closed/fixed.
>>>>
>>>> Cc: Al Stone <ahs3@redhat.com>
>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Cc: Feng Tian <feng.tian@intel.com>
>>>> Cc: Igor Mammedov <imammedo@redhat.com>
>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>> Cc: Leo Duran <leo.duran@amd.com>
>>>> Cc: Michael Tsirkin <mtsirkin@redhat.com>
>>>> Cc: Phil Dennis-Jordan <phil@philjordan.eu>
>>>> Cc: Star Zeng <star.zeng@intel.com>
>>>> Reported-by: Phil Dennis-Jordan <phil@philjordan.eu>
>>>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>>> Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>
>>>> ---
>>>>
>>>> Notes:
>>>> v2:
>>>> - simplify logic in RequireDsdtXDsdtExclusion() [Jiewen]
>>>> - pick up Phil's R-b nonetheless (the above change is a minimal
>>>> reformulation of code, with no behavioral difference)
>>>> - add reference to Mantis#1757 to the commit message
>>>>
>>>> v1:
>>>> NOTE for people on the CC list:
>>>>
>>>> If you are not presently subscribed to edk2-devel and wish to
>>>> comment on
>>>> this patch publicly, you need to subscribe first, and wait for the
>>>> subscription request to *complete* (see your inbox), *before*
>>>> sending
>>>> your followup. This is not ideal, but edk2-devel requires
>>>> subscription
>>>> before reflecting messages from someone.
>>>>
>>>> Subscribe at <https://lists.01.org/mailman/listinfo/edk2-devel>.
>>>> Thanks.
>>>>
>>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 62
>>>> +++++++++++++++++++-
>>>> 1 file changed, 59 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git
>>>> a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>>>> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>>>> index 7795ff7269ca..4bb848df5203 100644
>>>> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>>>> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>>>> @@ -430,6 +430,51 @@ ReallocateAcpiTableBuffer (
>>>> mEfiAcpiMaxNumTables = NewMaxTableNumber;
>>>> return EFI_SUCCESS;
>>>> }
>>>> +
>>>> +/**
>>>> + Determine whether the FADT table passed in as parameter requires
>>>> +mutual
>>>> + exclusion between the DSDT and X_DSDT fields. (That is, whether
>>>> +there exists
>>>> + an explicit requirement that at most one of those fields is
>>>> +permitted to be
>>>> + nonzero.)
>>>> +
>>>> + @param[in] Fadt The EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE
>>>> object to
>>>> + check.
>>>> +
>>>> + @retval TRUE Fadt requires mutual exclusion between DSDT and
>>>> X_DSDT.
>>>> + @retval FALSE Otherwise.
>>>> +**/
>>>> +BOOLEAN
>>>> +RequireDsdtXDsdtExclusion (
>>>> + IN EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *Fadt
>>>> + )
>>>> +{
>>>> + //
>>>> + // Mantis ticket #1393 was addressed in ACPI 5.1 Errata B.
>>>> +Unfortunately, we
>>>> + // can't tell apart 5.1 Errata A and 5.1 Errata B just from looking
>>>> +at the
>>>> + // FADT table. Therefore let's require exclusion for table
>>>> versions >= 5.1.
>>>> + //
>>>> + // While this needlessly covers 5.1 and 5.1A too, it is safer to
>>>> +require
>>>> + // DSDT<->X_DSDT exclusion for lax (5.1, 5.1A) versions of the spec
>>>> +than to
>>>> + // permit DSDT<->X_DSDT duplication for strict (5.1B) versions of
>>>> the spec.
>>>> + //
>>>> + // The same applies to 6.0 vs. 6.0A. While 6.0 does not require the
>>>> + // exclusion, 6.0A and 6.1 do. Since we cannot distinguish 6.0 from
>>>> +6.0A
>>>> + // based on just the FADT, we lump 6.0 in with the rest of >= 5.1.
>>>> + //
>>>> + if ((Fadt->Header.Revision < 5) ||
>>>> + ((Fadt->Header.Revision == 5) &&
>>>> + (((EFI_ACPI_5_1_FIXED_ACPI_DESCRIPTION_TABLE
>>>> *)Fadt)->MinorVersion == 0))) {
>>>> + //
>>>> + // version <= 5.0
>>>> + //
>>>> + return FALSE;
>>>> + }
>>>> + //
>>>> + // version >= 5.1
>>>> + //
>>>> + return TRUE;
>>>> +}
>>>> +
>>>> /**
>>>> This function adds an ACPI table to the table list. It will
>>>> detect FACS and
>>>> allocate the correct type of memory and properly align the table.
>>>> @@ -647,12 +692,16 @@ AddTableToList (
>>>> }
>>>> if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
>>>> AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN)
>>>> AcpiTableInstance->Dsdt3;
>>>> - ZeroMem (&AcpiTableInstance->Fadt3->XDsdt, sizeof (UINT64));
>>>> + if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) {
>>>> + Buffer64 = 0;
>>>> + } else {
>>>> + Buffer64 = AcpiTableInstance->Fadt3->Dsdt;
>>>> + }
>>>> } else {
>>>> AcpiTableInstance->Fadt3->Dsdt = 0;
>>>> Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
>>>> - CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64,
>>>> sizeof (UINT64));
>>>> }
>>>> + CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof
>>>> + (UINT64));
>>>>
>>>> //
>>>> // RSDP OEM information is updated to match the FADT OEM
>>>> information @@ -847,8 +896,15 @@ AddTableToList (
>>>> if (AcpiTableInstance->Fadt3 != NULL) {
>>>> if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) {
>>>> AcpiTableInstance->Fadt3->Dsdt = (UINT32) (UINTN)
>>>> AcpiTableInstance->Dsdt3;
>>>> + if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) {
>>>> + Buffer64 = 0;
>>>> + } else {
>>>> + Buffer64 = AcpiTableInstance->Fadt3->Dsdt;
>>>> + }
>>>> + } else {
>>>> + AcpiTableInstance->Fadt3->Dsdt = 0;
>>>> + Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
>>>> }
>>>> - Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3;
>>>> CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof
>>>> (UINT64));
>>>>
>>>> //
>>>> --
>>>> 2.9.3
>>>>
>>>> _______________________________________________
>>>> edk2-devel mailing list
>>>> edk2-devel@lists.01.org
>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>>
>>>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-03-15 15:10 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-03-08 19:58 [PATCH v2 0/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion Laszlo Ersek
2017-03-08 19:58 ` [PATCH v2 1/2] MdeModulePkg/AcpiTableDxe: condense whitespace around FADT.{DSDT, X_DSDT} Laszlo Ersek
2017-03-09 0:47 ` Yao, Jiewen
2017-03-08 19:58 ` [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion Laszlo Ersek
2017-03-09 0:47 ` Yao, Jiewen
2017-03-13 3:07 ` Fan, Jeff
2017-03-13 14:44 ` Laszlo Ersek
2017-03-14 7:56 ` Fan, Jeff
2017-03-14 8:33 ` Zeng, Star
2017-03-14 13:13 ` Laszlo Ersek
2017-03-15 1:22 ` Zeng, Star
2017-03-15 15:10 ` Laszlo Ersek
2017-03-09 1:59 ` [PATCH v2 0/2] " Zeng, Star
2017-03-09 14:06 ` Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox