From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.100; helo=mga07.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (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 220CD20348634 for ; Wed, 9 May 2018 20:27:33 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 May 2018 20:27:33 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,383,1520924400"; d="scan'208";a="39798432" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga007.jf.intel.com with ESMTP; 09 May 2018 20:27:33 -0700 Received: from shsmsx152.ccr.corp.intel.com (10.239.6.52) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 9 May 2018 20:27:32 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.240]) by SHSMSX152.ccr.corp.intel.com ([169.254.6.6]) with mapi id 14.03.0319.002; Thu, 10 May 2018 11:27:31 +0800 From: "Ni, Ruiyu" To: Marvin H?user , "edk2-devel@lists.01.org" CC: "Dong, Eric" , "Yao, Jiewen" , "Kinney, Michael D" , Laszlo Ersek , "Zeng, Star" Thread-Topic: [PATCH 1/2] MdeModulePkg: Add PlatformAcpiLib LibraryClass. Thread-Index: AQHT5yPk+OtHJaVgR0WrYxIFBUV3I6QnHwkAgABm62CAAMhq8A== Date: Thu, 10 May 2018 03:27:30 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5BC8DD39@SHSMSX104.ccr.corp.intel.com> References: <0C09AFA07DD0434D9E2A0C6AEB0483103BAE9AE2@shsmsx102.ccr.corp.intel.com> In-Reply-To: Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH 1/2] MdeModulePkg: Add PlatformAcpiLib LibraryClass. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 May 2018 03:27:34 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable I don't see much needs for a PlatformAcpiLib. Platform can call AcpiTable protocol to install ACPI table itself. The PlatformAcpiLib adds additional program interfaces but doesn't seem to = be very helpful. Any new program interfaces need to be carefully reviewed and only added whe= n necessary. Otherwise, people new to this project would be lost in many interfaces. Thanks/Ray > -----Original Message----- > From: edk2-devel On Behalf Of Marvin > H?user > Sent: Wednesday, May 9, 2018 11:38 PM > To: edk2-devel@lists.01.org > Cc: Ni, Ruiyu ; Dong, Eric ; Yao= , > Jiewen ; Kinney, Michael D > ; Laszlo Ersek ; Zeng, > Star > Subject: Re: [edk2] [PATCH 1/2] MdeModulePkg: Add PlatformAcpiLib > LibraryClass. >=20 > I continued to CC all people that were CC'd in Star's mail, even if I am = not sure > if those are interested in this patch. If anyone doesn't want to be CC'd,= I will > drop them of course. >=20 > Hey Star, >=20 > Indeed I expected it to be just for reference, however as most of the dri= vers > you linked share the FV-loading code with AcpiPlatformDxe, having it > "mainlined" makes sense to me. > Especially as the "Intel platform forks" of this driver seem to all have = a > common memory leak which the sample driver does not have: > https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal > /Acpi/AcpiPlatformDxe/AcpiPlatform.c#L244 > If they were using the mainlined driver, the fix could be made once for > MdeModulePkg and all other platforms would profit. > Furthermore, I think it is a good idea to have the platform code not rely= on > where the tables come from (here FV), but instead just be requested to > perform the required updates, not having to care about table origin, > checksum fixup, etc. > If I am not mistaken, all the additional platform code seen in those driv= ers > can be implemented well via a library constructor. >=20 > Maybe the driver deserves a new name though, I'm not sure whether > "AcpiPlatformDxe" is actually fitting. > MinPlatform uses the "AcpiPlatform" name to generate fundamental ACPI > tables, while "BoardAcpiDxe" is the "AcpiPlatformDxe" + "PlatformAcpiLib" > equivalent, if I am not mistaken. >=20 > Thanks in advance. >=20 > Best regards, > Marvin >=20 > > -----Original Message----- > > From: Zeng, Star > > Sent: Wednesday, May 9, 2018 11:33 AM > > To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org > > Cc: Yao, Jiewen ; Kinney, Michael D > > ; Laszlo Ersek (lersek@redhat.com) > > ; Ni, Ruiyu ; Dong, Eric > > ; Zeng, Star > > Subject: RE: [PATCH 1/2] MdeModulePkg: Add PlatformAcpiLib LibraryClass= . > > > > Marvin, > > > > It is good direction to reuse more code. > > What am I concerning about are > > 1. There are comments "Sample ACPI Platform Driver" in > > AcpiPlatformDxe.inf and AcpiPlatformDxe.c, the current position of > > AcpiPlatformDxe seems just for reference. > > 2. Almost all the platforms have their own AcpiPlatformDxe, for > > example > > > https://github.com/tianocore/edk2/tree/master/OvmfPkg/AcpiPlatformDxe > > > https://github.com/tianocore/edk2/tree/master/QuarkPlatformPkg/Acpi/Dx > > e/AcpiPlatform > > https://github.com/tianocore/edk2/tree/master/Vlv2TbltDevicePkg/AcpiPl > > a > > tform > > ... > > > > Should more evaluation be done on them to see whether we can do more > > or we just suggest platform should implement its own AcpiPlatformDxe? > > > > Including more experts. > > > > > > Thanks, > > Star > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > Marvin H?user > > Sent: Wednesday, May 9, 2018 7:26 AM > > To: edk2-devel@lists.01.org > > Cc: Ni, Ruiyu ; Dong, Eric ; > > Zeng, Star > > Subject: [edk2] [PATCH 1/2] MdeModulePkg: Add PlatformAcpiLib > > LibraryClass. > > > > PlatformAcpiLib can be consumed by the generic ACPI Platform driver to > > allow platform specific updates to the ACPI tables loaded from the > > configured Firmware Volume. This allows for more platforms to > > incorporate the generic ACPI Platform driver. > > > > This commit also provides a NULL implementation of PlatformAcpiLib. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Marvin Haeuser > > --- > > > > > MdeModulePkg/Library/BasePlatformAcpiLibNull/BasePlatformAcpiLibNull.c > > | 36 ++++++++++++++++++++ > > MdeModulePkg/Include/Library/PlatformAcpiLib.h = | 36 > > ++++++++++++++++++++ > > > > > MdeModulePkg/Library/BasePlatformAcpiLibNull/BasePlatformAcpiLibNull.i > > nf | 35 +++++++++++++++++++ > > > MdeModulePkg/Library/BasePlatformAcpiLibNull/BasePlatformAcpiLibNull.u > > ni | 18 ++++++++++ > > MdeModulePkg/MdeModulePkg.dec = | 4 +++ > > MdeModulePkg/MdeModulePkg.dsc = | 2 ++ > > 6 files changed, 131 insertions(+) > > > > diff --git > > > a/MdeModulePkg/Library/BasePlatformAcpiLibNull/BasePlatformAcpiLibNull > > .c > > > b/MdeModulePkg/Library/BasePlatformAcpiLibNull/BasePlatformAcpiLibNull > > .c > > new file mode 100644 > > index 000000000000..5d5d0d051e1b > > --- /dev/null > > +++ > > > b/MdeModulePkg/Library/BasePlatformAcpiLibNull/BasePlatformAcpiLibNu > > +++ ll.c > > @@ -0,0 +1,36 @@ > > +/** @file > > + Null Platform ACPI Library instance. > > + > > +Copyright (c) 2018, Intel Corporation. All rights reserved.
This > > +program and the accompanying materials are licensed and made > > +available under the terms and conditions of the BSD License that > > +accompanies this > > distribution. > > +The full text of the license may be found at > > +http://opensource.org/licenses/bsd-license.php. > > + > > +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" > > BASIS, > > +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER > > EXPRESS OR IMPLIED. > > + > > +**/ > > + > > +#include > > + > > +#include > > + > > +/** > > + Performs platform specific updates to CurrentTable. > > + > > + @param[in,out] CurrentTable The table to perform updates on. > > + > > + @retval RETURN_SUCCESS The platform specific ACPI table updates > > + were > > applied > > + successfully. > > + @retval other The platform specific ACPI table updates cou= ld not be > > + applied. > > + > > +**/ > > +RETURN_STATUS > > +PlatformAcpiUpdateTable ( > > + IN OUT EFI_ACPI_COMMON_HEADER *CurrentTable > > + ) > > +{ > > + return RETURN_SUCCESS; > > +} > > diff --git a/MdeModulePkg/Include/Library/PlatformAcpiLib.h > > b/MdeModulePkg/Include/Library/PlatformAcpiLib.h > > new file mode 100644 > > index 000000000000..a3e367f3ab61 > > --- /dev/null > > +++ b/MdeModulePkg/Include/Library/PlatformAcpiLib.h > > @@ -0,0 +1,36 @@ > > +/** @file > > + Platform ACPI library. Platform can provide an implementation of > > +this > > + library class to provide an ACPI table update routine that may be > > +required > > + for some type of platform initialization. > > + > > +Copyright (c) 2018, Intel Corporation. All rights reserved.
This > > +program and the accompanying materials are licensed and made > > +available under the terms and conditions of the BSD License that > > +accompanies this > > distribution. > > +The full text of the license may be found at > > +http://opensource.org/licenses/bsd-license.php. > > + > > +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" > > BASIS, > > +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER > > EXPRESS OR IMPLIED. > > + > > +**/ > > + > > +#ifndef __PLATFORM_ACPI_LIB__ > > +#define __PLATFORM_ACPI_LIB__ > > + > > +/** > > + Performs platform specific updates to CurrentTable. > > + > > + @param[in,out] CurrentTable The table to perform updates on. > > + > > + @retval RETURN_SUCCESS The platform specific ACPI table updates > > + were > > applied > > + successfully. > > + @retval other The platform specific ACPI table updates cou= ld not be > > + applied. > > + > > +**/ > > +RETURN_STATUS > > +PlatformAcpiUpdateTable ( > > + IN OUT EFI_ACPI_COMMON_HEADER *CurrentTable > > + ); > > + > > +#endif // __PLATFORM_ACPI_LIB__ > > diff --git > > > a/MdeModulePkg/Library/BasePlatformAcpiLibNull/BasePlatformAcpiLibNull > > .inf > > > b/MdeModulePkg/Library/BasePlatformAcpiLibNull/BasePlatformAcpiLibNull > > .inf > > new file mode 100644 > > index 000000000000..a548ecdd91a7 > > --- /dev/null > > +++ > > > b/MdeModulePkg/Library/BasePlatformAcpiLibNull/BasePlatformAcpiLibNu > > +++ ll.inf > > @@ -0,0 +1,35 @@ > > +## @file > > +# Null Platform ACPI Library instance. > > +# > > +# Copyright (c) 2018, Intel Corporation. All rights reserved.
# > > +# This program and the accompanying materials # are licensed and > > +made available under the terms and conditions of the BSD License # > > +which accompanies this distribution. The full text of the license may > > +be found at # http://opensource.org/licenses/bsd-license.php > > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" > > +BASIS, # WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, > > EITHER EXPRESS OR IMPLIED. > > +# > > +## > > + > > +[Defines] > > + INF_VERSION =3D 0x00010005 > > + BASE_NAME =3D BasePlatformAcpiLibNull > > + MODULE_UNI_FILE =3D BasePlatformAcpiLibNull.uni > > + FILE_GUID =3D 0957C6BA-0559-46E8-8180-CD0317F0A= EE7 > > + MODULE_TYPE =3D BASE > > + VERSION_STRING =3D 1.0 > > + LIBRARY_CLASS =3D PlatformAcpiLib > > + > > +# > > +# The following information is for reference only and not required by > > +the > > build tools. > > +# > > +# VALID_ARCHITECTURES =3D IA32 X64 IPF EBC > > +# > > + > > +[Sources] > > + BasePlatformAcpiLibNull.c > > + > > +[Packages] > > + MdePkg/MdePkg.dec > > + MdeModulePkg/MdeModulePkg.dec > > diff --git > > > a/MdeModulePkg/Library/BasePlatformAcpiLibNull/BasePlatformAcpiLibNull > > .uni > > > b/MdeModulePkg/Library/BasePlatformAcpiLibNull/BasePlatformAcpiLibNull > > .uni > > new file mode 100644 > > index 000000000000..d709a3cad8b5 > > --- /dev/null > > +++ > > > b/MdeModulePkg/Library/BasePlatformAcpiLibNull/BasePlatformAcpiLibNu > > +++ ll.uni > > @@ -0,0 +1,18 @@ > > +// /** @file > > +// Null Platform ACPI Library instance. > > +// > > +// Null Platform ACPI Library instance. > > +// > > +// Copyright (c) 2018, Intel Corporation. All rights reserved.
// > > +// This program and the accompanying materials // are licensed and > > +made available under the terms and conditions of the BSD License // > > +which accompanies this distribution. The full text of the license may > > +be found at // http://opensource.org/licenses/bsd-license.php > > +// THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" > > +BASIS, // WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, > > EITHER EXPRESS OR IMPLIED. > > +// > > +// **/ > > + > > +#string STR_MODULE_ABSTRACT #language en-US "Null Platform ACPI > > Library instance" > > +#string STR_MODULE_DESCRIPTION #language en-US "Null Platform ACPI > > Library instance." > > diff --git a/MdeModulePkg/MdeModulePkg.dec > > b/MdeModulePkg/MdeModulePkg.dec index > cc397185f7b9..8e3d392b3314 > > 100644 > > --- a/MdeModulePkg/MdeModulePkg.dec > > +++ b/MdeModulePkg/MdeModulePkg.dec > > @@ -93,6 +93,10 @@ [LibraryClasses] > > # > > DebugAgentLib|Include/Library/DebugAgentLib.h > > > > + ## @libraryclass Provide platform specific ACPI Table update funct= ions. > > + # > > + PlatformAcpiLib|Include/Library/PlatformAcpiLib.h > > + > > ## @libraryclass Provide platform specific hooks. > > # > > PlatformHookLib|Include/Library/PlatformHookLib.h > > diff --git a/MdeModulePkg/MdeModulePkg.dsc > > b/MdeModulePkg/MdeModulePkg.dsc index ec24a50c7d0a..b18506a9af9c > > 100644 > > --- a/MdeModulePkg/MdeModulePkg.dsc > > +++ b/MdeModulePkg/MdeModulePkg.dsc > > @@ -91,6 +91,7 @@ [LibraryClasses] > > > > > PeCoffExtraActionLib|MdePkg/Library/BasePeCoffExtraActionLibNull/BaseP > > eCoffExtraActionLibNull.inf > > > > > PerformanceLib|MdePkg/Library/BasePerformanceLibNull/BasePerformanc > > eLibNull.inf > > > > > DebugAgentLib|MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLi > > bNull.inf > > + > > + > > > PlatformAcpiLib|MdeModulePkg/Library/BasePlatformAcpiLibNull/BasePlatf > > + ormAcpiLibNull.inf > > > > > PlatformHookLib|MdeModulePkg/Library/BasePlatformHookLibNull/BasePla > > tformHookLibNull.inf > > > > > ResetSystemLib|MdeModulePkg/Library/BaseResetSystemLibNull/BaseRes > > etSystemLibNull.inf > > SmbusLib|MdePkg/Library/DxeSmbusLib/DxeSmbusLib.inf > > @@ -300,6 +301,7 @@ [Components] > > > > > MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib. > > inf > > > > > MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeRep > > ortStatusCodeLib.inf > > > > MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf > > + > > + > > > MdeModulePkg/Library/BasePlatformAcpiLibNull/BasePlatformAcpiLibNull.i > > + nf > > > > > MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull > > .inf > > > > > MdeModulePkg/Library/DxeDebugPrintErrorLevelLib/DxeDebugPrintErrorLe > > velLib.inf > > MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf > > -- > > 2.17.0.windows.1 > > > > _______________________________________________ > > 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