public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH 1/2] MdeModulePkg: Add PlatformAcpiLib LibraryClass.
@ 2018-05-08 23:25 Marvin Häuser
  2018-05-09  9:32 ` Zeng, Star
  0 siblings, 1 reply; 5+ messages in thread
From: Marvin Häuser @ 2018-05-08 23:25 UTC (permalink / raw)
  To: edk2-devel@lists.01.org
  Cc: star.zeng@intel.com, eric.dong@intel.com, ruiyu.ni@intel.com

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 <Marvin.Haeuser@outlook.com>
---
 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.<BR>
+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 <Base.h>
+
+#include <IndustryStandard/Acpi.h>
+
+/**
+  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.<BR>
+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.<BR>
+#
+#  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.<BR>
+//
+// 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
-- 
2.17.0.windows.1



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] MdeModulePkg: Add PlatformAcpiLib LibraryClass.
  2018-05-08 23:25 [PATCH 1/2] MdeModulePkg: Add PlatformAcpiLib LibraryClass Marvin Häuser
@ 2018-05-09  9:32 ` Zeng, Star
  2018-05-09 11:49   ` Laszlo Ersek
  2018-05-09 15:37   ` Marvin H?user
  0 siblings, 2 replies; 5+ messages in thread
From: Zeng, Star @ 2018-05-09  9:32 UTC (permalink / raw)
  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

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.


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 <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
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 <Marvin.Haeuser@outlook.com>
---
 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.<BR>
+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 <Base.h>
+
+#include <IndustryStandard/Acpi.h>
+
+/**
+  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.<BR>
+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.<BR>
+#
+#  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.<BR>
+//
+// 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
-- 
2.17.0.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] MdeModulePkg: Add PlatformAcpiLib LibraryClass.
  2018-05-09  9:32 ` Zeng, Star
@ 2018-05-09 11:49   ` Laszlo Ersek
  2018-05-09 15:37   ` Marvin H?user
  1 sibling, 0 replies; 5+ messages in thread
From: Laszlo Ersek @ 2018-05-09 11:49 UTC (permalink / raw)
  To: Zeng, Star, Marvin.Haeuser@outlook.com, edk2-devel@lists.01.org
  Cc: Yao, Jiewen, Kinney, Michael D, Ni, Ruiyu, Dong, Eric

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 <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> 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 <Marvin.Haeuser@outlook.com>
> ---
>  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.<BR>
> +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 <Base.h>
> +
> +#include <IndustryStandard/Acpi.h>
> +
> +/**
> +  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.<BR>
> +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.<BR>
> +#
> +#  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.<BR>
> +//
> +// 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
> 



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] MdeModulePkg: Add PlatformAcpiLib LibraryClass.
  2018-05-09  9:32 ` Zeng, Star
  2018-05-09 11:49   ` Laszlo Ersek
@ 2018-05-09 15:37   ` Marvin H?user
  2018-05-10  3:27     ` Ni, Ruiyu
  1 sibling, 1 reply; 5+ messages in thread
From: Marvin H?user @ 2018-05-09 15:37 UTC (permalink / raw)
  To: edk2-devel@lists.01.org
  Cc: Zeng, Star, Laszlo Ersek, jiewen.yao@intel.com,
	michael.d.kinney@intel.com, ruiyu.ni@intel.com,
	eric.dong@intel.com

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.

Hey Star,

Indeed I expected it to be just for reference, however as most of the drivers 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 drivers can be implemented well via a library constructor.

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.

Thanks in advance.

Best regards,
Marvin

> -----Original Message-----
> From: Zeng, Star <star.zeng@intel.com>
> Sent: Wednesday, May 9, 2018 11:33 AM
> To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Laszlo Ersek (lersek@redhat.com)
> <lersek@redhat.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> 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/AcpiPla
> 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 <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng,
> Star <star.zeng@intel.com>
> 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 <Marvin.Haeuser@outlook.com>
> ---
>  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.<BR> 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 <Base.h>
> +
> +#include <IndustryStandard/Acpi.h>
> +
> +/**
> +  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.<BR> 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/BasePlatformAcpiLibNu
> +++ ll.inf
> @@ -0,0 +1,35 @@
> +## @file
> +#  Null Platform ACPI Library instance.
> +#
> +#  Copyright (c) 2018, Intel Corporation. All rights reserved.<BR> # #
> +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/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.<BR> //
> +// 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/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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH 1/2] MdeModulePkg: Add PlatformAcpiLib LibraryClass.
  2018-05-09 15:37   ` Marvin H?user
@ 2018-05-10  3:27     ` Ni, Ruiyu
  0 siblings, 0 replies; 5+ messages in thread
From: Ni, Ruiyu @ 2018-05-10  3:27 UTC (permalink / raw)
  To: Marvin H?user, edk2-devel@lists.01.org
  Cc: Dong, Eric, Yao, Jiewen, Kinney, Michael D, Laszlo Ersek,
	Zeng, Star

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 when necessary.
Otherwise, people new to this project would be lost in many interfaces.

Thanks/Ray

> -----Original Message-----
> From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Marvin
> H?user
> Sent: Wednesday, May 9, 2018 11:38 PM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Laszlo Ersek <lersek@redhat.com>; Zeng,
> Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH 1/2] MdeModulePkg: Add PlatformAcpiLib
> LibraryClass.
> 
> 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.
> 
> Hey Star,
> 
> Indeed I expected it to be just for reference, however as most of the drivers
> 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 drivers
> can be implemented well via a library constructor.
> 
> 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.
> 
> Thanks in advance.
> 
> Best regards,
> Marvin
> 
> > -----Original Message-----
> > From: Zeng, Star <star.zeng@intel.com>
> > Sent: Wednesday, May 9, 2018 11:33 AM
> > To: Marvin.Haeuser@outlook.com; edk2-devel@lists.01.org
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Laszlo Ersek (lersek@redhat.com)
> > <lersek@redhat.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; Dong, Eric
> > <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> > 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 <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>;
> > Zeng, Star <star.zeng@intel.com>
> > 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 <Marvin.Haeuser@outlook.com>
> > ---
> >
> >
> 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.<BR> 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 <Base.h>
> > +
> > +#include <IndustryStandard/Acpi.h>
> > +
> > +/**
> > +  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.<BR> 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/BasePlatformAcpiLibNu
> > +++ ll.inf
> > @@ -0,0 +1,35 @@
> > +## @file
> > +#  Null Platform ACPI Library instance.
> > +#
> > +#  Copyright (c) 2018, Intel Corporation. All rights reserved.<BR> #
> > +# 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/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.<BR> //
> > +// 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/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


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2018-05-10  3:27 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-05-08 23:25 [PATCH 1/2] MdeModulePkg: Add PlatformAcpiLib LibraryClass Marvin Häuser
2018-05-09  9:32 ` Zeng, Star
2018-05-09 11:49   ` Laszlo Ersek
2018-05-09 15:37   ` Marvin H?user
2018-05-10  3:27     ` Ni, Ruiyu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox