From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 1A4E6803A2 for ; Wed, 15 Mar 2017 08:10:42 -0700 (PDT) Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 74296811A7; Wed, 15 Mar 2017 15:10:42 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 74296811A7 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 74296811A7 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-55.phx2.redhat.com [10.3.117.55]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id v2FFAdYx017274; Wed, 15 Mar 2017 11:10:39 -0400 To: "Zeng, Star" , "Fan, Jeff" , edk2-devel-01 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> <0C09AFA07DD0434D9E2A0C6AEB0483103B833AAB@shsmsx102.ccr.corp.intel.com> <58d77a24-5548-2ba8-a1ca-30d37714d15d@redhat.com> Cc: "Tian, Feng" , Michael Tsirkin , Ard Biesheuvel , Phil Dennis-Jordan , "Yao, Jiewen" , Leo Duran , Al Stone From: Laszlo Ersek Message-ID: Date: Wed, 15 Mar 2017 16:10:37 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Wed, 15 Mar 2017 15:10:42 +0000 (UTC) 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: Wed, 15 Mar 2017 15:10:42 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/15/17 02:22, Zeng, Star wrote: > On 2017/3/14 21:13, Laszlo Ersek wrote: >> On 03/14/17 09:33, Zeng, Star wrote: >>> 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. >> >> That's right. The revert would not solve this original problem. >> >>> >>> Is there any negative impact found to assign both Dsdt and XDsdt for >>> < 4G table except the spec volation? >> >> No, I don't think so; the spec violation is the only impact to my >> knowledge. >> >> Are you suggesting that we: >> - delete RequireDsdtXDsdtExclusion(), >> - replace all invocations of RequireDsdtXDsdtExclusion() with FALSE, >> - and then simplify the resultant code (that is, eliminate the >> always-FALSE branches, and un-indent the always-TRUE branches)? >> >> This would be [VARIANT A], regardless of ACPI spec version. >> >> I think that could work (as long as we don't mind breaking the ACPI spec >> versions that require mutual exclusion). > > Yes. Then the code could have maximum compatibility and the behavior > could be consistent, sure more comments about code behavior and ACPI > spec could be added. Do you want me to submit the patch, or can one of you guys do it? I'm happy to review the patch, but I would be slower to post it. Thanks! Laszlo > > Thanks, > Star > >> >> Thanks >> Laszlo >> >>> >>> Thanks, >>> Star >>> -----Original Message----- >>> From: Fan, Jeff >>> 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 Duran; 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, >>>> >>>> We found one Windows Server 2012 R2 blue screen issue with ACPI 6.1 >>>> FADT table. >>>> >>>> We did the following configuration test with DSDT under 4GB. >>>> .DSDT .X_DSDT Window Server 2012 R2 >>>> ---------- ------------ ------------------------------- >>>> set clear Failed // current >>>> implementation >>>> 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 precedence 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 share with us a list of ACPI spec versions that should be >>> exempted from RequireDsdtXDsdtExclusion() -- despite the spec clearly >>> requiring DSDT / X_DSDT exclusion --, 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 don'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 >>>> Laszlo Ersek >>>> Sent: Thursday, March 09, 2017 3:59 AM >>>> To: edk2-devel-01 >>>> Cc: Tian, Feng; Michael Tsirkin; Ard Biesheuvel; Phil Dennis-Jordan; >>>> Leo Duran; Yao, Jiewen; Al Stone; Zeng, Star >>>> Subject: [edk2] [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 >>>> , 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 >>>> , which is now >>>> closed/fixed. >>>> >>>> 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 >>>> >>>> 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 . >>>> 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 >>>> >>>> _______________________________________________ >>>> edk2-devel mailing list >>>> edk2-devel@lists.01.org >>>> https://lists.01.org/mailman/listinfo/edk2-devel >>>> >>> >