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

["Yao, Jiewen" <jiewen.yao@intel.com>]

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