From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 0645A8029A for ; Sun, 12 Mar 2017 20:07:24 -0700 (PDT) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga105.jf.intel.com with ESMTP; 12 Mar 2017 20:07:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,156,1486454400"; d="scan'208";a="59446294" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga002.jf.intel.com with ESMTP; 12 Mar 2017 20:07:23 -0700 Received: from FMSMSX110.amr.corp.intel.com (10.18.116.10) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sun, 12 Mar 2017 20:07:23 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx110.amr.corp.intel.com (10.18.116.10) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sun, 12 Mar 2017 20:07:22 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.88]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.177]) with mapi id 14.03.0248.002; Mon, 13 Mar 2017 11:07:20 +0800 From: "Fan, Jeff" To: Laszlo Ersek , edk2-devel-01 CC: "Tian, Feng" , Michael Tsirkin , Ard Biesheuvel , Phil Dennis-Jordan , Leo Duran , "Yao, Jiewen" , Al Stone , "Zeng, Star" Thread-Topic: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSDT, X_DSDT} mutual exclusion Thread-Index: AQHSmEZzhxC7OWcAl0C1lSQ02xUihKGSGrqw Date: Mon, 13 Mar 2017 03:07:19 +0000 Message-ID: <542CF652F8836A4AB8DBFAAD40ED192A4C55D701@shsmsx102.ccr.corp.intel.com> References: <20170308195839.18689-1-lersek@redhat.com> <20170308195839.18689-3-lersek@redhat.com> In-Reply-To: <20170308195839.18689-3-lersek@redhat.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMDUyNzU2NGMtODU1Ny00YmI5LTkyYjctNmE0ZmFjMDU5YWJmIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6Ik5ZN3N1WEQzXC9LMlRielpxYzFidG9DY1MzclJiNHlsTUY0eVUxbytZbWFRPSJ9 x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH v2 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, 13 Mar 2017 03:07:24 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, We found one Windows Server 2012 R2 blue screen issue with ACPI 6.1 FADT ta= ble. We did the following configuration test with DSDT under 4GB. .DSDT .X_DSDT Window Server 2012 R2 ---------- ------------ ------------------------------- set clear Failed // current implementatio= n clear set Succeed set set Succeed Thanks! Jeff -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Lasz= lo Ersek Sent: Thursday, March 09, 2017 3:59 AM To: edk2-devel-01 Cc: Tian, Feng; Michael Tsirkin; Ard Biesheuvel; Phil Dennis-Jordan; Leo Du= ran; Yao, Jiewen; Al Stone; Zeng, Star Subject: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.{DSD= T, X_DSDT} mutual exclusion The ACPI specification, up to and including revision 5.1 Errata A, allows t= he 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 an= d X_DSDT to be set to a nonzero value. MdeModulePkg/AcpiTableDxe handles this mutual exclusion somewhat inconsiste= ntly. - 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 befo= re 5.1, then implement [VARIANT A]. If the FADT version is equal to or larg= er 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 ata0def59428= 6d, "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 , which is now closed/fi= xed. Cc: Al Stone Cc: Ard Biesheuvel Cc: Feng Tian Cc: Igor Mammedov Cc: Jiewen Yao 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 Reviewed-by: Phil Dennis-Jordan --- 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 =20 v1: NOTE for people on the CC list: =20 If you are not presently subscribed to edk2-devel and wish to comment o= n 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. =20 Subscribe at . 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 =3D NewMaxTableNumber; return EFI_SUCCESS; } + +/** + Determine whether the FADT table passed in as parameter requires=20 +mutual + exclusion between the DSDT and X_DSDT fields. (That is, whether there=20 +exists + an explicit requirement that at most one of those fields is permitted=20 +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.=20 +Unfortunately, we + // can't tell apart 5.1 Errata A and 5.1 Errata B just from looking=20 +at the + // FADT table. Therefore let's require exclusion for table versions >=3D= 5.1. + // + // While this needlessly covers 5.1 and 5.1A too, it is safer to=20 +require + // DSDT<->X_DSDT exclusion for lax (5.1, 5.1A) versions of the spec=20 +than to + // permit DSDT<->X_DSDT duplication for strict (5.1B) versions of the sp= ec. + // + // 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=20 +6.0A + // based on just the FADT, we lump 6.0 in with the rest of >=3D 5.1. + // + if ((Fadt->Header.Revision < 5) || + ((Fadt->Header.Revision =3D=3D 5) && + (((EFI_ACPI_5_1_FIXED_ACPI_DESCRIPTION_TABLE *)Fadt)->MinorVersion = =3D=3D 0))) { + // + // version <=3D 5.0 + // + return FALSE; + } + // + // version >=3D 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 =3D (UINT32) (UINTN) AcpiTableInsta= nce->Dsdt3; - ZeroMem (&AcpiTableInstance->Fadt3->XDsdt, sizeof (UINT64)); + if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) { + Buffer64 =3D 0; + } else { + Buffer64 =3D AcpiTableInstance->Fadt3->Dsdt; + } } else { AcpiTableInstance->Fadt3->Dsdt =3D 0; Buffer64 =3D (UINT64) (UINTN) AcpiTableInstance->Dsdt3; - CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof (UINT= 64)); } + CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof=20 + (UINT64)); =20 // // RSDP OEM information is updated to match the FADT OEM information= @@ -847,8 +896,15 @@ AddTableToList ( if (AcpiTableInstance->Fadt3 !=3D NULL) { if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) { AcpiTableInstance->Fadt3->Dsdt =3D (UINT32) (UINTN) AcpiTableIns= tance->Dsdt3; + if (RequireDsdtXDsdtExclusion (AcpiTableInstance->Fadt3)) { + Buffer64 =3D 0; + } else { + Buffer64 =3D AcpiTableInstance->Fadt3->Dsdt; + } + } else { + AcpiTableInstance->Fadt3->Dsdt =3D 0; + Buffer64 =3D (UINT64) (UINTN) AcpiTableInstance->Dsdt3; } - Buffer64 =3D (UINT64) (UINTN) AcpiTableInstance->Dsdt3; CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof (UINT= 64)); =20 // -- 2.9.3 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel