From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 E270C20348620 for ; Wed, 9 May 2018 04:49:17 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1EFD4818F6E2; Wed, 9 May 2018 11:49:17 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-125.rdu2.redhat.com [10.10.121.125]) by smtp.corp.redhat.com (Postfix) with ESMTP id E726310EE6C1; Wed, 9 May 2018 11:49:15 +0000 (UTC) To: "Zeng, Star" , "Marvin.Haeuser@outlook.com" , "edk2-devel@lists.01.org" Cc: "Yao, Jiewen" , "Kinney, Michael D" , "Ni, Ruiyu" , "Dong, Eric" References: <0C09AFA07DD0434D9E2A0C6AEB0483103BAE9AE2@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: Date: Wed, 9 May 2018 13:49:14 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <0C09AFA07DD0434D9E2A0C6AEB0483103BAE9AE2@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Wed, 09 May 2018 11:49:17 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Wed, 09 May 2018 11:49:17 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' 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: Wed, 09 May 2018 11:49:18 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi, On 05/09/18 11:32, Zeng, Star wrote: > 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/Dxe/AcpiPlatform > https://github.com/tianocore/edk2/tree/master/Vlv2TbltDevicePkg/AcpiPlatform > ... > > 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. I can't really comment on other platforms, but for OvmfPkg and ArmVirtPkg, the ACPI platform driver(s) are totally custom. The ACPI payload is generated by VMM software (on the host), the driver loads the blobs and processes them in various ways (also controlled by the VMM), and finally the driver installs the tables. Marvin writes below: > -----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. In our use case, the ACPI tables are not loaded from firmware volumes at all. (OVMF still has such a fallback for extremely old QEMU versions, but that fallback should rather be removed than modified in any way.) Thanks, Laszlo > 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.inf | 35 +++++++++++++++++++ > MdeModulePkg/Library/BasePlatformAcpiLibNull/BasePlatformAcpiLibNull.uni | 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/BasePlatformAcpiLibNull.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 could 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 could 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/BasePlatformAcpiLibNull.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 = 0x00010005 > + BASE_NAME = BasePlatformAcpiLibNull > + MODULE_UNI_FILE = BasePlatformAcpiLibNull.uni > + FILE_GUID = 0957C6BA-0559-46E8-8180-CD0317F0AEE7 > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = PlatformAcpiLib > + > +# > +# The following information is for reference only and not required by the build tools. > +# > +# VALID_ARCHITECTURES = 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/BasePlatformAcpiLibNull.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 functions. > + # > + 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/BasePeCoffExtraActionLibNull.inf > PerformanceLib|MdePkg/Library/BasePerformanceLibNull/BasePerformanceLibNull.inf > DebugAgentLib|MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.inf > + PlatformAcpiLib|MdeModulePkg/Library/BasePlatformAcpiLibNull/BasePlatformAcpiLibNull.inf > PlatformHookLib|MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf > ResetSystemLib|MdeModulePkg/Library/BaseResetSystemLibNull/BaseResetSystemLibNull.inf > SmbusLib|MdePkg/Library/DxeSmbusLib/DxeSmbusLib.inf > @@ -300,6 +301,7 @@ [Components] > MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf > MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf > MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf > + MdeModulePkg/Library/BasePlatformAcpiLibNull/BasePlatformAcpiLibNull.inf > MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf > MdeModulePkg/Library/DxeDebugPrintErrorLevelLib/DxeDebugPrintErrorLevelLib.inf > MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf >