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

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

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