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


Laszlo Ersek
 

On 03/25/21 02:10, Ni, Ray wrote:
Ben,
I understand your point.
The purpose of changing the AcpiTableDxe to directly consume the HOB is to reduce the overall system complexity.
The complexity I see with your option is:
1. platform needs to include that driver to consume the ACPI table from bootloader
2. that driver (consume HOB and install table through AcpiTable protocol) needs to register a protocol notification on AcpiTable protocol and do the table installation in the callback.

Laszlo,
The change here is to meet the requirement that bootloader provides the ACPI table.
In OVMF case, it's not the bootloader but the virtual hardware who provides the ACPI table.
I can see the similarity between the two: the ACPI table is produced before the UEFI Payload runs.
Can you review the new option below and see whether it can meet the OVMF needs as well?
Let me clarify: there's no way I'm touching that part of OVMF. I don't
want any potential regressions there. It's been working stably for years.

I'd just like to avoid a duplication of functionality -- if the new HOB
logic in MdeModulePkg is heavy, then I'd like a possibility for
platforms to separate it out.

1. Define a new GUID gPldHobAcpiTable in MdeModulePkg.dec and a structure as below in MdeModulePkg/Include/Guid/Acpi.h.
typedef struct {
UINT64 TableAddress;
} PLD_HOB_ACPI_TABLE;
Looks good (but maybe use EFI_PHYSICAL_ADDRESS for type, and a more
telling name than "TableAddress" -- name precisely what ACPI table type
the pointer refers to?)

2. Change AcpiTableDxe driver to consume the HOB
Yes. And this is the part that, if it's complex or large, should go into
a separate source file (together with a new INF file), or be controlled
by a Feature PCD.

If it's not complex / large, and you can refactor AcpiTableDxe first
such that the HOB-based functionality is not littered over a bunch of
functions, then it's OK to stick with just one INF file (and no Feature
PCD).

3. Remove SYSTEM_TABLE_INFO. AcpiTableBase/AcpiTableSize.
Won't that break other systems that currently depend on it? Just asking.
I'm neutral, personally.

4. Remove the BlSupportDxe code that consume the ACPI table in SYSTEM_TABLE_INFO.
Same compatibility question for existent, dependent platforms.

Thanks
Laszlo



-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
Sent: Thursday, March 25, 2021 2:33 AM
To: Benjamin Doron <benjamin.doron00@gmail.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [Patch V2 0/2] Let AcpiTableDxe driver install Acpi table from Hob

On 03/24/21 17:55, Benjamin Doron wrote:


Hi all,
Would it be acceptable/feasible for AcpiTableDxe or AcpiPlatformDxe (in
MdeModulePkg) to use `EfiGetSystemConfigurationTable` to get the RSDP
and then install the tables? It's a solution that uses the regular
UefiLib, so it avoids platform-specific quirks (and as I see it, if RSDP
is in the configuration table, we probably always want those tables).
I'm sorry, I don't understand how this would help.
As I understand it, the issue is that this patchset changes MdeModulePkg to do platform-specific parsing.

Perhaps it would be an acceptable solution for platforms to retrieve the tables, then add
RSDP/them to the configuration table to be installed by AcpiTableDxe/AcpiPlatformDxe.
This allows MdeModulePkg to abstract away the parsing, only installing tables
available to it.
But this is already the best approach, and already what's happening --
when you call EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable() from the
platform's AcpiPlatformDxe, that's *how* you tell AcpiTableDxe in
MdeModulePkg to pick up the table and hook it into the RSDT / XSDT /
wherever, and also to manage RSD PTR as a UEFI configuration table.

Are you (more or less) proposing a new EFI_ACPI_TABLE_PROTOCOL member
function for taking a forest of ACPI tables, passed in by RSD PTR?

Sorry about being dense. :)

(Currently, UefiPayloadPkg's BlSupportDxe retrieves the data from a HOB and calls
`gBS->InstallConfigurationTable` with the address of RSDP).

I understand that this may not work for OVMF if tables are located differently in memory.



Regarding UefiPayloadPkg: AcpiTableDxe is currently compiled (listed in
DSC) but not added to a FV (not listed in FDF). So, how has this been
tested?
I assume through an out-of-tree platform. Many such core modules exist
in edk2 that are not consumed by any of the virtual platforms in the
edk2 repo itself (EmulatorPkg, ArmVirtPkg, OvmfPkg).
This makes sense, but AcpiTableDxe must be added to UefiPayloadPkg's FDF
if patch 2/2 is merged. Otherwise, ACPI tables will not be advertised.
Good point; I missed that. (I'm not familiar with the UefiPayloadPkg FDF
at all.)

Laszlo




Join devel@edk2.groups.io to automatically receive all group messages.