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 <ler...@redhat.com>
>> Sent: Tuesday, March 23, 2021 9:48 AM
>> To: devel@edk2.groups.io; Dong, Guo <guo.d...@intel.com>; Liu, Zhiguang
>> <zhiguang....@intel.com>; Ni, Ray <ray...@intel.com>
>> Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>;
>> Bi, Dandan <dandan...@intel.com>; Liming Gao <gaolim...@byosoft.com.cn>;
>> Andrew Fish <af...@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....@intel.com>; Ni, Ray <ray...@intel.com>;
>> Dong,
>>>> Guo <guo.d...@intel.com>
>>>> Cc: devel@edk2.groups.io; Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A
>>>> <hao.a...@intel.com>; Bi, Dandan <dandan...@intel.com>; Liming Gao
>>>> <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 (#73202): https://edk2.groups.io/g/devel/message/73202
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]
-=-=-=-=-=-=-=-=-=-=-=-
