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 617BE80403 for ; Thu, 16 Mar 2017 04:59:19 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CFC70342C6D; Thu, 16 Mar 2017 11:59:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com CFC70342C6D Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com CFC70342C6D Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-73.phx2.redhat.com [10.3.116.73]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9B54A627DE; Thu, 16 Mar 2017 11:59:18 +0000 (UTC) To: Star Zeng , edk2-devel@lists.01.org References: <1489652229-8940-1-git-send-email-star.zeng@intel.com> Cc: Jeff Fan , Jiewen Yao From: Laszlo Ersek Message-ID: <1002c38c-8bdf-d51a-3bc4-b604178d1295@redhat.com> Date: Thu, 16 Mar 2017 12:59:13 +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: <1489652229-8940-1-git-send-email-star.zeng@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Thu, 16 Mar 2017 11:59:20 +0000 (UTC) Subject: Re: [PATCH] MdeModulePkg/AcpiTableDxe: Not make FADT.{DSDT, X_DSDT} mutual exclusion X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Mar 2017 11:59:19 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/16/17 09:17, Star Zeng wrote: > 198a46d768fb90d2f9b16e26451b4814e7469eaf improved the DSDT and X_DSDT > fields mutual exclusion by checking FADT revision, but that breaks > some OS that has assumption to only consume X_DSDT field even the > DSDT address is < 4G. > > To have better compatibility, this patch is to update the code to not > make FADT.{DSDT,X_DSDT} mutual exclusion, but always set both DSDT and > X_DSDT fields in the FADT when the DSDT address is < 4G. > > Cc: Laszlo Ersek > Cc: Jeff Fan > Cc: Jiewen Yao > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Star Zeng > > NOTE: This patch comes out from the discussion at > https://lists.01.org/pipermail/edk2-devel/2017-March/008580.html. > --- > .../Acpi/AcpiTableDxe/AcpiTableProtocol.c | 88 +++++++++------------- > 1 file changed, 34 insertions(+), 54 deletions(-) Reviewed-by: Laszlo Ersek Thank you! Laszlo > diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > index 4bb848df5203..a4fd9aff845d 100644 > --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > @@ -432,50 +432,6 @@ ReallocateAcpiTableBuffer ( > } > > /** > - 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. > > @@ -692,11 +648,23 @@ AddTableToList ( > } > 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; > - } > + // > + // Comment block "the caller installs the tables in "DSDT, FADT" order" > + // The below comments are also in "the caller installs the tables in "FADT, DSDT" order" comment block. > + // > + // 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 fields to be set to a nonzero value, > + // but strangely an exception is 6.0 that has no this requirement. > + // > + // Here we do not make the DSDT and X_DSDT fields mutual exclusion conditionally > + // by checking FADT revision, but always set both DSDT and X_DSDT fields in the FADT > + // to have better compatibility as some OS may have assumption to only consume X_DSDT > + // field even the DSDT address is < 4G. > + // > + Buffer64 = AcpiTableInstance->Fadt3->Dsdt; > } else { > AcpiTableInstance->Fadt3->Dsdt = 0; > Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3; > @@ -896,11 +864,23 @@ 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; > - } > + // > + // Comment block "the caller installs the tables in "FADT, DSDT" order" > + // The below comments are also in "the caller installs the tables in "DSDT, FADT" order" comment block. > + // > + // 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 fields to be set to a nonzero value, > + // but strangely an exception is 6.0 that has no this requirement. > + // > + // Here we do not make the DSDT and X_DSDT fields mutual exclusion conditionally > + // by checking FADT revision, but always set both DSDT and X_DSDT fields in the FADT > + // to have better compatibility as some OS may have assumption to only consume X_DSDT > + // field even the DSDT address is < 4G. > + // > + Buffer64 = AcpiTableInstance->Fadt3->Dsdt; > } else { > AcpiTableInstance->Fadt3->Dsdt = 0; > Buffer64 = (UINT64) (UINTN) AcpiTableInstance->Dsdt3; >