On 02/17/16 18:23, Ard Biesheuvel wrote: > 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.
I agree with you on the scope of the words "both ... and ...". I disagree with your interpretation of the slash character between the words "RSDT" and "XSDT". I believe you say "RSDT and XSDT are alternatives, and it's platform prerogative to pick one (or both)" -- however I can't deduce that from the spec, and the current reference implementation doesn't seem to support it either (otherwise your patch might not be necessary). "Interpretation required", I guess. Anyway, please be aware that I'm not trying to block this patch at all. As long as the current behavior remains available, I'm all good. Thanks Laszlo > > > >>> 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
