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

["Ni, Ray" <ray.ni@intel.com>]

> 
> (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?

> 
> 
> 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.

> 
> (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.