Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob

["Laszlo Ersek" <lersek@redhat.com>]

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