[PATCH 0/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion

[PATCH 0/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion

[Laszlo Ersek]

Patch #2 explains it all.

Repo:   https://github.com/lersek/edk2/
Branch: fadt_dsdt

NOTE for people on the CC list:

If you are not presently subscribed to edk2-devel and wish to comment on
this series 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.

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: 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>

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 | 82 ++++++++++++++++----
 1 file changed, 67 insertions(+), 15 deletions(-)

-- 
2.9.3

[PATCH 1/2] MdeModulePkg/AcpiTableDxe: condense whitespace around FADT.{DSDT, X_DSDT}

[Laszlo Ersek]

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: 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>
---

Notes:
    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

Re: [PATCH 1/2] MdeModulePkg/AcpiTableDxe: condense whitespace around FADT.{DSDT, X_DSDT}

[Thomas Huth]

On 01.02.2017 18:56, Laszlo Ersek wrote:
> 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: 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>

Re: [PATCH 1/2] MdeModulePkg/AcpiTableDxe: condense whitespace around FADT.{DSDT, X_DSDT}

[Tian, Feng]

Reviewed-by: Feng Tian <feng.tian@intel.com>

Thanks
Feng

-----Original Message-----
From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Thursday, February 2, 2017 1:57 AM
To: edk2-devel-01 <edk2-devel@lists.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>; 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 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: 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>
---

Notes:
    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

[PATCH 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion

[Laszlo Ersek]

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)

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: 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>
---

Notes:
    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 | 66 +++++++++++++++++++-
 1 file changed, 63 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
index 7795ff7269ca..b2657f94a7bf 100644
--- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
+++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
@@ -430,6 +430,55 @@ 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.
+  //
+  // Check the FADT Major Version first (offset 8 decimal).
+  //
+  if (Fadt->Header.Revision < 5) {
+    return FALSE;
+  }
+  if (Fadt->Header.Revision > 5) {
+    return TRUE;
+  }
+  //
+  // For FADT Major Version 5, check the FADT Minor Version as well (offset 131
+  // decimal).
+  //
+  if (Fadt->Reserved2[2] < 1) {
+    return FALSE;
+  }
+  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 +696,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 +900,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

Re: [PATCH 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion

[Phil Dennis-Jordan]

Reviewed-by: Phil Dennis-Jordan <phil@philjordan.eu>

Thanks for writing the patch Laszlo! I've finally got around to testing it
out, and I can confirm that unlike previously the FADT it produces is
accepted by Windows 10, when Qemu provides a FADT with revision 3 or 4.
(Windows 10 appears to insist on both DSDT and X_DSDT. Issue with multiple
linker entries with same pointee notwithstanding.)

Phil


On Wed, Feb 1, 2017 at 6:56 PM, Laszlo Ersek <lersek@redhat.com> wrote:

> 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)
>
> 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: 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>
> ---
>
> Notes:
>     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 | 66
> +++++++++++++++++++-
>  1 file changed, 63 insertions(+), 3 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> index 7795ff7269ca..b2657f94a7bf 100644
> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> @@ -430,6 +430,55 @@ 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.
> +  //
> +  // Check the FADT Major Version first (offset 8 decimal).
> +  //
> +  if (Fadt->Header.Revision < 5) {
> +    return FALSE;
> +  }
> +  if (Fadt->Header.Revision > 5) {
> +    return TRUE;
> +  }
> +  //
> +  // For FADT Major Version 5, check the FADT Minor Version as well
> (offset 131
> +  // decimal).
> +  //
> +  if (Fadt->Reserved2[2] < 1) {
> +    return FALSE;
> +  }
> +  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 +696,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 +900,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
>

Re: [PATCH 0/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion

[Laszlo Ersek]

On 02/01/17 18:56, Laszlo Ersek wrote:
> Patch #2 explains it all.
> 
> Repo:   https://github.com/lersek/edk2/
> Branch: fadt_dsdt
> 
> NOTE for people on the CC list:
> 
> If you are not presently subscribed to edk2-devel and wish to comment on
> this series 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.
> 
> 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: 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>
> 
> 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 | 82 ++++++++++++++++----
>  1 file changed, 67 insertions(+), 15 deletions(-)
> 

Michael filed the related Mantis ticket #1757:

https://mantis.uefi.org/mantis/view.php?id=1757

Thanks
Laszlo

Re: [PATCH 0/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion

[Zeng, Star]

Laszlo,

I will give feedback tomorrow (PRC time) to this series, sorry for late.

Also Cc Jiewen.

Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Thursday, February 2, 2017 1:57 AM
To: edk2-devel-01 <edk2-devel@lists.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>; Al Stone <ahs3@redhat.com>; Zeng, Star <star.zeng@intel.com>
Subject: [edk2] [PATCH 0/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion

Patch #2 explains it all.

Repo:   https://github.com/lersek/edk2/
Branch: fadt_dsdt

NOTE for people on the CC list:

If you are not presently subscribed to edk2-devel and wish to comment on this series 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.

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: 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>

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 | 82 ++++++++++++++++----
 1 file changed, 67 insertions(+), 15 deletions(-)

--
2.9.3

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion

[Yao, Jiewen]

Hi Laszlo
I agree with you that the strange order issue and spec compliance issue need to be fixed.

I am OK on the final result in RequireDsdtXDsdtExclusion().

However, the logic below is a little hard to read for me, because I have to switch my mind between "return FALSE" and "return TRUE".
=========================================
+  if (Fadt->Header.Revision < 5) {
+    return FALSE;
+  }
+  if (Fadt->Header.Revision > 5) {
+    return TRUE;
+  }
+  //
+  // For FADT Major Version 5, check the FADT Minor Version as well (offset 131
+  // decimal).
+  //
+  if (Fadt->Reserved2[2] < 1) {
+    return FALSE;
+  }
+  return TRUE;
=========================================

How about we use a simpler logic? Such as below:
=========================================
+  // version <= 5.0
+  if ((Fadt->Header.Revision < 5) ||
+     ((Fadt->Header.Revision == 5) && (((EFI_ACPI_5_1_FIXED_ACPI_DESCRIPTION_TABLE *)Fadt)->MinorVersion == 0))) {
+    return FALSE;
+  }
+  // version >= 5.1
+  return TRUE;
=========================================

Thank you
Yao Jiewen




> -----Original Message-----
> From: Zeng, Star
> Sent: Tuesday, February 7, 2017 6:20 PM
> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel-01 <edk2-devel@lists.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>; Al Stone
> <ahs3@redhat.com>; Zeng, Star <star.zeng@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>
> Subject: RE: [edk2] [PATCH 0/2] MdeModulePkg/AcpiTableDxe: improve
> FADT.{DSDT, X_DSDT} mutual exclusion
> 
> Laszlo,
> 
> I will give feedback tomorrow (PRC time) to this series, sorry for late.
> 
> Also Cc Jiewen.
> 
> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo
> Ersek
> Sent: Thursday, February 2, 2017 1:57 AM
> To: edk2-devel-01 <edk2-devel@lists.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>; Al Stone
> <ahs3@redhat.com>; Zeng, Star <star.zeng@intel.com>
> Subject: [edk2] [PATCH 0/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT,
> X_DSDT} mutual exclusion
> 
> Patch #2 explains it all.
> 
> Repo:   https://github.com/lersek/edk2/
> Branch: fadt_dsdt
> 
> NOTE for people on the CC list:
> 
> If you are not presently subscribed to edk2-devel and wish to comment on this
> series 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.
> 
> 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: 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>
> 
> 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 | 82
> ++++++++++++++++----
>  1 file changed, 67 insertions(+), 15 deletions(-)
> 
> --
> 2.9.3
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel