On 17 February 2016 at 18:07, Graeme Gregory <[email protected]> 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 <[email protected]> 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 <[email protected]> >>>>> --- >>>>> 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? > > RSDT is optional for systems in ACPI 5.1+ specs. >
Indeed. >> [...] >> >> 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. >> No, it is 'linkage into the RSDT/XSDT' that is not optional. In the quote above, '..both the RSDT/XDST and ...' does not refer to both RSDT and XSDT but to the RSDT/XSDT on the one hand and the copy of the table being installed on the other. >> 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 [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
