From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ua0-x22e.google.com (mail-ua0-x22e.google.com [IPv6:2607:f8b0:400c:c08::22e]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 567C38200D for ; Mon, 6 Feb 2017 07:36:26 -0800 (PST) Received: by mail-ua0-x22e.google.com with SMTP id 35so63341444uak.1 for ; Mon, 06 Feb 2017 07:36:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=philjordan-eu.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=V8Wl2qY4fbJY3hIEq1rFJm6PWktkus+mFIvgkj9p3K8=; b=pBTx8Ogr+qWCRx5boCNLyxv2mwr813jxV/jYdhALnbHPI0F8i3Zq7aLQj+so6muZY5 MknMdxjrpjXpv4BAqwuLeYKKhSfRJ0NkP1LWd7R8k5flh4PraqovQVd6wubmRx0QHv5E ZnKBADZ8fM0f7bqzJWVMU13u7/LGz85EbU7zeMrsX+UWlDHjYc2AMyCiD8aTPtD5JMMV LK1L57zE4alJOdXH5O4BBSSGOsfecDm8DybNq8YTZgyF9thxwnvoyFyHUM17++usBkqP 06BQ8uZV82xNauKGZA3yfx0B1LnE+2dRyChwZ7IigyrO/eLdOwbiZOZrlRwYPSgyQj0o ihYA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=V8Wl2qY4fbJY3hIEq1rFJm6PWktkus+mFIvgkj9p3K8=; b=FuFDE+NXAqUjxScUuN9y9tvwkbyt7dA2uHQRfPH77mGstpWmhBKWtA3AyxezajX/wz YADMoHrXK+GvDSF5Z52LRJL4vy3s0wdEaWj8esjfR3erPbjxkRyp+LSaBAxCaSR+P1VX JSmBBpGckMSaMUaNzxw0PM0qmcXOqQYIPllX6wMF5rX0dci8E6r5CwUHw4Y2O48vH5Qg nwEgNCrh4fWUM3KP9YOMvvLI9l52Q5TTbuNZVH+vDjizGTrNqznVtUcvkc9QNIv9ogtr 170dFUuY19hesKa3bLX4ggzUuLPhQw56um2ti/QRQzocMRXSah/jTUzEuJEKDNwuR3ul Rciw== X-Gm-Message-State: AIkVDXKbsajg9CmEETxnNd8H9oIDdeSeUgzwUkpupMGkx/k8FC3MsD1Kq0Mtc0D/6unWg7VEwBq3gLbNfA708A== X-Received: by 10.176.23.22 with SMTP id j22mr5213623uaf.168.1486395385335; Mon, 06 Feb 2017 07:36:25 -0800 (PST) MIME-Version: 1.0 Received: by 10.31.133.195 with HTTP; Mon, 6 Feb 2017 07:36:05 -0800 (PST) In-Reply-To: <20170201175654.19425-3-lersek@redhat.com> References: <20170201175654.19425-1-lersek@redhat.com> <20170201175654.19425-3-lersek@redhat.com> From: Phil Dennis-Jordan Date: Mon, 6 Feb 2017 16:36:05 +0100 Message-ID: To: Laszlo Ersek Cc: edk2-devel-01 , Feng Tian , Michael Tsirkin , Ard Biesheuvel , Phil Dennis-Jordan , Leo Duran , Al Stone , Star Zeng X-Content-Filtered-By: Mailman/MimeDel 2.1.21 Subject: Re: [PATCH 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 06 Feb 2017 15:36:26 -0000 Content-Type: text/plain; charset=UTF-8 Reviewed-by: Phil Dennis-Jordan 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 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 > , 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 > Cc: Ard Biesheuvel > Cc: Feng Tian > Cc: Igor Mammedov > Cc: Leo Duran > Cc: Michael Tsirkin > Cc: Phil Dennis-Jordan > Cc: Star Zeng > Reported-by: Phil Dennis-Jordan > Suggested-by: Igor Mammedov > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek > --- > > 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 . > 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 >