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.doro...@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
>>
>>
>>
>>
>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73283): https://edk2.groups.io/g/devel/message/73283
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]
-=-=-=-=-=-=-=-=-=-=-=-
