On 17 February 2016 at 18:40, Laszlo Ersek <ler...@redhat.com> wrote: > On 02/17/16 18:23, Ard Biesheuvel wrote: >> On 17 February 2016 at 18:07, Graeme Gregory <graeme.greg...@linaro.org> >> 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? >>> >>> 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. >
Well, the wording is obviously not unambiguous :-) > 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. > Absolutely. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
