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

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

On 03/24/21 05:09, Ni, Ray wrote:
>>
>> (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.
> 
> The concern is about PEI passing ACPI Table location through two kinds
> of HOBs. It looks like a flaw in the design. I agree.
> 
> The HOB is produced by PEI phase, by some code that doesn't belong
> to edk2 repo.
> 
>>
>> (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).
> 
> I agree. At least the SYSTEM_TABLE_INFO.AcpiTableBase field should be removed
> and the accordingly code that consumes ACPI Table in BlSupportDxe should
> be updated to consume the new 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.
> 
> gEfiAcpiTableGuid is defined in MdePkg/Include/Guid/Acpi.h.
> The file header says the GUID is used for entry in EFI system table.
> Now we reuse this GUID for HOB data.
> I think it's ok to use a spec defined GUID for another implementation purpose.
> 
> I can create a file MdeModulePkg/Include/Guid/Acpi.h to define the HOB structure.
> Do you think it's ok?

I'd prefer a new GUID. New GUIDs are very easy to introduce; we won't
run out of them. It's always possible to use a new GUID for some purpose
that relates to an existent GUID. However, in the other direction, it's
difficult to split one use of a GUID from another use of the same GUID.

It's OK if the PI and UEFI specs agree on reusing gEfiAcpiTableGuid for
a new GUIDed HOB -- because they are managed by the same organization.
There's going to be coordination between those two spec working groups.
But the use case here is different.

It's not a problem to use "Acpi" in the GUID's symbolic name, in edk2.
An example for that is "gEfiAcpiVariableGuid" in
"MdeModulePkg/Include/Guid/AcpiS3Context.h". (Well, I think it should
not have used "Efi" in the name, but "Acpi" was fine.
"gEdkiiAcpiVariableGuid" would have been better.)

Especially if this is going to be documented in the universal boot
loader spec, then that spec should propose some C-language prefixes
anyway, such as (just a guess) "Ubl", and then one idea would be
"gUblAcpiTablesGuid", for the HOB.

> 
>>
>>
>> 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.
> 
> Again. I agree it's a flaw in the design. We should remove AcpiTableBase field.
> 
>>
>>
>> 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.
> 
> I am not aware of the similar OVMF requirement.
> 
> The requirement here is to support different bootloaders that may already
> create the essential ACPI table and DXE phase (payload) may use AcpiTable
> protocol to install/update tables.

The requirement has been the same with QEMU, for a very long time. QEMU
generates ACPI tables (more precisely, ACPI blobs) at runtime, in a
particular format. The guest firmware allocates memory, downloads the
blobs into them, and then performs a number of steps on them that QEMU
dictates in a "command script". This command script has commands like
"download this blob into memory you allocate", "update this pointer in
this ACPI blob to point at a specific offset in another ACPI blob",
"recalculate checksum over this range, and store the result at this
offset", and so on. The idea is that QEMU generates the ACPI content,
but the guest firmware places it the content into guest RAM, and then
(according to the QEMU instructions) fixes up the blobs, so they are
valid ACPI tables in the end. This allows the guest firmware to remain
independent of (or, put differently, "blind to") the ACPI content that
QEMU generates. This is a very important design goal, because the
platform hardware in QEMU changes frequently, and with it, the ACPI
content has to change together. If we kept the ACPI content in the guest
firmware (SeaBIOS and OVMF), then SeaBIOS and OVMF would have to be
updated in lock-step with QEMU. That's unsustainable.

What OVMF does is that interprets the command script, laying out the
tables / blobs in guest RAM as QEMU requires. However, when it's done,
it doesn't just use the RSDP / RSDT / XSDT from QEMU as the root tables,
circumventing AcpiTableDxe. Instead, OVMF traverses the tables itself,
"structurally", and identifies memory addresses that are "most likely"
the start offsets of ACPI tables. And then OVMF passes those to
EFI_ACPI_TABLE_PROTOCOL.InstallTable(). In the end, the originally laid
out tables (the result of the command script execution) is freed, and
only those ACPI tables will exist that EFI_ACPI_TABLE_PROTOCOL created
internally.

If the HOB you are now trying to introduce had existed 6-8 years ago,
the work I had to do in OvmfPkg's AcpiPlatformDxe would have been a
*fraction* of what I ended up doing.

This is one reason why I'm so annoyed. There's a double standard in edk2
core module maintenance. Extending the core of edk2 is more or less
*equally useful* for different platform owners, but those platform
owners face *very different difficulties* when they actually attempt the
core extensions. I never even attempted extending AcpiTableDxe in
MdeModulePkg, because I knew it was tied to the spec, and was convinced
that my QEMU-motivated request would be rejected anyway.

>> (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?)
> 
> The definition of spec breakage to me is we cannot do anything that's
> conflict with the spec. But we can do things that spec doesn't define.
> Please correct if I am wrong.

Sure, here's a counter-example: when I proposed adding new status codes
to UefiBootManagerLib, so that boot option loading and boot option
starting would be broadcast to the system using the "report status code"
facility, with rich data (boot option device path, and EFI_STATUS
result), you said that you didn't want to add such extensions to
UefiBootManagerLib; you wanted it to do what the spec required, and
*only* what the spec required. You told me to go change the spec first.
My proposal was entirely transparent to the PI and UEFI specs, both the
introduction of the new status codes (and the associated information
structure(s)), and the logic to emit them.

* [edk2] [PATCH 0/5]
  MdeModulePkg, OvmfPkg: more easily visible boot progress reporting

  http://mid.mail-archive.com/20171122235849.4177-1-lersek@redhat.com

We have a new process for that now ("code first", although I'm unsure
how it works in practice).

My point is: don't cut corners. It's already *much easier* for Intel to
extend core modules than for other contributors -- because the core
module maintainers mostly come from Intel, and they can negotiate and
get aligned with the Intel *platform owners* before anything happens on
the list! --, so at least *how* you implement the extension should be
squeaky clean. Don't make other platforms pay a steep price (in
complexity or code size that cannot be compiled out), don't reuse
standard GUIDs, don't shirk documentation requirements (new header for
the GUID structure), and so on.

Laszlo