On 18 February 2016 at 15:20, Yao, Jiewen <jiewen....@intel.com> wrote: > Hi > Thanks to bring this 4G issue again. I have several thought for your > consideration. > > 1) At 12/16/2015, Samer El-Haj-Mahmoud <samer.el-haj-mahm...@hpe.com> > submitted a patch: > [edk2] [PATCH] IntelFrameworkModulePkg : Allow ACPI tables to get > installed above 4GB > > Some ARM systems do not have available memory below 4GB, and still support > ACPI. This patch allows the tables to get loaded above 4GB if the allocation > below 4GB fails. > > I have given comments that: > -- I guess we need similar fix for MdeModulePkg\Universal\Acpi\AcpiTableDxe. >
Indeed :-) > -- Can we create a new function to allocate <4G at first, if fail, allocate > any memory? > Then we can update all caller to consume this new function. > We can avoid adding ERROR check anywhere. I also suggest we need add some > debug message there, we can add into one place. > > It is just another option to resolve ARM issue. Currently, we only add PCD > when necessary, because we got complain that we defined too many PCDs. > If there is a way to avoid introducing a new one. It worth considering. > No, this is not a good idea. This still assumes that allocating below 4 GB is preferred if possible, and that is simply not true on AArch64. Allocating below 4 GB if there is space there will result in some UEFI regions at the top of memory, and some regions below 4 GB. This needlessly fragments the address space, and results in reduced TLB efficiency, since ACPI tables are not mapped by default, and the [3 GB, 4 GB] interval can no longer be mapped using a single 1 GB block entry. Also, memory below 4 GB needs to be reserved for devices that can only do 32-bit DMA. > 2) I believe 4G table issue not only exist in ACPI table, but also exist in > others. For example, Smbios table. > SMBIOS 3.0 spec allows table be loaded >4G memory, while SMBIOS 2.X has 4G > limitation. > > I know ACPI table and SMBIOS table are different. Some platforms care only > one of them. > Personally, I hope we can have a consistent way to resolve this generic 4G > issue. > > I have seems SMBIOS table is using the way Samer submitted in last year. > (Allocate <4G at first, if fail, allocate at any place) > Do you see any limitation on this algorithm? > This only happens if you have lower than version 3.0 or the PcdSmbiosEntryPointProvideMethod has bit 0 set. On AARCH64 systems, we usually only enable the v3.0 entry point, and clear bit 0 in this PCD > I am not insist on some solutions. I just want to bring the other way for > discussion. > > BTW: I am not clear on memory map on >4G ARM platform. Can someone help me > understand where is runtime code/runtime data/ACPI NVS/Reserved memory? >4G > or <4G? > Sorry to be pedantic, but AARCH64 is an architecture, not a platform. Every AARCH64 platform has complete freedom as to where peripherals live and where system RAM lives, and some platforms have their RAM starting at 0x80_0000_0000 So every such region could be anywhere in the address space. > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Ard > Biesheuvel > Sent: Thursday, February 18, 2016 1:42 AM > To: Laszlo Ersek > Cc: Tian, Feng; Graeme Gregory; edk2-devel@lists.01.org; Leif Lindholm; Gao, > Liming; Zeng, Star > Subject: Re: [edk2] [RFC PATCH] MdeModulePkg: AcpiTableDxe: make 4 GB table > allocation limit optional > > 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
