On 02/17/16 18:07, Graeme Gregory wrote:
>
>
> On 17/02/2016 16:04, Laszlo Ersek wrote:
>> 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.
> Surely this is a simple case of the spec not being clear that RSDT/XDST
> means RSDT or XSDT as appropriate?
I don't know. Does the spec mean "RSDT or XSDT as appropriate"?
And what determines which is appropriate when? The user certainly can't
influence it, because the protocol interface doesn't allow it.
> RSDT is optional for systems in ACPI 5.1+ specs.
The only reference I'm finding in the UEFI 2.6 spec to ACPI version is
ACPI_10_TABLE_GUID vs. EFI_ACPI_TABLE_GUID, where "ACPI 2.0 or newer
tables should use EFI_ACPI_TABLE_GUID".
In particular,
Appendix Q References
Q.2.1 ACPI Specification
does not name any version.
In any case, I'm not debating whether RSDT should be optional or
mandatory. Instead, if it is indeed optional, the UEFI spec should be
clear on when it is appropriate to leave out / when it is not populated
automatically. It could mention that it is the jurisdiction of the
platform firmware (actual implementation) to decide whether tables will
only be linked from the XSDT.
Or else, allow the user to control that part of the behavior.
(This last part is actually something we'd use with QEMU -- there have
been ideas to expose some tables only in the XSDT, in order to hide them
from Windows XP, but expose them to other guest OSes.)
Thanks
Laszlo
>
> Graeme
>> [...]
>>
>> 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
