On 02/17/16 16:34, Ard Biesheuvel wrote:
> On 17 February 2016 at 16:11, Laszlo Ersek <ler...@redhat.com> wrote:
>> On 02/17/16 15:48, Ard Biesheuvel wrote:
>>> AARCH64 systems never require compatibility with legacy ACPI OSes, and
>>> may not have any 32-bit addressable system RAM. To support ACPI on these
>>> systems, we need to be able to relax the 4 GB allocation restriction.
>>>
>>> So add a PCD PcdAcpiAllocateTablesBelow4GB defaulting to TRUE, and wire
>>> it up to the memory allocation calls in AcpiTableDxe/AcpiTableProtocol.c
>>>
>>> Note that this will inhibit the publishing of any tables that carry only
>>> 32-bit addresses, i.e., RSDPv1, RSDTv1 and RSDTv3.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheu...@linaro.org>
>>> ---
>>> MdeModulePkg/MdeModulePkg.dec | 7 +
>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf | 2 +
>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 447
>>> ++++++++++++--------
>>> 3 files changed, 270 insertions(+), 186 deletions(-)
>>
>> Somewhat independently of your patch, I agree 100% that the interface
>> exposed by EFI_ACPI_TABLE_PROTOCOL is seriously lacking. It doesn't
>> allow the caller to control the allocation limit (--> see your patch),
>> nor the UEFI memory type that the copy of the table being installed will
>> be placed into (it would be valid for the caller to choose between
>> AcpiReclaim vs. AcpiNVS), nor the question whether the table being
>> installed should be linked into the RSDT, XSDT, or both.
>>
>> I don't find this an implementation problem; this is a problem with the
>> specification. In particular, in October last year I cross-posted an
>> email to the USWG and ASWG lists, entitled
>>
>> LoadTable vs. EFI_ACPI_TABLE_PROTOCOL
>>
>> In that email, I pointed out that the current edk2 implementation of
>> EFI_ACPI_TABLE_PROTOCOL makes it impossible to support the LoadTable
>> ACPI operator. Namely, the LoadTable operator requires the definition
>> block (= AML code) being loaded dynamically to come *from* AcpiNVS type
>> memory, but edk2's implementation doesn't allow the caller to install
>> tables into AcpiNVS type memory, *except* those with the signature
>> "UEFI" -- those however are *data* tables, not definition blocks (= AML
>> code, like SSDT or DSDT).
>>
>> For this reason, I proposed EFI_ACPI_TABLE2_PROTOCOL:
>>
>>> - same set of member functions
>>> - UninstallAcpiTable() works the same
>>> - InstallAcpiTable() gets a new parameter called MemoryType:
>>>
>>> typedef
>>> EFI_STATUS
>>> (EFIAPI *EFI_ACPI_TABLE2_INSTALL_ACPI_TABLE) (
>>> IN EFI_ACPI_TABLE2_PROTOCOL *This,
>>> IN VOID *AcpiTableBuffer,
>>> IN UINTN AcpiTableBufferSize,
>>> IN EFI_MEMORY_TYPE *MemoryType OPTIONAL,
>>> OUT UINTN *TableKey
>>> );
>>>
>>> - If MemoryType is NULL, then the new interface works identically to
>>> the preexistent one.
>>> - Otherwise, if EFI_ACPI_TABLE2_PROTOCOL can prove that *MemoryType is
>>> invalid for the signature detected in AcpiTableBuffer, then
>>> EFI_INVALID_PARAMETER is returned.
>>> - Otherwise, the copy of AcpiTableBuffer is allocated from *MemoryType
>>> memory.
>>>
>>> This would allow tables with any (non-standard) signatures to be
>>> placed in reserved or AcpiNVS memory, regardless of contents.
>>
>> My query to the USWG & ASWG garnered exactly *zero* replies. I guess
>> nobody cared about (or understood) my problem.
>>
>> Now I absolutely view your use case as a further parameter that
>> "EFI_ACPI_TABLE2_PROTOCOL" should expose. From that POV, I support your
>> use case.
>>
>> Regarding the implementation, I guess you are not enthusiastic about
>> turning this question into a standardization effort. (I tried and
>> failed.) So, whatever works for you -- I won't review the code, but as
>> long as it can remain compatible with current clients, given the right
>> PCD settings, I absolutely support the idea.
>>
>
> You may be right about the lacking EFI_ACPI_PROTOCOL interface, but
> the hardcoded 4 GB limit is simply an implementation detail, and
> should be treated as such imo.
Hm, maybe, maybe not.
Namely, RSDT cannot reference tables that are located above 4GB, and the
UEFI spec says, quote,
EFI_ACPI_TABLE_PROTOCOL.InstallAcpiTable()
[...]
The InstallAcpiTable() function allows a caller to install an ACPI
table. The ACPI table may either by a System Description Table or
the FACS. For all tables except for the DSDT and FACS, a copy of
the table will be linked by the RSDT/XSDT. For the FACS and DSDT,
the pointer to a copy of the table will be updated in the FADT, if
present.
[...]
On successful output, the EFI_ACPI_TABLE_PROTOCOL will ensure that
the checksum field is correct for both the RSDT/XSDT table and the
copy of the table being installed that is linked by the RSDT/XSDT.
[...]
That is, following the *letter* of the spec, linkage into the RSDT is
*not* optional. And that requirement can only be satisfied if the tables
are located under 4GB.
This may actually make the spec unsatisfiable on some aarch64 platforms!
(And the question could be more than an implementation detail.) Perhaps
some changes to the UEFI spec cannot be avoided here.
>
>> (QEMU currently depends on tables being allocated under 4GB, even for
>> aarch64 guests, as far as I recall.)
>>
>
> Yes, I wondered about that. After the tables are installed, a 16 KB
> reserved region remains in the UEFI memory map, Does that contain any
> live payload?
I can't say without looking at specifics, but I don't think it is
directly related to the ACPI linker/loader client under OvmfPkg. That
code never allocates reserved memory, only AcpiNVS (and frees most of
even that, in the end).
I also grepped "MdeModulePkg/Universal/Acpi/AcpiTableDxe" for
"EfiReservedMemoryType" -- no hits either.
Thanks!
Laszlo
>
>> Digressing a bit, I'll also mention that I found out (and reported) the
>> hard way that Microsoft's ACPI.SYS does not implement the
>> DataTableRegion operator. (ACPI CA does.) That operator is supposed to
>> be extremely powerful, it is not optional, and it dates back to the ACPI
>> 2.0 specification -- which Microsoft co-authored.
>>
>
> I remember that thread :-)
>
> Thanks,
> Ard.
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
