* [Patch V2 1/2] MdeModulePkg/ACPI: Install ACPI table from HOB.
@ 2021-03-23 3:24 Zhiguang Liu
2021-03-23 3:24 ` [Patch V2 2/2] UefiPayloadPkg: Remove code that installs APCI Zhiguang Liu
` (2 more replies)
0 siblings, 3 replies; 25+ messages in thread
From: Zhiguang Liu @ 2021-03-23 3:24 UTC (permalink / raw)
To: devel; +Cc: Jian J Wang, Hao A Wu, Dandan Bi, Liming Gao, Ray Ni
If HOB contains APCI table information, entry point of AcpiTableDxe.inf
should parse the APCI table from HOB, and install these tables.
We assume the whole ACPI table (starting with EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER)
is contained by a single gEfiAcpiTableGuid HOB.
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf | 3 ++-
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
2 files changed, 128 insertions(+), 9 deletions(-)
diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
index d341df439e..473127368d 100644
--- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
+++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
@@ -4,7 +4,7 @@
# This driver initializes ACPI tables (Rsdp, Rsdt and Xsdt) and produces UEFI/PI
# services to install/uninstall/manage ACPI tables.
#
-# Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>
# Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
@@ -51,6 +51,7 @@
DebugLib
BaseLib
PcdLib
+ HobLib
[Guids]
gEfiAcpi10TableGuid ## PRODUCES ## SystemTable
diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
index 5a2afdff27..6b58759620 100644
--- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
+++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
@@ -1,7 +1,7 @@
/** @file
ACPI Table Protocol Implementation
- Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>
Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -11,6 +11,8 @@
// Includes
//
#include "AcpiTable.h"
+#include <Library/HobLib.h>
+
//
// The maximum number of tables that pre-allocated.
//
@@ -30,6 +32,7 @@ STATIC EFI_ALLOCATE_TYPE mAcpiTableAllocType;
@param Table Table to add.
@param Checksum Does the table require checksumming.
@param Version The version of the list to add the table to.
+ @param IsFromHob True, if add Apci Table from Hob List.
@param Handle Pointer for returning the handle.
@return EFI_SUCCESS The function completed successfully.
@@ -44,6 +47,7 @@ AddTableToList (
IN VOID *Table,
IN BOOLEAN Checksum,
IN EFI_ACPI_TABLE_VERSION Version,
+ IN BOOLEAN IsFromHob,
OUT UINTN *Handle
);
@@ -238,6 +242,7 @@ InstallAcpiTable (
AcpiTableBufferConst,
TRUE,
Version,
+ FALSE,
TableKey
);
if (!EFI_ERROR (Status)) {
@@ -472,6 +477,7 @@ FreeTableMemory (
@param Table Table to add.
@param Checksum Does the table require checksumming.
@param Version The version of the list to add the table to.
+ @param IsFromHob True, if add Apci Table from Hob List.
@param Handle Pointer for returning the handle.
@return EFI_SUCCESS The function completed successfully.
@@ -487,6 +493,7 @@ AddTableToList (
IN VOID *Table,
IN BOOLEAN Checksum,
IN EFI_ACPI_TABLE_VERSION Version,
+ IN BOOLEAN IsFromHob,
OUT UINTN *Handle
)
{
@@ -552,13 +559,17 @@ AddTableToList (
// could be updated by OS present agent. For example, BufferPtrAddress in
// SMM communication ACPI table.
//
- ASSERT ((EFI_PAGE_SIZE % 64) == 0);
- Status = gBS->AllocatePages (
- AllocateMaxAddress,
- EfiACPIMemoryNVS,
- EFI_SIZE_TO_PAGES (CurrentTableList->TableSize),
- &AllocPhysAddress
- );
+ if (IsFromHob){
+ AllocPhysAddress = (UINTN)Table;
+ } else {
+ ASSERT ((EFI_PAGE_SIZE % 64) == 0);
+ Status = gBS->AllocatePages (
+ AllocateMaxAddress,
+ EfiACPIMemoryNVS,
+ EFI_SIZE_TO_PAGES (CurrentTableList->TableSize),
+ &AllocPhysAddress
+ );
+ }
} else if (mAcpiTableAllocType == AllocateAnyPages) {
//
// If there is no allocation limit, there is also no need to use page
@@ -1689,6 +1700,111 @@ ChecksumCommonTables (
return EFI_SUCCESS;
}
+/**
+ This function will find Guid Hob gEfiAcpiTableGuid, and install Acpi table from it.
+
+ @param AcpiTableInstance Protocol instance private data.
+
+ @return EFI_SUCCESS The function completed successfully.
+ @return EFI_NOT_FOUND The function doesn't find the gEfiAcpiTableGuid Guid Hob.
+ @return EFI_ABORTED The function could not complete successfully.
+
+**/
+EFI_STATUS
+InstallAcpiTableFromHob (
+ EFI_ACPI_TABLE_INSTANCE *AcpiTableInstance
+ )
+{
+ EFI_HOB_GUID_TYPE *GuidHob;
+ EFI_ACPI_TABLE_VERSION Version;
+ EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER *Rsdp;
+ EFI_ACPI_DESCRIPTION_HEADER *Rsdt;
+ EFI_ACPI_DESCRIPTION_HEADER *ChildTable;
+ UINT64 ChildTableAddress;
+ UINTN Count;
+ UINTN Index;
+ UINTN TableKey;
+ EFI_STATUS Status;
+ UINTN EntrySize;
+
+ TableKey = 0;
+ Version = PcdGet32 (PcdAcpiExposedTableVersions);
+
+ //
+ // HOB only contains the ACPI table in 2.0+ format.
+ //
+ GuidHob = GetFirstGuidHob (&gEfiAcpiTableGuid);
+ if (GuidHob == NULL) {
+ return EFI_NOT_FOUND;
+ }
+ Rsdp = (EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER*) (UINTN)(*((UINT64*)GET_GUID_HOB_DATA (GuidHob)));
+
+ //
+ // An ACPI-compatible OS must use the XSDT if present.
+ // It shouldn't happen that XsdtAddress points beyond 4G range in 32-bit environment.
+ //
+ ASSERT ((UINTN) Rsdp->XsdtAddress == Rsdp->XsdtAddress);
+
+ EntrySize = sizeof (UINT64);
+ Rsdt = (EFI_ACPI_DESCRIPTION_HEADER*) (UINTN) Rsdp->XsdtAddress;
+ if (Rsdt == NULL) {
+ //
+ // XsdtAddress is zero, then we use Rsdt which has 32 bit entry
+ //
+ Rsdt = (EFI_ACPI_DESCRIPTION_HEADER*) (UINTN) Rsdp->RsdtAddress;
+ EntrySize = sizeof (UINT32);
+ }
+ Count = (Rsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER))/EntrySize;
+
+ for (Index = 0; Index < Count; Index++){
+ ChildTableAddress = 0;
+ CopyMem(&ChildTableAddress, (VOID*)((UINTN) Rsdt + EntrySize * Index + sizeof (EFI_ACPI_DESCRIPTION_HEADER)), EntrySize);
+ //
+ // If the address is of UINT64 while this module runs at 32 bits,
+ // make sure the upper bits are all-zeros.
+ //
+ ASSERT (ChildTableAddress == (UINTN) ChildTableAddress);
+
+ ChildTable = (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN) ChildTableAddress;
+ Status = AddTableToList (
+ AcpiTableInstance,
+ ChildTable,
+ TRUE,
+ Version,
+ TRUE,
+ &TableKey
+ );
+ if (ChildTable->Signature == EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE){
+ //
+ // Add the FACS and DSDT tables.
+ //
+ Status = AddTableToList (
+ AcpiTableInstance,
+ (VOID *) (UINTN)((EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE*)ChildTable)->FirmwareCtrl,
+ TRUE,
+ Version,
+ TRUE,
+ &TableKey
+ );
+ Status = AddTableToList (
+ AcpiTableInstance,
+ (VOID *) (UINTN)((EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE*)ChildTable)->Dsdt,
+ TRUE,
+ Version,
+ TRUE,
+ &TableKey
+ );
+ }
+ }
+ if (!EFI_ERROR (Status)) {
+ Status = PublishTables (
+ AcpiTableInstance,
+ Version
+ );
+ }
+ ASSERT_EFI_ERROR (Status);
+ return Status;
+}
/**
Constructor for the ACPI table protocol. Initializes instance
@@ -1918,6 +2034,8 @@ AcpiTableAcpiTableConstructor (
ChecksumCommonTables (AcpiTableInstance);
+ InstallAcpiTableFromHob (AcpiTableInstance);
+
//
// Completed successfully
//
--
2.30.0.windows.2
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Patch V2 2/2] UefiPayloadPkg: Remove code that installs APCI
2021-03-23 3:24 [Patch V2 1/2] MdeModulePkg/ACPI: Install ACPI table from HOB Zhiguang Liu
@ 2021-03-23 3:24 ` Zhiguang Liu
2021-03-23 3:44 ` Ni, Ray
2021-03-23 5:19 ` Guo Dong
2021-03-23 3:24 ` [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob Zhiguang Liu
2021-03-23 3:44 ` [Patch V2 1/2] MdeModulePkg/ACPI: Install ACPI table from HOB Ni, Ray
2 siblings, 2 replies; 25+ messages in thread
From: Zhiguang Liu @ 2021-03-23 3:24 UTC (permalink / raw)
To: devel; +Cc: Maurice Ma, Guo Dong, Benjamin You, Ray Ni
MdeModulePkg\Universal\Acpi\AcpiTableDxe\AcpiTableDxe.inf is capable to install
ACPI table contained in HOB. All ACPI table shoulb be managed by that module.
Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Benjamin You <benjamin.you@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 13 ++-----------
UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h | 5 +----
UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf | 5 ++---
3 files changed, 5 insertions(+), 18 deletions(-)
diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
index a746d0581e..dfff8ecf0e 100644
--- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
+++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
@@ -1,8 +1,8 @@
/** @file
- This driver will report some MMIO/IO resources to dxe core, extract smbios and acpi
+ This driver will report some MMIO/IO resources to dxe core, extract smbios
tables from bootloader.
- Copyright (c) 2014 - 2020, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -120,15 +120,6 @@ BlDxeEntryPoint (
ASSERT (GuidHob != NULL);
SystemTableInfo = (SYSTEM_TABLE_INFO *)GET_GUID_HOB_DATA (GuidHob);
- //
- // Install Acpi Table
- //
- if (SystemTableInfo->AcpiTableBase != 0 && SystemTableInfo->AcpiTableSize != 0) {
- DEBUG ((DEBUG_ERROR, "Install Acpi Table at 0x%lx, length 0x%x\n", SystemTableInfo->AcpiTableBase, SystemTableInfo->AcpiTableSize));
- Status = gBS->InstallConfigurationTable (&gEfiAcpiTableGuid, (VOID *)(UINTN)SystemTableInfo->AcpiTableBase);
- ASSERT_EFI_ERROR (Status);
- }
-
//
// Install Smbios Table
//
diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
index 512105fafd..3332a30eae 100644
--- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
+++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
@@ -1,7 +1,7 @@
/** @file
The header file of bootloader support DXE.
-Copyright (c) 2014, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -19,12 +19,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Library/IoLib.h>
#include <Library/HobLib.h>
-#include <Guid/Acpi.h>
#include <Guid/SmBios.h>
#include <Guid/SystemTableInfoGuid.h>
#include <Guid/AcpiBoardInfoGuid.h>
#include <Guid/GraphicsInfoHob.h>
-#include <IndustryStandard/Acpi.h>
-
#endif
diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
index cebc811355..ad4e0a57a1 100644
--- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
+++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
@@ -1,9 +1,9 @@
## @file
# Bootloader Support DXE Module
#
-# Report some MMIO/IO resources to dxe core, extract smbios and acpi tables
+# Report some MMIO/IO resources to dxe core, extract smbios tables
#
-# Copyright (c) 2014 - 2020, Intel Corporation. All rights reserved.<BR>
+# Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR>
#
# SPDX-License-Identifier: BSD-2-Clause-Patent
#
@@ -42,7 +42,6 @@
HobLib
[Guids]
- gEfiAcpiTableGuid
gEfiSmbiosTableGuid
gUefiSystemTableInfoGuid
gUefiAcpiBoardInfoGuid
--
2.30.0.windows.2
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
2021-03-23 3:24 [Patch V2 1/2] MdeModulePkg/ACPI: Install ACPI table from HOB Zhiguang Liu
2021-03-23 3:24 ` [Patch V2 2/2] UefiPayloadPkg: Remove code that installs APCI Zhiguang Liu
@ 2021-03-23 3:24 ` Zhiguang Liu
2021-03-23 12:40 ` [edk2-devel] " Laszlo Ersek
2021-03-23 3:44 ` [Patch V2 1/2] MdeModulePkg/ACPI: Install ACPI table from HOB Ni, Ray
2 siblings, 1 reply; 25+ messages in thread
From: Zhiguang Liu @ 2021-03-23 3:24 UTC (permalink / raw)
To: devel
If HOB contains APCI table information, entry point of AcpiTableDxe.inf
should parse the APCI table from HOB, and install these tables.
We assume the whole ACPI table (starting with EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER)
is contained by a single gEfiAcpiTableGuid HOB.
This way, for UefiPayloadPkg, there is no need to specially hanle acpi table.
Zhiguang Liu (2):
MdeModulePkg/ACPI: Install ACPI table from HOB.
UefiPayloadPkg: Remove code that installs APCI
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf | 3 ++-
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 13 ++-----------
UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h | 5 +----
UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf | 5 ++---
5 files changed, 133 insertions(+), 27 deletions(-)
--
2.30.0.windows.2
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Patch V2 1/2] MdeModulePkg/ACPI: Install ACPI table from HOB.
2021-03-23 3:24 [Patch V2 1/2] MdeModulePkg/ACPI: Install ACPI table from HOB Zhiguang Liu
2021-03-23 3:24 ` [Patch V2 2/2] UefiPayloadPkg: Remove code that installs APCI Zhiguang Liu
2021-03-23 3:24 ` [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob Zhiguang Liu
@ 2021-03-23 3:44 ` Ni, Ray
2 siblings, 0 replies; 25+ messages in thread
From: Ni, Ray @ 2021-03-23 3:44 UTC (permalink / raw)
To: Liu, Zhiguang, devel@edk2.groups.io
Cc: Wang, Jian J, Wu, Hao A, Bi, Dandan, Liming Gao
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Tuesday, March 23, 2021 11:25 AM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Ni, Ray <ray.ni@intel.com>
> Subject: [Patch V2 1/2] MdeModulePkg/ACPI: Install ACPI table from HOB.
>
> If HOB contains APCI table information, entry point of AcpiTableDxe.inf
> should parse the APCI table from HOB, and install these tables.
> We assume the whole ACPI table (starting with EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER)
> is contained by a single gEfiAcpiTableGuid HOB.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf | 3 ++-
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 134
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++--------
> 2 files changed, 128 insertions(+), 9 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
> index d341df439e..473127368d 100644
> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
> @@ -4,7 +4,7 @@
> # This driver initializes ACPI tables (Rsdp, Rsdt and Xsdt) and produces UEFI/PI
>
> # services to install/uninstall/manage ACPI tables.
>
> #
>
> -# Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>
> +# Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>
>
> # Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
>
> # SPDX-License-Identifier: BSD-2-Clause-Patent
>
> #
>
> @@ -51,6 +51,7 @@
> DebugLib
>
> BaseLib
>
> PcdLib
>
> + HobLib
>
>
>
> [Guids]
>
> gEfiAcpi10TableGuid ## PRODUCES ## SystemTable
>
> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> index 5a2afdff27..6b58759620 100644
> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> @@ -1,7 +1,7 @@
> /** @file
>
> ACPI Table Protocol Implementation
>
>
>
> - Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>
> + Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>
>
> Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
>
> @@ -11,6 +11,8 @@
> // Includes
>
> //
>
> #include "AcpiTable.h"
>
> +#include <Library/HobLib.h>
>
> +
>
> //
>
> // The maximum number of tables that pre-allocated.
>
> //
>
> @@ -30,6 +32,7 @@ STATIC EFI_ALLOCATE_TYPE mAcpiTableAllocType;
> @param Table Table to add.
>
> @param Checksum Does the table require checksumming.
>
> @param Version The version of the list to add the table to.
>
> + @param IsFromHob True, if add Apci Table from Hob List.
>
> @param Handle Pointer for returning the handle.
>
>
>
> @return EFI_SUCCESS The function completed successfully.
>
> @@ -44,6 +47,7 @@ AddTableToList (
> IN VOID *Table,
>
> IN BOOLEAN Checksum,
>
> IN EFI_ACPI_TABLE_VERSION Version,
>
> + IN BOOLEAN IsFromHob,
>
> OUT UINTN *Handle
>
> );
>
>
>
> @@ -238,6 +242,7 @@ InstallAcpiTable (
> AcpiTableBufferConst,
>
> TRUE,
>
> Version,
>
> + FALSE,
>
> TableKey
>
> );
>
> if (!EFI_ERROR (Status)) {
>
> @@ -472,6 +477,7 @@ FreeTableMemory (
> @param Table Table to add.
>
> @param Checksum Does the table require checksumming.
>
> @param Version The version of the list to add the table to.
>
> + @param IsFromHob True, if add Apci Table from Hob List.
>
> @param Handle Pointer for returning the handle.
>
>
>
> @return EFI_SUCCESS The function completed successfully.
>
> @@ -487,6 +493,7 @@ AddTableToList (
> IN VOID *Table,
>
> IN BOOLEAN Checksum,
>
> IN EFI_ACPI_TABLE_VERSION Version,
>
> + IN BOOLEAN IsFromHob,
>
> OUT UINTN *Handle
>
> )
>
> {
>
> @@ -552,13 +559,17 @@ AddTableToList (
> // could be updated by OS present agent. For example, BufferPtrAddress in
>
> // SMM communication ACPI table.
>
> //
>
> - ASSERT ((EFI_PAGE_SIZE % 64) == 0);
>
> - Status = gBS->AllocatePages (
>
> - AllocateMaxAddress,
>
> - EfiACPIMemoryNVS,
>
> - EFI_SIZE_TO_PAGES (CurrentTableList->TableSize),
>
> - &AllocPhysAddress
>
> - );
>
> + if (IsFromHob){
>
> + AllocPhysAddress = (UINTN)Table;
>
> + } else {
>
> + ASSERT ((EFI_PAGE_SIZE % 64) == 0);
>
> + Status = gBS->AllocatePages (
>
> + AllocateMaxAddress,
>
> + EfiACPIMemoryNVS,
>
> + EFI_SIZE_TO_PAGES (CurrentTableList->TableSize),
>
> + &AllocPhysAddress
>
> + );
>
> + }
>
> } else if (mAcpiTableAllocType == AllocateAnyPages) {
>
> //
>
> // If there is no allocation limit, there is also no need to use page
>
> @@ -1689,6 +1700,111 @@ ChecksumCommonTables (
> return EFI_SUCCESS;
>
> }
>
>
>
> +/**
>
> + This function will find Guid Hob gEfiAcpiTableGuid, and install Acpi table from it.
>
> +
>
> + @param AcpiTableInstance Protocol instance private data.
>
> +
>
> + @return EFI_SUCCESS The function completed successfully.
>
> + @return EFI_NOT_FOUND The function doesn't find the gEfiAcpiTableGuid Guid Hob.
>
> + @return EFI_ABORTED The function could not complete successfully.
>
> +
>
> +**/
>
> +EFI_STATUS
>
> +InstallAcpiTableFromHob (
>
> + EFI_ACPI_TABLE_INSTANCE *AcpiTableInstance
>
> + )
>
> +{
>
> + EFI_HOB_GUID_TYPE *GuidHob;
>
> + EFI_ACPI_TABLE_VERSION Version;
>
> + EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER *Rsdp;
>
> + EFI_ACPI_DESCRIPTION_HEADER *Rsdt;
>
> + EFI_ACPI_DESCRIPTION_HEADER *ChildTable;
>
> + UINT64 ChildTableAddress;
>
> + UINTN Count;
>
> + UINTN Index;
>
> + UINTN TableKey;
>
> + EFI_STATUS Status;
>
> + UINTN EntrySize;
>
> +
>
> + TableKey = 0;
>
> + Version = PcdGet32 (PcdAcpiExposedTableVersions);
>
> +
>
> + //
>
> + // HOB only contains the ACPI table in 2.0+ format.
>
> + //
>
> + GuidHob = GetFirstGuidHob (&gEfiAcpiTableGuid);
>
> + if (GuidHob == NULL) {
>
> + return EFI_NOT_FOUND;
>
> + }
>
> + Rsdp = (EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER*) (UINTN)(*((UINT64*)GET_GUID_HOB_DATA (GuidHob)));
>
> +
>
> + //
>
> + // An ACPI-compatible OS must use the XSDT if present.
>
> + // It shouldn't happen that XsdtAddress points beyond 4G range in 32-bit environment.
>
> + //
>
> + ASSERT ((UINTN) Rsdp->XsdtAddress == Rsdp->XsdtAddress);
>
> +
>
> + EntrySize = sizeof (UINT64);
>
> + Rsdt = (EFI_ACPI_DESCRIPTION_HEADER*) (UINTN) Rsdp->XsdtAddress;
>
> + if (Rsdt == NULL) {
>
> + //
>
> + // XsdtAddress is zero, then we use Rsdt which has 32 bit entry
>
> + //
>
> + Rsdt = (EFI_ACPI_DESCRIPTION_HEADER*) (UINTN) Rsdp->RsdtAddress;
>
> + EntrySize = sizeof (UINT32);
>
> + }
>
> + Count = (Rsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER))/EntrySize;
>
> +
>
> + for (Index = 0; Index < Count; Index++){
>
> + ChildTableAddress = 0;
>
> + CopyMem(&ChildTableAddress, (VOID*)((UINTN) Rsdt + EntrySize * Index + sizeof (EFI_ACPI_DESCRIPTION_HEADER)),
> EntrySize);
>
> + //
>
> + // If the address is of UINT64 while this module runs at 32 bits,
>
> + // make sure the upper bits are all-zeros.
>
> + //
>
> + ASSERT (ChildTableAddress == (UINTN) ChildTableAddress);
>
> +
>
> + ChildTable = (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN) ChildTableAddress;
>
> + Status = AddTableToList (
>
> + AcpiTableInstance,
>
> + ChildTable,
>
> + TRUE,
>
> + Version,
>
> + TRUE,
>
> + &TableKey
>
> + );
>
> + if (ChildTable->Signature == EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE){
>
> + //
>
> + // Add the FACS and DSDT tables.
>
> + //
>
> + Status = AddTableToList (
>
> + AcpiTableInstance,
>
> + (VOID *) (UINTN)((EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE*)ChildTable)->FirmwareCtrl,
>
> + TRUE,
>
> + Version,
>
> + TRUE,
>
> + &TableKey
>
> + );
>
> + Status = AddTableToList (
>
> + AcpiTableInstance,
>
> + (VOID *) (UINTN)((EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE*)ChildTable)->Dsdt,
>
> + TRUE,
>
> + Version,
>
> + TRUE,
>
> + &TableKey
>
> + );
>
> + }
>
> + }
>
> + if (!EFI_ERROR (Status)) {
>
> + Status = PublishTables (
>
> + AcpiTableInstance,
>
> + Version
>
> + );
>
> + }
>
> + ASSERT_EFI_ERROR (Status);
>
> + return Status;
>
> +}
>
>
>
> /**
>
> Constructor for the ACPI table protocol. Initializes instance
>
> @@ -1918,6 +2034,8 @@ AcpiTableAcpiTableConstructor (
>
>
> ChecksumCommonTables (AcpiTableInstance);
>
>
>
> + InstallAcpiTableFromHob (AcpiTableInstance);
>
> +
>
> //
>
> // Completed successfully
>
> //
>
> --
> 2.30.0.windows.2
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Patch V2 2/2] UefiPayloadPkg: Remove code that installs APCI
2021-03-23 3:24 ` [Patch V2 2/2] UefiPayloadPkg: Remove code that installs APCI Zhiguang Liu
@ 2021-03-23 3:44 ` Ni, Ray
2021-03-23 5:19 ` Guo Dong
1 sibling, 0 replies; 25+ messages in thread
From: Ni, Ray @ 2021-03-23 3:44 UTC (permalink / raw)
To: Liu, Zhiguang, devel@edk2.groups.io; +Cc: Ma, Maurice, Dong, Guo, You, Benjamin
Reviewed-by: Ray Ni <ray.ni@intel.com>
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Tuesday, March 23, 2021 11:25 AM
> To: devel@edk2.groups.io
> Cc: Ma, Maurice <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>; Ni,
> Ray <ray.ni@intel.com>
> Subject: [Patch V2 2/2] UefiPayloadPkg: Remove code that installs APCI
>
> MdeModulePkg\Universal\Acpi\AcpiTableDxe\AcpiTableDxe.inf is capable to install
> ACPI table contained in HOB. All ACPI table shoulb be managed by that module.
>
> Cc: Maurice Ma <maurice.ma@intel.com>
> Cc: Guo Dong <guo.dong@intel.com>
> Cc: Benjamin You <benjamin.you@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 13 ++-----------
> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h | 5 +----
> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf | 5 ++---
> 3 files changed, 5 insertions(+), 18 deletions(-)
>
> diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
> index a746d0581e..dfff8ecf0e 100644
> --- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
> +++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
> @@ -1,8 +1,8 @@
> /** @file
>
> - This driver will report some MMIO/IO resources to dxe core, extract smbios and acpi
>
> + This driver will report some MMIO/IO resources to dxe core, extract smbios
>
> tables from bootloader.
>
>
>
> - Copyright (c) 2014 - 2020, Intel Corporation. All rights reserved.<BR>
>
> + Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
>
> **/
>
> @@ -120,15 +120,6 @@ BlDxeEntryPoint (
> ASSERT (GuidHob != NULL);
>
> SystemTableInfo = (SYSTEM_TABLE_INFO *)GET_GUID_HOB_DATA (GuidHob);
>
>
>
> - //
>
> - // Install Acpi Table
>
> - //
>
> - if (SystemTableInfo->AcpiTableBase != 0 && SystemTableInfo->AcpiTableSize != 0) {
>
> - DEBUG ((DEBUG_ERROR, "Install Acpi Table at 0x%lx, length 0x%x\n", SystemTableInfo->AcpiTableBase, SystemTableInfo-
> >AcpiTableSize));
>
> - Status = gBS->InstallConfigurationTable (&gEfiAcpiTableGuid, (VOID *)(UINTN)SystemTableInfo->AcpiTableBase);
>
> - ASSERT_EFI_ERROR (Status);
>
> - }
>
> -
>
> //
>
> // Install Smbios Table
>
> //
>
> diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
> index 512105fafd..3332a30eae 100644
> --- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
> +++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
> @@ -1,7 +1,7 @@
> /** @file
>
> The header file of bootloader support DXE.
>
>
>
> -Copyright (c) 2014, Intel Corporation. All rights reserved.<BR>
>
> +Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
>
> **/
>
> @@ -19,12 +19,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include <Library/IoLib.h>
>
> #include <Library/HobLib.h>
>
>
>
> -#include <Guid/Acpi.h>
>
> #include <Guid/SmBios.h>
>
> #include <Guid/SystemTableInfoGuid.h>
>
> #include <Guid/AcpiBoardInfoGuid.h>
>
> #include <Guid/GraphicsInfoHob.h>
>
>
>
> -#include <IndustryStandard/Acpi.h>
>
> -
>
> #endif
>
> diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
> index cebc811355..ad4e0a57a1 100644
> --- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
> +++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
> @@ -1,9 +1,9 @@
> ## @file
>
> # Bootloader Support DXE Module
>
> #
>
> -# Report some MMIO/IO resources to dxe core, extract smbios and acpi tables
>
> +# Report some MMIO/IO resources to dxe core, extract smbios tables
>
> #
>
> -# Copyright (c) 2014 - 2020, Intel Corporation. All rights reserved.<BR>
>
> +# Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR>
>
> #
>
> # SPDX-License-Identifier: BSD-2-Clause-Patent
>
> #
>
> @@ -42,7 +42,6 @@
> HobLib
>
>
>
> [Guids]
>
> - gEfiAcpiTableGuid
>
> gEfiSmbiosTableGuid
>
> gUefiSystemTableInfoGuid
>
> gUefiAcpiBoardInfoGuid
>
> --
> 2.30.0.windows.2
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [Patch V2 2/2] UefiPayloadPkg: Remove code that installs APCI
2021-03-23 3:24 ` [Patch V2 2/2] UefiPayloadPkg: Remove code that installs APCI Zhiguang Liu
2021-03-23 3:44 ` Ni, Ray
@ 2021-03-23 5:19 ` Guo Dong
1 sibling, 0 replies; 25+ messages in thread
From: Guo Dong @ 2021-03-23 5:19 UTC (permalink / raw)
To: Liu, Zhiguang, devel@edk2.groups.io; +Cc: Ma, Maurice, You, Benjamin, Ni, Ray
Thanks to help make this change.
Reviewed-by: Guo Dong <guo.dong@intel.com>
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Monday, March 22, 2021 8:25 PM
> To: devel@edk2.groups.io
> Cc: Ma, Maurice <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>;
> You, Benjamin <benjamin.you@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: [Patch V2 2/2] UefiPayloadPkg: Remove code that installs APCI
>
> MdeModulePkg\Universal\Acpi\AcpiTableDxe\AcpiTableDxe.inf is capable to
> install
> ACPI table contained in HOB. All ACPI table shoulb be managed by that module.
>
> Cc: Maurice Ma <maurice.ma@intel.com>
> Cc: Guo Dong <guo.dong@intel.com>
> Cc: Benjamin You <benjamin.you@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 13 ++-----------
> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h | 5 +----
> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf | 5 ++---
> 3 files changed, 5 insertions(+), 18 deletions(-)
>
> diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
> b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
> index a746d0581e..dfff8ecf0e 100644
> --- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
> +++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
> @@ -1,8 +1,8 @@
> /** @file
>
> - This driver will report some MMIO/IO resources to dxe core, extract smbios
> and acpi
>
> + This driver will report some MMIO/IO resources to dxe core, extract smbios
>
> tables from bootloader.
>
>
>
> - Copyright (c) 2014 - 2020, Intel Corporation. All rights reserved.<BR>
>
> + Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
>
> **/
>
> @@ -120,15 +120,6 @@ BlDxeEntryPoint (
> ASSERT (GuidHob != NULL);
>
> SystemTableInfo = (SYSTEM_TABLE_INFO *)GET_GUID_HOB_DATA (GuidHob);
>
>
>
> - //
>
> - // Install Acpi Table
>
> - //
>
> - if (SystemTableInfo->AcpiTableBase != 0 && SystemTableInfo-
> >AcpiTableSize != 0) {
>
> - DEBUG ((DEBUG_ERROR, "Install Acpi Table at 0x%lx, length 0x%x\n",
> SystemTableInfo->AcpiTableBase, SystemTableInfo->AcpiTableSize));
>
> - Status = gBS->InstallConfigurationTable (&gEfiAcpiTableGuid, (VOID
> *)(UINTN)SystemTableInfo->AcpiTableBase);
>
> - ASSERT_EFI_ERROR (Status);
>
> - }
>
> -
>
> //
>
> // Install Smbios Table
>
> //
>
> diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
> b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
> index 512105fafd..3332a30eae 100644
> --- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
> +++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
> @@ -1,7 +1,7 @@
> /** @file
>
> The header file of bootloader support DXE.
>
>
>
> -Copyright (c) 2014, Intel Corporation. All rights reserved.<BR>
>
> +Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
>
> **/
>
> @@ -19,12 +19,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include <Library/IoLib.h>
>
> #include <Library/HobLib.h>
>
>
>
> -#include <Guid/Acpi.h>
>
> #include <Guid/SmBios.h>
>
> #include <Guid/SystemTableInfoGuid.h>
>
> #include <Guid/AcpiBoardInfoGuid.h>
>
> #include <Guid/GraphicsInfoHob.h>
>
>
>
> -#include <IndustryStandard/Acpi.h>
>
> -
>
> #endif
>
> diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
> b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
> index cebc811355..ad4e0a57a1 100644
> --- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
> +++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
> @@ -1,9 +1,9 @@
> ## @file
>
> # Bootloader Support DXE Module
>
> #
>
> -# Report some MMIO/IO resources to dxe core, extract smbios and acpi tables
>
> +# Report some MMIO/IO resources to dxe core, extract smbios tables
>
> #
>
> -# Copyright (c) 2014 - 2020, Intel Corporation. All rights reserved.<BR>
>
> +# Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR>
>
> #
>
> # SPDX-License-Identifier: BSD-2-Clause-Patent
>
> #
>
> @@ -42,7 +42,6 @@
> HobLib
>
>
>
> [Guids]
>
> - gEfiAcpiTableGuid
>
> gEfiSmbiosTableGuid
>
> gUefiSystemTableInfoGuid
>
> gUefiAcpiBoardInfoGuid
>
> --
> 2.30.0.windows.2
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
2021-03-23 3:24 ` [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob Zhiguang Liu
@ 2021-03-23 12:40 ` Laszlo Ersek
2021-03-23 15:45 ` Guo Dong
2021-03-23 23:52 ` Benjamin Doron
0 siblings, 2 replies; 25+ messages in thread
From: Laszlo Ersek @ 2021-03-23 12:40 UTC (permalink / raw)
To: zhiguang.liu, Ray Ni, Dong Guo
Cc: devel, Jian J Wang, Hao A Wu, Dandan Bi, Liming Gao
On 03/23/21 04:24, Zhiguang Liu wrote:
> If HOB contains APCI table information, entry point of AcpiTableDxe.inf
> should parse the APCI table from HOB, and install these tables.
> We assume the whole ACPI table (starting with EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER)
> is contained by a single gEfiAcpiTableGuid HOB.
> This way, for UefiPayloadPkg, there is no need to specially hanle acpi table.
>
> Zhiguang Liu (2):
> MdeModulePkg/ACPI: Install ACPI table from HOB.
> UefiPayloadPkg: Remove code that installs APCI
>
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf | 3 ++-
> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 13 ++-----------
> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h | 5 +----
> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf | 5 ++---
> 5 files changed, 133 insertions(+), 27 deletions(-)
>
Where does this idea come from?
(1) There is no bugzilla for this, apparently (not linked in the commit
messages anyway).
(2) Also, I'm not sure if reusing an existing (and standardized) GUID
for this purpose is a good idea. I think an edk2-only
(MdeModulePkg-defined), brand new GUID, for the HOB, would be better.
(3) I'm also not convinced at all that this *whole approach* is sound.
The fact that UefiPayloadPkg has the ACPI content available to it in a
particular data representation (as a HOB, for example) is just a
platform trait. Why should that platform trait leak into the central
implementation of the ACPI table protocol?
UefiPayloadPkg can call the ACPI table protocol interfaces to install
its tables. OVMF does the same -- OVMF also does not construct its own
ACPI tables, but downloads them in a quirky representation from QEMU. We
didn't hack the central AcpiTableDxe driver for that use case; instead,
we dissected the blob (in OvmfPkg) into individual tables, and called
the proper ACPI table protocol method (InstallAcpiTable()) with the
individual tables.
I disagree with the code complexity / platform quirk in AcpiTableDxe. At
the bare minimum, this feature should be possible to compile out
altogether. I don't necessarily mean a FeaturePCD; there could be a new
INF file too, that shares most of the functionality with the current
core driver, but also includes (from a different source file) the new logic.
But I'd really like if this mess were kept out of MdeModulePkg
altogether. It's the job of the platform ACPI driver to use the ACPI
table protocol.
Of course if you can show that this HOB usage is standard / publicly
specified, that's different.
Please do not merge this series.
Laszlo
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
2021-03-23 12:40 ` [edk2-devel] " Laszlo Ersek
@ 2021-03-23 15:45 ` Guo Dong
2021-03-23 16:12 ` Andrew Fish
2021-03-23 16:48 ` Laszlo Ersek
2021-03-23 23:52 ` Benjamin Doron
1 sibling, 2 replies; 25+ messages in thread
From: Guo Dong @ 2021-03-23 15:45 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com, Liu, Zhiguang, Ni, Ray
Cc: Wang, Jian J, Wu, Hao A, Bi, Dandan, Liming Gao
Add my comments on the ideas behind.
UefiPayloadPkg is not a platform specific package, it tries to provide a generic payload using platform independent Modules. This allows to reuse the payload for different boot firmware solutions (Slim Bootloader, Coreboot, EDK2 bootloader) on different platforms.
Just like other DXE modules (e.g. variable DXE driver, firmware performance DXE driver), standardizing and modularizing platform independent modules through a HOB as the AcpiTableDxe patch did to support potential data from PEI/bootloader is a nature way for EDKII modules reuse. Understood the concerns to keep AcpiTableDxe as simple as possible. I think it deserve for code reuse by adding PEI/bootloader HOB support.
Thanks,
Guo
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> Sent: Tuesday, March 23, 2021 5:40 AM
> To: Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>; Dong,
> Guo <guo.dong@intel.com>
> Cc: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>
> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi
> table from Hob
>
> On 03/23/21 04:24, Zhiguang Liu wrote:
> > If HOB contains APCI table information, entry point of AcpiTableDxe.inf
> > should parse the APCI table from HOB, and install these tables.
> > We assume the whole ACPI table (starting with
> EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER)
> > is contained by a single gEfiAcpiTableGuid HOB.
> > This way, for UefiPayloadPkg, there is no need to specially hanle acpi table.
> >
> > Zhiguang Liu (2):
> > MdeModulePkg/ACPI: Install ACPI table from HOB.
> > UefiPayloadPkg: Remove code that installs APCI
> >
> > MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf | 3 ++-
> > MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 134
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> ----
> > UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 13 ++-----------
> > UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h | 5 +----
> > UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf | 5 ++---
> > 5 files changed, 133 insertions(+), 27 deletions(-)
> >
>
> Where does this idea come from?
>
> (1) There is no bugzilla for this, apparently (not linked in the commit
> messages anyway).
>
> (2) Also, I'm not sure if reusing an existing (and standardized) GUID
> for this purpose is a good idea. I think an edk2-only
> (MdeModulePkg-defined), brand new GUID, for the HOB, would be better.
>
> (3) I'm also not convinced at all that this *whole approach* is sound.
>
> The fact that UefiPayloadPkg has the ACPI content available to it in a
> particular data representation (as a HOB, for example) is just a
> platform trait. Why should that platform trait leak into the central
> implementation of the ACPI table protocol?
>
> UefiPayloadPkg can call the ACPI table protocol interfaces to install
> its tables. OVMF does the same -- OVMF also does not construct its own
> ACPI tables, but downloads them in a quirky representation from QEMU. We
> didn't hack the central AcpiTableDxe driver for that use case; instead,
> we dissected the blob (in OvmfPkg) into individual tables, and called
> the proper ACPI table protocol method (InstallAcpiTable()) with the
> individual tables.
>
> I disagree with the code complexity / platform quirk in AcpiTableDxe. At
> the bare minimum, this feature should be possible to compile out
> altogether. I don't necessarily mean a FeaturePCD; there could be a new
> INF file too, that shares most of the functionality with the current
> core driver, but also includes (from a different source file) the new logic.
>
> But I'd really like if this mess were kept out of MdeModulePkg
> altogether. It's the job of the platform ACPI driver to use the ACPI
> table protocol.
>
> Of course if you can show that this HOB usage is standard / publicly
> specified, that's different.
>
> Please do not merge this series.
>
> Laszlo
>
>
>
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
2021-03-23 15:45 ` Guo Dong
@ 2021-03-23 16:12 ` Andrew Fish
2021-03-23 17:29 ` Guo Dong
2021-03-24 5:30 ` Ni, Ray
2021-03-23 16:48 ` Laszlo Ersek
1 sibling, 2 replies; 25+ messages in thread
From: Andrew Fish @ 2021-03-23 16:12 UTC (permalink / raw)
To: edk2-devel-groups-io, Dong, Guo
Cc: lersek@redhat.com, Liu, Zhiguang, Ni, Ray, Wang, Jian J,
Wu, Hao A, Bi, Dandan, Liming Gao
[-- Attachment #1: Type: text/plain, Size: 5013 bytes --]
My concern is gEfiAcpiTableGuid is owned by the UEFI Spec and any off label usage should probably be defined by the PI Spec.
Is this a code 1st proposal or just an implementation?
Thanks,
Andrew Fish
> On Mar 23, 2021, at 8:45 AM, Guo Dong <guo.dong@intel.com> wrote:
>
>
> Add my comments on the ideas behind.
> UefiPayloadPkg is not a platform specific package, it tries to provide a generic payload using platform independent Modules. This allows to reuse the payload for different boot firmware solutions (Slim Bootloader, Coreboot, EDK2 bootloader) on different platforms.
>
> Just like other DXE modules (e.g. variable DXE driver, firmware performance DXE driver), standardizing and modularizing platform independent modules through a HOB as the AcpiTableDxe patch did to support potential data from PEI/bootloader is a nature way for EDKII modules reuse. Understood the concerns to keep AcpiTableDxe as simple as possible. I think it deserve for code reuse by adding PEI/bootloader HOB support.
>
> Thanks,
> Guo
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of Laszlo
>> Ersek
>> Sent: Tuesday, March 23, 2021 5:40 AM
>> To: Liu, Zhiguang <zhiguang.liu@intel.com <mailto:zhiguang.liu@intel.com>>; Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Dong,
>> Guo <guo.dong@intel.com <mailto:guo.dong@intel.com>>
>> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Wu, Hao A
>> <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Bi, Dandan <dandan.bi@intel.com <mailto:dandan.bi@intel.com>>; Liming Gao
>> <gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>>
>> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi
>> table from Hob
>>
>> On 03/23/21 04:24, Zhiguang Liu wrote:
>>> If HOB contains APCI table information, entry point of AcpiTableDxe.inf
>>> should parse the APCI table from HOB, and install these tables.
>>> We assume the whole ACPI table (starting with
>> EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER)
>>> is contained by a single gEfiAcpiTableGuid HOB.
>>> This way, for UefiPayloadPkg, there is no need to specially hanle acpi table.
>>>
>>> Zhiguang Liu (2):
>>> MdeModulePkg/ACPI: Install ACPI table from HOB.
>>> UefiPayloadPkg: Remove code that installs APCI
>>>
>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf | 3 ++-
>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 134
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>> ----
>>> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 13 ++-----------
>>> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h | 5 +----
>>> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf | 5 ++---
>>> 5 files changed, 133 insertions(+), 27 deletions(-)
>>>
>>
>> Where does this idea come from?
>>
>> (1) There is no bugzilla for this, apparently (not linked in the commit
>> messages anyway).
>>
>> (2) Also, I'm not sure if reusing an existing (and standardized) GUID
>> for this purpose is a good idea. I think an edk2-only
>> (MdeModulePkg-defined), brand new GUID, for the HOB, would be better.
>>
>> (3) I'm also not convinced at all that this *whole approach* is sound.
>>
>> The fact that UefiPayloadPkg has the ACPI content available to it in a
>> particular data representation (as a HOB, for example) is just a
>> platform trait. Why should that platform trait leak into the central
>> implementation of the ACPI table protocol?
>>
>> UefiPayloadPkg can call the ACPI table protocol interfaces to install
>> its tables. OVMF does the same -- OVMF also does not construct its own
>> ACPI tables, but downloads them in a quirky representation from QEMU. We
>> didn't hack the central AcpiTableDxe driver for that use case; instead,
>> we dissected the blob (in OvmfPkg) into individual tables, and called
>> the proper ACPI table protocol method (InstallAcpiTable()) with the
>> individual tables.
>>
>> I disagree with the code complexity / platform quirk in AcpiTableDxe. At
>> the bare minimum, this feature should be possible to compile out
>> altogether. I don't necessarily mean a FeaturePCD; there could be a new
>> INF file too, that shares most of the functionality with the current
>> core driver, but also includes (from a different source file) the new logic.
>>
>> But I'd really like if this mess were kept out of MdeModulePkg
>> altogether. It's the job of the platform ACPI driver to use the ACPI
>> table protocol.
>>
>> Of course if you can show that this HOB usage is standard / publicly
>> specified, that's different.
>>
>> Please do not merge this series.
>>
>> Laszlo
>>
>>
>>
>>
>>
>
>
>
>
[-- Attachment #2: Type: text/html, Size: 13810 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
2021-03-23 15:45 ` Guo Dong
2021-03-23 16:12 ` Andrew Fish
@ 2021-03-23 16:48 ` Laszlo Ersek
2021-03-23 17:15 ` Guo Dong
2021-03-24 4:09 ` Ni, Ray
1 sibling, 2 replies; 25+ messages in thread
From: Laszlo Ersek @ 2021-03-23 16:48 UTC (permalink / raw)
To: devel, guo.dong, Liu, Zhiguang, Ni, Ray
Cc: Wang, Jian J, Wu, Hao A, Bi, Dandan, Liming Gao, Andrew Fish
On 03/23/21 16:45, Guo Dong wrote:
>
> Add my comments on the ideas behind.
> UefiPayloadPkg is not a platform specific package, it tries to provide a generic payload using platform independent Modules. This allows to reuse the payload for different boot firmware solutions (Slim Bootloader, Coreboot, EDK2 bootloader) on different platforms.
>
> Just like other DXE modules (e.g. variable DXE driver, firmware performance DXE driver), standardizing and modularizing platform independent modules through a HOB as the AcpiTableDxe patch did to support potential data from PEI/bootloader is a nature way for EDKII modules reuse. Understood the concerns to keep AcpiTableDxe as simple as possible. I think it deserve for code reuse by adding PEI/bootloader HOB support.
I don't understand this argument.
(1) Currently, BlSupportDxe expects the ACPI content to come from
"SYSTEM_TABLE_INFO.AcpiTableBase" [Include/Guid/SystemTableInfoGuid.h].
That header file is at least *moderately* documented (it's better than
nothing). Additionally, BlSupportDxe is a DXE-phase component.
The patch set removes the handling of "SYSTEM_TABLE_INFO.AcpiTableBase"
from BlSupportDxe. That means that platforms currently relying on
BlSupportDxe to expose the ACPI content will break (until they start
producing the new HOB). I don't see the HOB (with this particular GUID)
being produced in UefiPayloadPkg anywhere.
(2) The UefiPayloadEntry module ("This is the first module for UEFI
payload") still relies on "SYSTEM_TABLE_INFO.AcpiTableBase", for parsing
various pieces of information into the "AcpiBoardInfo" structure. So
even if the HOB producer phase exposes the ACPI payload via a dedicated
HOB, it will only create inconsistency between the information parsed by
UefiPayloadEntry (from "SYSTEM_TABLE_INFO.AcpiTableBase") and the OS
(which will the ACPI contents from the dedicated HOB).
(3) The new HOB's structure (regardless of GUID) is not declared in any
MdeModulePkg header file, nor the "MdeModulePkg.dec" file. All the info
we have is hidden in the source code:
Rsdp = (EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER*)
(UINTN)(*((UINT64*)GET_GUID_HOB_DATA (GuidHob)));
If a platform's PEI phase actually inteded to produce this new HOB, it
couldn't rely on a header file / DEC file.
This is actually a *step back* from the SystemTableInfoGuid declaration
-- header file and DEC file -- that we currently have in UefiPayloadPkg.
So how can this be called "standardizing and modularizing"?
You need a new GUID, a new GUID HOB structure (declared in MdeModulePkg
DEC and GUID header); you need to spell out the priority order between
the HOB and "SYSTEM_TABLE_INFO.AcpiTableBase" for UefiPayloadPkg, and
you need to update all driver in UefiPayloadPkg accordingly.
I will also not make a secret of my annoyance that, the first time Intel
needs such a core extension for some platform feature, it immediately
gets all approvals. Whereas, when we needed the exact same feature in
OVMF, we struggled for months, if not *years*, to reliably split the
ACPI content that OVMF downloaded from QEMU, into blobs that were
suitable for the standard ACPI table protocol interfaces. For years I've
been telling my colleagues that all this complexity in OVMF's ACPI
platform driver is necessary because the EFI_ACPI_TABLE_PROTOCOL
implementation in edk2 cannot simply accept a "root pointer", to the
ACPI table "forest" that's already laid out in memory. Now I find it
just a little bit too convenient that the first time Intel needs the
same, we immediately call it "standardizing and modularizing" -- without
as much as a header file describing the actual contents of the new GUID HOB.
(Meanwhile we argue for months about actual, proven spec breakage in
edk2, such as signaling ready to boot around recovery options or
whatever. Standardization matters as long as *you* need it, huh?)
Laszlo
>
> Thanks,
> Guo
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
>> Ersek
>> Sent: Tuesday, March 23, 2021 5:40 AM
>> To: Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>; Dong,
>> Guo <guo.dong@intel.com>
>> Cc: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
>> <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Liming Gao
>> <gaoliming@byosoft.com.cn>
>> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi
>> table from Hob
>>
>> On 03/23/21 04:24, Zhiguang Liu wrote:
>>> If HOB contains APCI table information, entry point of AcpiTableDxe.inf
>>> should parse the APCI table from HOB, and install these tables.
>>> We assume the whole ACPI table (starting with
>> EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER)
>>> is contained by a single gEfiAcpiTableGuid HOB.
>>> This way, for UefiPayloadPkg, there is no need to specially hanle acpi table.
>>>
>>> Zhiguang Liu (2):
>>> MdeModulePkg/ACPI: Install ACPI table from HOB.
>>> UefiPayloadPkg: Remove code that installs APCI
>>>
>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf | 3 ++-
>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 134
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>> ----
>>> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 13 ++-----------
>>> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h | 5 +----
>>> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf | 5 ++---
>>> 5 files changed, 133 insertions(+), 27 deletions(-)
>>>
>>
>> Where does this idea come from?
>>
>> (1) There is no bugzilla for this, apparently (not linked in the commit
>> messages anyway).
>>
>> (2) Also, I'm not sure if reusing an existing (and standardized) GUID
>> for this purpose is a good idea. I think an edk2-only
>> (MdeModulePkg-defined), brand new GUID, for the HOB, would be better.
>>
>> (3) I'm also not convinced at all that this *whole approach* is sound.
>>
>> The fact that UefiPayloadPkg has the ACPI content available to it in a
>> particular data representation (as a HOB, for example) is just a
>> platform trait. Why should that platform trait leak into the central
>> implementation of the ACPI table protocol?
>>
>> UefiPayloadPkg can call the ACPI table protocol interfaces to install
>> its tables. OVMF does the same -- OVMF also does not construct its own
>> ACPI tables, but downloads them in a quirky representation from QEMU. We
>> didn't hack the central AcpiTableDxe driver for that use case; instead,
>> we dissected the blob (in OvmfPkg) into individual tables, and called
>> the proper ACPI table protocol method (InstallAcpiTable()) with the
>> individual tables.
>>
>> I disagree with the code complexity / platform quirk in AcpiTableDxe. At
>> the bare minimum, this feature should be possible to compile out
>> altogether. I don't necessarily mean a FeaturePCD; there could be a new
>> INF file too, that shares most of the functionality with the current
>> core driver, but also includes (from a different source file) the new logic.
>>
>> But I'd really like if this mess were kept out of MdeModulePkg
>> altogether. It's the job of the platform ACPI driver to use the ACPI
>> table protocol.
>>
>> Of course if you can show that this HOB usage is standard / publicly
>> specified, that's different.
>>
>> Please do not merge this series.
>>
>> Laszlo
>>
>>
>>
>>
>>
>
>
>
>
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
2021-03-23 16:48 ` Laszlo Ersek
@ 2021-03-23 17:15 ` Guo Dong
2021-03-24 9:50 ` Laszlo Ersek
2021-03-24 4:09 ` Ni, Ray
1 sibling, 1 reply; 25+ messages in thread
From: Guo Dong @ 2021-03-23 17:15 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io, Liu, Zhiguang, Ni, Ray
Cc: Wang, Jian J, Wu, Hao A, Bi, Dandan, Liming Gao, Andrew Fish
Hi Laszlo,
I don't mean currently UEFI payload is already standardized and modularized.
There are still lots of things to do and I think the AcpiTableDxe patch is the one it needed.
More information on ideas behind could be found from https://universalpayload.github.io/documentation/spec/spec.html.
A universal payload interface is proposed between the bootloader and the payload to allow reuse for the same payload for different boot firmware solutions on different platforms. It describes the interface between the bootloader and the payload, including what parameters need pass to payload, how to pass parameters to payload, the parameters format, the payload image format, and the payload boot mode, etc.
I am not saying UefiPayloadPkg would fully follow this proposed interface for now. But we are trying to align with the proposed interface under EDKII framework.
Thanks,
Guo
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, March 23, 2021 9:48 AM
> To: devel@edk2.groups.io; Dong, Guo <guo.dong@intel.com>; Liu, Zhiguang
> <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Bi, Dandan <dandan.bi@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
> Andrew Fish <afish@apple.com>
> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi
> table from Hob
>
> On 03/23/21 16:45, Guo Dong wrote:
> >
> > Add my comments on the ideas behind.
> > UefiPayloadPkg is not a platform specific package, it tries to provide a
> generic payload using platform independent Modules. This allows to reuse the
> payload for different boot firmware solutions (Slim Bootloader, Coreboot, EDK2
> bootloader) on different platforms.
> >
> > Just like other DXE modules (e.g. variable DXE driver, firmware performance
> DXE driver), standardizing and modularizing platform independent modules
> through a HOB as the AcpiTableDxe patch did to support potential data from
> PEI/bootloader is a nature way for EDKII modules reuse. Understood the
> concerns to keep AcpiTableDxe as simple as possible. I think it deserve for code
> reuse by adding PEI/bootloader HOB support.
>
> I don't understand this argument.
>
> (1) Currently, BlSupportDxe expects the ACPI content to come from
> "SYSTEM_TABLE_INFO.AcpiTableBase" [Include/Guid/SystemTableInfoGuid.h].
> That header file is at least *moderately* documented (it's better than
> nothing). Additionally, BlSupportDxe is a DXE-phase component.
>
> The patch set removes the handling of "SYSTEM_TABLE_INFO.AcpiTableBase"
> from BlSupportDxe. That means that platforms currently relying on
> BlSupportDxe to expose the ACPI content will break (until they start
> producing the new HOB). I don't see the HOB (with this particular GUID)
> being produced in UefiPayloadPkg anywhere.
>
> (2) The UefiPayloadEntry module ("This is the first module for UEFI
> payload") still relies on "SYSTEM_TABLE_INFO.AcpiTableBase", for parsing
> various pieces of information into the "AcpiBoardInfo" structure. So
> even if the HOB producer phase exposes the ACPI payload via a dedicated
> HOB, it will only create inconsistency between the information parsed by
> UefiPayloadEntry (from "SYSTEM_TABLE_INFO.AcpiTableBase") and the OS
> (which will the ACPI contents from the dedicated HOB).
>
> (3) The new HOB's structure (regardless of GUID) is not declared in any
> MdeModulePkg header file, nor the "MdeModulePkg.dec" file. All the info
> we have is hidden in the source code:
>
> Rsdp = (EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER*)
> (UINTN)(*((UINT64*)GET_GUID_HOB_DATA (GuidHob)));
>
> If a platform's PEI phase actually inteded to produce this new HOB, it
> couldn't rely on a header file / DEC file.
>
> This is actually a *step back* from the SystemTableInfoGuid declaration
> -- header file and DEC file -- that we currently have in UefiPayloadPkg.
>
>
> So how can this be called "standardizing and modularizing"?
>
> You need a new GUID, a new GUID HOB structure (declared in MdeModulePkg
> DEC and GUID header); you need to spell out the priority order between
> the HOB and "SYSTEM_TABLE_INFO.AcpiTableBase" for UefiPayloadPkg, and
> you need to update all driver in UefiPayloadPkg accordingly.
>
>
> I will also not make a secret of my annoyance that, the first time Intel
> needs such a core extension for some platform feature, it immediately
> gets all approvals. Whereas, when we needed the exact same feature in
> OVMF, we struggled for months, if not *years*, to reliably split the
> ACPI content that OVMF downloaded from QEMU, into blobs that were
> suitable for the standard ACPI table protocol interfaces. For years I've
> been telling my colleagues that all this complexity in OVMF's ACPI
> platform driver is necessary because the EFI_ACPI_TABLE_PROTOCOL
> implementation in edk2 cannot simply accept a "root pointer", to the
> ACPI table "forest" that's already laid out in memory. Now I find it
> just a little bit too convenient that the first time Intel needs the
> same, we immediately call it "standardizing and modularizing" -- without
> as much as a header file describing the actual contents of the new GUID HOB.
>
> (Meanwhile we argue for months about actual, proven spec breakage in
> edk2, such as signaling ready to boot around recovery options or
> whatever. Standardization matters as long as *you* need it, huh?)
>
> Laszlo
>
> >
> > Thanks,
> > Guo
> >
> >> -----Original Message-----
> >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> >> Ersek
> >> Sent: Tuesday, March 23, 2021 5:40 AM
> >> To: Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>;
> Dong,
> >> Guo <guo.dong@intel.com>
> >> Cc: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> >> <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Liming Gao
> >> <gaoliming@byosoft.com.cn>
> >> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi
> >> table from Hob
> >>
> >> On 03/23/21 04:24, Zhiguang Liu wrote:
> >>> If HOB contains APCI table information, entry point of AcpiTableDxe.inf
> >>> should parse the APCI table from HOB, and install these tables.
> >>> We assume the whole ACPI table (starting with
> >> EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER)
> >>> is contained by a single gEfiAcpiTableGuid HOB.
> >>> This way, for UefiPayloadPkg, there is no need to specially hanle acpi table.
> >>>
> >>> Zhiguang Liu (2):
> >>> MdeModulePkg/ACPI: Install ACPI table from HOB.
> >>> UefiPayloadPkg: Remove code that installs APCI
> >>>
> >>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf | 3 ++-
> >>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 134
> >>
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >>
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
> >> ----
> >>> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 13 ++-----------
> >>> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h | 5 +----
> >>> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf | 5 ++---
> >>> 5 files changed, 133 insertions(+), 27 deletions(-)
> >>>
> >>
> >> Where does this idea come from?
> >>
> >> (1) There is no bugzilla for this, apparently (not linked in the commit
> >> messages anyway).
> >>
> >> (2) Also, I'm not sure if reusing an existing (and standardized) GUID
> >> for this purpose is a good idea. I think an edk2-only
> >> (MdeModulePkg-defined), brand new GUID, for the HOB, would be better.
> >>
> >> (3) I'm also not convinced at all that this *whole approach* is sound.
> >>
> >> The fact that UefiPayloadPkg has the ACPI content available to it in a
> >> particular data representation (as a HOB, for example) is just a
> >> platform trait. Why should that platform trait leak into the central
> >> implementation of the ACPI table protocol?
> >>
> >> UefiPayloadPkg can call the ACPI table protocol interfaces to install
> >> its tables. OVMF does the same -- OVMF also does not construct its own
> >> ACPI tables, but downloads them in a quirky representation from QEMU. We
> >> didn't hack the central AcpiTableDxe driver for that use case; instead,
> >> we dissected the blob (in OvmfPkg) into individual tables, and called
> >> the proper ACPI table protocol method (InstallAcpiTable()) with the
> >> individual tables.
> >>
> >> I disagree with the code complexity / platform quirk in AcpiTableDxe. At
> >> the bare minimum, this feature should be possible to compile out
> >> altogether. I don't necessarily mean a FeaturePCD; there could be a new
> >> INF file too, that shares most of the functionality with the current
> >> core driver, but also includes (from a different source file) the new logic.
> >>
> >> But I'd really like if this mess were kept out of MdeModulePkg
> >> altogether. It's the job of the platform ACPI driver to use the ACPI
> >> table protocol.
> >>
> >> Of course if you can show that this HOB usage is standard / publicly
> >> specified, that's different.
> >>
> >> Please do not merge this series.
> >>
> >> Laszlo
> >>
> >>
> >>
> >>
> >>
> >
> >
> >
> >
> >
> >
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
2021-03-23 16:12 ` Andrew Fish
@ 2021-03-23 17:29 ` Guo Dong
2021-03-24 5:30 ` Ni, Ray
1 sibling, 0 replies; 25+ messages in thread
From: Guo Dong @ 2021-03-23 17:29 UTC (permalink / raw)
To: Andrew Fish, edk2-devel-groups-io
Cc: lersek@redhat.com, Liu, Zhiguang, Ni, Ray, Wang, Jian J,
Wu, Hao A, Bi, Dandan, Liming Gao
[-- Attachment #1: Type: text/plain, Size: 5512 bytes --]
The universal payload specification<https://universalpayload.github.io/documentation/spec/spec.html> proposes to re-use the existing GUID from UEFI for the HOB.
I don't know if we have to update the UEFI spec firstly in order to reuse the GUID.
Any idea?
Thanks,
Guo
From: Andrew Fish <afish@apple.com>
Sent: Tuesday, March 23, 2021 9:13 AM
To: edk2-devel-groups-io <devel@edk2.groups.io>; Dong, Guo <guo.dong@intel.com>
Cc: lersek@redhat.com; Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>
Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
My concern is gEfiAcpiTableGuid is owned by the UEFI Spec and any off label usage should probably be defined by the PI Spec.
Is this a code 1st proposal or just an implementation?
Thanks,
Andrew Fish
On Mar 23, 2021, at 8:45 AM, Guo Dong <guo.dong@intel.com<mailto:guo.dong@intel.com>> wrote:
Add my comments on the ideas behind.
UefiPayloadPkg is not a platform specific package, it tries to provide a generic payload using platform independent Modules. This allows to reuse the payload for different boot firmware solutions (Slim Bootloader, Coreboot, EDK2 bootloader) on different platforms.
Just like other DXE modules (e.g. variable DXE driver, firmware performance DXE driver), standardizing and modularizing platform independent modules through a HOB as the AcpiTableDxe patch did to support potential data from PEI/bootloader is a nature way for EDKII modules reuse. Understood the concerns to keep AcpiTableDxe as simple as possible. I think it deserve for code reuse by adding PEI/bootloader HOB support.
Thanks,
Guo
-----Original Message-----
From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Laszlo
Ersek
Sent: Tuesday, March 23, 2021 5:40 AM
To: Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Dong,
Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
<hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Bi, Dandan <dandan.bi@intel.com<mailto:dandan.bi@intel.com>>; Liming Gao
<gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>
Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi
table from Hob
On 03/23/21 04:24, Zhiguang Liu wrote:
If HOB contains APCI table information, entry point of AcpiTableDxe.inf
should parse the APCI table from HOB, and install these tables.
We assume the whole ACPI table (starting with
EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER)
is contained by a single gEfiAcpiTableGuid HOB.
This way, for UefiPayloadPkg, there is no need to specially hanle acpi table.
Zhiguang Liu (2):
MdeModulePkg/ACPI: Install ACPI table from HOB.
UefiPayloadPkg: Remove code that installs APCI
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf | 3 ++-
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 134
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
----
UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 13 ++-----------
UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h | 5 +----
UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf | 5 ++---
5 files changed, 133 insertions(+), 27 deletions(-)
Where does this idea come from?
(1) There is no bugzilla for this, apparently (not linked in the commit
messages anyway).
(2) Also, I'm not sure if reusing an existing (and standardized) GUID
for this purpose is a good idea. I think an edk2-only
(MdeModulePkg-defined), brand new GUID, for the HOB, would be better.
(3) I'm also not convinced at all that this *whole approach* is sound.
The fact that UefiPayloadPkg has the ACPI content available to it in a
particular data representation (as a HOB, for example) is just a
platform trait. Why should that platform trait leak into the central
implementation of the ACPI table protocol?
UefiPayloadPkg can call the ACPI table protocol interfaces to install
its tables. OVMF does the same -- OVMF also does not construct its own
ACPI tables, but downloads them in a quirky representation from QEMU. We
didn't hack the central AcpiTableDxe driver for that use case; instead,
we dissected the blob (in OvmfPkg) into individual tables, and called
the proper ACPI table protocol method (InstallAcpiTable()) with the
individual tables.
I disagree with the code complexity / platform quirk in AcpiTableDxe. At
the bare minimum, this feature should be possible to compile out
altogether. I don't necessarily mean a FeaturePCD; there could be a new
INF file too, that shares most of the functionality with the current
core driver, but also includes (from a different source file) the new logic.
But I'd really like if this mess were kept out of MdeModulePkg
altogether. It's the job of the platform ACPI driver to use the ACPI
table protocol.
Of course if you can show that this HOB usage is standard / publicly
specified, that's different.
Please do not merge this series.
Laszlo
[-- Attachment #2: Type: text/html, Size: 13106 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
2021-03-23 12:40 ` [edk2-devel] " Laszlo Ersek
2021-03-23 15:45 ` Guo Dong
@ 2021-03-23 23:52 ` Benjamin Doron
2021-03-24 9:53 ` Laszlo Ersek
1 sibling, 1 reply; 25+ messages in thread
From: Benjamin Doron @ 2021-03-23 23:52 UTC (permalink / raw)
To: Laszlo Ersek, devel
[-- Attachment #1: Type: text/plain, Size: 543 bytes --]
Hi all,
Would it be acceptable/feasible for AcpiTableDxe or AcpiPlatformDxe (in MdeModulePkg) to use `EfiGetSystemConfigurationTable` to get the RSDP and then install the tables? It's a solution that uses the regular UefiLib, so it avoids platform-specific quirks (and as I see it, if RSDP is in the configuration table, we probably always want those tables).
Regarding UefiPayloadPkg: AcpiTableDxe is currently compiled (listed in DSC) but not added to a FV (not listed in FDF). So, how has this been tested?
Regards,
Benjamin Doron
[-- Attachment #2: Type: text/html, Size: 567 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
2021-03-23 16:48 ` Laszlo Ersek
2021-03-23 17:15 ` Guo Dong
@ 2021-03-24 4:09 ` Ni, Ray
2021-03-24 10:29 ` Laszlo Ersek
1 sibling, 1 reply; 25+ messages in thread
From: Ni, Ray @ 2021-03-24 4:09 UTC (permalink / raw)
To: Laszlo Ersek, devel@edk2.groups.io, Dong, Guo, Liu, Zhiguang
Cc: Wang, Jian J, Wu, Hao A, Bi, Dandan, Liming Gao, Andrew Fish
>
> (1) Currently, BlSupportDxe expects the ACPI content to come from
> "SYSTEM_TABLE_INFO.AcpiTableBase"
> [Include/Guid/SystemTableInfoGuid.h].
> That header file is at least *moderately* documented (it's better than
> nothing). Additionally, BlSupportDxe is a DXE-phase component.
>
> The patch set removes the handling of
> "SYSTEM_TABLE_INFO.AcpiTableBase"
> from BlSupportDxe. That means that platforms currently relying on
> BlSupportDxe to expose the ACPI content will break (until they start
> producing the new HOB). I don't see the HOB (with this particular GUID)
> being produced in UefiPayloadPkg anywhere.
The concern is about PEI passing ACPI Table location through two kinds
of HOBs. It looks like a flaw in the design. I agree.
The HOB is produced by PEI phase, by some code that doesn't belong
to edk2 repo.
>
> (2) The UefiPayloadEntry module ("This is the first module for UEFI
> payload") still relies on "SYSTEM_TABLE_INFO.AcpiTableBase", for parsing
> various pieces of information into the "AcpiBoardInfo" structure. So
> even if the HOB producer phase exposes the ACPI payload via a dedicated
> HOB, it will only create inconsistency between the information parsed by
> UefiPayloadEntry (from "SYSTEM_TABLE_INFO.AcpiTableBase") and the OS
> (which will the ACPI contents from the dedicated HOB).
I agree. At least the SYSTEM_TABLE_INFO.AcpiTableBase field should be removed
and the accordingly code that consumes ACPI Table in BlSupportDxe should
be updated to consume the new HOB.
>
> (3) The new HOB's structure (regardless of GUID) is not declared in any
> MdeModulePkg header file, nor the "MdeModulePkg.dec" file. All the info
> we have is hidden in the source code:
>
> Rsdp = (EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER*)
> (UINTN)(*((UINT64*)GET_GUID_HOB_DATA (GuidHob)));
>
> If a platform's PEI phase actually inteded to produce this new HOB, it
> couldn't rely on a header file / DEC file.
>
> This is actually a *step back* from the SystemTableInfoGuid declaration
> -- header file and DEC file -- that we currently have in UefiPayloadPkg.
gEfiAcpiTableGuid is defined in MdePkg/Include/Guid/Acpi.h.
The file header says the GUID is used for entry in EFI system table.
Now we reuse this GUID for HOB data.
I think it's ok to use a spec defined GUID for another implementation purpose.
I can create a file MdeModulePkg/Include/Guid/Acpi.h to define the HOB structure.
Do you think it's ok?
>
>
> So how can this be called "standardizing and modularizing"?
>
> You need a new GUID, a new GUID HOB structure (declared in
> MdeModulePkg
> DEC and GUID header); you need to spell out the priority order between
> the HOB and "SYSTEM_TABLE_INFO.AcpiTableBase" for UefiPayloadPkg, and
> you need to update all driver in UefiPayloadPkg accordingly.
Again. I agree it's a flaw in the design. We should remove AcpiTableBase field.
>
>
> I will also not make a secret of my annoyance that, the first time Intel
> needs such a core extension for some platform feature, it immediately
> gets all approvals. Whereas, when we needed the exact same feature in
> OVMF, we struggled for months, if not *years*, to reliably split the
> ACPI content that OVMF downloaded from QEMU, into blobs that were
> suitable for the standard ACPI table protocol interfaces. For years I've
> been telling my colleagues that all this complexity in OVMF's ACPI
> platform driver is necessary because the EFI_ACPI_TABLE_PROTOCOL
> implementation in edk2 cannot simply accept a "root pointer", to the
> ACPI table "forest" that's already laid out in memory. Now I find it
> just a little bit too convenient that the first time Intel needs the
> same, we immediately call it "standardizing and modularizing" -- without
> as much as a header file describing the actual contents of the new GUID HOB.
I am not aware of the similar OVMF requirement.
The requirement here is to support different bootloaders that may already
create the essential ACPI table and DXE phase (payload) may use AcpiTable
protocol to install/update tables.
>
> (Meanwhile we argue for months about actual, proven spec breakage in
> edk2, such as signaling ready to boot around recovery options or
> whatever. Standardization matters as long as *you* need it, huh?)
The definition of spec breakage to me is we cannot do anything that's
conflict with the spec. But we can do things that spec doesn't define.
Please correct if I am wrong.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
2021-03-23 16:12 ` Andrew Fish
2021-03-23 17:29 ` Guo Dong
@ 2021-03-24 5:30 ` Ni, Ray
1 sibling, 0 replies; 25+ messages in thread
From: Ni, Ray @ 2021-03-24 5:30 UTC (permalink / raw)
To: Andrew Fish, edk2-devel-groups-io, Dong, Guo
Cc: lersek@redhat.com, Liu, Zhiguang, Wang, Jian J, Wu, Hao A,
Bi, Dandan, Liming Gao
[-- Attachment #1: Type: text/plain, Size: 5654 bytes --]
Andrew,
If the change is to use the gEfiAcpiTableGuid to identify another entry in the EFI configuration table, I agree it's a violation.
We position this as a pure implementation that reuses a spec defined GUID.
We didn't realize that it's a violation to the spec.
We could define a new GUID for the HOB data. But using the same GUID avoids introducing new GUID for the similar purpose.
Thanks,
Ray
From: Andrew Fish <afish@apple.com>
Sent: Wednesday, March 24, 2021 12:13 AM
To: edk2-devel-groups-io <devel@edk2.groups.io>; Dong, Guo <guo.dong@intel.com>
Cc: lersek@redhat.com; Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>
Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
My concern is gEfiAcpiTableGuid is owned by the UEFI Spec and any off label usage should probably be defined by the PI Spec.
Is this a code 1st proposal or just an implementation?
Thanks,
Andrew Fish
On Mar 23, 2021, at 8:45 AM, Guo Dong <guo.dong@intel.com<mailto:guo.dong@intel.com>> wrote:
Add my comments on the ideas behind.
UefiPayloadPkg is not a platform specific package, it tries to provide a generic payload using platform independent Modules. This allows to reuse the payload for different boot firmware solutions (Slim Bootloader, Coreboot, EDK2 bootloader) on different platforms.
Just like other DXE modules (e.g. variable DXE driver, firmware performance DXE driver), standardizing and modularizing platform independent modules through a HOB as the AcpiTableDxe patch did to support potential data from PEI/bootloader is a nature way for EDKII modules reuse. Understood the concerns to keep AcpiTableDxe as simple as possible. I think it deserve for code reuse by adding PEI/bootloader HOB support.
Thanks,
Guo
-----Original Message-----
From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Laszlo
Ersek
Sent: Tuesday, March 23, 2021 5:40 AM
To: Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Dong,
Guo <guo.dong@intel.com<mailto:guo.dong@intel.com>>
Cc: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
<hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; Bi, Dandan <dandan.bi@intel.com<mailto:dandan.bi@intel.com>>; Liming Gao
<gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>
Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi
table from Hob
On 03/23/21 04:24, Zhiguang Liu wrote:
If HOB contains APCI table information, entry point of AcpiTableDxe.inf
should parse the APCI table from HOB, and install these tables.
We assume the whole ACPI table (starting with
EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER)
is contained by a single gEfiAcpiTableGuid HOB.
This way, for UefiPayloadPkg, there is no need to specially hanle acpi table.
Zhiguang Liu (2):
MdeModulePkg/ACPI: Install ACPI table from HOB.
UefiPayloadPkg: Remove code that installs APCI
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf | 3 ++-
MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 134
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
----
UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 13 ++-----------
UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h | 5 +----
UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf | 5 ++---
5 files changed, 133 insertions(+), 27 deletions(-)
Where does this idea come from?
(1) There is no bugzilla for this, apparently (not linked in the commit
messages anyway).
(2) Also, I'm not sure if reusing an existing (and standardized) GUID
for this purpose is a good idea. I think an edk2-only
(MdeModulePkg-defined), brand new GUID, for the HOB, would be better.
(3) I'm also not convinced at all that this *whole approach* is sound.
The fact that UefiPayloadPkg has the ACPI content available to it in a
particular data representation (as a HOB, for example) is just a
platform trait. Why should that platform trait leak into the central
implementation of the ACPI table protocol?
UefiPayloadPkg can call the ACPI table protocol interfaces to install
its tables. OVMF does the same -- OVMF also does not construct its own
ACPI tables, but downloads them in a quirky representation from QEMU. We
didn't hack the central AcpiTableDxe driver for that use case; instead,
we dissected the blob (in OvmfPkg) into individual tables, and called
the proper ACPI table protocol method (InstallAcpiTable()) with the
individual tables.
I disagree with the code complexity / platform quirk in AcpiTableDxe. At
the bare minimum, this feature should be possible to compile out
altogether. I don't necessarily mean a FeaturePCD; there could be a new
INF file too, that shares most of the functionality with the current
core driver, but also includes (from a different source file) the new logic.
But I'd really like if this mess were kept out of MdeModulePkg
altogether. It's the job of the platform ACPI driver to use the ACPI
table protocol.
Of course if you can show that this HOB usage is standard / publicly
specified, that's different.
Please do not merge this series.
Laszlo
[-- Attachment #2: Type: text/html, Size: 11416 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
2021-03-23 17:15 ` Guo Dong
@ 2021-03-24 9:50 ` Laszlo Ersek
0 siblings, 0 replies; 25+ messages in thread
From: Laszlo Ersek @ 2021-03-24 9:50 UTC (permalink / raw)
To: Dong, Guo, devel@edk2.groups.io, Liu, Zhiguang, Ni, Ray
Cc: Wang, Jian J, Wu, Hao A, Bi, Dandan, Liming Gao, Andrew Fish
On 03/23/21 18:15, Dong, Guo wrote:
>
> Hi Laszlo,
>
> I don't mean currently UEFI payload is already standardized and modularized.
> There are still lots of things to do and I think the AcpiTableDxe patch is the one it needed.
>
> More information on ideas behind could be found from https://universalpayload.github.io/documentation/spec/spec.html.
> A universal payload interface is proposed between the bootloader and the payload to allow reuse for the same payload for different boot firmware solutions on different platforms. It describes the interface between the bootloader and the payload, including what parameters need pass to payload, how to pass parameters to payload, the parameters format, the payload image format, and the payload boot mode, etc.
Sounds good, but then I would say a new GUID is needed even more.
Laszlo
>
> I am not saying UefiPayloadPkg would fully follow this proposed interface for now. But we are trying to align with the proposed interface under EDKII framework.
>
> Thanks,
> Guo
>
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Tuesday, March 23, 2021 9:48 AM
>> To: devel@edk2.groups.io; Dong, Guo <guo.dong@intel.com>; Liu, Zhiguang
>> <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>
>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
>> Bi, Dandan <dandan.bi@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>;
>> Andrew Fish <afish@apple.com>
>> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi
>> table from Hob
>>
>> On 03/23/21 16:45, Guo Dong wrote:
>>>
>>> Add my comments on the ideas behind.
>>> UefiPayloadPkg is not a platform specific package, it tries to provide a
>> generic payload using platform independent Modules. This allows to reuse the
>> payload for different boot firmware solutions (Slim Bootloader, Coreboot, EDK2
>> bootloader) on different platforms.
>>>
>>> Just like other DXE modules (e.g. variable DXE driver, firmware performance
>> DXE driver), standardizing and modularizing platform independent modules
>> through a HOB as the AcpiTableDxe patch did to support potential data from
>> PEI/bootloader is a nature way for EDKII modules reuse. Understood the
>> concerns to keep AcpiTableDxe as simple as possible. I think it deserve for code
>> reuse by adding PEI/bootloader HOB support.
>>
>> I don't understand this argument.
>>
>> (1) Currently, BlSupportDxe expects the ACPI content to come from
>> "SYSTEM_TABLE_INFO.AcpiTableBase" [Include/Guid/SystemTableInfoGuid.h].
>> That header file is at least *moderately* documented (it's better than
>> nothing). Additionally, BlSupportDxe is a DXE-phase component.
>>
>> The patch set removes the handling of "SYSTEM_TABLE_INFO.AcpiTableBase"
>> from BlSupportDxe. That means that platforms currently relying on
>> BlSupportDxe to expose the ACPI content will break (until they start
>> producing the new HOB). I don't see the HOB (with this particular GUID)
>> being produced in UefiPayloadPkg anywhere.
>>
>> (2) The UefiPayloadEntry module ("This is the first module for UEFI
>> payload") still relies on "SYSTEM_TABLE_INFO.AcpiTableBase", for parsing
>> various pieces of information into the "AcpiBoardInfo" structure. So
>> even if the HOB producer phase exposes the ACPI payload via a dedicated
>> HOB, it will only create inconsistency between the information parsed by
>> UefiPayloadEntry (from "SYSTEM_TABLE_INFO.AcpiTableBase") and the OS
>> (which will the ACPI contents from the dedicated HOB).
>>
>> (3) The new HOB's structure (regardless of GUID) is not declared in any
>> MdeModulePkg header file, nor the "MdeModulePkg.dec" file. All the info
>> we have is hidden in the source code:
>>
>> Rsdp = (EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER*)
>> (UINTN)(*((UINT64*)GET_GUID_HOB_DATA (GuidHob)));
>>
>> If a platform's PEI phase actually inteded to produce this new HOB, it
>> couldn't rely on a header file / DEC file.
>>
>> This is actually a *step back* from the SystemTableInfoGuid declaration
>> -- header file and DEC file -- that we currently have in UefiPayloadPkg.
>>
>>
>> So how can this be called "standardizing and modularizing"?
>>
>> You need a new GUID, a new GUID HOB structure (declared in MdeModulePkg
>> DEC and GUID header); you need to spell out the priority order between
>> the HOB and "SYSTEM_TABLE_INFO.AcpiTableBase" for UefiPayloadPkg, and
>> you need to update all driver in UefiPayloadPkg accordingly.
>>
>>
>> I will also not make a secret of my annoyance that, the first time Intel
>> needs such a core extension for some platform feature, it immediately
>> gets all approvals. Whereas, when we needed the exact same feature in
>> OVMF, we struggled for months, if not *years*, to reliably split the
>> ACPI content that OVMF downloaded from QEMU, into blobs that were
>> suitable for the standard ACPI table protocol interfaces. For years I've
>> been telling my colleagues that all this complexity in OVMF's ACPI
>> platform driver is necessary because the EFI_ACPI_TABLE_PROTOCOL
>> implementation in edk2 cannot simply accept a "root pointer", to the
>> ACPI table "forest" that's already laid out in memory. Now I find it
>> just a little bit too convenient that the first time Intel needs the
>> same, we immediately call it "standardizing and modularizing" -- without
>> as much as a header file describing the actual contents of the new GUID HOB.
>>
>> (Meanwhile we argue for months about actual, proven spec breakage in
>> edk2, such as signaling ready to boot around recovery options or
>> whatever. Standardization matters as long as *you* need it, huh?)
>>
>> Laszlo
>>
>>>
>>> Thanks,
>>> Guo
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
>>>> Ersek
>>>> Sent: Tuesday, March 23, 2021 5:40 AM
>>>> To: Liu, Zhiguang <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>;
>> Dong,
>>>> Guo <guo.dong@intel.com>
>>>> Cc: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
>>>> <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Liming Gao
>>>> <gaoliming@byosoft.com.cn>
>>>> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi
>>>> table from Hob
>>>>
>>>> On 03/23/21 04:24, Zhiguang Liu wrote:
>>>>> If HOB contains APCI table information, entry point of AcpiTableDxe.inf
>>>>> should parse the APCI table from HOB, and install these tables.
>>>>> We assume the whole ACPI table (starting with
>>>> EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER)
>>>>> is contained by a single gEfiAcpiTableGuid HOB.
>>>>> This way, for UefiPayloadPkg, there is no need to specially hanle acpi table.
>>>>>
>>>>> Zhiguang Liu (2):
>>>>> MdeModulePkg/ACPI: Install ACPI table from HOB.
>>>>> UefiPayloadPkg: Remove code that installs APCI
>>>>>
>>>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf | 3 ++-
>>>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 134
>>>>
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>>>> ----
>>>>> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 13 ++-----------
>>>>> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h | 5 +----
>>>>> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf | 5 ++---
>>>>> 5 files changed, 133 insertions(+), 27 deletions(-)
>>>>>
>>>>
>>>> Where does this idea come from?
>>>>
>>>> (1) There is no bugzilla for this, apparently (not linked in the commit
>>>> messages anyway).
>>>>
>>>> (2) Also, I'm not sure if reusing an existing (and standardized) GUID
>>>> for this purpose is a good idea. I think an edk2-only
>>>> (MdeModulePkg-defined), brand new GUID, for the HOB, would be better.
>>>>
>>>> (3) I'm also not convinced at all that this *whole approach* is sound.
>>>>
>>>> The fact that UefiPayloadPkg has the ACPI content available to it in a
>>>> particular data representation (as a HOB, for example) is just a
>>>> platform trait. Why should that platform trait leak into the central
>>>> implementation of the ACPI table protocol?
>>>>
>>>> UefiPayloadPkg can call the ACPI table protocol interfaces to install
>>>> its tables. OVMF does the same -- OVMF also does not construct its own
>>>> ACPI tables, but downloads them in a quirky representation from QEMU. We
>>>> didn't hack the central AcpiTableDxe driver for that use case; instead,
>>>> we dissected the blob (in OvmfPkg) into individual tables, and called
>>>> the proper ACPI table protocol method (InstallAcpiTable()) with the
>>>> individual tables.
>>>>
>>>> I disagree with the code complexity / platform quirk in AcpiTableDxe. At
>>>> the bare minimum, this feature should be possible to compile out
>>>> altogether. I don't necessarily mean a FeaturePCD; there could be a new
>>>> INF file too, that shares most of the functionality with the current
>>>> core driver, but also includes (from a different source file) the new logic.
>>>>
>>>> But I'd really like if this mess were kept out of MdeModulePkg
>>>> altogether. It's the job of the platform ACPI driver to use the ACPI
>>>> table protocol.
>>>>
>>>> Of course if you can show that this HOB usage is standard / publicly
>>>> specified, that's different.
>>>>
>>>> Please do not merge this series.
>>>>
>>>> Laszlo
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>>
>>>
>>>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
2021-03-23 23:52 ` Benjamin Doron
@ 2021-03-24 9:53 ` Laszlo Ersek
2021-03-24 16:55 ` Benjamin Doron
0 siblings, 1 reply; 25+ messages in thread
From: Laszlo Ersek @ 2021-03-24 9:53 UTC (permalink / raw)
To: Benjamin Doron, devel
On 03/24/21 00:52, Benjamin Doron wrote:
> Hi all,
> Would it be acceptable/feasible for AcpiTableDxe or AcpiPlatformDxe (in
> MdeModulePkg) to use `EfiGetSystemConfigurationTable` to get the RSDP
> and then install the tables? It's a solution that uses the regular
> UefiLib, so it avoids platform-specific quirks (and as I see it, if RSDP
> is in the configuration table, we probably always want those tables).
I'm sorry, I don't understand how this would help.
> Regarding UefiPayloadPkg: AcpiTableDxe is currently compiled (listed in
> DSC) but not added to a FV (not listed in FDF). So, how has this been
> tested?
I assume through an out-of-tree platform. Many such core modules exist
in edk2 that are not consumed by any of the virtual platforms in the
edk2 repo itself (EmulatorPkg, ArmVirtPkg, OvmfPkg).
Thanks
Laszlo
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
2021-03-24 4:09 ` Ni, Ray
@ 2021-03-24 10:29 ` Laszlo Ersek
0 siblings, 0 replies; 25+ messages in thread
From: Laszlo Ersek @ 2021-03-24 10:29 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io, Dong, Guo, Liu, Zhiguang
Cc: Wang, Jian J, Wu, Hao A, Bi, Dandan, Liming Gao, Andrew Fish
On 03/24/21 05:09, Ni, Ray wrote:
>>
>> (1) Currently, BlSupportDxe expects the ACPI content to come from
>> "SYSTEM_TABLE_INFO.AcpiTableBase"
>> [Include/Guid/SystemTableInfoGuid.h].
>> That header file is at least *moderately* documented (it's better than
>> nothing). Additionally, BlSupportDxe is a DXE-phase component.
>>
>> The patch set removes the handling of
>> "SYSTEM_TABLE_INFO.AcpiTableBase"
>> from BlSupportDxe. That means that platforms currently relying on
>> BlSupportDxe to expose the ACPI content will break (until they start
>> producing the new HOB). I don't see the HOB (with this particular GUID)
>> being produced in UefiPayloadPkg anywhere.
>
> The concern is about PEI passing ACPI Table location through two kinds
> of HOBs. It looks like a flaw in the design. I agree.
>
> The HOB is produced by PEI phase, by some code that doesn't belong
> to edk2 repo.
>
>>
>> (2) The UefiPayloadEntry module ("This is the first module for UEFI
>> payload") still relies on "SYSTEM_TABLE_INFO.AcpiTableBase", for parsing
>> various pieces of information into the "AcpiBoardInfo" structure. So
>> even if the HOB producer phase exposes the ACPI payload via a dedicated
>> HOB, it will only create inconsistency between the information parsed by
>> UefiPayloadEntry (from "SYSTEM_TABLE_INFO.AcpiTableBase") and the OS
>> (which will the ACPI contents from the dedicated HOB).
>
> I agree. At least the SYSTEM_TABLE_INFO.AcpiTableBase field should be removed
> and the accordingly code that consumes ACPI Table in BlSupportDxe should
> be updated to consume the new HOB.
>
>>
>> (3) The new HOB's structure (regardless of GUID) is not declared in any
>> MdeModulePkg header file, nor the "MdeModulePkg.dec" file. All the info
>> we have is hidden in the source code:
>>
>> Rsdp = (EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER*)
>> (UINTN)(*((UINT64*)GET_GUID_HOB_DATA (GuidHob)));
>>
>> If a platform's PEI phase actually inteded to produce this new HOB, it
>> couldn't rely on a header file / DEC file.
>>
>> This is actually a *step back* from the SystemTableInfoGuid declaration
>> -- header file and DEC file -- that we currently have in UefiPayloadPkg.
>
> gEfiAcpiTableGuid is defined in MdePkg/Include/Guid/Acpi.h.
> The file header says the GUID is used for entry in EFI system table.
> Now we reuse this GUID for HOB data.
> I think it's ok to use a spec defined GUID for another implementation purpose.
>
> I can create a file MdeModulePkg/Include/Guid/Acpi.h to define the HOB structure.
> Do you think it's ok?
I'd prefer a new GUID. New GUIDs are very easy to introduce; we won't
run out of them. It's always possible to use a new GUID for some purpose
that relates to an existent GUID. However, in the other direction, it's
difficult to split one use of a GUID from another use of the same GUID.
It's OK if the PI and UEFI specs agree on reusing gEfiAcpiTableGuid for
a new GUIDed HOB -- because they are managed by the same organization.
There's going to be coordination between those two spec working groups.
But the use case here is different.
It's not a problem to use "Acpi" in the GUID's symbolic name, in edk2.
An example for that is "gEfiAcpiVariableGuid" in
"MdeModulePkg/Include/Guid/AcpiS3Context.h". (Well, I think it should
not have used "Efi" in the name, but "Acpi" was fine.
"gEdkiiAcpiVariableGuid" would have been better.)
Especially if this is going to be documented in the universal boot
loader spec, then that spec should propose some C-language prefixes
anyway, such as (just a guess) "Ubl", and then one idea would be
"gUblAcpiTablesGuid", for the HOB.
>
>>
>>
>> So how can this be called "standardizing and modularizing"?
>>
>> You need a new GUID, a new GUID HOB structure (declared in
>> MdeModulePkg
>> DEC and GUID header); you need to spell out the priority order between
>> the HOB and "SYSTEM_TABLE_INFO.AcpiTableBase" for UefiPayloadPkg, and
>> you need to update all driver in UefiPayloadPkg accordingly.
>
> Again. I agree it's a flaw in the design. We should remove AcpiTableBase field.
>
>>
>>
>> I will also not make a secret of my annoyance that, the first time Intel
>> needs such a core extension for some platform feature, it immediately
>> gets all approvals. Whereas, when we needed the exact same feature in
>> OVMF, we struggled for months, if not *years*, to reliably split the
>> ACPI content that OVMF downloaded from QEMU, into blobs that were
>> suitable for the standard ACPI table protocol interfaces. For years I've
>> been telling my colleagues that all this complexity in OVMF's ACPI
>> platform driver is necessary because the EFI_ACPI_TABLE_PROTOCOL
>> implementation in edk2 cannot simply accept a "root pointer", to the
>> ACPI table "forest" that's already laid out in memory. Now I find it
>> just a little bit too convenient that the first time Intel needs the
>> same, we immediately call it "standardizing and modularizing" -- without
>> as much as a header file describing the actual contents of the new GUID HOB.
>
> I am not aware of the similar OVMF requirement.
>
> The requirement here is to support different bootloaders that may already
> create the essential ACPI table and DXE phase (payload) may use AcpiTable
> protocol to install/update tables.
The requirement has been the same with QEMU, for a very long time. QEMU
generates ACPI tables (more precisely, ACPI blobs) at runtime, in a
particular format. The guest firmware allocates memory, downloads the
blobs into them, and then performs a number of steps on them that QEMU
dictates in a "command script". This command script has commands like
"download this blob into memory you allocate", "update this pointer in
this ACPI blob to point at a specific offset in another ACPI blob",
"recalculate checksum over this range, and store the result at this
offset", and so on. The idea is that QEMU generates the ACPI content,
but the guest firmware places it the content into guest RAM, and then
(according to the QEMU instructions) fixes up the blobs, so they are
valid ACPI tables in the end. This allows the guest firmware to remain
independent of (or, put differently, "blind to") the ACPI content that
QEMU generates. This is a very important design goal, because the
platform hardware in QEMU changes frequently, and with it, the ACPI
content has to change together. If we kept the ACPI content in the guest
firmware (SeaBIOS and OVMF), then SeaBIOS and OVMF would have to be
updated in lock-step with QEMU. That's unsustainable.
What OVMF does is that interprets the command script, laying out the
tables / blobs in guest RAM as QEMU requires. However, when it's done,
it doesn't just use the RSDP / RSDT / XSDT from QEMU as the root tables,
circumventing AcpiTableDxe. Instead, OVMF traverses the tables itself,
"structurally", and identifies memory addresses that are "most likely"
the start offsets of ACPI tables. And then OVMF passes those to
EFI_ACPI_TABLE_PROTOCOL.InstallTable(). In the end, the originally laid
out tables (the result of the command script execution) is freed, and
only those ACPI tables will exist that EFI_ACPI_TABLE_PROTOCOL created
internally.
If the HOB you are now trying to introduce had existed 6-8 years ago,
the work I had to do in OvmfPkg's AcpiPlatformDxe would have been a
*fraction* of what I ended up doing.
This is one reason why I'm so annoyed. There's a double standard in edk2
core module maintenance. Extending the core of edk2 is more or less
*equally useful* for different platform owners, but those platform
owners face *very different difficulties* when they actually attempt the
core extensions. I never even attempted extending AcpiTableDxe in
MdeModulePkg, because I knew it was tied to the spec, and was convinced
that my QEMU-motivated request would be rejected anyway.
>> (Meanwhile we argue for months about actual, proven spec breakage in
>> edk2, such as signaling ready to boot around recovery options or
>> whatever. Standardization matters as long as *you* need it, huh?)
>
> The definition of spec breakage to me is we cannot do anything that's
> conflict with the spec. But we can do things that spec doesn't define.
> Please correct if I am wrong.
Sure, here's a counter-example: when I proposed adding new status codes
to UefiBootManagerLib, so that boot option loading and boot option
starting would be broadcast to the system using the "report status code"
facility, with rich data (boot option device path, and EFI_STATUS
result), you said that you didn't want to add such extensions to
UefiBootManagerLib; you wanted it to do what the spec required, and
*only* what the spec required. You told me to go change the spec first.
My proposal was entirely transparent to the PI and UEFI specs, both the
introduction of the new status codes (and the associated information
structure(s)), and the logic to emit them.
* [edk2] [PATCH 0/5]
MdeModulePkg, OvmfPkg: more easily visible boot progress reporting
http://mid.mail-archive.com/20171122235849.4177-1-lersek@redhat.com
We have a new process for that now ("code first", although I'm unsure
how it works in practice).
My point is: don't cut corners. It's already *much easier* for Intel to
extend core modules than for other contributors -- because the core
module maintainers mostly come from Intel, and they can negotiate and
get aligned with the Intel *platform owners* before anything happens on
the list! --, so at least *how* you implement the extension should be
squeaky clean. Don't make other platforms pay a steep price (in
complexity or code size that cannot be compiled out), don't reuse
standard GUIDs, don't shirk documentation requirements (new header for
the GUID structure), and so on.
Laszlo
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
2021-03-24 9:53 ` Laszlo Ersek
@ 2021-03-24 16:55 ` Benjamin Doron
2021-03-24 18:33 ` Laszlo Ersek
0 siblings, 1 reply; 25+ messages in thread
From: Benjamin Doron @ 2021-03-24 16:55 UTC (permalink / raw)
To: Laszlo Ersek, devel
[-- Attachment #1: Type: text/plain, Size: 1599 bytes --]
>
>
>> Hi all,
>> Would it be acceptable/feasible for AcpiTableDxe or AcpiPlatformDxe (in
>> MdeModulePkg) to use `EfiGetSystemConfigurationTable` to get the RSDP
>> and then install the tables? It's a solution that uses the regular
>> UefiLib, so it avoids platform-specific quirks (and as I see it, if RSDP
>> is in the configuration table, we probably always want those tables).
>
> I'm sorry, I don't understand how this would help.
As I understand it, the issue is that this patchset changes MdeModulePkg to do platform-specific parsing.
Perhaps it would be an acceptable solution for platforms to retrieve the tables, then add
RSDP/them to the configuration table to be installed by AcpiTableDxe/AcpiPlatformDxe.
This allows MdeModulePkg to abstract away the parsing, only installing tables
available to it.
(Currently, UefiPayloadPkg's BlSupportDxe retrieves the data from a HOB and calls
`gBS->InstallConfigurationTable` with the address of RSDP).
I understand that this may not work for OVMF if tables are located differently in memory.
>
>
>> Regarding UefiPayloadPkg: AcpiTableDxe is currently compiled (listed in
>> DSC) but not added to a FV (not listed in FDF). So, how has this been
>> tested?
>
> I assume through an out-of-tree platform. Many such core modules exist
> in edk2 that are not consumed by any of the virtual platforms in the
> edk2 repo itself (EmulatorPkg, ArmVirtPkg, OvmfPkg).
This makes sense, but AcpiTableDxe must be added to UefiPayloadPkg's FDF
if patch 2/2 is merged. Otherwise, ACPI tables will not be advertised.
[-- Attachment #2: Type: text/html, Size: 1723 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
2021-03-24 16:55 ` Benjamin Doron
@ 2021-03-24 18:33 ` Laszlo Ersek
2021-03-25 1:10 ` Ni, Ray
2021-03-25 1:39 ` Benjamin Doron
0 siblings, 2 replies; 25+ messages in thread
From: Laszlo Ersek @ 2021-03-24 18:33 UTC (permalink / raw)
To: Benjamin Doron, devel
On 03/24/21 17:55, Benjamin Doron wrote:
>>
>>
>>> Hi all,
>>> Would it be acceptable/feasible for AcpiTableDxe or AcpiPlatformDxe (in
>>> MdeModulePkg) to use `EfiGetSystemConfigurationTable` to get the RSDP
>>> and then install the tables? It's a solution that uses the regular
>>> UefiLib, so it avoids platform-specific quirks (and as I see it, if RSDP
>>> is in the configuration table, we probably always want those tables).
>>
>> I'm sorry, I don't understand how this would help.
>
> As I understand it, the issue is that this patchset changes MdeModulePkg to do platform-specific parsing.
>
> Perhaps it would be an acceptable solution for platforms to retrieve the tables, then add
> RSDP/them to the configuration table to be installed by AcpiTableDxe/AcpiPlatformDxe.
> This allows MdeModulePkg to abstract away the parsing, only installing tables
> available to it.
But this is already the best approach, and already what's happening --
when you call EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() from the
platform's AcpiPlatformDxe, that's *how* you tell AcpiTableDxe in
MdeModulePkg to pick up the table and hook it into the RSDT / XSDT /
wherever, and also to manage RSD PTR as a UEFI configuration table.
Are you (more or less) proposing a new EFI_ACPI_TABLE_PROTOCOL member
function for taking a forest of ACPI tables, passed in by RSD PTR?
Sorry about being dense. :)
> (Currently, UefiPayloadPkg's BlSupportDxe retrieves the data from a HOB and calls
> `gBS->InstallConfigurationTable` with the address of RSDP).
>
> I understand that this may not work for OVMF if tables are located differently in memory.
>
>>
>>
>>> Regarding UefiPayloadPkg: AcpiTableDxe is currently compiled (listed in
>>> DSC) but not added to a FV (not listed in FDF). So, how has this been
>>> tested?
>>
>> I assume through an out-of-tree platform. Many such core modules exist
>> in edk2 that are not consumed by any of the virtual platforms in the
>> edk2 repo itself (EmulatorPkg, ArmVirtPkg, OvmfPkg).
>
> This makes sense, but AcpiTableDxe must be added to UefiPayloadPkg's FDF
> if patch 2/2 is merged. Otherwise, ACPI tables will not be advertised.
>
Good point; I missed that. (I'm not familiar with the UefiPayloadPkg FDF
at all.)
Laszlo
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
2021-03-24 18:33 ` Laszlo Ersek
@ 2021-03-25 1:10 ` Ni, Ray
2021-03-25 3:55 ` Andrew Fish
2021-03-25 17:33 ` Laszlo Ersek
2021-03-25 1:39 ` Benjamin Doron
1 sibling, 2 replies; 25+ messages in thread
From: Ni, Ray @ 2021-03-25 1:10 UTC (permalink / raw)
To: devel@edk2.groups.io, lersek@redhat.com, Benjamin Doron
Ben,
I understand your point.
The purpose of changing the AcpiTableDxe to directly consume the HOB is to reduce the overall system complexity.
The complexity I see with your option is:
1. platform needs to include that driver to consume the ACPI table from bootloader
2. that driver (consume HOB and install table through AcpiTable protocol) needs to register a protocol notification on AcpiTable protocol and do the table installation in the callback.
Laszlo,
The change here is to meet the requirement that bootloader provides the ACPI table.
In OVMF case, it's not the bootloader but the virtual hardware who provides the ACPI table.
I can see the similarity between the two: the ACPI table is produced before the UEFI Payload runs.
Can you review the new option below and see whether it can meet the OVMF needs as well?
1. Define a new GUID gPldHobAcpiTable in MdeModulePkg.dec and a structure as below in MdeModulePkg/Include/Guid/Acpi.h.
typedef struct {
UINT64 TableAddress;
} PLD_HOB_ACPI_TABLE;
2. Change AcpiTableDxe driver to consume the HOB
3. Remove SYSTEM_TABLE_INFO. AcpiTableBase/AcpiTableSize.
4. Remove the BlSupportDxe code that consume the ACPI table in SYSTEM_TABLE_INFO.
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
> Sent: Thursday, March 25, 2021 2:33 AM
> To: Benjamin Doron <benjamin.doron00@gmail.com>; devel@edk2.groups.io
> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
>
> On 03/24/21 17:55, Benjamin Doron wrote:
> >>
> >>
> >>> Hi all,
> >>> Would it be acceptable/feasible for AcpiTableDxe or AcpiPlatformDxe (in
> >>> MdeModulePkg) to use `EfiGetSystemConfigurationTable` to get the RSDP
> >>> and then install the tables? It's a solution that uses the regular
> >>> UefiLib, so it avoids platform-specific quirks (and as I see it, if RSDP
> >>> is in the configuration table, we probably always want those tables).
> >>
> >> I'm sorry, I don't understand how this would help.
> >
> > As I understand it, the issue is that this patchset changes MdeModulePkg to do platform-specific parsing.
> >
> > Perhaps it would be an acceptable solution for platforms to retrieve the tables, then add
> > RSDP/them to the configuration table to be installed by AcpiTableDxe/AcpiPlatformDxe.
> > This allows MdeModulePkg to abstract away the parsing, only installing tables
> > available to it.
>
> But this is already the best approach, and already what's happening --
> when you call EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() from the
> platform's AcpiPlatformDxe, that's *how* you tell AcpiTableDxe in
> MdeModulePkg to pick up the table and hook it into the RSDT / XSDT /
> wherever, and also to manage RSD PTR as a UEFI configuration table.
>
> Are you (more or less) proposing a new EFI_ACPI_TABLE_PROTOCOL member
> function for taking a forest of ACPI tables, passed in by RSD PTR?
>
> Sorry about being dense. :)
>
> > (Currently, UefiPayloadPkg's BlSupportDxe retrieves the data from a HOB and calls
> > `gBS->InstallConfigurationTable` with the address of RSDP).
> >
> > I understand that this may not work for OVMF if tables are located differently in memory.
> >
> >>
> >>
> >>> Regarding UefiPayloadPkg: AcpiTableDxe is currently compiled (listed in
> >>> DSC) but not added to a FV (not listed in FDF). So, how has this been
> >>> tested?
> >>
> >> I assume through an out-of-tree platform. Many such core modules exist
> >> in edk2 that are not consumed by any of the virtual platforms in the
> >> edk2 repo itself (EmulatorPkg, ArmVirtPkg, OvmfPkg).
> >
> > This makes sense, but AcpiTableDxe must be added to UefiPayloadPkg's FDF
> > if patch 2/2 is merged. Otherwise, ACPI tables will not be advertised.
> >
>
> Good point; I missed that. (I'm not familiar with the UefiPayloadPkg FDF
> at all.)
>
> Laszlo
>
>
>
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
2021-03-24 18:33 ` Laszlo Ersek
2021-03-25 1:10 ` Ni, Ray
@ 2021-03-25 1:39 ` Benjamin Doron
1 sibling, 0 replies; 25+ messages in thread
From: Benjamin Doron @ 2021-03-25 1:39 UTC (permalink / raw)
To: Laszlo Ersek, devel
[-- Attachment #1: Type: text/plain, Size: 2186 bytes --]
On Wed, Mar 24, 2021 at 02:33 PM, Laszlo Ersek wrote:
>
>
>>
>>>
>>>> Hi all,
>>>> Would it be acceptable/feasible for AcpiTableDxe or AcpiPlatformDxe (in
>>>> MdeModulePkg) to use `EfiGetSystemConfigurationTable` to get the RSDP
>>>> and then install the tables? It's a solution that uses the regular
>>>> UefiLib, so it avoids platform-specific quirks (and as I see it, if RSDP
>>>> is in the configuration table, we probably always want those tables).
>>>
>>> I'm sorry, I don't understand how this would help.
>>
>> As I understand it, the issue is that this patchset changes MdeModulePkg
>> to do platform-specific parsing.
>>
>> Perhaps it would be an acceptable solution for platforms to retrieve the
>> tables, then add
>> RSDP/them to the configuration table to be installed by
>> AcpiTableDxe/AcpiPlatformDxe.
>> This allows MdeModulePkg to abstract away the parsing, only installing
>> tables
>> available to it.
>
> But this is already the best approach, and already what's happening --
> when you call EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() from the
> platform's AcpiPlatformDxe, that's *how* you tell AcpiTableDxe in
> MdeModulePkg to pick up the table and hook it into the RSDT / XSDT /
> wherever, and also to manage RSD PTR as a UEFI configuration table.
>
> Are you (more or less) proposing a new EFI_ACPI_TABLE_PROTOCOL member
> function for taking a forest of ACPI tables, passed in by RSD PTR?
Yes.
I thought the implementation of AcpiPlatformDxe in MdeModulePkg could take it, so it will install
ACPI tables from flash or from memory.
Regarding UefiPayloadPkg: gEfiAcpiTableGuid is not a HOB. It's an entry in
gUefiSystemTableInfoGuid (which is a HOB) that was installed in the config table. Therefore,
retrieving its GUID as a HOB in AcpiTableDxe fails.
To make this patchset work in UefiPayloadPkg (actually a fork), I have to add AcpiTableDxe to its
FDF, drop patch 2/2 and make `InstallAcpiTableFromHob` do
`Status = EfiGetSystemConfigurationTable (&gEfiAcpiTableGuid, (VOID **) &Rsdp);`.
Then, this patchset works: "acpiview" shell command shows tables from QEMU + coreboot, as well
as a BGRT.
[-- Attachment #2: Type: text/html, Size: 2300 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
2021-03-25 1:10 ` Ni, Ray
@ 2021-03-25 3:55 ` Andrew Fish
2021-03-25 17:35 ` Laszlo Ersek
2021-03-25 17:33 ` Laszlo Ersek
1 sibling, 1 reply; 25+ messages in thread
From: Andrew Fish @ 2021-03-25 3:55 UTC (permalink / raw)
To: edk2-devel-groups-io, ray.ni; +Cc: lersek@redhat.com, Benjamin Doron
[-- Attachment #1: Type: text/plain, Size: 4917 bytes --]
Ray,
The other option would be to define a Library Class for the AcpiTableDxe driver to consume and have a NULL version of it in the MdeModulePkg. That would allow more flexibility? It kind of depends if you want to make the implementation depend on a HOB or a LibraryClass? It at least gives the non pure PI implementations more potential options? I’m not sure with the problem you are trying to solve that helps or hurts, but I though I would throw out another option. The other advantage about the lib is could let you define the HOB in another package, if that makes more sense.
Not trying to slow down the solution, but just giving some other options in case another partitioning of this problem would be helpful?
Thanks,
Andrew Fish
> On Mar 24, 2021, at 6:10 PM, Ni, Ray <ray.ni@intel.com> wrote:
>
> Ben,
> I understand your point.
> The purpose of changing the AcpiTableDxe to directly consume the HOB is to reduce the overall system complexity.
> The complexity I see with your option is:
> 1. platform needs to include that driver to consume the ACPI table from bootloader
> 2. that driver (consume HOB and install table through AcpiTable protocol) needs to register a protocol notification on AcpiTable protocol and do the table installation in the callback.
>
> Laszlo,
> The change here is to meet the requirement that bootloader provides the ACPI table.
> In OVMF case, it's not the bootloader but the virtual hardware who provides the ACPI table.
> I can see the similarity between the two: the ACPI table is produced before the UEFI Payload runs.
> Can you review the new option below and see whether it can meet the OVMF needs as well?
> 1. Define a new GUID gPldHobAcpiTable in MdeModulePkg.dec and a structure as below in MdeModulePkg/Include/Guid/Acpi.h.
> typedef struct {
> UINT64 TableAddress;
> } PLD_HOB_ACPI_TABLE;
> 2. Change AcpiTableDxe driver to consume the HOB
> 3. Remove SYSTEM_TABLE_INFO. AcpiTableBase/AcpiTableSize.
> 4. Remove the BlSupportDxe code that consume the ACPI table in SYSTEM_TABLE_INFO.
>
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Thursday, March 25, 2021 2:33 AM
>> To: Benjamin Doron <benjamin.doron00@gmail.com>; devel@edk2.groups.io
>> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
>>
>> On 03/24/21 17:55, Benjamin Doron wrote:
>>>>
>>>>
>>>>> Hi all,
>>>>> Would it be acceptable/feasible for AcpiTableDxe or AcpiPlatformDxe (in
>>>>> MdeModulePkg) to use `EfiGetSystemConfigurationTable` to get the RSDP
>>>>> and then install the tables? It's a solution that uses the regular
>>>>> UefiLib, so it avoids platform-specific quirks (and as I see it, if RSDP
>>>>> is in the configuration table, we probably always want those tables).
>>>>
>>>> I'm sorry, I don't understand how this would help.
>>>
>>> As I understand it, the issue is that this patchset changes MdeModulePkg to do platform-specific parsing.
>>>
>>> Perhaps it would be an acceptable solution for platforms to retrieve the tables, then add
>>> RSDP/them to the configuration table to be installed by AcpiTableDxe/AcpiPlatformDxe.
>>> This allows MdeModulePkg to abstract away the parsing, only installing tables
>>> available to it.
>>
>> But this is already the best approach, and already what's happening --
>> when you call EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() from the
>> platform's AcpiPlatformDxe, that's *how* you tell AcpiTableDxe in
>> MdeModulePkg to pick up the table and hook it into the RSDT / XSDT /
>> wherever, and also to manage RSD PTR as a UEFI configuration table.
>>
>> Are you (more or less) proposing a new EFI_ACPI_TABLE_PROTOCOL member
>> function for taking a forest of ACPI tables, passed in by RSD PTR?
>>
>> Sorry about being dense. :)
>>
>>> (Currently, UefiPayloadPkg's BlSupportDxe retrieves the data from a HOB and calls
>>> `gBS->InstallConfigurationTable` with the address of RSDP).
>>>
>>> I understand that this may not work for OVMF if tables are located differently in memory.
>>>
>>>>
>>>>
>>>>> Regarding UefiPayloadPkg: AcpiTableDxe is currently compiled (listed in
>>>>> DSC) but not added to a FV (not listed in FDF). So, how has this been
>>>>> tested?
>>>>
>>>> I assume through an out-of-tree platform. Many such core modules exist
>>>> in edk2 that are not consumed by any of the virtual platforms in the
>>>> edk2 repo itself (EmulatorPkg, ArmVirtPkg, OvmfPkg).
>>>
>>> This makes sense, but AcpiTableDxe must be added to UefiPayloadPkg's FDF
>>> if patch 2/2 is merged. Otherwise, ACPI tables will not be advertised.
>>>
>>
>> Good point; I missed that. (I'm not familiar with the UefiPayloadPkg FDF
>> at all.)
>>
>> Laszlo
>>
>>
>>
>>
>>
>
>
>
>
[-- Attachment #2: Type: text/html, Size: 22332 bytes --]
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
2021-03-25 1:10 ` Ni, Ray
2021-03-25 3:55 ` Andrew Fish
@ 2021-03-25 17:33 ` Laszlo Ersek
1 sibling, 0 replies; 25+ messages in thread
From: Laszlo Ersek @ 2021-03-25 17:33 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io, Benjamin Doron
On 03/25/21 02:10, Ni, Ray wrote:
> Ben,
> I understand your point.
> The purpose of changing the AcpiTableDxe to directly consume the HOB is to reduce the overall system complexity.
> The complexity I see with your option is:
> 1. platform needs to include that driver to consume the ACPI table from bootloader
> 2. that driver (consume HOB and install table through AcpiTable protocol) needs to register a protocol notification on AcpiTable protocol and do the table installation in the callback.
>
> Laszlo,
> The change here is to meet the requirement that bootloader provides the ACPI table.
> In OVMF case, it's not the bootloader but the virtual hardware who provides the ACPI table.
> I can see the similarity between the two: the ACPI table is produced before the UEFI Payload runs.
> Can you review the new option below and see whether it can meet the OVMF needs as well?
Let me clarify: there's no way I'm touching that part of OVMF. I don't
want any potential regressions there. It's been working stably for years.
I'd just like to avoid a duplication of functionality -- if the new HOB
logic in MdeModulePkg is heavy, then I'd like a possibility for
platforms to separate it out.
> 1. Define a new GUID gPldHobAcpiTable in MdeModulePkg.dec and a structure as below in MdeModulePkg/Include/Guid/Acpi.h.
> typedef struct {
> UINT64 TableAddress;
> } PLD_HOB_ACPI_TABLE;
Looks good (but maybe use EFI_PHYSICAL_ADDRESS for type, and a more
telling name than "TableAddress" -- name precisely what ACPI table type
the pointer refers to?)
> 2. Change AcpiTableDxe driver to consume the HOB
Yes. And this is the part that, if it's complex or large, should go into
a separate source file (together with a new INF file), or be controlled
by a Feature PCD.
If it's not complex / large, and you can refactor AcpiTableDxe first
such that the HOB-based functionality is not littered over a bunch of
functions, then it's OK to stick with just one INF file (and no Feature
PCD).
> 3. Remove SYSTEM_TABLE_INFO. AcpiTableBase/AcpiTableSize.
Won't that break other systems that currently depend on it? Just asking.
I'm neutral, personally.
> 4. Remove the BlSupportDxe code that consume the ACPI table in SYSTEM_TABLE_INFO.
Same compatibility question for existent, dependent platforms.
Thanks
Laszlo
>
>
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Thursday, March 25, 2021 2:33 AM
>> To: Benjamin Doron <benjamin.doron00@gmail.com>; devel@edk2.groups.io
>> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
>>
>> On 03/24/21 17:55, Benjamin Doron wrote:
>>>>
>>>>
>>>>> Hi all,
>>>>> Would it be acceptable/feasible for AcpiTableDxe or AcpiPlatformDxe (in
>>>>> MdeModulePkg) to use `EfiGetSystemConfigurationTable` to get the RSDP
>>>>> and then install the tables? It's a solution that uses the regular
>>>>> UefiLib, so it avoids platform-specific quirks (and as I see it, if RSDP
>>>>> is in the configuration table, we probably always want those tables).
>>>>
>>>> I'm sorry, I don't understand how this would help.
>>>
>>> As I understand it, the issue is that this patchset changes MdeModulePkg to do platform-specific parsing.
>>>
>>> Perhaps it would be an acceptable solution for platforms to retrieve the tables, then add
>>> RSDP/them to the configuration table to be installed by AcpiTableDxe/AcpiPlatformDxe.
>>> This allows MdeModulePkg to abstract away the parsing, only installing tables
>>> available to it.
>>
>> But this is already the best approach, and already what's happening --
>> when you call EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() from the
>> platform's AcpiPlatformDxe, that's *how* you tell AcpiTableDxe in
>> MdeModulePkg to pick up the table and hook it into the RSDT / XSDT /
>> wherever, and also to manage RSD PTR as a UEFI configuration table.
>>
>> Are you (more or less) proposing a new EFI_ACPI_TABLE_PROTOCOL member
>> function for taking a forest of ACPI tables, passed in by RSD PTR?
>>
>> Sorry about being dense. :)
>>
>>> (Currently, UefiPayloadPkg's BlSupportDxe retrieves the data from a HOB and calls
>>> `gBS->InstallConfigurationTable` with the address of RSDP).
>>>
>>> I understand that this may not work for OVMF if tables are located differently in memory.
>>>
>>>>
>>>>
>>>>> Regarding UefiPayloadPkg: AcpiTableDxe is currently compiled (listed in
>>>>> DSC) but not added to a FV (not listed in FDF). So, how has this been
>>>>> tested?
>>>>
>>>> I assume through an out-of-tree platform. Many such core modules exist
>>>> in edk2 that are not consumed by any of the virtual platforms in the
>>>> edk2 repo itself (EmulatorPkg, ArmVirtPkg, OvmfPkg).
>>>
>>> This makes sense, but AcpiTableDxe must be added to UefiPayloadPkg's FDF
>>> if patch 2/2 is merged. Otherwise, ACPI tables will not be advertised.
>>>
>>
>> Good point; I missed that. (I'm not familiar with the UefiPayloadPkg FDF
>> at all.)
>>
>> Laszlo
>>
>>
>>
>>
>>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob
2021-03-25 3:55 ` Andrew Fish
@ 2021-03-25 17:35 ` Laszlo Ersek
0 siblings, 0 replies; 25+ messages in thread
From: Laszlo Ersek @ 2021-03-25 17:35 UTC (permalink / raw)
To: Andrew Fish, edk2-devel-groups-io, ray.ni; +Cc: Benjamin Doron
On 03/25/21 04:55, Andrew Fish wrote:
> Ray,
>
> The other option would be to define a Library Class for the AcpiTableDxe driver to consume and have a NULL version of it in the MdeModulePkg. That would allow more flexibility? It kind of depends if you want to make the implementation depend on a HOB or a LibraryClass? It at least gives the non pure PI implementations more potential options? I’m not sure with the problem you are trying to solve that helps or hurts, but I though I would throw out another option. The other advantage about the lib is could let you define the HOB in another package, if that makes more sense.
>
> Not trying to slow down the solution, but just giving some other options in case another partitioning of this problem would be helpful?
I agree this is another option for separating out the HOB consumption
logic, in case it is complex or large.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2021-03-25 17:35 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-03-23 3:24 [Patch V2 1/2] MdeModulePkg/ACPI: Install ACPI table from HOB Zhiguang Liu
2021-03-23 3:24 ` [Patch V2 2/2] UefiPayloadPkg: Remove code that installs APCI Zhiguang Liu
2021-03-23 3:44 ` Ni, Ray
2021-03-23 5:19 ` Guo Dong
2021-03-23 3:24 ` [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob Zhiguang Liu
2021-03-23 12:40 ` [edk2-devel] " Laszlo Ersek
2021-03-23 15:45 ` Guo Dong
2021-03-23 16:12 ` Andrew Fish
2021-03-23 17:29 ` Guo Dong
2021-03-24 5:30 ` Ni, Ray
2021-03-23 16:48 ` Laszlo Ersek
2021-03-23 17:15 ` Guo Dong
2021-03-24 9:50 ` Laszlo Ersek
2021-03-24 4:09 ` Ni, Ray
2021-03-24 10:29 ` Laszlo Ersek
2021-03-23 23:52 ` Benjamin Doron
2021-03-24 9:53 ` Laszlo Ersek
2021-03-24 16:55 ` Benjamin Doron
2021-03-24 18:33 ` Laszlo Ersek
2021-03-25 1:10 ` Ni, Ray
2021-03-25 3:55 ` Andrew Fish
2021-03-25 17:35 ` Laszlo Ersek
2021-03-25 17:33 ` Laszlo Ersek
2021-03-25 1:39 ` Benjamin Doron
2021-03-23 3:44 ` [Patch V2 1/2] MdeModulePkg/ACPI: Install ACPI table from HOB Ni, Ray
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox