From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (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 B5E9C803D1 for ; Mon, 20 Mar 2017 02:57:12 -0700 (PDT) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga104.jf.intel.com with ESMTP; 20 Mar 2017 02:57:12 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,193,1486454400"; d="scan'208";a="1110274862" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by orsmga001.jf.intel.com with ESMTP; 20 Mar 2017 02:57:12 -0700 Received: from fmsmsx125.amr.corp.intel.com (10.18.125.40) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 20 Mar 2017 02:57:12 -0700 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by FMSMSX125.amr.corp.intel.com (10.18.125.40) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 20 Mar 2017 02:57:11 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.88]) by SHSMSX104.ccr.corp.intel.com ([10.239.4.70]) with mapi id 14.03.0248.002; Mon, 20 Mar 2017 17:57:09 +0800 From: "Zeng, Star" To: Laszlo Ersek , edk2-devel-01 , "Kinney, Michael D" , "Yao, Jiewen" CC: Leif Lindholm , Ard Biesheuvel Thread-Topic: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library Thread-Index: AQHSn1/Ct9FsN6en9E+84QMKKTQOM6GdAZxQ///vKwCAAItusA== Date: Mon, 20 Mar 2017 09:57:08 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103B8360CA@shsmsx102.ccr.corp.intel.com> References: <20170317204731.31488-1-lersek@redhat.com> <20170317204731.31488-6-lersek@redhat.com> <0C09AFA07DD0434D9E2A0C6AEB0483103B835B4B@shsmsx102.ccr.corp.intel.com> In-Reply-To: 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 05/12] ArmPkg: introduce EDKII Platform Has ACPI Protocol, and plug-in library 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: Mon, 20 Mar 2017 09:57:12 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Oh got it, returning unsuccessfully in constructor of library does not work= . :( For (1), I definitely have no any comments. :) For (2), I got no strong reason to support or reject it. We'd better could = get comments from Mike and Jiewen. Thanks, Star -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Lasz= lo Ersek Sent: Monday, March 20, 2017 5:18 PM To: Zeng, Star ; edk2-devel-01 Cc: Leif Lindholm ; Ard Biesheuvel Subject: Re: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has A= CPI Protocol, and plug-in library On 03/20/17 03:23, Zeng, Star wrote: > Laszlo, > > Could the implementation have multiple instances of PlatformHasAcpiLib=20 > to return unsuccessfully in the constructor when=20 > EFI_ACPI_TABLE_PROTOCOL is not needed? And then eliminate=20 > PlatformHasAcpiDtDxe, gEdkiiPlatformHasAcpiProtocolGuid and=20 > gEdkiiPlatformHasDeviceTreeProtocolGuid? If a library constructor fails, then the client module does not see an expl= icit error (or automatic unloading); instead, an assertion failure is trigg= ered in auto-generated code. For example, in the "Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/MdeModulePkg/Universal/Acpi/A= cpiTableDxe/AcpiTableDxe/DEBUG/AutoGen.c" file, with this series applied, we find: > VOID > EFIAPI > ProcessLibraryConstructorList ( > IN EFI_HANDLE ImageHandle, > IN EFI_SYSTEM_TABLE *SystemTable > ) > { > EFI_STATUS Status; > > Status =3D HobLibConstructor (ImageHandle, SystemTable); > ASSERT_EFI_ERROR (Status); > > Status =3D FdtPL011SerialPortLibInitialize (); > ASSERT_EFI_ERROR (Status); > > Status =3D BaseDebugLibSerialPortConstructor (); > ASSERT_EFI_ERROR (Status); > > Status =3D UefiBootServicesTableLibConstructor (ImageHandle, SystemTabl= e); > ASSERT_EFI_ERROR (Status); > > Status =3D UefiRuntimeServicesTableLibConstructor (ImageHandle, SystemT= able); > ASSERT_EFI_ERROR (Status); > > Status =3D UefiLibConstructor (ImageHandle, SystemTable); > ASSERT_EFI_ERROR (Status); > > Status =3D PlatformHasAcpiInitialize (); <--------------- constructor o= f PlatformHasAcpiLib plug-in lib > ASSERT_EFI_ERROR (Status); > > } What we could do instead would be a new library class, with an explicit ini= tialization function. However, for that to work, AcpiTableDxe would have to= be extended with a new library class dependency and a call to the init fun= ction. Additionally, out-of-tree platform DSC files would have to resolve t= his library class to the default instance. Introducing a boolean dynamic PCD for the same would be much simpler then; = the DEC default would cover out-of-tree platform DSCs without any change in= behavior. In summary, there are two alternatives in my opinion: (1) If we prefer not to modify MdeModulePkg: stick with this series. The on= ly update I'd see justified would be to move gEdkiiPlatformHasDeviceTreePro= tocolGuid to EmbeddedPkg as Leif requested. (2) If we are willing to modify MdeModulePkg for this problem: introduce a = new dynamic PCD such as "PcdAcpiTableEnable", and make AcpiTableDxe exit ea= rly if the PCD is FALSE. Thanks, Laszlo > > Thanks, > Star > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of=20 > Laszlo Ersek > Sent: Saturday, March 18, 2017 4:47 AM > To: edk2-devel-01 > Cc: Leif Lindholm ; Ard Biesheuvel=20 > > Subject: [edk2] [PATCH v2 05/12] ArmPkg: introduce EDKII Platform Has=20 > ACPI Protocol, and plug-in library > > The presence of this protocol in the DXE protocol database implies that t= he platform provides the operating system with an ACPI-based hardware descr= iption. This is not necessarily mutually exclusive with a Device Tree-based= hardware description. A platform driver is supposed to produce a single in= stance of the protocol (with NULL contents), if appropriate. > > The decision to produce the protocol is platform specific; for example, i= t could depend on an HII checkbox / underlying non-volatile UEFI variable. > > The protocol is meant to be consumed by=20 > "MdeModulePkg/Universal/Acpi/AcpiTableDxe", through the PlatformHasAcpiLi= b plug-in. By linking this library into AcpiTableDxe via NULL resolution in= the platform DSC, the platform makes EFI_ACPI_TABLE_PROTOCOL and (if enabl= ed) EFI_ACPI_SDT_PROTOCOL dependent on the above dynamic decision. > > In turn, other (platform and universal) DXE drivers that produce ACPI tab= les will wait for EFI_ACPI_TABLE_PROTOCOL / EFI_ACPI_SDT_PROTOCOL, via DEPE= X, protocol notify, or a simple gBS->LocateProtocol() in a "late enough" ca= llback (such as Ready To Boot). > > Because this protocol is not standard, it is prefixed with EDKII / Edkii,= as seen elsewhere in MdeModulePkg and SecurityPkg, for example. (ARM / Arm= doesn't look future-proof enough; future UEFI platforms could face the sam= e issue.) In addition, an effort is made to avoid the phrase "AcpiPlatform"= , as that belongs to drivers / libraries that produce platform specific ACP= I content (as opposed to deciding whether the entire firmware will have acc= ess to EFI_ACPI_TABLE_PROTOCOL). > > Cc: Ard Biesheuvel > Cc: Leif Lindholm > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek > --- > ArmPkg/ArmPkg.dec | 4 ++ > ArmPkg/Library/PlatformHasAcpiLib/PlatformHasAcpiLib.inf | 40 ++++++++++= ++++++++++ > ArmPkg/Include/Protocol/PlatformHasAcpi.h | 34 ++++++++++= +++++++ > ArmPkg/Library/PlatformHasAcpiLib/PlatformHasAcpiLib.c | 36 ++++++++++= ++++++++ > 4 files changed, 114 insertions(+) > > diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec index=20 > c4b4da2f95bb..0e49360a386a 100644 > --- a/ArmPkg/ArmPkg.dec > +++ b/ArmPkg/ArmPkg.dec > @@ -52,6 +52,10 @@ [Ppis] > ## Include/Ppi/ArmMpCoreInfo.h > gArmMpCoreInfoPpiGuid =3D { 0x6847cc74, 0xe9ec, 0x4f8f, {0xa2, 0x9d,=20 > 0xab, 0x44, 0xe7, 0x54, 0xa8, 0xfc} } > > +[Protocols] > + ## Include/Protocol/PlatformHasAcpi.h > + gEdkiiPlatformHasAcpiProtocolGuid =3D { 0xf0966b41, 0xc23f, 0x41= b9, { 0x96, 0x04, 0x0f, 0xf7, 0xe1, 0x11, 0x96, 0x5a } } > + > [PcdsFeatureFlag.common] > =20 > gArmTokenSpaceGuid.PcdCpuDxeProduceDebugSupport|FALSE|BOOLEAN|0x000000 > 01 > > diff --git a/ArmPkg/Library/PlatformHasAcpiLib/PlatformHasAcpiLib.inf=20 > b/ArmPkg/Library/PlatformHasAcpiLib/PlatformHasAcpiLib.inf > new file mode 100644 > index 000000000000..c83da4d8e98a > --- /dev/null > +++ b/ArmPkg/Library/PlatformHasAcpiLib/PlatformHasAcpiLib.inf > @@ -0,0 +1,40 @@ > +## @file > +# A hook-in library for MdeModulePkg/Universal/Acpi/AcpiTableDxe. > +# > +# Plugging this library instance into AcpiTableDxe makes #=20 > +EFI_ACPI_TABLE_PROTOCOL and (if enabled) EFI_ACPI_SDT_PROTOCOL depend=20 > +on the # platform's dynamic decision whether to expose an ACPI-based=20 > +hardware # description to the operating system. > +# > +# Universal and platform specific DXE drivers that produce ACPI=20 > +tables depend # on EFI_ACPI_TABLE_PROTOCOL / EFI_ACPI_SDT_PROTOCOL in tu= rn. > +# > +# Copyright (C) 2017, Red Hat, Inc. > +# > +# This program and the accompanying materials are licensed and made=20 > +available # under the terms and conditions of the BSD License which=20 > +accompanies this # distribution. The full text of the license may be=20 > +found at # http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"=20 > +BASIS, WITHOUT # WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRE= SS OR IMPLIED. > +## > + > +[Defines] > + INF_VERSION =3D 1.25 > + BASE_NAME =3D PlatformHasAcpiLib > + FILE_GUID =3D 29beb028-0958-447b-be0a-12229235d77= d > + MODULE_TYPE =3D BASE > + VERSION_STRING =3D 1.0 > + LIBRARY_CLASS =3D PlatformHasAcpiLib|DXE_DRIVER > + CONSTRUCTOR =3D PlatformHasAcpiInitialize > + > +[Sources] > + PlatformHasAcpiLib.c > + > +[Packages] > + ArmPkg/ArmPkg.dec > + MdePkg/MdePkg.dec > + > +[Depex] > + gEdkiiPlatformHasAcpiProtocolGuid > diff --git a/ArmPkg/Include/Protocol/PlatformHasAcpi.h=20 > b/ArmPkg/Include/Protocol/PlatformHasAcpi.h > new file mode 100644 > index 000000000000..3cd0cfe4515d > --- /dev/null > +++ b/ArmPkg/Include/Protocol/PlatformHasAcpi.h > @@ -0,0 +1,34 @@ > +/** @file > + EDKII Platform Has ACPI Protocol > + > + The presence of this protocol in the DXE protocol database implies=20 > + that the platform provides the operating system with an ACPI-based=20 > + hardware description. Note that this is not necessarily mutually=20 > + exclusive with a Device Tree-based hardware description. A platform=20 > + driver is supposed to produce a single instance of the protocol=20 > + (with NULL contents), if appropriate. > + > + Copyright (C) 2017, Red Hat, Inc. > + > + This program and the accompanying materials are licensed and made=20 > + available under the terms and conditions of the BSD License that=20 > + accompanies this distribution. The full text of the license may be=20 > + found at http://opensource.org/licenses/bsd-license.php. > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"=20 > +BASIS, WITHOUT > + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +**/ > + > + > +#ifndef __EDKII_PLATFORM_HAS_ACPI_PROTOCOL_H__ > +#define __EDKII_PLATFORM_HAS_ACPI_PROTOCOL_H__ > + > +#define EDKII_PLATFORM_HAS_ACPI_PROTOCOL_GUID \ > + { \ > + 0xf0966b41, 0xc23f, 0x41b9, \ > + { 0x96, 0x04, 0x0f, 0xf7, 0xe1, 0x11, 0x96, 0x5a } \ > + } > + > +extern EFI_GUID gEdkiiPlatformHasAcpiProtocolGuid; > + > +#endif > diff --git a/ArmPkg/Library/PlatformHasAcpiLib/PlatformHasAcpiLib.c=20 > b/ArmPkg/Library/PlatformHasAcpiLib/PlatformHasAcpiLib.c > new file mode 100644 > index 000000000000..79072da21c2b > --- /dev/null > +++ b/ArmPkg/Library/PlatformHasAcpiLib/PlatformHasAcpiLib.c > @@ -0,0 +1,36 @@ > +/** @file > + A hook-in library for MdeModulePkg/Universal/Acpi/AcpiTableDxe. > + > + Plugging this library instance into AcpiTableDxe makes=20 > + EFI_ACPI_TABLE_PROTOCOL and (if enabled) EFI_ACPI_SDT_PROTOCOL=20 > + depend on the platform's dynamic decision whether to expose an=20 > + ACPI-based hardware description to the operating system. > + > + Universal and platform specific DXE drivers that produce ACPI=20 > + tables depend on EFI_ACPI_TABLE_PROTOCOL / EFI_ACPI_SDT_PROTOCOL in tu= rn. > + > + Copyright (C) 2017, Red Hat, Inc. > + > + This program and the accompanying materials are licensed and made=20 > + available under the terms and conditions of the BSD License which=20 > + accompanies this distribution. The full text of the license may be=20 > + found at http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS"=20 > +BASIS, WITHOUT > + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > +**/ > + > +#include > + > +RETURN_STATUS > +EFIAPI > +PlatformHasAcpiInitialize ( > + VOID > + ) > +{ > + // > + // Do nothing, just imbue AcpiTableDxe with an > + // EDKII_PLATFORM_HAS_ACPI_PROTOCOL dependency. > + // > + return RETURN_SUCCESS; > +} > -- > 2.9.3 > > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel