I agree. The version support is mess in this driver. There is some historic reason which makes code like this today. Just differentiate 1_0B and other is good enough, it matched UEFI spec, too. UEFI spec only defines "ACPI1.0b" and "ACPI2.0+above" in configuration table.
Again, thanks for your contribution to fix this issue. Thank you Yao Jiewen -----Original Message----- From: edk2-devel [mailto:[email protected]] On Behalf Of Ard Biesheuvel Sent: Thursday, February 18, 2016 11:47 PM To: Yao, Jiewen Cc: Tian, Feng; Graeme Gregory; [email protected]; Leif Lindholm; Gao, Liming; Laszlo Ersek; Zeng, Star Subject: Re: [edk2] [RFC PATCH] MdeModulePkg: AcpiTableDxe: make 4 GB table allocation limit optional On 18 February 2016 at 16:44, Yao, Jiewen <[email protected]> wrote: > I found PI spec vol 5 has below definition in EFI_ACPI_SDT_PROTOCOL. Maybe we > can define PcdAcpiTableVersion to match this. > > #define UINT32 EFI_ACPI_TABLE_VERSION; #define > EFI_ACPI_TABLE_VERSION_NONE (1 << 0) #define > EFI_ACPI_TABLE_VERSION_1_0B (1 << 1) #define > EFI_ACPI_TABLE_VERSION_2_0 (1 << 2) #define EFI_ACPI_TABLE_VERSION_3_0 > (1 << 3) #define EFI_ACPI_TABLE_VERSION_4_0 (1 << 4) #define > EFI_ACPI_TABLE_VERSION_5_0 (1 << 5) > OK, that makes sense. But I will only implement the difference between 'all supported' and 'all except 1.0B supported' > -----Original Message----- > From: Yao, Jiewen > Sent: Thursday, February 18, 2016 11:27 PM > To: Ard Biesheuvel > Cc: Tian, Feng; Graeme Gregory; [email protected]; Leif > Lindholm; Gao, Liming; Laszlo Ersek; Zeng, Star; Yao, Jiewen > Subject: RE: [edk2] [RFC PATCH] MdeModulePkg: AcpiTableDxe: make 4 GB > table allocation limit optional > > XSDT is introduced in ACPI2.0. > ACPI1.0/ACPI1.0b only defined 32bit address range. ACPI2.0 resolved 4G issue > immediately, but in order to keep compatibility, most platform produced both > DSDT and XSDT, and with FADT1.0 and FADT2.0, at that time. > > For now, I believe most OSes support ACPI2.0 and above. So it should be safe. > > Thank you > Yao Jiewen > > > -----Original Message----- > From: Ard Biesheuvel [mailto:[email protected]] > Sent: Thursday, February 18, 2016 11:15 PM > To: Yao, Jiewen > Cc: Tian, Feng; Graeme Gregory; [email protected]; Leif > Lindholm; Gao, Liming; Laszlo Ersek; Zeng, Star > Subject: Re: [edk2] [RFC PATCH] MdeModulePkg: AcpiTableDxe: make 4 GB > table allocation limit optional > > On 18 February 2016 at 16:07, Yao, Jiewen <[email protected]> wrote: >> Or PcdAcpiSupportVersion, bit0 as ACPI1.0. >> Bit0 set means allocation Below4G. >> Bit0 clear means allocation from everywhere. >> > > That is also fine with me. But I think it was ACPI 5.0 that introduced XSDT, > right? So if you clear this bit, you would expect a ACPI system compatible > with a version older than 5.0 to still work, but with allocations above 4 GB > and no RSDT, only XSDT, would that be ok? > > So perhaps it should define the lowest supported ACPI level, with legal > values of '1' and '5' for the moment, and we can always expand it later if we > need to. > > >> >> -----Original Message----- >> From: Yao, Jiewen >> Sent: Thursday, February 18, 2016 11:05 PM >> To: Ard Biesheuvel >> Cc: Tian, Feng; Graeme Gregory; [email protected]; Leif >> Lindholm; Gao, Liming; Laszlo Ersek; Zeng, Star; Yao, Jiewen >> Subject: RE: [edk2] [RFC PATCH] MdeModulePkg: AcpiTableDxe: make 4 GB >> table allocation limit optional >> >> Thanks to detail explain. I understand and I agree with you that >> fragmentation is problem. >> So basically, I agree on PCD solution to choose preferred location of >> allocation. >> >> Maybe you can consider adding fragmentation issue as description to help >> more people on why we need this PCD. >> >> I also think the PCD name - do we consider PcdAcpi1Support, or similar? >> People need strong tech background to choose why and if he wants Below4GB or >> not. Using ACPI1.0 as PCD might be better, because if ACPI1.0 is not POR, it >> can be dropped. It is more clear. >> >> Just like SMBIOS table, we use PcdSmbiosEntryPointProvideMethod, instead of >> Below4GB. >> >> Thank you >> Yao Jiewen >> >> >> -----Original Message----- >> From: edk2-devel [mailto:[email protected]] On Behalf >> Of Ard Biesheuvel >> Sent: Thursday, February 18, 2016 10:39 PM >> To: Yao, Jiewen >> Cc: Tian, Feng; Graeme Gregory; [email protected]; Leif >> Lindholm; Gao, Liming; Laszlo Ersek; Zeng, Star >> Subject: Re: [edk2] [RFC PATCH] MdeModulePkg: AcpiTableDxe: make 4 GB >> table allocation limit optional >> >> On 18 February 2016 at 15:20, Yao, Jiewen <[email protected]> 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 <[email protected]> >>> 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:[email protected]] On Behalf >>> Of Ard Biesheuvel >>> Sent: Thursday, February 18, 2016 1:42 AM >>> To: Laszlo Ersek >>> Cc: Tian, Feng; Graeme Gregory; [email protected]; 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 <[email protected]> wrote: >>>> 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. >>>> >>> >>> 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 >>> [email protected] >>> https://lists.01.org/mailman/listinfo/edk2-devel >> _______________________________________________ >> edk2-devel mailing list >> [email protected] >> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
