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

["Andrew Fish" <afish@apple.com>]

[-- Attachment #1: Type: text/plain, Size: 5013 bytes --]

My concern is gEfiAcpiTableGuid is owned by the UEFI Spec and any off label usage should probably be defined by the PI Spec. 

Is this a code 1st proposal or just an implementation? 

Thanks,

Andrew Fish

> On Mar 23, 2021, at 8:45 AM, Guo Dong <guo.dong@intel.com> wrote:
> 
> 
> Add my comments on the ideas behind.
> UefiPayloadPkg is not a platform specific package, it tries to provide a generic payload using platform independent Modules. This allows to reuse the payload for different boot firmware solutions (Slim Bootloader, Coreboot, EDK2 bootloader) on different platforms.
> 
> Just like other DXE modules (e.g. variable DXE driver,  firmware performance DXE driver), standardizing and modularizing platform independent modules through a HOB as the AcpiTableDxe patch did to support potential data from PEI/bootloader is a nature way for EDKII modules reuse. Understood the concerns to keep AcpiTableDxe as simple as possible. I think it deserve for code reuse by adding PEI/bootloader HOB support.
> 
> Thanks,
> Guo
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of Laszlo
>> Ersek
>> Sent: Tuesday, March 23, 2021 5:40 AM
>> To: Liu, Zhiguang <zhiguang.liu@intel.com <mailto:zhiguang.liu@intel.com>>; Ni, Ray <ray.ni@intel.com <mailto:ray.ni@intel.com>>; Dong,
>> Guo <guo.dong@intel.com <mailto:guo.dong@intel.com>>
>> Cc: devel@edk2.groups.io <mailto:devel@edk2.groups.io>; Wang, Jian J <jian.j.wang@intel.com <mailto:jian.j.wang@intel.com>>; Wu, Hao A
>> <hao.a.wu@intel.com <mailto:hao.a.wu@intel.com>>; Bi, Dandan <dandan.bi@intel.com <mailto:dandan.bi@intel.com>>; Liming Gao
>> <gaoliming@byosoft.com.cn <mailto:gaoliming@byosoft.com.cn>>
>> Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi
>> table from Hob
>> 
>> On 03/23/21 04:24, Zhiguang Liu wrote:
>>> If HOB contains APCI table information, entry point of AcpiTableDxe.inf
>>> should parse the APCI table from HOB, and install these tables.
>>> We assume the whole ACPI table (starting with
>> EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER)
>>> is contained by a single gEfiAcpiTableGuid HOB.
>>> This way, for UefiPayloadPkg, there is no need to specially hanle acpi table.
>>> 
>>> Zhiguang Liu (2):
>>>  MdeModulePkg/ACPI: Install ACPI table from HOB.
>>>  UefiPayloadPkg: Remove code that installs APCI
>>> 
>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf    |   3 ++-
>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 134
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
>> ----
>>> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c                   |  13 ++-----------
>>> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h                   |   5 +----
>>> UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf                 |   5 ++---
>>> 5 files changed, 133 insertions(+), 27 deletions(-)
>>> 
>> 
>> Where does this idea come from?
>> 
>> (1) There is no bugzilla for this, apparently (not linked in the commit
>> messages anyway).
>> 
>> (2) Also, I'm not sure if reusing an existing (and standardized) GUID
>> for this purpose is a good idea. I think an edk2-only
>> (MdeModulePkg-defined), brand new GUID, for the HOB, would be better.
>> 
>> (3) I'm also not convinced at all that this *whole approach* is sound.
>> 
>> The fact that UefiPayloadPkg has the ACPI content available to it in a
>> particular data representation (as a HOB, for example) is just a
>> platform trait. Why should that platform trait leak into the central
>> implementation of the ACPI table protocol?
>> 
>> UefiPayloadPkg can call the ACPI table protocol interfaces to install
>> its tables. OVMF does the same -- OVMF also does not construct its own
>> ACPI tables, but downloads them in a quirky representation from QEMU. We
>> didn't hack the central AcpiTableDxe driver for that use case; instead,
>> we dissected the blob (in OvmfPkg) into individual tables, and called
>> the proper ACPI table protocol method (InstallAcpiTable()) with the
>> individual tables.
>> 
>> I disagree with the code complexity / platform quirk in AcpiTableDxe. At
>> the bare minimum, this feature should be possible to compile out
>> altogether. I don't necessarily mean a FeaturePCD; there could be a new
>> INF file too, that shares most of the functionality with the current
>> core driver, but also includes (from a different source file) the new logic.
>> 
>> But I'd really like if this mess were kept out of MdeModulePkg
>> altogether. It's the job of the platform ACPI driver to use the ACPI
>> table protocol.
>> 
>> Of course if you can show that this HOB usage is standard / publicly
>> specified, that's different.
>> 
>> Please do not merge this series.
>> 
>> Laszlo
>> 
>> 
>> 
>> 
>> 
> 
> 
> 
> 


[-- Attachment #2: Type: text/html, Size: 13810 bytes --]