From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (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 212918033C for ; Tue, 14 Mar 2017 00:56:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=intel.com; i=@intel.com; q=dns/txt; s=intel; t=1489478217; x=1521014217; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=MorkYSBGftEDc3TFr8uxJ2eds/ZEpw0+PeOVbBocqiU=; b=wWlcfCGFe8CjeOZVn5HceXo3iKwAFfeGNS6SdRaiFtVoC6aMsJJyW3UL iS8wHVmRcnCrvEBSKX07mt+S0/Tx2A==; Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Mar 2017 00:56:56 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,162,1486454400"; d="scan'208";a="67194301" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga004.jf.intel.com with ESMTP; 14 Mar 2017 00:56:56 -0700 Received: from fmsmsx101.amr.corp.intel.com (10.18.124.199) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 14 Mar 2017 00:56:56 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx101.amr.corp.intel.com (10.18.124.199) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 14 Mar 2017 00:56:55 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.88]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.132]) with mapi id 14.03.0248.002; Tue, 14 Mar 2017 15:56:53 +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: AQHSmEZzhxC7OWcAl0C1lSQ02xUihKGSGrqwgAA/IgCAAaWeEA== Date: Tue, 14 Mar 2017 07:56:53 +0000 Message-ID: <542CF652F8836A4AB8DBFAAD40ED192A4C55F3F4@shsmsx102.ccr.corp.intel.com> References: <20170308195839.18689-1-lersek@redhat.com> <20170308195839.18689-3-lersek@redhat.com> <542CF652F8836A4AB8DBFAAD40ED192A4C55D701@shsmsx102.ccr.corp.intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMmM1YjYxMjEtMTc4Ny00ZWE5LTljODYtOTE0ZGRjYzBlOTJjIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6Im9XWlY4QUVOWlc4SG4zNWU5YkVpMVwvd09nNU10TVNFQUxIdVwvSnBOTDhuND0ifQ== 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: Tue, 14 Mar 2017 07:56:57 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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]=20 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 Du= ran; 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, >=20 > We found one Windows Server 2012 R2 blue screen issue with ACPI 6.1 FADT = table. >=20 > We did the following configuration test with DSDT under 4GB. > .DSDT .X_DSDT Window Server 2012 R2 > ---------- ------------ ------------------------------- > set clear Failed // current implementat= ion > 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 prec= edence 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 sha= re with us a list of ACPI spec versions that should be exempted from Requir= eDsdtXDsdtExclusion() -- despite the spec clearly requiring DSDT / X_DSDT e= xclusion --, 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 d= on'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=20 > Laszlo Ersek > Sent: Thursday, March 09, 2017 3:59 AM > To: edk2-devel-01 > Cc: Tian, Feng; Michael Tsirkin; Ard Biesheuvel; Phil Dennis-Jordan;=20 > Leo Duran; Yao, Jiewen; Al Stone; Zeng, Star > Subject: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve=20 > FADT.{DSDT, X_DSDT} mutual exclusion >=20 > The ACPI specification, up to and including revision 5.1 Errata A,=20 > allows the DSDT and X_DSDT fields to be both set in the FADT.=20 > (Obviously, this only makes sense if the DSDT address is representable=20 > in 4 bytes.) >=20 > 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. >=20 > MdeModulePkg/AcpiTableDxe handles this mutual exclusion somewhat inconsis= tently. >=20 > - If the caller of EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() installs th= e > tables in "DSDT, FADT" order, then we enforce the exclusion between the > DSDT and X_DSDT fields: >=20 > DSDT under 4GB FADT.DSDT FADT.X_DSDT [VARIANT B] > -------------- --------- ----------- > yes set clear > no clear set >=20 > This behavior conforms to 5.1 Errata B. (And it's not required by > earlier versions of the spec.) >=20 > - If the caller passes in the tables in "FADT, DSDT" relative order, then > we do not enforce the exclusion: >=20 > DSDT under 4GB FADT.DSDT FADT.X_DSDT [VARIANT A] > -------------- --------- ----------- > yes set set > no clear set >=20 > This satisfies 5.1 Errata A and earlier, but breaks 5.1 Errata B and > later. >=20 > Unify the handling of both relative orders. In particular, check the majo= r and minor version numbers in the FADT. If the FADT version is strictly be= fore 5.1, then implement [VARIANT A]. If the FADT version is equal to or la= rger than 5.1, then implement [VARIANT B]. >=20 > We make three observations: >=20 > - 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"= . >=20 > - 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. >=20 > - 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). >=20 > 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. >=20 > Regression tested with the following KVM guests (QEMU built at ata0def594= 286d, "Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' int= o staging", 2017-01-30): >=20 > - 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) >=20 > This change is connected to ASWG ticket=20 > , which is now closed/= fixed. >=20 > 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 > --- >=20 > 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= 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 subscriptio= n > before reflecting messages from someone. > =20 > Subscribe at . Than= ks. >=20 > MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 62=20 > +++++++++++++++++++- > 1 file changed, 59 insertions(+), 3 deletions(-) >=20 > diff --git=20 > a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c=20 > 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=20 > +there exists > + an explicit requirement that at most one of those fields is=20 > +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_DSD= T. > + @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 = 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=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)->MinorVersio= n =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 FA= CS 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) AcpiTableIns= tance->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 (UI= NT64)); > } > + CopyMem (&AcpiTableInstance->Fadt3->XDsdt, &Buffer64, sizeof=20 > + (UINT64)); > =20 > // > // RSDP OEM information is updated to match the FADT OEM informati= on @@ -847,8 +896,15 @@ AddTableToList ( > if (AcpiTableInstance->Fadt3 !=3D NULL) { > if ((UINT64)(UINTN)AcpiTableInstance->Dsdt3 < BASE_4GB) { > AcpiTableInstance->Fadt3->Dsdt =3D (UINT32) (UINTN)=20 > AcpiTableInstance->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=20 > (UINT64)); > =20 > // > -- > 2.9.3 >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >=20