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.d...@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....@intel.com <mailto:zhiguang....@intel.com>>; >> Ni, Ray <ray...@intel.com <mailto:ray...@intel.com>>; Dong, >> Guo <guo.d...@intel.com <mailto:guo.d...@intel.com>> >> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Wang, Jian J >> <jian.j.w...@intel.com <mailto:jian.j.w...@intel.com>>; Wu, Hao A >> <hao.a...@intel.com <mailto:hao.a...@intel.com>>; Bi, Dandan >> <dandan...@intel.com <mailto:dandan...@intel.com>>; Liming Gao >> <gaolim...@byosoft.com.cn <mailto:gaolim...@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 >> >> >> >> >> > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#73189): https://edk2.groups.io/g/devel/message/73189 Mute This Topic: https://groups.io/mt/81543419/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
