From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) (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 75DF4803D6 for ; Tue, 14 Mar 2017 01:33:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=intel.com; i=@intel.com; q=dns/txt; s=intel; t=1489480390; x=1521016390; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=J9Gp6EoghvKFwt+XvA/D+dn6N51JO7FBTkeBJOZ8xjA=; b=Gr8xcn7aGk7wSBalKtA7i1jiJ3V9/rL7I40D6WU2Nam5GP9LQBKen9xA yYwGDlJKMOPESMHBW9JSHSiXv5zx6Q==; Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 14 Mar 2017 01:33:09 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,162,1486454400"; d="scan'208";a="60042137" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga002.jf.intel.com with ESMTP; 14 Mar 2017 01:33:09 -0700 Received: from fmsmsx156.amr.corp.intel.com (10.18.116.74) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 14 Mar 2017 01:33:09 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by fmsmsx156.amr.corp.intel.com (10.18.116.74) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 14 Mar 2017 01:33:08 -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 16:33:07 +0800 From: "Zeng, Star" To: "Fan, Jeff" , 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: AQHSm6btYRSlkSIHxUiXHToTa1WpBqGSUxsAgAEgkYCAAIuM4A== Date: Tue, 14 Mar 2017 08:33:06 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103B833AAB@shsmsx102.ccr.corp.intel.com> References: <20170308195839.18689-1-lersek@redhat.com> <20170308195839.18689-3-lersek@redhat.com> <542CF652F8836A4AB8DBFAAD40ED192A4C55D701@shsmsx102.ccr.corp.intel.com> <542CF652F8836A4AB8DBFAAD40ED192A4C55F3F4@shsmsx102.ccr.corp.intel.com> In-Reply-To: <542CF652F8836A4AB8DBFAAD40ED192A4C55F3F4@shsmsx102.ccr.corp.intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: 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 08:33:10 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable In original code for < 4G table, Dsdt and XDsdt will be both assigned if FADT is installed before DSDT, but Dsdt and XDsdt will have mutual exclusion if FADT is installed after DSDT. They are inconsistent. Is there any negative impact found to assign both Dsdt and XDsdt for < 4G t= able except the spec volation? Thanks, Star -----Original Message----- From: Fan, Jeff=20 Sent: Tuesday, March 14, 2017 3:57 PM To: Laszlo Ersek ; edk2-devel-01 Cc: Tian, Feng ; Michael Tsirkin = ; Ard Biesheuvel ; Phil Dennis-Jordan ; Leo Duran ; Yao, Jiewen ; Al Stone ; Zeng, Star Subject: RE: [edk2] [PATCH v2 2/2] MdeModulePkg/AcpiTableDxe: improve FADT.= {DSDT, X_DSDT} mutual exclusion 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] 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. > (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 > +++++++++++++++++++- > 1 file changed, 59 insertions(+), 3 deletions(-) >=20 > 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=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) > 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