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 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 > On Behalf Of Laszlo >> Ersek >> Sent: Tuesday, March 23, 2021 5:40 AM >> To: Liu, Zhiguang >; Ni, Ray >; Dong, >> Guo > >> Cc: devel@edk2.groups.io ; Wang, Jian J >; Wu, Hao A >> >; Bi, Dandan >; Liming Gao >> > >> 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 >> >> >> >> >> > > > >