[Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob


Zhiguang Liu
 

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


Laszlo Ersek
 

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


Guo Dong
 

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





Andrew Fish
 

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@...> 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 <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: Tuesday, March 23, 2021 5:40 AM
To: Liu, Zhiguang <zhiguang.liu@...>; Ni, Ray <ray.ni@...>; Dong,
Guo <guo.dong@...>
Cc: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@...>; Wu, Hao A
<hao.a.wu@...>; Bi, Dandan <dandan.bi@...>; Liming Gao
<gaoliming@...>
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










Laszlo Ersek
 

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









Guo Dong
 

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









Guo Dong
 

 

The universal payload specification 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@...>
Sent: Tuesday, March 23, 2021 9:13 AM
To: edk2-devel-groups-io <devel@edk2.groups.io>; Dong, Guo <guo.dong@...>
Cc: lersek@...; Liu, Zhiguang <zhiguang.liu@...>; Ni, Ray <ray.ni@...>; Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Bi, Dandan <dandan.bi@...>; Liming Gao <gaoliming@...>
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@...> 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 <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: Tuesday, March 23, 2021 5:40 AM
To: Liu, Zhiguang <
zhiguang.liu@...>; Ni, Ray <ray.ni@...>; Dong,
Guo <
guo.dong@...>
Cc: 
devel@edk2.groups.io; Wang, Jian J <jian.j.wang@...>; Wu, Hao A
<
hao.a.wu@...>; Bi, Dandan <dandan.bi@...>; Liming Gao
<
gaoliming@...>
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







 


Ni, Ray
 


(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.


Ni, Ray
 

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@...>
Sent: Wednesday, March 24, 2021 12:13 AM
To: edk2-devel-groups-io <devel@edk2.groups.io>; Dong, Guo <guo.dong@...>
Cc: lersek@...; Liu, Zhiguang <zhiguang.liu@...>; Ni, Ray <ray.ni@...>; Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Bi, Dandan <dandan.bi@...>; Liming Gao <gaoliming@...>
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@...> 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 <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: Tuesday, March 23, 2021 5:40 AM
To: Liu, Zhiguang <zhiguang.liu@...>; Ni, Ray <ray.ni@...>; Dong,
Guo <guo.dong@...>
Cc: devel@edk2.groups.io; Wang, Jian J <jian.j.wang@...>; Wu, Hao A
<hao.a.wu@...>; Bi, Dandan <dandan.bi@...>; Liming Gao
<gaoliming@...>
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







 


Laszlo Ersek
 

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









Laszlo Ersek
 

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


Laszlo Ersek
 

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


Benjamin Doron
 

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


Benjamin Doron
 

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.


Laszlo Ersek
 

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


Ni, Ray
 

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





Benjamin Doron
 

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.


Andrew Fish
 

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@...> 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@...>; 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










Laszlo Ersek
 

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





Laszlo Ersek
 

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