Thanks Jiewen. On ARCH64 systems, it is possible not to have any memory below 4GB at all.
Ard, Are you OK with a solution that does not use a PCD? -----Original Message----- From: Yao, Jiewen [mailto:jiewen....@intel.com] Sent: Thursday, February 18, 2016 8:35 AM To: El-Haj-Mahmoud, Samer <samer.el-haj-mahm...@hpe.com>; Ard Biesheuvel <ard.biesheu...@linaro.org>; Laszlo Ersek <ler...@redhat.com> Cc: Tian, Feng <feng.t...@intel.com>; Graeme Gregory <graeme.greg...@linaro.org>; edk2-devel@lists.01.org <edk2-de...@ml01.01.org>; Leif Lindholm <leif.lindh...@linaro.org>; Gao, Liming <liming....@intel.com>; Zeng, Star <star.z...@intel.com>; Yao, Jiewen <jiewen....@intel.com> Subject: RE: [edk2] [RFC PATCH] MdeModulePkg: AcpiTableDxe: make 4 GB table allocation limit optional Yes. Good question: to PCD or not to PCD. :-) If it is 2010, I will choose PCD, because I want the flexibility. But now, when more and more people working on that, they start complaining the number of *possible* configuration. So we are trying to not add too much PCD, unless it is necessary. That is why I changed my mind later. However, if there is known limitation that we cannot use fail/allocate algorithm, and PCD is the only way to resolve it. I am OK to add this PCD. And I want to be educated on >4G ARM platform, too. Thank you Yao Jiewen -----Original Message----- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of El-Haj-Mahmoud, Samer Sent: Thursday, February 18, 2016 10:27 PM To: Yao, Jiewen; Ard Biesheuvel; 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 Jiewen, I think ACPI and SMBIOS solutions could be the same, or could be different. The SMBIOS 3.0 spec allows SMBIOS table to be above 4GB, and some platforms may chose that even if they have memory below 4GB. For the ACPI patch, it looks like Ard's feedback and yours are contradictory. Ard suggested a PCD, while you suggested no PCD and use the method to try below 4GB, and if it fails move to above 4GB. We have no issue submitting the patch one way or the other, but we need agreement on the preferred solution. So, to PCD or not to PCD... -----Original Message----- From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Yao, Jiewen Sent: Thursday, February 18, 2016 8:20 AM To: Ard Biesheuvel <ard.biesheu...@linaro.org>; Laszlo Ersek <ler...@redhat.com> Cc: Tian, Feng <feng.t...@intel.com>; Graeme Gregory <graeme.greg...@linaro.org>; edk2-devel@lists.01.org <edk2-de...@ml01.01.org>; Leif Lindholm <leif.lindh...@linaro.org>; Yao, Jiewen <jiewen....@intel.com>; Gao, Liming <liming....@intel.com>; Zeng, Star <star.z...@intel.com> Subject: Re: [edk2] [RFC PATCH] MdeModulePkg: AcpiTableDxe: make 4 GB table allocation limit optional 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. -- 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. 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? 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? Thank you Yao Jiewen -----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 _______________________________________________ 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
