* [PATCH 0/3] MdeModulePkg: use pool allocations for ACPI tables
@ 2020-10-16 15:49 Ard Biesheuvel
2020-10-16 15:49 ` [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use pool allocations when possible Ard Biesheuvel
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: Ard Biesheuvel @ 2020-10-16 15:49 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Dandan Bi, Liming Gao, Jian J Wang, Hao A Wu,
Sami Mujawar, Laszlo Ersek, Leif Lindholm
Currently, the AcpiTableDxe memory allocator uses page based allocations,
for which the only reason seems to be that it permits the use of a memory
limit, which is necessary for ACPI 1.0 tables that need to reside in the
first 4 GB of memory.
That requirement does not exist on AArch64, and since page based allocations
are rounded up to 64 KB multiples, this wastes some memory in a way that
can easily be avoided. So let's use the existing 'mAcpiTableAllocType'
policy variable, and switch to pool allocations if it is set to
'AllocateAnyPages'
Example output from Linux booting on ArmVirtQemu:
Before:
ACPI: RSDP 0x0000000078510000 000024 (v02 BOCHS )
ACPI: XSDT 0x0000000078500000 00004C (v01 BOCHS BXPCFACP 00000001 01000013)
ACPI: FACP 0x00000000784C0000 00010C (v05 BOCHS BXPCFACP 00000001 BXPC 00000001)
ACPI: DSDT 0x00000000784D0000 0014BB (v02 BOCHS BXPCDSDT 00000001 BXPC 00000001)
ACPI: APIC 0x00000000784B0000 0000A8 (v03 BOCHS BXPCAPIC 00000001 BXPC 00000001)
ACPI: GTDT 0x00000000784A0000 000060 (v02 BOCHS BXPCGTDT 00000001 BXPC 00000001)
ACPI: MCFG 0x0000000078490000 00003C (v01 BOCHS BXPCMCFG 00000001 BXPC 00000001)
ACPI: SPCR 0x0000000078480000 000050 (v02 BOCHS BXPCSPCR 00000001 BXPC 00000001)
After:
ACPI: RSDP 0x000000007C030018 000024 (v02 BOCHS )
ACPI: XSDT 0x000000007C03FE98 00004C (v01 BOCHS BXPCFACP 00000001 01000013)
ACPI: FACP 0x000000007C03FA98 00010C (v05 BOCHS BXPCFACP 00000001 BXPC 00000001)
ACPI: DSDT 0x000000007C037518 0014BB (v02 BOCHS BXPCDSDT 00000001 BXPC 00000001)
ACPI: APIC 0x000000007C03FC18 0000A8 (v03 BOCHS BXPCAPIC 00000001 BXPC 00000001)
ACPI: GTDT 0x000000007C03FD18 000060 (v02 BOCHS BXPCGTDT 00000001 BXPC 00000001)
ACPI: MCFG 0x000000007C03FE18 00003C (v01 BOCHS BXPCMCFG 00000001 BXPC 00000001)
ACPI: SPCR 0x000000007C03FF98 000050 (v02 BOCHS BXPCSPCR 00000001 BXPC 00000001)
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Ard Biesheuvel (3):
MdeModulePkg/AcpiTableDxe: use pool allocations when possible
MdeModulePkg/AcpiTableDxe: use pool allocation for RSDT/XSDT if
possible
MdeModulePkg/AcpiTableDxe: use pool allocation for RSDP if possible
.../Universal/Acpi/AcpiTableDxe/AcpiTable.h | 4 +-
.../Universal/Acpi/AcpiTableDxe/AcpiSdt.c | 4 +-
.../Acpi/AcpiTableDxe/AcpiTableProtocol.c | 216 +++++++++++-------
3 files changed, 143 insertions(+), 81 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use pool allocations when possible 2020-10-16 15:49 [PATCH 0/3] MdeModulePkg: use pool allocations for ACPI tables Ard Biesheuvel @ 2020-10-16 15:49 ` Ard Biesheuvel 2020-10-19 19:47 ` [edk2-devel] " Laszlo Ersek 2020-10-16 15:49 ` [PATCH 2/3] MdeModulePkg/AcpiTableDxe: use pool allocation for RSDT/XSDT if possible Ard Biesheuvel ` (2 subsequent siblings) 3 siblings, 1 reply; 18+ messages in thread From: Ard Biesheuvel @ 2020-10-16 15:49 UTC (permalink / raw) To: devel Cc: Ard Biesheuvel, Dandan Bi, Liming Gao, Jian J Wang, Hao A Wu, Sami Mujawar, Laszlo Ersek, Leif Lindholm On AArch64 systems, page based allocations for memory types that are relevant to the OS are rounded up to 64 KB multiples. This wastes some space in the ACPI table memory allocator, since it uses page based allocations in order to be able to place the ACPI tables low in memory. Since the latter requirement does not exist on AArch64, switch to pool allocations for all ACPI tables except the root tables if the active allocation policy permits them to be anywhere in memory. The root tables will be handled in a subsequent patch. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> --- MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h | 4 +- MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c | 4 +- MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 75 ++++++++++++++------ 3 files changed, 57 insertions(+), 26 deletions(-) diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h index 425a462fd92b..6e600e7acd24 100644 --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h @@ -64,9 +64,9 @@ typedef struct { LIST_ENTRY Link; EFI_ACPI_TABLE_VERSION Version; EFI_ACPI_COMMON_HEADER *Table; - EFI_PHYSICAL_ADDRESS PageAddress; - UINTN NumberOfPages; + UINTN TableSize; UINTN Handle; + BOOLEAN PoolAllocation; } EFI_ACPI_TABLE_LIST; // diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c index b1cba20c8ed7..14ced68e645f 100644 --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c @@ -68,8 +68,8 @@ FindTableByBuffer ( while (CurrentLink != StartLink) { CurrentTableList = EFI_ACPI_TABLE_LIST_FROM_LINK (CurrentLink); - if (((UINTN)CurrentTableList->PageAddress <= (UINTN)Buffer) && - ((UINTN)CurrentTableList->PageAddress + EFI_PAGES_TO_SIZE(CurrentTableList->NumberOfPages) > (UINTN)Buffer)) { + if (((UINTN)CurrentTableList->Table <= (UINTN)Buffer) && + ((UINTN)CurrentTableList->Table + CurrentTableList->TableSize > (UINTN)Buffer)) { // // Good! Found Table. // diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c index ad7baf8205b4..b05e57e6584f 100644 --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c @@ -428,6 +428,26 @@ ReallocateAcpiTableBuffer ( return EFI_SUCCESS; } +/** + Free the memory associated with provided the EFI_ACPI_TABLE_LIST instance + + @param TableEntry EFI_ACPI_TABLE_LIST instance pointer + +**/ +STATIC +VOID +FreeTableMemory ( + EFI_ACPI_TABLE_LIST *TableEntry + ) +{ + if (TableEntry->PoolAllocation) { + gBS->FreePool (TableEntry->Table); + } else { + gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)TableEntry->Table, + EFI_SIZE_TO_PAGES (TableEntry->TableSize)); + } +} + /** This function adds an ACPI table to the table list. It will detect FACS and allocate the correct type of memory and properly align the table. @@ -454,14 +474,15 @@ AddTableToList ( OUT UINTN *Handle ) { - EFI_STATUS Status; - EFI_ACPI_TABLE_LIST *CurrentTableList; - UINT32 CurrentTableSignature; - UINT32 CurrentTableSize; - UINT32 *CurrentRsdtEntry; - VOID *CurrentXsdtEntry; - UINT64 Buffer64; - BOOLEAN AddToRsdt; + EFI_STATUS Status; + EFI_ACPI_TABLE_LIST *CurrentTableList; + UINT32 CurrentTableSignature; + UINT32 CurrentTableSize; + UINT32 *CurrentRsdtEntry; + VOID *CurrentXsdtEntry; + EFI_PHYSICAL_ADDRESS AllocPhysAddress; + UINT64 Buffer64; + BOOLEAN AddToRsdt; // // Check for invalid input parameters @@ -496,8 +517,8 @@ AddTableToList ( // There is no architectural reason these should be below 4GB, it is purely // for convenience of implementation that we force memory below 4GB. // - CurrentTableList->PageAddress = 0xFFFFFFFF; - CurrentTableList->NumberOfPages = EFI_SIZE_TO_PAGES (CurrentTableSize); + AllocPhysAddress = 0xFFFFFFFF; + CurrentTableList->TableSize = CurrentTableSize; // // Allocation memory type depends on the type of the table @@ -518,9 +539,22 @@ AddTableToList ( Status = gBS->AllocatePages ( AllocateMaxAddress, EfiACPIMemoryNVS, - CurrentTableList->NumberOfPages, - &CurrentTableList->PageAddress + EFI_SIZE_TO_PAGES (CurrentTableList->TableSize), + &AllocPhysAddress ); + CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER *)(UINTN)AllocPhysAddress; + } else if (mAcpiTableAllocType == AllocateAnyPages) { + // + // If there is no allocation limit, there is also no need to use page + // based allocations for ACPI tables, which may be wasteful on platforms + // such as AArch64 that allocate multiples of 64 KB + // + Status = gBS->AllocatePool ( + EfiACPIReclaimMemory, + CurrentTableList->TableSize, + (VOID **)&CurrentTableList->Table + ); + CurrentTableList->PoolAllocation = TRUE; } else { // // All other tables are ACPI reclaim memory, no alignment requirements. @@ -528,9 +562,10 @@ AddTableToList ( Status = gBS->AllocatePages ( mAcpiTableAllocType, EfiACPIReclaimMemory, - CurrentTableList->NumberOfPages, - &CurrentTableList->PageAddress + EFI_SIZE_TO_PAGES (CurrentTableList->TableSize), + &AllocPhysAddress ); + CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER *)(UINTN)AllocPhysAddress; } // // Check return value from memory alloc. @@ -539,10 +574,6 @@ AddTableToList ( gBS->FreePool (CurrentTableList); return EFI_OUT_OF_RESOURCES; } - // - // Update the table pointer with the allocated memory start - // - CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER *) (UINTN) CurrentTableList->PageAddress; // // Initialize the table contents @@ -575,7 +606,7 @@ AddTableToList ( if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && AcpiTableInstance->Fadt1 != NULL) || ((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && AcpiTableInstance->Fadt3 != NULL) ) { - gBS->FreePages (CurrentTableList->PageAddress, CurrentTableList->NumberOfPages); + FreeTableMemory (CurrentTableList); gBS->FreePool (CurrentTableList); return EFI_ACCESS_DENIED; } @@ -729,7 +760,7 @@ AddTableToList ( if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && AcpiTableInstance->Facs1 != NULL) || ((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && AcpiTableInstance->Facs3 != NULL) ) { - gBS->FreePages (CurrentTableList->PageAddress, CurrentTableList->NumberOfPages); + FreeTableMemory (CurrentTableList); gBS->FreePool (CurrentTableList); return EFI_ACCESS_DENIED; } @@ -813,7 +844,7 @@ AddTableToList ( if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && AcpiTableInstance->Dsdt1 != NULL) || ((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && AcpiTableInstance->Dsdt3 != NULL) ) { - gBS->FreePages (CurrentTableList->PageAddress, CurrentTableList->NumberOfPages); + FreeTableMemory (CurrentTableList); gBS->FreePool (CurrentTableList); return EFI_ACCESS_DENIED; } @@ -1449,7 +1480,7 @@ DeleteTable ( // // Free the Table // - gBS->FreePages (Table->PageAddress, Table->NumberOfPages); + FreeTableMemory (Table); RemoveEntryList (&(Table->Link)); gBS->FreePool (Table); } -- 2.17.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use pool allocations when possible 2020-10-16 15:49 ` [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use pool allocations when possible Ard Biesheuvel @ 2020-10-19 19:47 ` Laszlo Ersek 2020-10-22 2:01 ` 回复: " gaoliming 0 siblings, 1 reply; 18+ messages in thread From: Laszlo Ersek @ 2020-10-19 19:47 UTC (permalink / raw) To: devel, ard.biesheuvel Cc: Dandan Bi, Liming Gao, Jian J Wang, Hao A Wu, Sami Mujawar, Leif Lindholm On 10/16/20 17:49, Ard Biesheuvel wrote: > On AArch64 systems, page based allocations for memory types that are > relevant to the OS are rounded up to 64 KB multiples. This wastes > some space in the ACPI table memory allocator, since it uses page > based allocations in order to be able to place the ACPI tables low > in memory. > > Since the latter requirement does not exist on AArch64, switch to pool > allocations for all ACPI tables except the root tables if the active > allocation policy permits them to be anywhere in memory. The root > tables will be handled in a subsequent patch. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> > --- > MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h | 4 +- > MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c | 4 +- > MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 75 ++++++++++++++------ > 3 files changed, 57 insertions(+), 26 deletions(-) > > diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h > index 425a462fd92b..6e600e7acd24 100644 > --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h > +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h > @@ -64,9 +64,9 @@ typedef struct { > LIST_ENTRY Link; > EFI_ACPI_TABLE_VERSION Version; > EFI_ACPI_COMMON_HEADER *Table; > - EFI_PHYSICAL_ADDRESS PageAddress; > - UINTN NumberOfPages; > + UINTN TableSize; > UINTN Handle; > + BOOLEAN PoolAllocation; > } EFI_ACPI_TABLE_LIST; > > // (1) The preceding comment (which explains the fields) should be updated. > diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c > index b1cba20c8ed7..14ced68e645f 100644 > --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c > +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c > @@ -68,8 +68,8 @@ FindTableByBuffer ( > > while (CurrentLink != StartLink) { > CurrentTableList = EFI_ACPI_TABLE_LIST_FROM_LINK (CurrentLink); > - if (((UINTN)CurrentTableList->PageAddress <= (UINTN)Buffer) && > - ((UINTN)CurrentTableList->PageAddress + EFI_PAGES_TO_SIZE(CurrentTableList->NumberOfPages) > (UINTN)Buffer)) { > + if (((UINTN)CurrentTableList->Table <= (UINTN)Buffer) && > + ((UINTN)CurrentTableList->Table + CurrentTableList->TableSize > (UINTN)Buffer)) { > // > // Good! Found Table. > // > diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > index ad7baf8205b4..b05e57e6584f 100644 > --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > @@ -428,6 +428,26 @@ ReallocateAcpiTableBuffer ( > return EFI_SUCCESS; > } > > +/** > + Free the memory associated with provided the EFI_ACPI_TABLE_LIST instance (2) s/with provided the/with the provided/ > + > + @param TableEntry EFI_ACPI_TABLE_LIST instance pointer > + > +**/ > +STATIC > +VOID > +FreeTableMemory ( > + EFI_ACPI_TABLE_LIST *TableEntry > + ) > +{ > + if (TableEntry->PoolAllocation) { > + gBS->FreePool (TableEntry->Table); > + } else { > + gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)TableEntry->Table, > + EFI_SIZE_TO_PAGES (TableEntry->TableSize)); > + } > +} > + > /** > This function adds an ACPI table to the table list. It will detect FACS and > allocate the correct type of memory and properly align the table. > @@ -454,14 +474,15 @@ AddTableToList ( > OUT UINTN *Handle > ) > { > - EFI_STATUS Status; > - EFI_ACPI_TABLE_LIST *CurrentTableList; > - UINT32 CurrentTableSignature; > - UINT32 CurrentTableSize; > - UINT32 *CurrentRsdtEntry; > - VOID *CurrentXsdtEntry; > - UINT64 Buffer64; > - BOOLEAN AddToRsdt; > + EFI_STATUS Status; > + EFI_ACPI_TABLE_LIST *CurrentTableList; > + UINT32 CurrentTableSignature; > + UINT32 CurrentTableSize; > + UINT32 *CurrentRsdtEntry; > + VOID *CurrentXsdtEntry; > + EFI_PHYSICAL_ADDRESS AllocPhysAddress; > + UINT64 Buffer64; > + BOOLEAN AddToRsdt; > > // > // Check for invalid input parameters > @@ -496,8 +517,8 @@ AddTableToList ( > // There is no architectural reason these should be below 4GB, it is purely > // for convenience of implementation that we force memory below 4GB. > // > - CurrentTableList->PageAddress = 0xFFFFFFFF; > - CurrentTableList->NumberOfPages = EFI_SIZE_TO_PAGES (CurrentTableSize); > + AllocPhysAddress = 0xFFFFFFFF; > + CurrentTableList->TableSize = CurrentTableSize; > > // > // Allocation memory type depends on the type of the table > @@ -518,9 +539,22 @@ AddTableToList ( > Status = gBS->AllocatePages ( > AllocateMaxAddress, > EfiACPIMemoryNVS, > - CurrentTableList->NumberOfPages, > - &CurrentTableList->PageAddress > + EFI_SIZE_TO_PAGES (CurrentTableList->TableSize), > + &AllocPhysAddress > ); > + CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER *)(UINTN)AllocPhysAddress; > + } else if (mAcpiTableAllocType == AllocateAnyPages) { > + // > + // If there is no allocation limit, there is also no need to use page > + // based allocations for ACPI tables, which may be wasteful on platforms > + // such as AArch64 that allocate multiples of 64 KB > + // > + Status = gBS->AllocatePool ( > + EfiACPIReclaimMemory, > + CurrentTableList->TableSize, > + (VOID **)&CurrentTableList->Table > + ); > + CurrentTableList->PoolAllocation = TRUE; > } else { > // > // All other tables are ACPI reclaim memory, no alignment requirements. > @@ -528,9 +562,10 @@ AddTableToList ( > Status = gBS->AllocatePages ( > mAcpiTableAllocType, > EfiACPIReclaimMemory, > - CurrentTableList->NumberOfPages, > - &CurrentTableList->PageAddress > + EFI_SIZE_TO_PAGES (CurrentTableList->TableSize), > + &AllocPhysAddress > ); > + CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER *)(UINTN)AllocPhysAddress; > } > // > // Check return value from memory alloc. > @@ -539,10 +574,6 @@ AddTableToList ( > gBS->FreePool (CurrentTableList); > return EFI_OUT_OF_RESOURCES; > } > - // > - // Update the table pointer with the allocated memory start > - // > - CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER *) (UINTN) CurrentTableList->PageAddress; > > // > // Initialize the table contents (3) Thus far, in the changes to AddTableToList(), two things bother me: (3a) Before the patch, we'd only convert the physical address to a pointer once we knew that the allocation succeeded. Post-patch, we now have two branches where we might convert bogus AllocPhysAddress values (such as even 0xFFFFFFFF) to (EFI_ACPI_COMMON_HEADER*). I think we shouldn't do this. (It's not just the dereferencing of bogus pointers that we should avoid, but also the evaluation thereof.) (3b) "CurrentTableList" is allocated with AllocatePool() not AllocateZeroPool(), so *not* setting the "PoolAllocation" field to FALSE on the two affected branches is wrong. The patch looks OK to me otherwise. Thanks Laszlo > @@ -575,7 +606,7 @@ AddTableToList ( > if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && AcpiTableInstance->Fadt1 != NULL) || > ((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && AcpiTableInstance->Fadt3 != NULL) > ) { > - gBS->FreePages (CurrentTableList->PageAddress, CurrentTableList->NumberOfPages); > + FreeTableMemory (CurrentTableList); > gBS->FreePool (CurrentTableList); > return EFI_ACCESS_DENIED; > } > @@ -729,7 +760,7 @@ AddTableToList ( > if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && AcpiTableInstance->Facs1 != NULL) || > ((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && AcpiTableInstance->Facs3 != NULL) > ) { > - gBS->FreePages (CurrentTableList->PageAddress, CurrentTableList->NumberOfPages); > + FreeTableMemory (CurrentTableList); > gBS->FreePool (CurrentTableList); > return EFI_ACCESS_DENIED; > } > @@ -813,7 +844,7 @@ AddTableToList ( > if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && AcpiTableInstance->Dsdt1 != NULL) || > ((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && AcpiTableInstance->Dsdt3 != NULL) > ) { > - gBS->FreePages (CurrentTableList->PageAddress, CurrentTableList->NumberOfPages); > + FreeTableMemory (CurrentTableList); > gBS->FreePool (CurrentTableList); > return EFI_ACCESS_DENIED; > } > @@ -1449,7 +1480,7 @@ DeleteTable ( > // > // Free the Table > // > - gBS->FreePages (Table->PageAddress, Table->NumberOfPages); > + FreeTableMemory (Table); > RemoveEntryList (&(Table->Link)); > gBS->FreePool (Table); > } > ^ permalink raw reply [flat|nested] 18+ messages in thread
* 回复: [edk2-devel] [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use pool allocations when possible 2020-10-19 19:47 ` [edk2-devel] " Laszlo Ersek @ 2020-10-22 2:01 ` gaoliming 2020-10-22 12:55 ` Ard Biesheuvel 0 siblings, 1 reply; 18+ messages in thread From: gaoliming @ 2020-10-22 2:01 UTC (permalink / raw) To: 'Laszlo Ersek', devel, ard.biesheuvel Cc: 'Dandan Bi', 'Jian J Wang', 'Hao A Wu', 'Sami Mujawar', 'Leif Lindholm' Ard: Can you help to collect memmap information on Shell before this change and after this change? I want to know what’s impact on the memory layout. Thanks Liming > -----邮件原件----- > 发件人: Laszlo Ersek <lersek@redhat.com> > 发送时间: 2020年10月20日 3:48 > 收件人: devel@edk2.groups.io; ard.biesheuvel@arm.com > 抄送: Dandan Bi <dandan.bi@intel.com>; Liming Gao > <gaoliming@byosoft.com.cn>; Jian J Wang <jian.j.wang@intel.com>; Hao A > Wu <hao.a.wu@intel.com>; Sami Mujawar <sami.mujawar@arm.com>; Leif > Lindholm <leif@nuviainc.com> > 主题: Re: [edk2-devel] [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use pool > allocations when possible > > On 10/16/20 17:49, Ard Biesheuvel wrote: > > On AArch64 systems, page based allocations for memory types that are > > relevant to the OS are rounded up to 64 KB multiples. This wastes > > some space in the ACPI table memory allocator, since it uses page > > based allocations in order to be able to place the ACPI tables low > > in memory. > > > > Since the latter requirement does not exist on AArch64, switch to pool > > allocations for all ACPI tables except the root tables if the active > > allocation policy permits them to be anywhere in memory. The root > > tables will be handled in a subsequent patch. > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> > > --- > > MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h | 4 > +- > > MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c | > 4 +- > > MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 75 > ++++++++++++++------ > > 3 files changed, 57 insertions(+), 26 deletions(-) > > > > diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h > b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h > > index 425a462fd92b..6e600e7acd24 100644 > > --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h > > +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h > > @@ -64,9 +64,9 @@ typedef struct { > > LIST_ENTRY Link; > > EFI_ACPI_TABLE_VERSION Version; > > EFI_ACPI_COMMON_HEADER *Table; > > - EFI_PHYSICAL_ADDRESS PageAddress; > > - UINTN NumberOfPages; > > + UINTN TableSize; > > UINTN Handle; > > + BOOLEAN PoolAllocation; > > } EFI_ACPI_TABLE_LIST; > > > > // > > (1) The preceding comment (which explains the fields) should be updated. > > > diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c > b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c > > index b1cba20c8ed7..14ced68e645f 100644 > > --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c > > +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c > > @@ -68,8 +68,8 @@ FindTableByBuffer ( > > > > while (CurrentLink != StartLink) { > > CurrentTableList = EFI_ACPI_TABLE_LIST_FROM_LINK (CurrentLink); > > - if (((UINTN)CurrentTableList->PageAddress <= (UINTN)Buffer) && > > - ((UINTN)CurrentTableList->PageAddress + > EFI_PAGES_TO_SIZE(CurrentTableList->NumberOfPages) > (UINTN)Buffer)) { > > + if (((UINTN)CurrentTableList->Table <= (UINTN)Buffer) && > > + ((UINTN)CurrentTableList->Table + CurrentTableList->TableSize > > (UINTN)Buffer)) { > > // > > // Good! Found Table. > > // > > diff --git > a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > > index ad7baf8205b4..b05e57e6584f 100644 > > --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > > +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > > @@ -428,6 +428,26 @@ ReallocateAcpiTableBuffer ( > > return EFI_SUCCESS; > > } > > > > +/** > > + Free the memory associated with provided the EFI_ACPI_TABLE_LIST > instance > > (2) s/with provided the/with the provided/ > > > + > > + @param TableEntry EFI_ACPI_TABLE_LIST > instance pointer > > + > > +**/ > > +STATIC > > +VOID > > +FreeTableMemory ( > > + EFI_ACPI_TABLE_LIST *TableEntry > > + ) > > +{ > > + if (TableEntry->PoolAllocation) { > > + gBS->FreePool (TableEntry->Table); > > + } else { > > + gBS->FreePages > ((EFI_PHYSICAL_ADDRESS)(UINTN)TableEntry->Table, > > + EFI_SIZE_TO_PAGES (TableEntry->TableSize)); > > + } > > +} > > + > > /** > > This function adds an ACPI table to the table list. It will detect FACS > and > > allocate the correct type of memory and properly align the table. > > @@ -454,14 +474,15 @@ AddTableToList ( > > OUT UINTN *Handle > > ) > > { > > - EFI_STATUS Status; > > - EFI_ACPI_TABLE_LIST *CurrentTableList; > > - UINT32 CurrentTableSignature; > > - UINT32 CurrentTableSize; > > - UINT32 *CurrentRsdtEntry; > > - VOID *CurrentXsdtEntry; > > - UINT64 Buffer64; > > - BOOLEAN AddToRsdt; > > + EFI_STATUS Status; > > + EFI_ACPI_TABLE_LIST *CurrentTableList; > > + UINT32 CurrentTableSignature; > > + UINT32 CurrentTableSize; > > + UINT32 *CurrentRsdtEntry; > > + VOID *CurrentXsdtEntry; > > + EFI_PHYSICAL_ADDRESS AllocPhysAddress; > > + UINT64 Buffer64; > > + BOOLEAN AddToRsdt; > > > > // > > // Check for invalid input parameters > > @@ -496,8 +517,8 @@ AddTableToList ( > > // There is no architectural reason these should be below 4GB, it is > purely > > // for convenience of implementation that we force memory below > 4GB. > > // > > - CurrentTableList->PageAddress = 0xFFFFFFFF; > > - CurrentTableList->NumberOfPages = EFI_SIZE_TO_PAGES > (CurrentTableSize); > > + AllocPhysAddress = 0xFFFFFFFF; > > + CurrentTableList->TableSize = CurrentTableSize; > > > > // > > // Allocation memory type depends on the type of the table > > @@ -518,9 +539,22 @@ AddTableToList ( > > Status = gBS->AllocatePages ( > > AllocateMaxAddress, > > EfiACPIMemoryNVS, > > - CurrentTableList->NumberOfPages, > > - &CurrentTableList->PageAddress > > + EFI_SIZE_TO_PAGES > (CurrentTableList->TableSize), > > + &AllocPhysAddress > > ); > > + CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER > *)(UINTN)AllocPhysAddress; > > + } else if (mAcpiTableAllocType == AllocateAnyPages) { > > + // > > + // If there is no allocation limit, there is also no need to use page > > + // based allocations for ACPI tables, which may be wasteful on > platforms > > + // such as AArch64 that allocate multiples of 64 KB > > + // > > + Status = gBS->AllocatePool ( > > + EfiACPIReclaimMemory, > > + CurrentTableList->TableSize, > > + (VOID **)&CurrentTableList->Table > > + ); > > + CurrentTableList->PoolAllocation = TRUE; > > } else { > > // > > // All other tables are ACPI reclaim memory, no alignment > requirements. > > @@ -528,9 +562,10 @@ AddTableToList ( > > Status = gBS->AllocatePages ( > > mAcpiTableAllocType, > > EfiACPIReclaimMemory, > > - CurrentTableList->NumberOfPages, > > - &CurrentTableList->PageAddress > > + EFI_SIZE_TO_PAGES > (CurrentTableList->TableSize), > > + &AllocPhysAddress > > ); > > + CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER > *)(UINTN)AllocPhysAddress; > > } > > // > > // Check return value from memory alloc. > > @@ -539,10 +574,6 @@ AddTableToList ( > > gBS->FreePool (CurrentTableList); > > return EFI_OUT_OF_RESOURCES; > > } > > - // > > - // Update the table pointer with the allocated memory start > > - // > > - CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER *) (UINTN) > CurrentTableList->PageAddress; > > > > // > > // Initialize the table contents > > (3) Thus far, in the changes to AddTableToList(), two things bother me: > > (3a) Before the patch, we'd only convert the physical address to a > pointer once we knew that the allocation succeeded. Post-patch, we now > have two branches where we might convert bogus AllocPhysAddress values > (such as even 0xFFFFFFFF) to (EFI_ACPI_COMMON_HEADER*). I think we > shouldn't do this. (It's not just the dereferencing of bogus pointers > that we should avoid, but also the evaluation thereof.) > > (3b) "CurrentTableList" is allocated with AllocatePool() not > AllocateZeroPool(), so *not* setting the "PoolAllocation" field to FALSE > on the two affected branches is wrong. > > The patch looks OK to me otherwise. > > Thanks > Laszlo > > > @@ -575,7 +606,7 @@ AddTableToList ( > > if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && > AcpiTableInstance->Fadt1 != NULL) || > > ((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && > AcpiTableInstance->Fadt3 != NULL) > > ) { > > - gBS->FreePages (CurrentTableList->PageAddress, > CurrentTableList->NumberOfPages); > > + FreeTableMemory (CurrentTableList); > > gBS->FreePool (CurrentTableList); > > return EFI_ACCESS_DENIED; > > } > > @@ -729,7 +760,7 @@ AddTableToList ( > > if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && > AcpiTableInstance->Facs1 != NULL) || > > ((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && > AcpiTableInstance->Facs3 != NULL) > > ) { > > - gBS->FreePages (CurrentTableList->PageAddress, > CurrentTableList->NumberOfPages); > > + FreeTableMemory (CurrentTableList); > > gBS->FreePool (CurrentTableList); > > return EFI_ACCESS_DENIED; > > } > > @@ -813,7 +844,7 @@ AddTableToList ( > > if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && > AcpiTableInstance->Dsdt1 != NULL) || > > ((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && > AcpiTableInstance->Dsdt3 != NULL) > > ) { > > - gBS->FreePages (CurrentTableList->PageAddress, > CurrentTableList->NumberOfPages); > > + FreeTableMemory (CurrentTableList); > > gBS->FreePool (CurrentTableList); > > return EFI_ACCESS_DENIED; > > } > > @@ -1449,7 +1480,7 @@ DeleteTable ( > > // > > // Free the Table > > // > > - gBS->FreePages (Table->PageAddress, Table->NumberOfPages); > > + FreeTableMemory (Table); > > RemoveEntryList (&(Table->Link)); > > gBS->FreePool (Table); > > } > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: 回复: [edk2-devel] [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use pool allocations when possible 2020-10-22 2:01 ` 回复: " gaoliming @ 2020-10-22 12:55 ` Ard Biesheuvel 2020-10-26 1:35 ` 回复: " gaoliming 0 siblings, 1 reply; 18+ messages in thread From: Ard Biesheuvel @ 2020-10-22 12:55 UTC (permalink / raw) To: gaoliming, 'Laszlo Ersek', devel Cc: 'Dandan Bi', 'Jian J Wang', 'Hao A Wu', 'Sami Mujawar', 'Leif Lindholm' On 10/22/20 4:01 AM, gaoliming wrote: > Ard: > Can you help to collect memmap information on Shell before this change and after this change? I want to know what’s impact on the memory layout. > Before: =================== Type Start End # Pages Available 0000000040000000-00000000403B2FFF 00000000000003B3 LoaderCode 00000000403B3000-000000004047FFFF 00000000000000CD ACPI_Recl 0000000040480000-00000000404DFFFF 0000000000000060 Available 00000000404E0000-00000000404FFFFF 0000000000000020 ACPI_Recl 0000000040500000-000000004051FFFF 0000000000000020 RT_Code 0000000040520000-000000004059FFFF 0000000000000080 RT_Data 00000000405A0000-000000004068FFFF 00000000000000F0 RT_Code 0000000040690000-000000004074FFFF 00000000000000C0 Available 0000000040750000-000000004188FFFF 0000000000001140 BS_Data 0000000041890000-00000000418ADFFF 000000000000001E Available 00000000418AE000-00000000418CBFFF 000000000000001E BS_Data 00000000418CC000-000000004299AFFF 00000000000010CF Available 000000004299B000-000000004299BFFF 0000000000000001 BS_Data 000000004299C000-0000000043643FFF 0000000000000CA8 Available 0000000043644000-000000004391AFFF 00000000000002D7 BS_Code 000000004391B000-0000000043C1FFFF 0000000000000305 RT_Code 0000000043C20000-0000000043DAFFFF 0000000000000190 RT_Data 0000000043DB0000-0000000043FFFFFF 0000000000000250 Available 0000000044000000-000000004401EFFF 000000000000001F BS_Data 000000004401F000-000000004401FFFF 0000000000000001 Available 0000000044020000-00000000477F7FFF 00000000000037D8 BS_Data 00000000477F8000-0000000047819FFF 0000000000000022 BS_Code 000000004781A000-000000004784FFFF 0000000000000036 BS_Data 0000000047850000-0000000047EEBFFF 000000000000069C BS_Code 0000000047EEC000-0000000047EEFFFF 0000000000000004 BS_Data 0000000047EF0000-0000000047FF0FFF 0000000000000101 BS_Code 0000000047FF1000-0000000047FFAFFF 000000000000000A BS_Data 0000000047FFB000-0000000047FFFFFF 0000000000000005 MMIO 0000000004000000-0000000007FFFFFF 0000000000004000 MMIO 0000000009010000-0000000009010FFF 0000000000000001 Reserved : 0 Pages (0 Bytes) LoaderCode: 205 Pages (839,680 Bytes) LoaderData: 0 Pages (0 Bytes) BS_Code : 841 Pages (3,444,736 Bytes) BS_Data : 9,561 Pages (39,161,856 Bytes) RT_Code : 720 Pages (2,949,120 Bytes) RT_Data : 832 Pages (3,407,872 Bytes) ACPI_Recl : 128 Pages (524,288 Bytes) ACPI_NVS : 0 Pages (0 Bytes) MMIO : 16,385 Pages (67,112,960 Bytes) MMIO_Port : 0 Pages (0 Bytes) PalCode : 0 Pages (0 Bytes) Available : 20,481 Pages (83,890,176 Bytes) Persistent: 0 Pages (0 Bytes) -------------- Total Memory: 128 MB (134,217,728 Bytes) After: =================== Type Start End # Pages Available 0000000040000000-0000000043EE1FFF 0000000000003EE2 LoaderCode 0000000043EE2000-0000000043FBFFFF 00000000000000DE RT_Code 0000000043FC0000-0000000043FFFFFF 0000000000000040 BS_Data 0000000044000000-000000004401FFFF 0000000000000020 Available 0000000044020000-000000004402FFFF 0000000000000010 ACPI_Recl 0000000044030000-000000004403FFFF 0000000000000010 RT_Code 0000000044040000-000000004407FFFF 0000000000000040 RT_Data 0000000044080000-000000004413FFFF 00000000000000C0 RT_Code 0000000044140000-000000004418FFFF 0000000000000050 RT_Data 0000000044190000-000000004422FFFF 00000000000000A0 RT_Code 0000000044230000-000000004431FFFF 00000000000000F0 Available 0000000044320000-0000000045481FFF 0000000000001162 BS_Data 0000000045482000-000000004549FFFF 000000000000001E Available 00000000454A0000-00000000454BDFFF 000000000000001E BS_Data 00000000454BE000-00000000454CAFFF 000000000000000D Available 00000000454CB000-00000000454CFFFF 0000000000000005 BS_Data 00000000454D0000-0000000047217FFF 0000000000001D48 Available 0000000047218000-00000000472AAFFF 0000000000000093 BS_Code 00000000472AB000-00000000475FFFFF 0000000000000355 RT_Code 0000000047600000-000000004768FFFF 0000000000000090 Available 0000000047690000-000000004769FFFF 0000000000000010 RT_Data 00000000476A0000-00000000477BFFFF 0000000000000120 Available 00000000477C0000-00000000477DEFFF 000000000000001F BS_Data 00000000477DF000-0000000047801FFF 0000000000000023 BS_Code 0000000047802000-0000000047848FFF 0000000000000047 BS_Data 0000000047849000-0000000047EDCFFF 0000000000000694 BS_Code 0000000047EDD000-0000000047EE4FFF 0000000000000008 BS_Data 0000000047EE5000-0000000047FE5FFF 0000000000000101 BS_Code 0000000047FE6000-0000000047FFAFFF 0000000000000015 BS_Data 0000000047FFB000-0000000047FFFFFF 0000000000000005 MMIO 0000000004000000-0000000007FFFFFF 0000000000004000 MMIO 0000000009010000-0000000009010FFF 0000000000000001 Reserved : 0 Pages (0 Bytes) LoaderCode: 222 Pages (909,312 Bytes) LoaderData: 0 Pages (0 Bytes) BS_Code : 953 Pages (3,903,488 Bytes) BS_Data : 9,552 Pages (39,124,992 Bytes) RT_Code : 592 Pages (2,424,832 Bytes) RT_Data : 640 Pages (2,621,440 Bytes) ACPI_Recl : 16 Pages (65,536 Bytes) ACPI_NVS : 0 Pages (0 Bytes) MMIO : 16,385 Pages (67,112,960 Bytes) MMIO_Port : 0 Pages (0 Bytes) PalCode : 0 Pages (0 Bytes) Available : 20,793 Pages (85,168,128 Bytes) Persistent: 0 Pages (0 Bytes) -------------- Total Memory: 128 MB (134,217,728 Bytes) > Liming >> -----邮件原件----- >> 发件人: Laszlo Ersek <lersek@redhat.com> >> 发送时间: 2020年10月20日 3:48 >> 收件人: devel@edk2.groups.io; ard.biesheuvel@arm.com >> 抄送: Dandan Bi <dandan.bi@intel.com>; Liming Gao >> <gaoliming@byosoft.com.cn>; Jian J Wang <jian.j.wang@intel.com>; Hao A >> Wu <hao.a.wu@intel.com>; Sami Mujawar <sami.mujawar@arm.com>; Leif >> Lindholm <leif@nuviainc.com> >> 主题: Re: [edk2-devel] [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use pool >> allocations when possible >> >> On 10/16/20 17:49, Ard Biesheuvel wrote: >>> On AArch64 systems, page based allocations for memory types that are >>> relevant to the OS are rounded up to 64 KB multiples. This wastes >>> some space in the ACPI table memory allocator, since it uses page >>> based allocations in order to be able to place the ACPI tables low >>> in memory. >>> >>> Since the latter requirement does not exist on AArch64, switch to pool >>> allocations for all ACPI tables except the root tables if the active >>> allocation policy permits them to be anywhere in memory. The root >>> tables will be handled in a subsequent patch. >>> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> >>> --- >>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h | 4 >> +- >>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c | >> 4 +- >>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 75 >> ++++++++++++++------ >>> 3 files changed, 57 insertions(+), 26 deletions(-) >>> >>> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h >> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h >>> index 425a462fd92b..6e600e7acd24 100644 >>> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h >>> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h >>> @@ -64,9 +64,9 @@ typedef struct { >>> LIST_ENTRY Link; >>> EFI_ACPI_TABLE_VERSION Version; >>> EFI_ACPI_COMMON_HEADER *Table; >>> - EFI_PHYSICAL_ADDRESS PageAddress; >>> - UINTN NumberOfPages; >>> + UINTN TableSize; >>> UINTN Handle; >>> + BOOLEAN PoolAllocation; >>> } EFI_ACPI_TABLE_LIST; >>> >>> // >> >> (1) The preceding comment (which explains the fields) should be updated. >> >>> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c >> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c >>> index b1cba20c8ed7..14ced68e645f 100644 >>> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c >>> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c >>> @@ -68,8 +68,8 @@ FindTableByBuffer ( >>> >>> while (CurrentLink != StartLink) { >>> CurrentTableList = EFI_ACPI_TABLE_LIST_FROM_LINK (CurrentLink); >>> - if (((UINTN)CurrentTableList->PageAddress <= (UINTN)Buffer) && >>> - ((UINTN)CurrentTableList->PageAddress + >> EFI_PAGES_TO_SIZE(CurrentTableList->NumberOfPages) > (UINTN)Buffer)) { >>> + if (((UINTN)CurrentTableList->Table <= (UINTN)Buffer) && >>> + ((UINTN)CurrentTableList->Table + CurrentTableList->TableSize > >> (UINTN)Buffer)) { >>> // >>> // Good! Found Table. >>> // >>> diff --git >> a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c >> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c >>> index ad7baf8205b4..b05e57e6584f 100644 >>> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c >>> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c >>> @@ -428,6 +428,26 @@ ReallocateAcpiTableBuffer ( >>> return EFI_SUCCESS; >>> } >>> >>> +/** >>> + Free the memory associated with provided the EFI_ACPI_TABLE_LIST >> instance >> >> (2) s/with provided the/with the provided/ >> >>> + >>> + @param TableEntry EFI_ACPI_TABLE_LIST >> instance pointer >>> + >>> +**/ >>> +STATIC >>> +VOID >>> +FreeTableMemory ( >>> + EFI_ACPI_TABLE_LIST *TableEntry >>> + ) >>> +{ >>> + if (TableEntry->PoolAllocation) { >>> + gBS->FreePool (TableEntry->Table); >>> + } else { >>> + gBS->FreePages >> ((EFI_PHYSICAL_ADDRESS)(UINTN)TableEntry->Table, >>> + EFI_SIZE_TO_PAGES (TableEntry->TableSize)); >>> + } >>> +} >>> + >>> /** >>> This function adds an ACPI table to the table list. It will detect FACS >> and >>> allocate the correct type of memory and properly align the table. >>> @@ -454,14 +474,15 @@ AddTableToList ( >>> OUT UINTN *Handle >>> ) >>> { >>> - EFI_STATUS Status; >>> - EFI_ACPI_TABLE_LIST *CurrentTableList; >>> - UINT32 CurrentTableSignature; >>> - UINT32 CurrentTableSize; >>> - UINT32 *CurrentRsdtEntry; >>> - VOID *CurrentXsdtEntry; >>> - UINT64 Buffer64; >>> - BOOLEAN AddToRsdt; >>> + EFI_STATUS Status; >>> + EFI_ACPI_TABLE_LIST *CurrentTableList; >>> + UINT32 CurrentTableSignature; >>> + UINT32 CurrentTableSize; >>> + UINT32 *CurrentRsdtEntry; >>> + VOID *CurrentXsdtEntry; >>> + EFI_PHYSICAL_ADDRESS AllocPhysAddress; >>> + UINT64 Buffer64; >>> + BOOLEAN AddToRsdt; >>> >>> // >>> // Check for invalid input parameters >>> @@ -496,8 +517,8 @@ AddTableToList ( >>> // There is no architectural reason these should be below 4GB, it is >> purely >>> // for convenience of implementation that we force memory below >> 4GB. >>> // >>> - CurrentTableList->PageAddress = 0xFFFFFFFF; >>> - CurrentTableList->NumberOfPages = EFI_SIZE_TO_PAGES >> (CurrentTableSize); >>> + AllocPhysAddress = 0xFFFFFFFF; >>> + CurrentTableList->TableSize = CurrentTableSize; >>> >>> // >>> // Allocation memory type depends on the type of the table >>> @@ -518,9 +539,22 @@ AddTableToList ( >>> Status = gBS->AllocatePages ( >>> AllocateMaxAddress, >>> EfiACPIMemoryNVS, >>> - CurrentTableList->NumberOfPages, >>> - &CurrentTableList->PageAddress >>> + EFI_SIZE_TO_PAGES >> (CurrentTableList->TableSize), >>> + &AllocPhysAddress >>> ); >>> + CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER >> *)(UINTN)AllocPhysAddress; >>> + } else if (mAcpiTableAllocType == AllocateAnyPages) { >>> + // >>> + // If there is no allocation limit, there is also no need to use page >>> + // based allocations for ACPI tables, which may be wasteful on >> platforms >>> + // such as AArch64 that allocate multiples of 64 KB >>> + // >>> + Status = gBS->AllocatePool ( >>> + EfiACPIReclaimMemory, >>> + CurrentTableList->TableSize, >>> + (VOID **)&CurrentTableList->Table >>> + ); >>> + CurrentTableList->PoolAllocation = TRUE; >>> } else { >>> // >>> // All other tables are ACPI reclaim memory, no alignment >> requirements. >>> @@ -528,9 +562,10 @@ AddTableToList ( >>> Status = gBS->AllocatePages ( >>> mAcpiTableAllocType, >>> EfiACPIReclaimMemory, >>> - CurrentTableList->NumberOfPages, >>> - &CurrentTableList->PageAddress >>> + EFI_SIZE_TO_PAGES >> (CurrentTableList->TableSize), >>> + &AllocPhysAddress >>> ); >>> + CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER >> *)(UINTN)AllocPhysAddress; >>> } >>> // >>> // Check return value from memory alloc. >>> @@ -539,10 +574,6 @@ AddTableToList ( >>> gBS->FreePool (CurrentTableList); >>> return EFI_OUT_OF_RESOURCES; >>> } >>> - // >>> - // Update the table pointer with the allocated memory start >>> - // >>> - CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER *) (UINTN) >> CurrentTableList->PageAddress; >>> >>> // >>> // Initialize the table contents >> >> (3) Thus far, in the changes to AddTableToList(), two things bother me: >> >> (3a) Before the patch, we'd only convert the physical address to a >> pointer once we knew that the allocation succeeded. Post-patch, we now >> have two branches where we might convert bogus AllocPhysAddress values >> (such as even 0xFFFFFFFF) to (EFI_ACPI_COMMON_HEADER*). I think we >> shouldn't do this. (It's not just the dereferencing of bogus pointers >> that we should avoid, but also the evaluation thereof.) >> >> (3b) "CurrentTableList" is allocated with AllocatePool() not >> AllocateZeroPool(), so *not* setting the "PoolAllocation" field to FALSE >> on the two affected branches is wrong. >> >> The patch looks OK to me otherwise. >> >> Thanks >> Laszlo >> >>> @@ -575,7 +606,7 @@ AddTableToList ( >>> if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && >> AcpiTableInstance->Fadt1 != NULL) || >>> ((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && >> AcpiTableInstance->Fadt3 != NULL) >>> ) { >>> - gBS->FreePages (CurrentTableList->PageAddress, >> CurrentTableList->NumberOfPages); >>> + FreeTableMemory (CurrentTableList); >>> gBS->FreePool (CurrentTableList); >>> return EFI_ACCESS_DENIED; >>> } >>> @@ -729,7 +760,7 @@ AddTableToList ( >>> if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && >> AcpiTableInstance->Facs1 != NULL) || >>> ((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && >> AcpiTableInstance->Facs3 != NULL) >>> ) { >>> - gBS->FreePages (CurrentTableList->PageAddress, >> CurrentTableList->NumberOfPages); >>> + FreeTableMemory (CurrentTableList); >>> gBS->FreePool (CurrentTableList); >>> return EFI_ACCESS_DENIED; >>> } >>> @@ -813,7 +844,7 @@ AddTableToList ( >>> if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && >> AcpiTableInstance->Dsdt1 != NULL) || >>> ((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && >> AcpiTableInstance->Dsdt3 != NULL) >>> ) { >>> - gBS->FreePages (CurrentTableList->PageAddress, >> CurrentTableList->NumberOfPages); >>> + FreeTableMemory (CurrentTableList); >>> gBS->FreePool (CurrentTableList); >>> return EFI_ACCESS_DENIED; >>> } >>> @@ -1449,7 +1480,7 @@ DeleteTable ( >>> // >>> // Free the Table >>> // >>> - gBS->FreePages (Table->PageAddress, Table->NumberOfPages); >>> + FreeTableMemory (Table); >>> RemoveEntryList (&(Table->Link)); >>> gBS->FreePool (Table); >>> } >>> > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* 回复: 回复: [edk2-devel] [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use pool allocations when possible 2020-10-22 12:55 ` Ard Biesheuvel @ 2020-10-26 1:35 ` gaoliming 2020-10-26 6:25 ` Laszlo Ersek 0 siblings, 1 reply; 18+ messages in thread From: gaoliming @ 2020-10-26 1:35 UTC (permalink / raw) To: 'Ard Biesheuvel', 'Laszlo Ersek', devel Cc: 'Dandan Bi', 'Jian J Wang', 'Hao A Wu', 'Sami Mujawar', 'Leif Lindholm' Ard: I verify this patch on OvmfX64 and collect the memmap info. I don't see the difference in memmap. So, this enhancement is for AARCH64 only. Is it right? Thanks Liming > -----邮件原件----- > 发件人: Ard Biesheuvel <ard.biesheuvel@arm.com> > 发送时间: 2020年10月22日 20:56 > 收件人: gaoliming <gaoliming@byosoft.com.cn>; 'Laszlo Ersek' > <lersek@redhat.com>; devel@edk2.groups.io > 抄送: 'Dandan Bi' <dandan.bi@intel.com>; 'Jian J Wang' > <jian.j.wang@intel.com>; 'Hao A Wu' <hao.a.wu@intel.com>; 'Sami Mujawar' > <sami.mujawar@arm.com>; 'Leif Lindholm' <leif@nuviainc.com> > 主题: Re: 回复: [edk2-devel] [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use > pool allocations when possible > > On 10/22/20 4:01 AM, gaoliming wrote: > > Ard: > > Can you help to collect memmap information on Shell before this > change and after this change? I want to know what’s impact on the memory > layout. > > > > > Before: > =================== > Type Start End # Pages > Available 0000000040000000-00000000403B2FFF 00000000000003B3 > LoaderCode 00000000403B3000-000000004047FFFF 00000000000000CD > ACPI_Recl 0000000040480000-00000000404DFFFF 0000000000000060 > Available 00000000404E0000-00000000404FFFFF 0000000000000020 > ACPI_Recl 0000000040500000-000000004051FFFF 0000000000000020 > RT_Code 0000000040520000-000000004059FFFF 0000000000000080 > RT_Data 00000000405A0000-000000004068FFFF 00000000000000F0 > RT_Code 0000000040690000-000000004074FFFF 00000000000000C0 > Available 0000000040750000-000000004188FFFF 0000000000001140 > BS_Data 0000000041890000-00000000418ADFFF 000000000000001E > Available 00000000418AE000-00000000418CBFFF 000000000000001E > BS_Data 00000000418CC000-000000004299AFFF 00000000000010CF > Available 000000004299B000-000000004299BFFF 0000000000000001 > BS_Data 000000004299C000-0000000043643FFF 0000000000000CA8 > Available 0000000043644000-000000004391AFFF 00000000000002D7 > BS_Code 000000004391B000-0000000043C1FFFF 0000000000000305 > RT_Code 0000000043C20000-0000000043DAFFFF 0000000000000190 > RT_Data 0000000043DB0000-0000000043FFFFFF 0000000000000250 > Available 0000000044000000-000000004401EFFF 000000000000001F > BS_Data 000000004401F000-000000004401FFFF 0000000000000001 > Available 0000000044020000-00000000477F7FFF 00000000000037D8 > BS_Data 00000000477F8000-0000000047819FFF 0000000000000022 > BS_Code 000000004781A000-000000004784FFFF 0000000000000036 > BS_Data 0000000047850000-0000000047EEBFFF 000000000000069C > BS_Code 0000000047EEC000-0000000047EEFFFF 0000000000000004 > BS_Data 0000000047EF0000-0000000047FF0FFF 0000000000000101 > BS_Code 0000000047FF1000-0000000047FFAFFF 000000000000000A > BS_Data 0000000047FFB000-0000000047FFFFFF 0000000000000005 > MMIO 0000000004000000-0000000007FFFFFF 0000000000004000 > MMIO 0000000009010000-0000000009010FFF 0000000000000001 > > Reserved : 0 Pages (0 Bytes) > LoaderCode: 205 Pages (839,680 Bytes) > LoaderData: 0 Pages (0 Bytes) > BS_Code : 841 Pages (3,444,736 Bytes) > BS_Data : 9,561 Pages (39,161,856 Bytes) > RT_Code : 720 Pages (2,949,120 Bytes) > RT_Data : 832 Pages (3,407,872 Bytes) > ACPI_Recl : 128 Pages (524,288 Bytes) > ACPI_NVS : 0 Pages (0 Bytes) > MMIO : 16,385 Pages (67,112,960 Bytes) > MMIO_Port : 0 Pages (0 Bytes) > PalCode : 0 Pages (0 Bytes) > Available : 20,481 Pages (83,890,176 Bytes) > Persistent: 0 Pages (0 Bytes) > -------------- > Total Memory: 128 MB (134,217,728 Bytes) > > > After: > =================== > Type Start End # Pages > Available 0000000040000000-0000000043EE1FFF 0000000000003EE2 > LoaderCode 0000000043EE2000-0000000043FBFFFF 00000000000000DE > RT_Code 0000000043FC0000-0000000043FFFFFF 0000000000000040 > BS_Data 0000000044000000-000000004401FFFF 0000000000000020 > Available 0000000044020000-000000004402FFFF 0000000000000010 > ACPI_Recl 0000000044030000-000000004403FFFF 0000000000000010 > RT_Code 0000000044040000-000000004407FFFF 0000000000000040 > RT_Data 0000000044080000-000000004413FFFF 00000000000000C0 > RT_Code 0000000044140000-000000004418FFFF 0000000000000050 > RT_Data 0000000044190000-000000004422FFFF 00000000000000A0 > RT_Code 0000000044230000-000000004431FFFF 00000000000000F0 > Available 0000000044320000-0000000045481FFF 0000000000001162 > BS_Data 0000000045482000-000000004549FFFF 000000000000001E > Available 00000000454A0000-00000000454BDFFF 000000000000001E > BS_Data 00000000454BE000-00000000454CAFFF 000000000000000D > Available 00000000454CB000-00000000454CFFFF 0000000000000005 > BS_Data 00000000454D0000-0000000047217FFF 0000000000001D48 > Available 0000000047218000-00000000472AAFFF 0000000000000093 > BS_Code 00000000472AB000-00000000475FFFFF 0000000000000355 > RT_Code 0000000047600000-000000004768FFFF 0000000000000090 > Available 0000000047690000-000000004769FFFF 0000000000000010 > RT_Data 00000000476A0000-00000000477BFFFF 0000000000000120 > Available 00000000477C0000-00000000477DEFFF 000000000000001F > BS_Data 00000000477DF000-0000000047801FFF 0000000000000023 > BS_Code 0000000047802000-0000000047848FFF 0000000000000047 > BS_Data 0000000047849000-0000000047EDCFFF 0000000000000694 > BS_Code 0000000047EDD000-0000000047EE4FFF 0000000000000008 > BS_Data 0000000047EE5000-0000000047FE5FFF 0000000000000101 > BS_Code 0000000047FE6000-0000000047FFAFFF 0000000000000015 > BS_Data 0000000047FFB000-0000000047FFFFFF 0000000000000005 > MMIO 0000000004000000-0000000007FFFFFF 0000000000004000 > MMIO 0000000009010000-0000000009010FFF 0000000000000001 > > Reserved : 0 Pages (0 Bytes) > LoaderCode: 222 Pages (909,312 Bytes) > LoaderData: 0 Pages (0 Bytes) > BS_Code : 953 Pages (3,903,488 Bytes) > BS_Data : 9,552 Pages (39,124,992 Bytes) > RT_Code : 592 Pages (2,424,832 Bytes) > RT_Data : 640 Pages (2,621,440 Bytes) > ACPI_Recl : 16 Pages (65,536 Bytes) > ACPI_NVS : 0 Pages (0 Bytes) > MMIO : 16,385 Pages (67,112,960 Bytes) > MMIO_Port : 0 Pages (0 Bytes) > PalCode : 0 Pages (0 Bytes) > Available : 20,793 Pages (85,168,128 Bytes) > Persistent: 0 Pages (0 Bytes) > -------------- > Total Memory: 128 MB (134,217,728 Bytes) > > > > Liming > >> -----邮件原件----- > >> 发件人: Laszlo Ersek <lersek@redhat.com> > >> 发送时间: 2020年10月20日 3:48 > >> 收件人: devel@edk2.groups.io; ard.biesheuvel@arm.com > >> 抄送: Dandan Bi <dandan.bi@intel.com>; Liming Gao > >> <gaoliming@byosoft.com.cn>; Jian J Wang <jian.j.wang@intel.com>; Hao > A > >> Wu <hao.a.wu@intel.com>; Sami Mujawar <sami.mujawar@arm.com>; > Leif > >> Lindholm <leif@nuviainc.com> > >> 主题: Re: [edk2-devel] [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use > pool > >> allocations when possible > >> > >> On 10/16/20 17:49, Ard Biesheuvel wrote: > >>> On AArch64 systems, page based allocations for memory types that are > >>> relevant to the OS are rounded up to 64 KB multiples. This wastes > >>> some space in the ACPI table memory allocator, since it uses page > >>> based allocations in order to be able to place the ACPI tables low > >>> in memory. > >>> > >>> Since the latter requirement does not exist on AArch64, switch to pool > >>> allocations for all ACPI tables except the root tables if the active > >>> allocation policy permits them to be anywhere in memory. The root > >>> tables will be handled in a subsequent patch. > >>> > >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> > >>> --- > >>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h | > 4 > >> +- > >>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c > | > >> 4 +- > >>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | > 75 > >> ++++++++++++++------ > >>> 3 files changed, 57 insertions(+), 26 deletions(-) > >>> > >>> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h > >> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h > >>> index 425a462fd92b..6e600e7acd24 100644 > >>> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h > >>> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h > >>> @@ -64,9 +64,9 @@ typedef struct { > >>> LIST_ENTRY Link; > >>> EFI_ACPI_TABLE_VERSION Version; > >>> EFI_ACPI_COMMON_HEADER *Table; > >>> - EFI_PHYSICAL_ADDRESS PageAddress; > >>> - UINTN NumberOfPages; > >>> + UINTN TableSize; > >>> UINTN Handle; > >>> + BOOLEAN PoolAllocation; > >>> } EFI_ACPI_TABLE_LIST; > >>> > >>> // > >> > >> (1) The preceding comment (which explains the fields) should be updated. > >> > >>> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c > >> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c > >>> index b1cba20c8ed7..14ced68e645f 100644 > >>> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c > >>> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c > >>> @@ -68,8 +68,8 @@ FindTableByBuffer ( > >>> > >>> while (CurrentLink != StartLink) { > >>> CurrentTableList = EFI_ACPI_TABLE_LIST_FROM_LINK > (CurrentLink); > >>> - if (((UINTN)CurrentTableList->PageAddress <= (UINTN)Buffer) && > >>> - ((UINTN)CurrentTableList->PageAddress + > >> EFI_PAGES_TO_SIZE(CurrentTableList->NumberOfPages) > (UINTN)Buffer)) > { > >>> + if (((UINTN)CurrentTableList->Table <= (UINTN)Buffer) && > >>> + ((UINTN)CurrentTableList->Table + > CurrentTableList->TableSize > > >> (UINTN)Buffer)) { > >>> // > >>> // Good! Found Table. > >>> // > >>> diff --git > >> a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > >> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > >>> index ad7baf8205b4..b05e57e6584f 100644 > >>> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > >>> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > >>> @@ -428,6 +428,26 @@ ReallocateAcpiTableBuffer ( > >>> return EFI_SUCCESS; > >>> } > >>> > >>> +/** > >>> + Free the memory associated with provided the EFI_ACPI_TABLE_LIST > >> instance > >> > >> (2) s/with provided the/with the provided/ > >> > >>> + > >>> + @param TableEntry EFI_ACPI_TABLE_LIST > >> instance pointer > >>> + > >>> +**/ > >>> +STATIC > >>> +VOID > >>> +FreeTableMemory ( > >>> + EFI_ACPI_TABLE_LIST *TableEntry > >>> + ) > >>> +{ > >>> + if (TableEntry->PoolAllocation) { > >>> + gBS->FreePool (TableEntry->Table); > >>> + } else { > >>> + gBS->FreePages > >> ((EFI_PHYSICAL_ADDRESS)(UINTN)TableEntry->Table, > >>> + EFI_SIZE_TO_PAGES (TableEntry->TableSize)); > >>> + } > >>> +} > >>> + > >>> /** > >>> This function adds an ACPI table to the table list. It will detect > FACS > >> and > >>> allocate the correct type of memory and properly align the table. > >>> @@ -454,14 +474,15 @@ AddTableToList ( > >>> OUT UINTN *Handle > >>> ) > >>> { > >>> - EFI_STATUS Status; > >>> - EFI_ACPI_TABLE_LIST *CurrentTableList; > >>> - UINT32 CurrentTableSignature; > >>> - UINT32 CurrentTableSize; > >>> - UINT32 *CurrentRsdtEntry; > >>> - VOID *CurrentXsdtEntry; > >>> - UINT64 Buffer64; > >>> - BOOLEAN AddToRsdt; > >>> + EFI_STATUS Status; > >>> + EFI_ACPI_TABLE_LIST *CurrentTableList; > >>> + UINT32 CurrentTableSignature; > >>> + UINT32 CurrentTableSize; > >>> + UINT32 *CurrentRsdtEntry; > >>> + VOID *CurrentXsdtEntry; > >>> + EFI_PHYSICAL_ADDRESS AllocPhysAddress; > >>> + UINT64 Buffer64; > >>> + BOOLEAN AddToRsdt; > >>> > >>> // > >>> // Check for invalid input parameters > >>> @@ -496,8 +517,8 @@ AddTableToList ( > >>> // There is no architectural reason these should be below 4GB, it is > >> purely > >>> // for convenience of implementation that we force memory below > >> 4GB. > >>> // > >>> - CurrentTableList->PageAddress = 0xFFFFFFFF; > >>> - CurrentTableList->NumberOfPages = EFI_SIZE_TO_PAGES > >> (CurrentTableSize); > >>> + AllocPhysAddress = 0xFFFFFFFF; > >>> + CurrentTableList->TableSize = CurrentTableSize; > >>> > >>> // > >>> // Allocation memory type depends on the type of the table > >>> @@ -518,9 +539,22 @@ AddTableToList ( > >>> Status = gBS->AllocatePages ( > >>> AllocateMaxAddress, > >>> EfiACPIMemoryNVS, > >>> - CurrentTableList->NumberOfPages, > >>> - &CurrentTableList->PageAddress > >>> + EFI_SIZE_TO_PAGES > >> (CurrentTableList->TableSize), > >>> + &AllocPhysAddress > >>> ); > >>> + CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER > >> *)(UINTN)AllocPhysAddress; > >>> + } else if (mAcpiTableAllocType == AllocateAnyPages) { > >>> + // > >>> + // If there is no allocation limit, there is also no need to use page > >>> + // based allocations for ACPI tables, which may be wasteful on > >> platforms > >>> + // such as AArch64 that allocate multiples of 64 KB > >>> + // > >>> + Status = gBS->AllocatePool ( > >>> + EfiACPIReclaimMemory, > >>> + CurrentTableList->TableSize, > >>> + (VOID **)&CurrentTableList->Table > >>> + ); > >>> + CurrentTableList->PoolAllocation = TRUE; > >>> } else { > >>> // > >>> // All other tables are ACPI reclaim memory, no alignment > >> requirements. > >>> @@ -528,9 +562,10 @@ AddTableToList ( > >>> Status = gBS->AllocatePages ( > >>> mAcpiTableAllocType, > >>> EfiACPIReclaimMemory, > >>> - CurrentTableList->NumberOfPages, > >>> - &CurrentTableList->PageAddress > >>> + EFI_SIZE_TO_PAGES > >> (CurrentTableList->TableSize), > >>> + &AllocPhysAddress > >>> ); > >>> + CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER > >> *)(UINTN)AllocPhysAddress; > >>> } > >>> // > >>> // Check return value from memory alloc. > >>> @@ -539,10 +574,6 @@ AddTableToList ( > >>> gBS->FreePool (CurrentTableList); > >>> return EFI_OUT_OF_RESOURCES; > >>> } > >>> - // > >>> - // Update the table pointer with the allocated memory start > >>> - // > >>> - CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER *) (UINTN) > >> CurrentTableList->PageAddress; > >>> > >>> // > >>> // Initialize the table contents > >> > >> (3) Thus far, in the changes to AddTableToList(), two things bother me: > >> > >> (3a) Before the patch, we'd only convert the physical address to a > >> pointer once we knew that the allocation succeeded. Post-patch, we now > >> have two branches where we might convert bogus AllocPhysAddress > values > >> (such as even 0xFFFFFFFF) to (EFI_ACPI_COMMON_HEADER*). I think we > >> shouldn't do this. (It's not just the dereferencing of bogus pointers > >> that we should avoid, but also the evaluation thereof.) > >> > >> (3b) "CurrentTableList" is allocated with AllocatePool() not > >> AllocateZeroPool(), so *not* setting the "PoolAllocation" field to FALSE > >> on the two affected branches is wrong. > >> > >> The patch looks OK to me otherwise. > >> > >> Thanks > >> Laszlo > >> > >>> @@ -575,7 +606,7 @@ AddTableToList ( > >>> if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && > >> AcpiTableInstance->Fadt1 != NULL) || > >>> ((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && > >> AcpiTableInstance->Fadt3 != NULL) > >>> ) { > >>> - gBS->FreePages (CurrentTableList->PageAddress, > >> CurrentTableList->NumberOfPages); > >>> + FreeTableMemory (CurrentTableList); > >>> gBS->FreePool (CurrentTableList); > >>> return EFI_ACCESS_DENIED; > >>> } > >>> @@ -729,7 +760,7 @@ AddTableToList ( > >>> if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && > >> AcpiTableInstance->Facs1 != NULL) || > >>> ((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && > >> AcpiTableInstance->Facs3 != NULL) > >>> ) { > >>> - gBS->FreePages (CurrentTableList->PageAddress, > >> CurrentTableList->NumberOfPages); > >>> + FreeTableMemory (CurrentTableList); > >>> gBS->FreePool (CurrentTableList); > >>> return EFI_ACCESS_DENIED; > >>> } > >>> @@ -813,7 +844,7 @@ AddTableToList ( > >>> if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && > >> AcpiTableInstance->Dsdt1 != NULL) || > >>> ((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && > >> AcpiTableInstance->Dsdt3 != NULL) > >>> ) { > >>> - gBS->FreePages (CurrentTableList->PageAddress, > >> CurrentTableList->NumberOfPages); > >>> + FreeTableMemory (CurrentTableList); > >>> gBS->FreePool (CurrentTableList); > >>> return EFI_ACCESS_DENIED; > >>> } > >>> @@ -1449,7 +1480,7 @@ DeleteTable ( > >>> // > >>> // Free the Table > >>> // > >>> - gBS->FreePages (Table->PageAddress, Table->NumberOfPages); > >>> + FreeTableMemory (Table); > >>> RemoveEntryList (&(Table->Link)); > >>> gBS->FreePool (Table); > >>> } > >>> > > > > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: 回复: 回复: [edk2-devel] [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use pool allocations when possible 2020-10-26 1:35 ` 回复: " gaoliming @ 2020-10-26 6:25 ` Laszlo Ersek 2020-10-26 7:42 ` Ard Biesheuvel 0 siblings, 1 reply; 18+ messages in thread From: Laszlo Ersek @ 2020-10-26 6:25 UTC (permalink / raw) To: gaoliming, 'Ard Biesheuvel', devel Cc: 'Dandan Bi', 'Jian J Wang', 'Hao A Wu', 'Sami Mujawar', 'Leif Lindholm' On 10/26/20 02:35, gaoliming wrote: > Ard: > I verify this patch on OvmfX64 and collect the memmap info. I don't see the difference in memmap. > So, this enhancement is for AARCH64 only. Is it right? That's my understanding, yes. OVMF enables ACPI 1.0b support in the bitmask PCD, and so the compat code for 32-bit allocations (= page allocations for expressing the 4GB limit) remains active. Laszlo >> -----邮件原件----- >> 发件人: Ard Biesheuvel <ard.biesheuvel@arm.com> >> 发送时间: 2020年10月22日 20:56 >> 收件人: gaoliming <gaoliming@byosoft.com.cn>; 'Laszlo Ersek' >> <lersek@redhat.com>; devel@edk2.groups.io >> 抄送: 'Dandan Bi' <dandan.bi@intel.com>; 'Jian J Wang' >> <jian.j.wang@intel.com>; 'Hao A Wu' <hao.a.wu@intel.com>; 'Sami Mujawar' >> <sami.mujawar@arm.com>; 'Leif Lindholm' <leif@nuviainc.com> >> 主题: Re: 回复: [edk2-devel] [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use >> pool allocations when possible >> >> On 10/22/20 4:01 AM, gaoliming wrote: >>> Ard: >>> Can you help to collect memmap information on Shell before this >> change and after this change? I want to know what’s impact on the memory >> layout. >>> >> >> >> Before: >> =================== >> Type Start End # Pages >> Available 0000000040000000-00000000403B2FFF 00000000000003B3 >> LoaderCode 00000000403B3000-000000004047FFFF 00000000000000CD >> ACPI_Recl 0000000040480000-00000000404DFFFF 0000000000000060 >> Available 00000000404E0000-00000000404FFFFF 0000000000000020 >> ACPI_Recl 0000000040500000-000000004051FFFF 0000000000000020 >> RT_Code 0000000040520000-000000004059FFFF 0000000000000080 >> RT_Data 00000000405A0000-000000004068FFFF 00000000000000F0 >> RT_Code 0000000040690000-000000004074FFFF 00000000000000C0 >> Available 0000000040750000-000000004188FFFF 0000000000001140 >> BS_Data 0000000041890000-00000000418ADFFF 000000000000001E >> Available 00000000418AE000-00000000418CBFFF 000000000000001E >> BS_Data 00000000418CC000-000000004299AFFF 00000000000010CF >> Available 000000004299B000-000000004299BFFF 0000000000000001 >> BS_Data 000000004299C000-0000000043643FFF 0000000000000CA8 >> Available 0000000043644000-000000004391AFFF 00000000000002D7 >> BS_Code 000000004391B000-0000000043C1FFFF 0000000000000305 >> RT_Code 0000000043C20000-0000000043DAFFFF 0000000000000190 >> RT_Data 0000000043DB0000-0000000043FFFFFF 0000000000000250 >> Available 0000000044000000-000000004401EFFF 000000000000001F >> BS_Data 000000004401F000-000000004401FFFF 0000000000000001 >> Available 0000000044020000-00000000477F7FFF 00000000000037D8 >> BS_Data 00000000477F8000-0000000047819FFF 0000000000000022 >> BS_Code 000000004781A000-000000004784FFFF 0000000000000036 >> BS_Data 0000000047850000-0000000047EEBFFF 000000000000069C >> BS_Code 0000000047EEC000-0000000047EEFFFF 0000000000000004 >> BS_Data 0000000047EF0000-0000000047FF0FFF 0000000000000101 >> BS_Code 0000000047FF1000-0000000047FFAFFF 000000000000000A >> BS_Data 0000000047FFB000-0000000047FFFFFF 0000000000000005 >> MMIO 0000000004000000-0000000007FFFFFF 0000000000004000 >> MMIO 0000000009010000-0000000009010FFF 0000000000000001 >> >> Reserved : 0 Pages (0 Bytes) >> LoaderCode: 205 Pages (839,680 Bytes) >> LoaderData: 0 Pages (0 Bytes) >> BS_Code : 841 Pages (3,444,736 Bytes) >> BS_Data : 9,561 Pages (39,161,856 Bytes) >> RT_Code : 720 Pages (2,949,120 Bytes) >> RT_Data : 832 Pages (3,407,872 Bytes) >> ACPI_Recl : 128 Pages (524,288 Bytes) >> ACPI_NVS : 0 Pages (0 Bytes) >> MMIO : 16,385 Pages (67,112,960 Bytes) >> MMIO_Port : 0 Pages (0 Bytes) >> PalCode : 0 Pages (0 Bytes) >> Available : 20,481 Pages (83,890,176 Bytes) >> Persistent: 0 Pages (0 Bytes) >> -------------- >> Total Memory: 128 MB (134,217,728 Bytes) >> >> >> After: >> =================== >> Type Start End # Pages >> Available 0000000040000000-0000000043EE1FFF 0000000000003EE2 >> LoaderCode 0000000043EE2000-0000000043FBFFFF 00000000000000DE >> RT_Code 0000000043FC0000-0000000043FFFFFF 0000000000000040 >> BS_Data 0000000044000000-000000004401FFFF 0000000000000020 >> Available 0000000044020000-000000004402FFFF 0000000000000010 >> ACPI_Recl 0000000044030000-000000004403FFFF 0000000000000010 >> RT_Code 0000000044040000-000000004407FFFF 0000000000000040 >> RT_Data 0000000044080000-000000004413FFFF 00000000000000C0 >> RT_Code 0000000044140000-000000004418FFFF 0000000000000050 >> RT_Data 0000000044190000-000000004422FFFF 00000000000000A0 >> RT_Code 0000000044230000-000000004431FFFF 00000000000000F0 >> Available 0000000044320000-0000000045481FFF 0000000000001162 >> BS_Data 0000000045482000-000000004549FFFF 000000000000001E >> Available 00000000454A0000-00000000454BDFFF 000000000000001E >> BS_Data 00000000454BE000-00000000454CAFFF 000000000000000D >> Available 00000000454CB000-00000000454CFFFF 0000000000000005 >> BS_Data 00000000454D0000-0000000047217FFF 0000000000001D48 >> Available 0000000047218000-00000000472AAFFF 0000000000000093 >> BS_Code 00000000472AB000-00000000475FFFFF 0000000000000355 >> RT_Code 0000000047600000-000000004768FFFF 0000000000000090 >> Available 0000000047690000-000000004769FFFF 0000000000000010 >> RT_Data 00000000476A0000-00000000477BFFFF 0000000000000120 >> Available 00000000477C0000-00000000477DEFFF 000000000000001F >> BS_Data 00000000477DF000-0000000047801FFF 0000000000000023 >> BS_Code 0000000047802000-0000000047848FFF 0000000000000047 >> BS_Data 0000000047849000-0000000047EDCFFF 0000000000000694 >> BS_Code 0000000047EDD000-0000000047EE4FFF 0000000000000008 >> BS_Data 0000000047EE5000-0000000047FE5FFF 0000000000000101 >> BS_Code 0000000047FE6000-0000000047FFAFFF 0000000000000015 >> BS_Data 0000000047FFB000-0000000047FFFFFF 0000000000000005 >> MMIO 0000000004000000-0000000007FFFFFF 0000000000004000 >> MMIO 0000000009010000-0000000009010FFF 0000000000000001 >> >> Reserved : 0 Pages (0 Bytes) >> LoaderCode: 222 Pages (909,312 Bytes) >> LoaderData: 0 Pages (0 Bytes) >> BS_Code : 953 Pages (3,903,488 Bytes) >> BS_Data : 9,552 Pages (39,124,992 Bytes) >> RT_Code : 592 Pages (2,424,832 Bytes) >> RT_Data : 640 Pages (2,621,440 Bytes) >> ACPI_Recl : 16 Pages (65,536 Bytes) >> ACPI_NVS : 0 Pages (0 Bytes) >> MMIO : 16,385 Pages (67,112,960 Bytes) >> MMIO_Port : 0 Pages (0 Bytes) >> PalCode : 0 Pages (0 Bytes) >> Available : 20,793 Pages (85,168,128 Bytes) >> Persistent: 0 Pages (0 Bytes) >> -------------- >> Total Memory: 128 MB (134,217,728 Bytes) >> >> >>> Liming >>>> -----邮件原件----- >>>> 发件人: Laszlo Ersek <lersek@redhat.com> >>>> 发送时间: 2020年10月20日 3:48 >>>> 收件人: devel@edk2.groups.io; ard.biesheuvel@arm.com >>>> 抄送: Dandan Bi <dandan.bi@intel.com>; Liming Gao >>>> <gaoliming@byosoft.com.cn>; Jian J Wang <jian.j.wang@intel.com>; Hao >> A >>>> Wu <hao.a.wu@intel.com>; Sami Mujawar <sami.mujawar@arm.com>; >> Leif >>>> Lindholm <leif@nuviainc.com> >>>> 主题: Re: [edk2-devel] [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use >> pool >>>> allocations when possible >>>> >>>> On 10/16/20 17:49, Ard Biesheuvel wrote: >>>>> On AArch64 systems, page based allocations for memory types that are >>>>> relevant to the OS are rounded up to 64 KB multiples. This wastes >>>>> some space in the ACPI table memory allocator, since it uses page >>>>> based allocations in order to be able to place the ACPI tables low >>>>> in memory. >>>>> >>>>> Since the latter requirement does not exist on AArch64, switch to pool >>>>> allocations for all ACPI tables except the root tables if the active >>>>> allocation policy permits them to be anywhere in memory. The root >>>>> tables will be handled in a subsequent patch. >>>>> >>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> >>>>> --- >>>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h | >> 4 >>>> +- >>>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c >> | >>>> 4 +- >>>>> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | >> 75 >>>> ++++++++++++++------ >>>>> 3 files changed, 57 insertions(+), 26 deletions(-) >>>>> >>>>> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h >>>> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h >>>>> index 425a462fd92b..6e600e7acd24 100644 >>>>> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h >>>>> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h >>>>> @@ -64,9 +64,9 @@ typedef struct { >>>>> LIST_ENTRY Link; >>>>> EFI_ACPI_TABLE_VERSION Version; >>>>> EFI_ACPI_COMMON_HEADER *Table; >>>>> - EFI_PHYSICAL_ADDRESS PageAddress; >>>>> - UINTN NumberOfPages; >>>>> + UINTN TableSize; >>>>> UINTN Handle; >>>>> + BOOLEAN PoolAllocation; >>>>> } EFI_ACPI_TABLE_LIST; >>>>> >>>>> // >>>> >>>> (1) The preceding comment (which explains the fields) should be updated. >>>> >>>>> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c >>>> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c >>>>> index b1cba20c8ed7..14ced68e645f 100644 >>>>> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c >>>>> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiSdt.c >>>>> @@ -68,8 +68,8 @@ FindTableByBuffer ( >>>>> >>>>> while (CurrentLink != StartLink) { >>>>> CurrentTableList = EFI_ACPI_TABLE_LIST_FROM_LINK >> (CurrentLink); >>>>> - if (((UINTN)CurrentTableList->PageAddress <= (UINTN)Buffer) && >>>>> - ((UINTN)CurrentTableList->PageAddress + >>>> EFI_PAGES_TO_SIZE(CurrentTableList->NumberOfPages) > (UINTN)Buffer)) >> { >>>>> + if (((UINTN)CurrentTableList->Table <= (UINTN)Buffer) && >>>>> + ((UINTN)CurrentTableList->Table + >> CurrentTableList->TableSize > >>>> (UINTN)Buffer)) { >>>>> // >>>>> // Good! Found Table. >>>>> // >>>>> diff --git >>>> a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c >>>> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c >>>>> index ad7baf8205b4..b05e57e6584f 100644 >>>>> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c >>>>> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c >>>>> @@ -428,6 +428,26 @@ ReallocateAcpiTableBuffer ( >>>>> return EFI_SUCCESS; >>>>> } >>>>> >>>>> +/** >>>>> + Free the memory associated with provided the EFI_ACPI_TABLE_LIST >>>> instance >>>> >>>> (2) s/with provided the/with the provided/ >>>> >>>>> + >>>>> + @param TableEntry EFI_ACPI_TABLE_LIST >>>> instance pointer >>>>> + >>>>> +**/ >>>>> +STATIC >>>>> +VOID >>>>> +FreeTableMemory ( >>>>> + EFI_ACPI_TABLE_LIST *TableEntry >>>>> + ) >>>>> +{ >>>>> + if (TableEntry->PoolAllocation) { >>>>> + gBS->FreePool (TableEntry->Table); >>>>> + } else { >>>>> + gBS->FreePages >>>> ((EFI_PHYSICAL_ADDRESS)(UINTN)TableEntry->Table, >>>>> + EFI_SIZE_TO_PAGES (TableEntry->TableSize)); >>>>> + } >>>>> +} >>>>> + >>>>> /** >>>>> This function adds an ACPI table to the table list. It will detect >> FACS >>>> and >>>>> allocate the correct type of memory and properly align the table. >>>>> @@ -454,14 +474,15 @@ AddTableToList ( >>>>> OUT UINTN *Handle >>>>> ) >>>>> { >>>>> - EFI_STATUS Status; >>>>> - EFI_ACPI_TABLE_LIST *CurrentTableList; >>>>> - UINT32 CurrentTableSignature; >>>>> - UINT32 CurrentTableSize; >>>>> - UINT32 *CurrentRsdtEntry; >>>>> - VOID *CurrentXsdtEntry; >>>>> - UINT64 Buffer64; >>>>> - BOOLEAN AddToRsdt; >>>>> + EFI_STATUS Status; >>>>> + EFI_ACPI_TABLE_LIST *CurrentTableList; >>>>> + UINT32 CurrentTableSignature; >>>>> + UINT32 CurrentTableSize; >>>>> + UINT32 *CurrentRsdtEntry; >>>>> + VOID *CurrentXsdtEntry; >>>>> + EFI_PHYSICAL_ADDRESS AllocPhysAddress; >>>>> + UINT64 Buffer64; >>>>> + BOOLEAN AddToRsdt; >>>>> >>>>> // >>>>> // Check for invalid input parameters >>>>> @@ -496,8 +517,8 @@ AddTableToList ( >>>>> // There is no architectural reason these should be below 4GB, it is >>>> purely >>>>> // for convenience of implementation that we force memory below >>>> 4GB. >>>>> // >>>>> - CurrentTableList->PageAddress = 0xFFFFFFFF; >>>>> - CurrentTableList->NumberOfPages = EFI_SIZE_TO_PAGES >>>> (CurrentTableSize); >>>>> + AllocPhysAddress = 0xFFFFFFFF; >>>>> + CurrentTableList->TableSize = CurrentTableSize; >>>>> >>>>> // >>>>> // Allocation memory type depends on the type of the table >>>>> @@ -518,9 +539,22 @@ AddTableToList ( >>>>> Status = gBS->AllocatePages ( >>>>> AllocateMaxAddress, >>>>> EfiACPIMemoryNVS, >>>>> - CurrentTableList->NumberOfPages, >>>>> - &CurrentTableList->PageAddress >>>>> + EFI_SIZE_TO_PAGES >>>> (CurrentTableList->TableSize), >>>>> + &AllocPhysAddress >>>>> ); >>>>> + CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER >>>> *)(UINTN)AllocPhysAddress; >>>>> + } else if (mAcpiTableAllocType == AllocateAnyPages) { >>>>> + // >>>>> + // If there is no allocation limit, there is also no need to use page >>>>> + // based allocations for ACPI tables, which may be wasteful on >>>> platforms >>>>> + // such as AArch64 that allocate multiples of 64 KB >>>>> + // >>>>> + Status = gBS->AllocatePool ( >>>>> + EfiACPIReclaimMemory, >>>>> + CurrentTableList->TableSize, >>>>> + (VOID **)&CurrentTableList->Table >>>>> + ); >>>>> + CurrentTableList->PoolAllocation = TRUE; >>>>> } else { >>>>> // >>>>> // All other tables are ACPI reclaim memory, no alignment >>>> requirements. >>>>> @@ -528,9 +562,10 @@ AddTableToList ( >>>>> Status = gBS->AllocatePages ( >>>>> mAcpiTableAllocType, >>>>> EfiACPIReclaimMemory, >>>>> - CurrentTableList->NumberOfPages, >>>>> - &CurrentTableList->PageAddress >>>>> + EFI_SIZE_TO_PAGES >>>> (CurrentTableList->TableSize), >>>>> + &AllocPhysAddress >>>>> ); >>>>> + CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER >>>> *)(UINTN)AllocPhysAddress; >>>>> } >>>>> // >>>>> // Check return value from memory alloc. >>>>> @@ -539,10 +574,6 @@ AddTableToList ( >>>>> gBS->FreePool (CurrentTableList); >>>>> return EFI_OUT_OF_RESOURCES; >>>>> } >>>>> - // >>>>> - // Update the table pointer with the allocated memory start >>>>> - // >>>>> - CurrentTableList->Table = (EFI_ACPI_COMMON_HEADER *) (UINTN) >>>> CurrentTableList->PageAddress; >>>>> >>>>> // >>>>> // Initialize the table contents >>>> >>>> (3) Thus far, in the changes to AddTableToList(), two things bother me: >>>> >>>> (3a) Before the patch, we'd only convert the physical address to a >>>> pointer once we knew that the allocation succeeded. Post-patch, we now >>>> have two branches where we might convert bogus AllocPhysAddress >> values >>>> (such as even 0xFFFFFFFF) to (EFI_ACPI_COMMON_HEADER*). I think we >>>> shouldn't do this. (It's not just the dereferencing of bogus pointers >>>> that we should avoid, but also the evaluation thereof.) >>>> >>>> (3b) "CurrentTableList" is allocated with AllocatePool() not >>>> AllocateZeroPool(), so *not* setting the "PoolAllocation" field to FALSE >>>> on the two affected branches is wrong. >>>> >>>> The patch looks OK to me otherwise. >>>> >>>> Thanks >>>> Laszlo >>>> >>>>> @@ -575,7 +606,7 @@ AddTableToList ( >>>>> if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && >>>> AcpiTableInstance->Fadt1 != NULL) || >>>>> ((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && >>>> AcpiTableInstance->Fadt3 != NULL) >>>>> ) { >>>>> - gBS->FreePages (CurrentTableList->PageAddress, >>>> CurrentTableList->NumberOfPages); >>>>> + FreeTableMemory (CurrentTableList); >>>>> gBS->FreePool (CurrentTableList); >>>>> return EFI_ACCESS_DENIED; >>>>> } >>>>> @@ -729,7 +760,7 @@ AddTableToList ( >>>>> if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && >>>> AcpiTableInstance->Facs1 != NULL) || >>>>> ((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && >>>> AcpiTableInstance->Facs3 != NULL) >>>>> ) { >>>>> - gBS->FreePages (CurrentTableList->PageAddress, >>>> CurrentTableList->NumberOfPages); >>>>> + FreeTableMemory (CurrentTableList); >>>>> gBS->FreePool (CurrentTableList); >>>>> return EFI_ACCESS_DENIED; >>>>> } >>>>> @@ -813,7 +844,7 @@ AddTableToList ( >>>>> if (((Version & EFI_ACPI_TABLE_VERSION_1_0B) != 0 && >>>> AcpiTableInstance->Dsdt1 != NULL) || >>>>> ((Version & ACPI_TABLE_VERSION_GTE_2_0) != 0 && >>>> AcpiTableInstance->Dsdt3 != NULL) >>>>> ) { >>>>> - gBS->FreePages (CurrentTableList->PageAddress, >>>> CurrentTableList->NumberOfPages); >>>>> + FreeTableMemory (CurrentTableList); >>>>> gBS->FreePool (CurrentTableList); >>>>> return EFI_ACCESS_DENIED; >>>>> } >>>>> @@ -1449,7 +1480,7 @@ DeleteTable ( >>>>> // >>>>> // Free the Table >>>>> // >>>>> - gBS->FreePages (Table->PageAddress, Table->NumberOfPages); >>>>> + FreeTableMemory (Table); >>>>> RemoveEntryList (&(Table->Link)); >>>>> gBS->FreePool (Table); >>>>> } >>>>> >>> >>> >>> > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: 回复: 回复: [edk2-devel] [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use pool allocations when possible 2020-10-26 6:25 ` Laszlo Ersek @ 2020-10-26 7:42 ` Ard Biesheuvel 2020-10-27 8:45 ` 回复: " gaoliming 2020-10-27 11:07 ` Laszlo Ersek 0 siblings, 2 replies; 18+ messages in thread From: Ard Biesheuvel @ 2020-10-26 7:42 UTC (permalink / raw) To: Laszlo Ersek, gaoliming, devel Cc: 'Dandan Bi', 'Jian J Wang', 'Hao A Wu', 'Sami Mujawar', 'Leif Lindholm' On 10/26/20 7:25 AM, Laszlo Ersek wrote: > On 10/26/20 02:35, gaoliming wrote: >> Ard: >> I verify this patch on OvmfX64 and collect the memmap info. I don't see the difference in memmap. >> So, this enhancement is for AARCH64 only. Is it right? > > That's my understanding, yes. OVMF enables ACPI 1.0b support in the > bitmask PCD, and so the compat code for 32-bit allocations (= page > allocations for expressing the 4GB limit) remains active. > Indeed. Any platform that removes BIT1 from gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions will switch over to pool allocations, but OVMF retains support for ACPI 1.0b for some reason. ^ permalink raw reply [flat|nested] 18+ messages in thread
* 回复: 回复: 回复: [edk2-devel] [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use pool allocations when possible 2020-10-26 7:42 ` Ard Biesheuvel @ 2020-10-27 8:45 ` gaoliming 2020-10-27 9:05 ` Ard Biesheuvel 2020-10-27 11:07 ` Laszlo Ersek 1 sibling, 1 reply; 18+ messages in thread From: gaoliming @ 2020-10-27 8:45 UTC (permalink / raw) To: 'Ard Biesheuvel', 'Laszlo Ersek', devel Cc: 'Dandan Bi', 'Jian J Wang', 'Hao A Wu', 'Sami Mujawar', 'Leif Lindholm' Ard: After removes BIT1 from gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions for OVMFX64, the memory ACPI_Recl will be reduced with this patch set. I also review the code logic. I don't find other issue. I see Laszlo gives one comment on CurrentTableList. It is from AllocatePool(). If so, CurrentTableList->PoolAllocation value should be set to FALSE. Thanks Liming > -----邮件原件----- > 发件人: Ard Biesheuvel <ard.biesheuvel@arm.com> > 发送时间: 2020年10月26日 15:43 > 收件人: Laszlo Ersek <lersek@redhat.com>; gaoliming > <gaoliming@byosoft.com.cn>; devel@edk2.groups.io > 抄送: 'Dandan Bi' <dandan.bi@intel.com>; 'Jian J Wang' > <jian.j.wang@intel.com>; 'Hao A Wu' <hao.a.wu@intel.com>; 'Sami Mujawar' > <sami.mujawar@arm.com>; 'Leif Lindholm' <leif@nuviainc.com> > 主题: Re: 回复: 回复: [edk2-devel] [PATCH 1/3] > MdeModulePkg/AcpiTableDxe: use pool allocations when possible > > On 10/26/20 7:25 AM, Laszlo Ersek wrote: > > On 10/26/20 02:35, gaoliming wrote: > >> Ard: > >> I verify this patch on OvmfX64 and collect the memmap info. I don't > see the difference in memmap. > >> So, this enhancement is for AARCH64 only. Is it right? > > > > That's my understanding, yes. OVMF enables ACPI 1.0b support in the > > bitmask PCD, and so the compat code for 32-bit allocations (= page > > allocations for expressing the 4GB limit) remains active. > > > > Indeed. Any platform that removes BIT1 from > > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions > > will switch over to pool allocations, but OVMF retains support for ACPI > 1.0b for some reason. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: 回复: 回复: 回复: [edk2-devel] [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use pool allocations when possible 2020-10-27 8:45 ` 回复: " gaoliming @ 2020-10-27 9:05 ` Ard Biesheuvel 0 siblings, 0 replies; 18+ messages in thread From: Ard Biesheuvel @ 2020-10-27 9:05 UTC (permalink / raw) To: gaoliming, 'Laszlo Ersek', devel Cc: 'Dandan Bi', 'Jian J Wang', 'Hao A Wu', 'Sami Mujawar', 'Leif Lindholm' On 10/27/20 9:45 AM, gaoliming wrote: > Ard: > After removes BIT1 from gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions for OVMFX64, the memory ACPI_Recl will be reduced with this patch set. > > I also review the code logic. I don't find other issue. I see Laszlo gives one comment on CurrentTableList. It is from AllocatePool(). If so, CurrentTableList->PoolAllocation value should be set to FALSE. > Thank you Liming. I will respin the series with Laszlo's comments addressed. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: 回复: 回复: [edk2-devel] [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use pool allocations when possible 2020-10-26 7:42 ` Ard Biesheuvel 2020-10-27 8:45 ` 回复: " gaoliming @ 2020-10-27 11:07 ` Laszlo Ersek 1 sibling, 0 replies; 18+ messages in thread From: Laszlo Ersek @ 2020-10-27 11:07 UTC (permalink / raw) To: Ard Biesheuvel, gaoliming, devel Cc: 'Dandan Bi', 'Jian J Wang', 'Hao A Wu', 'Sami Mujawar', 'Leif Lindholm' On 10/26/20 08:42, Ard Biesheuvel wrote: > On 10/26/20 7:25 AM, Laszlo Ersek wrote: >> On 10/26/20 02:35, gaoliming wrote: >>> Ard: >>> I verify this patch on OvmfX64 and collect the memmap info. I >>> don't see the difference in memmap. >>> So, this enhancement is for AARCH64 only. Is it right? >> >> That's my understanding, yes. OVMF enables ACPI 1.0b support in the >> bitmask PCD, and so the compat code for 32-bit allocations (= page >> allocations for expressing the 4GB limit) remains active. >> > > Indeed. Any platform that removes BIT1 from > > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions > > will switch over to pool allocations, but OVMF retains support for ACPI > 1.0b for some reason. > I believe OVMF has always had that bit set, and clearing it would risk regressions. The ACPI (AML) generator in QEMU tends restricts itself to ACPI 1.0 constructs (opcodes) because at least the Windows 7 guest OS family cannot deal with ACPI 2.0, as far as I remember. Thanks, Laszlo ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/3] MdeModulePkg/AcpiTableDxe: use pool allocation for RSDT/XSDT if possible 2020-10-16 15:49 [PATCH 0/3] MdeModulePkg: use pool allocations for ACPI tables Ard Biesheuvel 2020-10-16 15:49 ` [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use pool allocations when possible Ard Biesheuvel @ 2020-10-16 15:49 ` Ard Biesheuvel 2020-10-19 20:06 ` [edk2-devel] " Laszlo Ersek 2020-10-16 15:49 ` [PATCH 3/3] MdeModulePkg/AcpiTableDxe: use pool allocation for RSDP " Ard Biesheuvel 2020-10-19 1:28 ` FW: [PATCH 0/3] MdeModulePkg: use pool allocations for ACPI tables Wu, Hao A 3 siblings, 1 reply; 18+ messages in thread From: Ard Biesheuvel @ 2020-10-16 15:49 UTC (permalink / raw) To: devel Cc: Ard Biesheuvel, Dandan Bi, Liming Gao, Jian J Wang, Hao A Wu, Sami Mujawar, Laszlo Ersek, Leif Lindholm If no memory allocation limit is in effect for ACPI tables, prefer pool allocations over page allocations, to avoid wasting memory on systems where page based allocations are rounded up to 64 KB, such as AArch64. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> --- MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 111 ++++++++++++-------- 1 file changed, 65 insertions(+), 46 deletions(-) diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c index b05e57e6584f..22f49a8489e7 100644 --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c @@ -355,28 +355,35 @@ ReallocateAcpiTableBuffer ( NewMaxTableNumber * sizeof (UINT32); } - // - // Allocate memory in the lower 32 bit of address range for - // compatibility with ACPI 1.0 OS. - // - // This is done because ACPI 1.0 pointers are 32 bit values. - // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses. - // There is no architectural reason these should be below 4GB, it is purely - // for convenience of implementation that we force memory below 4GB. - // - PageAddress = 0xFFFFFFFF; - Status = gBS->AllocatePages ( - mAcpiTableAllocType, - EfiACPIReclaimMemory, - EFI_SIZE_TO_PAGES (TotalSize), - &PageAddress - ); + if (mAcpiTableAllocType != AllocateAnyPages) { + // + // Allocate memory in the lower 32 bit of address range for + // compatibility with ACPI 1.0 OS. + // + // This is done because ACPI 1.0 pointers are 32 bit values. + // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses. + // There is no architectural reason these should be below 4GB, it is purely + // for convenience of implementation that we force memory below 4GB. + // + PageAddress = 0xFFFFFFFF; + Status = gBS->AllocatePages ( + mAcpiTableAllocType, + EfiACPIReclaimMemory, + EFI_SIZE_TO_PAGES (TotalSize), + &PageAddress + ); + Pointer = (UINT8 *)(UINTN)PageAddress; + } else { + Status = gBS->AllocatePool ( + EfiACPIReclaimMemory, + TotalSize, + (VOID **)&Pointer); + } if (EFI_ERROR (Status)) { return EFI_OUT_OF_RESOURCES; } - Pointer = (UINT8 *) (UINTN) PageAddress; ZeroMem (Pointer, TotalSize); AcpiTableInstance->Rsdt1 = (EFI_ACPI_DESCRIPTION_HEADER *) Pointer; @@ -406,21 +413,26 @@ ReallocateAcpiTableBuffer ( } CopyMem (AcpiTableInstance->Xsdt, TempPrivateData.Xsdt, (sizeof (EFI_ACPI_DESCRIPTION_HEADER) + mEfiAcpiMaxNumTables * sizeof (UINT64))); - // - // Calculate orignal ACPI table buffer size - // - TotalSize = sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 2.0/3.0 XSDT - mEfiAcpiMaxNumTables * sizeof (UINT64); + if (mAcpiTableAllocType != AllocateAnyPages) { + // + // Calculate orignal ACPI table buffer size + // + TotalSize = sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 2.0/3.0 XSDT + mEfiAcpiMaxNumTables * sizeof (UINT64); - if ((PcdGet32 (PcdAcpiExposedTableVersions) & EFI_ACPI_TABLE_VERSION_1_0B) != 0) { - TotalSize += sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 1.0 RSDT - mEfiAcpiMaxNumTables * sizeof (UINT32) + - sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 2.0/3.0 RSDT - mEfiAcpiMaxNumTables * sizeof (UINT32); + if ((PcdGet32 (PcdAcpiExposedTableVersions) & EFI_ACPI_TABLE_VERSION_1_0B) != 0) { + TotalSize += sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 1.0 RSDT + mEfiAcpiMaxNumTables * sizeof (UINT32) + + sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 2.0/3.0 RSDT + mEfiAcpiMaxNumTables * sizeof (UINT32); + } + + gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)TempPrivateData.Rsdt1, + EFI_SIZE_TO_PAGES (TotalSize)); + } else { + gBS->FreePool (TempPrivateData.Rsdt1); } - gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)TempPrivateData.Rsdt1, EFI_SIZE_TO_PAGES (TotalSize)); - // // Update the Max ACPI table number // @@ -1759,29 +1771,36 @@ AcpiTableAcpiTableConstructor ( mEfiAcpiMaxNumTables * sizeof (UINT32); } - // - // Allocate memory in the lower 32 bit of address range for - // compatibility with ACPI 1.0 OS. - // - // This is done because ACPI 1.0 pointers are 32 bit values. - // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses. - // There is no architectural reason these should be below 4GB, it is purely - // for convenience of implementation that we force memory below 4GB. - // - PageAddress = 0xFFFFFFFF; - Status = gBS->AllocatePages ( - mAcpiTableAllocType, - EfiACPIReclaimMemory, - EFI_SIZE_TO_PAGES (TotalSize), - &PageAddress - ); + if (mAcpiTableAllocType != AllocateAnyPages) { + // + // Allocate memory in the lower 32 bit of address range for + // compatibility with ACPI 1.0 OS. + // + // This is done because ACPI 1.0 pointers are 32 bit values. + // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses. + // There is no architectural reason these should be below 4GB, it is purely + // for convenience of implementation that we force memory below 4GB. + // + PageAddress = 0xFFFFFFFF; + Status = gBS->AllocatePages ( + mAcpiTableAllocType, + EfiACPIReclaimMemory, + EFI_SIZE_TO_PAGES (TotalSize), + &PageAddress + ); + Pointer = (UINT8 *)(UINTN)PageAddress; + } else { + Status = gBS->AllocatePool ( + EfiACPIReclaimMemory, + TotalSize, + (VOID **)&Pointer); + } if (EFI_ERROR (Status)) { gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)AcpiTableInstance->Rsdp1, EFI_SIZE_TO_PAGES (RsdpTableSize)); return EFI_OUT_OF_RESOURCES; } - Pointer = (UINT8 *) (UINTN) PageAddress; ZeroMem (Pointer, TotalSize); AcpiTableInstance->Rsdt1 = (EFI_ACPI_DESCRIPTION_HEADER *) Pointer; -- 2.17.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 2/3] MdeModulePkg/AcpiTableDxe: use pool allocation for RSDT/XSDT if possible 2020-10-16 15:49 ` [PATCH 2/3] MdeModulePkg/AcpiTableDxe: use pool allocation for RSDT/XSDT if possible Ard Biesheuvel @ 2020-10-19 20:06 ` Laszlo Ersek 2020-10-19 20:11 ` Laszlo Ersek 0 siblings, 1 reply; 18+ messages in thread From: Laszlo Ersek @ 2020-10-19 20:06 UTC (permalink / raw) To: devel, ard.biesheuvel Cc: Dandan Bi, Liming Gao, Jian J Wang, Hao A Wu, Sami Mujawar, Leif Lindholm On 10/16/20 17:49, Ard Biesheuvel wrote: > If no memory allocation limit is in effect for ACPI tables, prefer > pool allocations over page allocations, to avoid wasting memory on > systems where page based allocations are rounded up to 64 KB, such > as AArch64. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> > --- > MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 111 ++++++++++++-------- > 1 file changed, 65 insertions(+), 46 deletions(-) > > diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > index b05e57e6584f..22f49a8489e7 100644 > --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > @@ -355,28 +355,35 @@ ReallocateAcpiTableBuffer ( > NewMaxTableNumber * sizeof (UINT32); > } > > - // > - // Allocate memory in the lower 32 bit of address range for > - // compatibility with ACPI 1.0 OS. > - // > - // This is done because ACPI 1.0 pointers are 32 bit values. > - // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses. > - // There is no architectural reason these should be below 4GB, it is purely > - // for convenience of implementation that we force memory below 4GB. > - // > - PageAddress = 0xFFFFFFFF; > - Status = gBS->AllocatePages ( > - mAcpiTableAllocType, > - EfiACPIReclaimMemory, > - EFI_SIZE_TO_PAGES (TotalSize), > - &PageAddress > - ); > + if (mAcpiTableAllocType != AllocateAnyPages) { > + // > + // Allocate memory in the lower 32 bit of address range for > + // compatibility with ACPI 1.0 OS. > + // > + // This is done because ACPI 1.0 pointers are 32 bit values. > + // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses. > + // There is no architectural reason these should be below 4GB, it is purely > + // for convenience of implementation that we force memory below 4GB. Hmmm. I first thought that the last two lines of the commend should not be re-instated. But I was wrong. Because supporting ACPI 1.0b causes us to allocate even areas pointed-to by XSDT entries under 4GB. > + // > + PageAddress = 0xFFFFFFFF; > + Status = gBS->AllocatePages ( > + mAcpiTableAllocType, > + EfiACPIReclaimMemory, > + EFI_SIZE_TO_PAGES (TotalSize), > + &PageAddress > + ); > + Pointer = (UINT8 *)(UINTN)PageAddress; (1) Same comment as before; we shouldn't convert PageAddress to a pointer unless the allocation succeeds. > + } else { > + Status = gBS->AllocatePool ( > + EfiACPIReclaimMemory, > + TotalSize, > + (VOID **)&Pointer); > + } > > if (EFI_ERROR (Status)) { > return EFI_OUT_OF_RESOURCES; > } > > - Pointer = (UINT8 *) (UINTN) PageAddress; > ZeroMem (Pointer, TotalSize); > > AcpiTableInstance->Rsdt1 = (EFI_ACPI_DESCRIPTION_HEADER *) Pointer; > @@ -406,21 +413,26 @@ ReallocateAcpiTableBuffer ( > } > CopyMem (AcpiTableInstance->Xsdt, TempPrivateData.Xsdt, (sizeof (EFI_ACPI_DESCRIPTION_HEADER) + mEfiAcpiMaxNumTables * sizeof (UINT64))); > > - // > - // Calculate orignal ACPI table buffer size > - // > - TotalSize = sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 2.0/3.0 XSDT > - mEfiAcpiMaxNumTables * sizeof (UINT64); > + if (mAcpiTableAllocType != AllocateAnyPages) { > + // > + // Calculate orignal ACPI table buffer size > + // > + TotalSize = sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 2.0/3.0 XSDT > + mEfiAcpiMaxNumTables * sizeof (UINT64); > > - if ((PcdGet32 (PcdAcpiExposedTableVersions) & EFI_ACPI_TABLE_VERSION_1_0B) != 0) { > - TotalSize += sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 1.0 RSDT > - mEfiAcpiMaxNumTables * sizeof (UINT32) + > - sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 2.0/3.0 RSDT > - mEfiAcpiMaxNumTables * sizeof (UINT32); > + if ((PcdGet32 (PcdAcpiExposedTableVersions) & EFI_ACPI_TABLE_VERSION_1_0B) != 0) { > + TotalSize += sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 1.0 RSDT > + mEfiAcpiMaxNumTables * sizeof (UINT32) + > + sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 2.0/3.0 RSDT > + mEfiAcpiMaxNumTables * sizeof (UINT32); > + } > + > + gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)TempPrivateData.Rsdt1, > + EFI_SIZE_TO_PAGES (TotalSize)); > + } else { > + gBS->FreePool (TempPrivateData.Rsdt1); > } > > - gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)TempPrivateData.Rsdt1, EFI_SIZE_TO_PAGES (TotalSize)); > - > // > // Update the Max ACPI table number > // > @@ -1759,29 +1771,36 @@ AcpiTableAcpiTableConstructor ( > mEfiAcpiMaxNumTables * sizeof (UINT32); > } > > - // > - // Allocate memory in the lower 32 bit of address range for > - // compatibility with ACPI 1.0 OS. > - // > - // This is done because ACPI 1.0 pointers are 32 bit values. > - // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses. > - // There is no architectural reason these should be below 4GB, it is purely > - // for convenience of implementation that we force memory below 4GB. > - // > - PageAddress = 0xFFFFFFFF; > - Status = gBS->AllocatePages ( > - mAcpiTableAllocType, > - EfiACPIReclaimMemory, > - EFI_SIZE_TO_PAGES (TotalSize), > - &PageAddress > - ); > + if (mAcpiTableAllocType != AllocateAnyPages) { > + // > + // Allocate memory in the lower 32 bit of address range for > + // compatibility with ACPI 1.0 OS. > + // > + // This is done because ACPI 1.0 pointers are 32 bit values. > + // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses. > + // There is no architectural reason these should be below 4GB, it is purely > + // for convenience of implementation that we force memory below 4GB. > + // > + PageAddress = 0xFFFFFFFF; > + Status = gBS->AllocatePages ( > + mAcpiTableAllocType, > + EfiACPIReclaimMemory, > + EFI_SIZE_TO_PAGES (TotalSize), > + &PageAddress > + ); > + Pointer = (UINT8 *)(UINTN)PageAddress; (2) Same as (1). Looks reasonable otherwise. Thanks Laszlo > + } else { > + Status = gBS->AllocatePool ( > + EfiACPIReclaimMemory, > + TotalSize, > + (VOID **)&Pointer); > + } > > if (EFI_ERROR (Status)) { > gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)AcpiTableInstance->Rsdp1, EFI_SIZE_TO_PAGES (RsdpTableSize)); > return EFI_OUT_OF_RESOURCES; > } > > - Pointer = (UINT8 *) (UINTN) PageAddress; > ZeroMem (Pointer, TotalSize); > > AcpiTableInstance->Rsdt1 = (EFI_ACPI_DESCRIPTION_HEADER *) Pointer; > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 2/3] MdeModulePkg/AcpiTableDxe: use pool allocation for RSDT/XSDT if possible 2020-10-19 20:06 ` [edk2-devel] " Laszlo Ersek @ 2020-10-19 20:11 ` Laszlo Ersek 0 siblings, 0 replies; 18+ messages in thread From: Laszlo Ersek @ 2020-10-19 20:11 UTC (permalink / raw) To: devel, ard.biesheuvel Cc: Dandan Bi, Liming Gao, Jian J Wang, Hao A Wu, Sami Mujawar, Leif Lindholm On 10/19/20 22:06, Laszlo Ersek wrote: > On 10/16/20 17:49, Ard Biesheuvel wrote: >> If no memory allocation limit is in effect for ACPI tables, prefer >> pool allocations over page allocations, to avoid wasting memory on >> systems where page based allocations are rounded up to 64 KB, such >> as AArch64. >> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> >> --- >> MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 111 ++++++++++++-------- >> 1 file changed, 65 insertions(+), 46 deletions(-) >> >> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c >> index b05e57e6584f..22f49a8489e7 100644 >> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c >> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c >> @@ -355,28 +355,35 @@ ReallocateAcpiTableBuffer ( >> NewMaxTableNumber * sizeof (UINT32); >> } >> >> - // >> - // Allocate memory in the lower 32 bit of address range for >> - // compatibility with ACPI 1.0 OS. >> - // >> - // This is done because ACPI 1.0 pointers are 32 bit values. >> - // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses. >> - // There is no architectural reason these should be below 4GB, it is purely >> - // for convenience of implementation that we force memory below 4GB. >> - // >> - PageAddress = 0xFFFFFFFF; >> - Status = gBS->AllocatePages ( >> - mAcpiTableAllocType, >> - EfiACPIReclaimMemory, >> - EFI_SIZE_TO_PAGES (TotalSize), >> - &PageAddress >> - ); >> + if (mAcpiTableAllocType != AllocateAnyPages) { >> + // >> + // Allocate memory in the lower 32 bit of address range for >> + // compatibility with ACPI 1.0 OS. >> + // >> + // This is done because ACPI 1.0 pointers are 32 bit values. >> + // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses. >> + // There is no architectural reason these should be below 4GB, it is purely >> + // for convenience of implementation that we force memory below 4GB. > > Hmmm. I first thought that the last two lines of the commend should not > be re-instated. But I was wrong. Because supporting ACPI 1.0b causes us > to allocate even areas pointed-to by XSDT entries under 4GB. > >> + // >> + PageAddress = 0xFFFFFFFF; >> + Status = gBS->AllocatePages ( >> + mAcpiTableAllocType, >> + EfiACPIReclaimMemory, >> + EFI_SIZE_TO_PAGES (TotalSize), >> + &PageAddress >> + ); >> + Pointer = (UINT8 *)(UINTN)PageAddress; > > (1) Same comment as before; we shouldn't convert PageAddress to a > pointer unless the allocation succeeds. > >> + } else { >> + Status = gBS->AllocatePool ( >> + EfiACPIReclaimMemory, >> + TotalSize, >> + (VOID **)&Pointer); >> + } >> >> if (EFI_ERROR (Status)) { >> return EFI_OUT_OF_RESOURCES; >> } >> >> - Pointer = (UINT8 *) (UINTN) PageAddress; >> ZeroMem (Pointer, TotalSize); >> >> AcpiTableInstance->Rsdt1 = (EFI_ACPI_DESCRIPTION_HEADER *) Pointer; >> @@ -406,21 +413,26 @@ ReallocateAcpiTableBuffer ( >> } >> CopyMem (AcpiTableInstance->Xsdt, TempPrivateData.Xsdt, (sizeof (EFI_ACPI_DESCRIPTION_HEADER) + mEfiAcpiMaxNumTables * sizeof (UINT64))); >> >> - // >> - // Calculate orignal ACPI table buffer size >> - // >> - TotalSize = sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 2.0/3.0 XSDT >> - mEfiAcpiMaxNumTables * sizeof (UINT64); >> + if (mAcpiTableAllocType != AllocateAnyPages) { >> + // >> + // Calculate orignal ACPI table buffer size >> + // >> + TotalSize = sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 2.0/3.0 XSDT >> + mEfiAcpiMaxNumTables * sizeof (UINT64); >> >> - if ((PcdGet32 (PcdAcpiExposedTableVersions) & EFI_ACPI_TABLE_VERSION_1_0B) != 0) { >> - TotalSize += sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 1.0 RSDT >> - mEfiAcpiMaxNumTables * sizeof (UINT32) + >> - sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 2.0/3.0 RSDT >> - mEfiAcpiMaxNumTables * sizeof (UINT32); >> + if ((PcdGet32 (PcdAcpiExposedTableVersions) & EFI_ACPI_TABLE_VERSION_1_0B) != 0) { >> + TotalSize += sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 1.0 RSDT >> + mEfiAcpiMaxNumTables * sizeof (UINT32) + >> + sizeof (EFI_ACPI_DESCRIPTION_HEADER) + // for ACPI 2.0/3.0 RSDT >> + mEfiAcpiMaxNumTables * sizeof (UINT32); >> + } >> + >> + gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)TempPrivateData.Rsdt1, >> + EFI_SIZE_TO_PAGES (TotalSize)); >> + } else { >> + gBS->FreePool (TempPrivateData.Rsdt1); >> } >> >> - gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)TempPrivateData.Rsdt1, EFI_SIZE_TO_PAGES (TotalSize)); >> - >> // >> // Update the Max ACPI table number >> // >> @@ -1759,29 +1771,36 @@ AcpiTableAcpiTableConstructor ( >> mEfiAcpiMaxNumTables * sizeof (UINT32); >> } >> >> - // >> - // Allocate memory in the lower 32 bit of address range for >> - // compatibility with ACPI 1.0 OS. >> - // >> - // This is done because ACPI 1.0 pointers are 32 bit values. >> - // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses. >> - // There is no architectural reason these should be below 4GB, it is purely >> - // for convenience of implementation that we force memory below 4GB. >> - // >> - PageAddress = 0xFFFFFFFF; >> - Status = gBS->AllocatePages ( >> - mAcpiTableAllocType, >> - EfiACPIReclaimMemory, >> - EFI_SIZE_TO_PAGES (TotalSize), >> - &PageAddress >> - ); >> + if (mAcpiTableAllocType != AllocateAnyPages) { >> + // >> + // Allocate memory in the lower 32 bit of address range for >> + // compatibility with ACPI 1.0 OS. >> + // >> + // This is done because ACPI 1.0 pointers are 32 bit values. >> + // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses. >> + // There is no architectural reason these should be below 4GB, it is purely >> + // for convenience of implementation that we force memory below 4GB. >> + // >> + PageAddress = 0xFFFFFFFF; >> + Status = gBS->AllocatePages ( >> + mAcpiTableAllocType, >> + EfiACPIReclaimMemory, >> + EFI_SIZE_TO_PAGES (TotalSize), >> + &PageAddress >> + ); >> + Pointer = (UINT8 *)(UINTN)PageAddress; > > (2) Same as (1). > > Looks reasonable otherwise. (3) Style: for both gBS->AllocatePool() calls added in this patch, please either break the closing paren to a new line, or use the "condensed" style (which you do use elsewhere in these patches). Thanks! Laszlo > >> + } else { >> + Status = gBS->AllocatePool ( >> + EfiACPIReclaimMemory, >> + TotalSize, >> + (VOID **)&Pointer); >> + } >> >> if (EFI_ERROR (Status)) { >> gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)AcpiTableInstance->Rsdp1, EFI_SIZE_TO_PAGES (RsdpTableSize)); >> return EFI_OUT_OF_RESOURCES; >> } >> >> - Pointer = (UINT8 *) (UINTN) PageAddress; >> ZeroMem (Pointer, TotalSize); >> >> AcpiTableInstance->Rsdt1 = (EFI_ACPI_DESCRIPTION_HEADER *) Pointer; >> > ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/3] MdeModulePkg/AcpiTableDxe: use pool allocation for RSDP if possible 2020-10-16 15:49 [PATCH 0/3] MdeModulePkg: use pool allocations for ACPI tables Ard Biesheuvel 2020-10-16 15:49 ` [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use pool allocations when possible Ard Biesheuvel 2020-10-16 15:49 ` [PATCH 2/3] MdeModulePkg/AcpiTableDxe: use pool allocation for RSDT/XSDT if possible Ard Biesheuvel @ 2020-10-16 15:49 ` Ard Biesheuvel 2020-10-19 20:13 ` [edk2-devel] " Laszlo Ersek 2020-10-19 1:28 ` FW: [PATCH 0/3] MdeModulePkg: use pool allocations for ACPI tables Wu, Hao A 3 siblings, 1 reply; 18+ messages in thread From: Ard Biesheuvel @ 2020-10-16 15:49 UTC (permalink / raw) To: devel Cc: Ard Biesheuvel, Dandan Bi, Liming Gao, Jian J Wang, Hao A Wu, Sami Mujawar, Laszlo Ersek, Leif Lindholm Use a pool allocation for the RSDP ACPI root pointer structure if no memory limit is in effect that forces us to use page based allocation, which may be wasteful if they get rounded up to 64 KB as is the case on AArch64. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> --- MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 30 ++++++++++++++------ 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c index 22f49a8489e7..fb939aa00f49 100644 --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c @@ -1737,19 +1737,26 @@ AcpiTableAcpiTableConstructor ( RsdpTableSize += sizeof (EFI_ACPI_1_0_ROOT_SYSTEM_DESCRIPTION_POINTER); } - PageAddress = 0xFFFFFFFF; - Status = gBS->AllocatePages ( - mAcpiTableAllocType, - EfiACPIReclaimMemory, - EFI_SIZE_TO_PAGES (RsdpTableSize), - &PageAddress - ); + if (mAcpiTableAllocType != AllocateAnyPages) { + PageAddress = 0xFFFFFFFF; + Status = gBS->AllocatePages ( + mAcpiTableAllocType, + EfiACPIReclaimMemory, + EFI_SIZE_TO_PAGES (RsdpTableSize), + &PageAddress + ); + Pointer = (UINT8 *)(UINTN)PageAddress; + } else { + Status = gBS->AllocatePool ( + EfiACPIReclaimMemory, + RsdpTableSize, + (VOID **)&Pointer); + } if (EFI_ERROR (Status)) { return EFI_OUT_OF_RESOURCES; } - Pointer = (UINT8 *) (UINTN) PageAddress; ZeroMem (Pointer, RsdpTableSize); AcpiTableInstance->Rsdp1 = (EFI_ACPI_1_0_ROOT_SYSTEM_DESCRIPTION_POINTER *) Pointer; @@ -1797,7 +1804,12 @@ AcpiTableAcpiTableConstructor ( } if (EFI_ERROR (Status)) { - gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)AcpiTableInstance->Rsdp1, EFI_SIZE_TO_PAGES (RsdpTableSize)); + if (mAcpiTableAllocType != AllocateAnyPages) { + gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)AcpiTableInstance->Rsdp1, + EFI_SIZE_TO_PAGES (RsdpTableSize)); + } else { + gBS->FreePool (AcpiTableInstance->Rsdp1); + } return EFI_OUT_OF_RESOURCES; } -- 2.17.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [edk2-devel] [PATCH 3/3] MdeModulePkg/AcpiTableDxe: use pool allocation for RSDP if possible 2020-10-16 15:49 ` [PATCH 3/3] MdeModulePkg/AcpiTableDxe: use pool allocation for RSDP " Ard Biesheuvel @ 2020-10-19 20:13 ` Laszlo Ersek 0 siblings, 0 replies; 18+ messages in thread From: Laszlo Ersek @ 2020-10-19 20:13 UTC (permalink / raw) To: devel, ard.biesheuvel Cc: Dandan Bi, Liming Gao, Jian J Wang, Hao A Wu, Sami Mujawar, Leif Lindholm On 10/16/20 17:49, Ard Biesheuvel wrote: > Use a pool allocation for the RSDP ACPI root pointer structure if no > memory limit is in effect that forces us to use page based allocation, > which may be wasteful if they get rounded up to 64 KB as is the case > on AArch64. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> > --- > MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 30 ++++++++++++++------ > 1 file changed, 21 insertions(+), 9 deletions(-) > > diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > index 22f49a8489e7..fb939aa00f49 100644 > --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c > @@ -1737,19 +1737,26 @@ AcpiTableAcpiTableConstructor ( > RsdpTableSize += sizeof (EFI_ACPI_1_0_ROOT_SYSTEM_DESCRIPTION_POINTER); > } > > - PageAddress = 0xFFFFFFFF; > - Status = gBS->AllocatePages ( > - mAcpiTableAllocType, > - EfiACPIReclaimMemory, > - EFI_SIZE_TO_PAGES (RsdpTableSize), > - &PageAddress > - ); > + if (mAcpiTableAllocType != AllocateAnyPages) { > + PageAddress = 0xFFFFFFFF; > + Status = gBS->AllocatePages ( > + mAcpiTableAllocType, > + EfiACPIReclaimMemory, > + EFI_SIZE_TO_PAGES (RsdpTableSize), > + &PageAddress > + ); > + Pointer = (UINT8 *)(UINTN)PageAddress; (1) (same as earlier) please check for success before assigning the pointer > + } else { > + Status = gBS->AllocatePool ( > + EfiACPIReclaimMemory, > + RsdpTableSize, > + (VOID **)&Pointer); (2) (same as earlier) style: please break off the closing paren, or condense all the arguments. Looks OK other than these. Thanks Laszlo > + } > > if (EFI_ERROR (Status)) { > return EFI_OUT_OF_RESOURCES; > } > > - Pointer = (UINT8 *) (UINTN) PageAddress; > ZeroMem (Pointer, RsdpTableSize); > > AcpiTableInstance->Rsdp1 = (EFI_ACPI_1_0_ROOT_SYSTEM_DESCRIPTION_POINTER *) Pointer; > @@ -1797,7 +1804,12 @@ AcpiTableAcpiTableConstructor ( > } > > if (EFI_ERROR (Status)) { > - gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)AcpiTableInstance->Rsdp1, EFI_SIZE_TO_PAGES (RsdpTableSize)); > + if (mAcpiTableAllocType != AllocateAnyPages) { > + gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)AcpiTableInstance->Rsdp1, > + EFI_SIZE_TO_PAGES (RsdpTableSize)); > + } else { > + gBS->FreePool (AcpiTableInstance->Rsdp1); > + } > return EFI_OUT_OF_RESOURCES; > } > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* FW: [PATCH 0/3] MdeModulePkg: use pool allocations for ACPI tables 2020-10-16 15:49 [PATCH 0/3] MdeModulePkg: use pool allocations for ACPI tables Ard Biesheuvel ` (2 preceding siblings ...) 2020-10-16 15:49 ` [PATCH 3/3] MdeModulePkg/AcpiTableDxe: use pool allocation for RSDP " Ard Biesheuvel @ 2020-10-19 1:28 ` Wu, Hao A 2020-10-19 1:59 ` Wu, Hao A 3 siblings, 1 reply; 18+ messages in thread From: Wu, Hao A @ 2020-10-19 1:28 UTC (permalink / raw) To: Liming Gao (Byosoft address); +Cc: devel@edk2.groups.io, ard.biesheuvel@arm.com Forward to Liming's latest mail address. Best Regards, Hao Wu -----Original Message----- From: Ard Biesheuvel <ard.biesheuvel@arm.com> Sent: Friday, October 16, 2020 11:49 PM To: devel@edk2.groups.io Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Bi, Dandan <dandan.bi@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Sami Mujawar <sami.mujawar@arm.com>; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com> Subject: [PATCH 0/3] MdeModulePkg: use pool allocations for ACPI tables Currently, the AcpiTableDxe memory allocator uses page based allocations, for which the only reason seems to be that it permits the use of a memory limit, which is necessary for ACPI 1.0 tables that need to reside in the first 4 GB of memory. That requirement does not exist on AArch64, and since page based allocations are rounded up to 64 KB multiples, this wastes some memory in a way that can easily be avoided. So let's use the existing 'mAcpiTableAllocType' policy variable, and switch to pool allocations if it is set to 'AllocateAnyPages' Example output from Linux booting on ArmVirtQemu: Before: ACPI: RSDP 0x0000000078510000 000024 (v02 BOCHS ) ACPI: XSDT 0x0000000078500000 00004C (v01 BOCHS BXPCFACP 00000001 01000013) ACPI: FACP 0x00000000784C0000 00010C (v05 BOCHS BXPCFACP 00000001 BXPC 00000001) ACPI: DSDT 0x00000000784D0000 0014BB (v02 BOCHS BXPCDSDT 00000001 BXPC 00000001) ACPI: APIC 0x00000000784B0000 0000A8 (v03 BOCHS BXPCAPIC 00000001 BXPC 00000001) ACPI: GTDT 0x00000000784A0000 000060 (v02 BOCHS BXPCGTDT 00000001 BXPC 00000001) ACPI: MCFG 0x0000000078490000 00003C (v01 BOCHS BXPCMCFG 00000001 BXPC 00000001) ACPI: SPCR 0x0000000078480000 000050 (v02 BOCHS BXPCSPCR 00000001 BXPC 00000001) After: ACPI: RSDP 0x000000007C030018 000024 (v02 BOCHS ) ACPI: XSDT 0x000000007C03FE98 00004C (v01 BOCHS BXPCFACP 00000001 01000013) ACPI: FACP 0x000000007C03FA98 00010C (v05 BOCHS BXPCFACP 00000001 BXPC 00000001) ACPI: DSDT 0x000000007C037518 0014BB (v02 BOCHS BXPCDSDT 00000001 BXPC 00000001) ACPI: APIC 0x000000007C03FC18 0000A8 (v03 BOCHS BXPCAPIC 00000001 BXPC 00000001) ACPI: GTDT 0x000000007C03FD18 000060 (v02 BOCHS BXPCGTDT 00000001 BXPC 00000001) ACPI: MCFG 0x000000007C03FE18 00003C (v01 BOCHS BXPCMCFG 00000001 BXPC 00000001) ACPI: SPCR 0x000000007C03FF98 000050 (v02 BOCHS BXPCSPCR 00000001 BXPC 00000001) Cc: Dandan Bi <dandan.bi@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Hao A Wu <hao.a.wu@intel.com> Cc: Sami Mujawar <sami.mujawar@arm.com> Cc: Laszlo Ersek <lersek@redhat.com> Cc: Leif Lindholm <leif@nuviainc.com> Ard Biesheuvel (3): MdeModulePkg/AcpiTableDxe: use pool allocations when possible MdeModulePkg/AcpiTableDxe: use pool allocation for RSDT/XSDT if possible MdeModulePkg/AcpiTableDxe: use pool allocation for RSDP if possible .../Universal/Acpi/AcpiTableDxe/AcpiTable.h | 4 +- .../Universal/Acpi/AcpiTableDxe/AcpiSdt.c | 4 +- .../Acpi/AcpiTableDxe/AcpiTableProtocol.c | 216 +++++++++++------- 3 files changed, 143 insertions(+), 81 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] MdeModulePkg: use pool allocations for ACPI tables 2020-10-19 1:28 ` FW: [PATCH 0/3] MdeModulePkg: use pool allocations for ACPI tables Wu, Hao A @ 2020-10-19 1:59 ` Wu, Hao A 0 siblings, 0 replies; 18+ messages in thread From: Wu, Hao A @ 2020-10-19 1:59 UTC (permalink / raw) To: devel@edk2.groups.io, Wu, Hao A, Liming Gao (Byosoft address) Cc: ard.biesheuvel@arm.com Just realized that the right mail address was used for Liming. Please ignore the previous mail, really sorry for the noise. Best Regards, Hao Wu > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao A > Sent: Monday, October 19, 2020 9:28 AM > To: Liming Gao (Byosoft address) <gaoliming@byosoft.com.cn> > Cc: devel@edk2.groups.io; ard.biesheuvel@arm.com > Subject: [edk2-devel] FW: [PATCH 0/3] MdeModulePkg: use pool allocations for > ACPI tables > > Forward to Liming's latest mail address. > > Best Regards, > Hao Wu > > -----Original Message----- > From: Ard Biesheuvel <ard.biesheuvel@arm.com> > Sent: Friday, October 16, 2020 11:49 PM > To: devel@edk2.groups.io > Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>; Bi, Dandan > <dandan.bi@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Wang, Jian > J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Sami Mujawar > <sami.mujawar@arm.com>; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm > <leif@nuviainc.com> > Subject: [PATCH 0/3] MdeModulePkg: use pool allocations for ACPI tables > > Currently, the AcpiTableDxe memory allocator uses page based allocations, for > which the only reason seems to be that it permits the use of a memory limit, > which is necessary for ACPI 1.0 tables that need to reside in the first 4 GB of > memory. > > That requirement does not exist on AArch64, and since page based allocations > are rounded up to 64 KB multiples, this wastes some memory in a way that can > easily be avoided. So let's use the existing 'mAcpiTableAllocType' > policy variable, and switch to pool allocations if it is set to 'AllocateAnyPages' > > Example output from Linux booting on ArmVirtQemu: > > Before: > ACPI: RSDP 0x0000000078510000 000024 (v02 BOCHS ) > ACPI: XSDT 0x0000000078500000 00004C (v01 BOCHS BXPCFACP 00000001 > 01000013) > ACPI: FACP 0x00000000784C0000 00010C (v05 BOCHS BXPCFACP 00000001 > BXPC 00000001) > ACPI: DSDT 0x00000000784D0000 0014BB (v02 BOCHS BXPCDSDT 00000001 > BXPC 00000001) > ACPI: APIC 0x00000000784B0000 0000A8 (v03 BOCHS BXPCAPIC 00000001 > BXPC 00000001) > ACPI: GTDT 0x00000000784A0000 000060 (v02 BOCHS BXPCGTDT 00000001 > BXPC 00000001) > ACPI: MCFG 0x0000000078490000 00003C (v01 BOCHS BXPCMCFG 00000001 > BXPC 00000001) > ACPI: SPCR 0x0000000078480000 000050 (v02 BOCHS BXPCSPCR 00000001 > BXPC 00000001) > > After: > ACPI: RSDP 0x000000007C030018 000024 (v02 BOCHS ) > ACPI: XSDT 0x000000007C03FE98 00004C (v01 BOCHS BXPCFACP 00000001 > 01000013) > ACPI: FACP 0x000000007C03FA98 00010C (v05 BOCHS BXPCFACP 00000001 > BXPC 00000001) > ACPI: DSDT 0x000000007C037518 0014BB (v02 BOCHS BXPCDSDT 00000001 > BXPC 00000001) > ACPI: APIC 0x000000007C03FC18 0000A8 (v03 BOCHS BXPCAPIC 00000001 > BXPC 00000001) > ACPI: GTDT 0x000000007C03FD18 000060 (v02 BOCHS BXPCGTDT 00000001 > BXPC 00000001) > ACPI: MCFG 0x000000007C03FE18 00003C (v01 BOCHS BXPCMCFG 00000001 > BXPC 00000001) > ACPI: SPCR 0x000000007C03FF98 000050 (v02 BOCHS BXPCSPCR 00000001 > BXPC 00000001) > > Cc: Dandan Bi <dandan.bi@intel.com> > Cc: Liming Gao <gaoliming@byosoft.com.cn> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Hao A Wu <hao.a.wu@intel.com> > Cc: Sami Mujawar <sami.mujawar@arm.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Cc: Leif Lindholm <leif@nuviainc.com> > > Ard Biesheuvel (3): > MdeModulePkg/AcpiTableDxe: use pool allocations when possible > MdeModulePkg/AcpiTableDxe: use pool allocation for RSDT/XSDT if > possible > MdeModulePkg/AcpiTableDxe: use pool allocation for RSDP if possible > > .../Universal/Acpi/AcpiTableDxe/AcpiTable.h | 4 +- > .../Universal/Acpi/AcpiTableDxe/AcpiSdt.c | 4 +- > .../Acpi/AcpiTableDxe/AcpiTableProtocol.c | 216 +++++++++++------- > 3 files changed, 143 insertions(+), 81 deletions(-) > > -- > 2.17.1 > > > > > ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2020-10-27 11:07 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-10-16 15:49 [PATCH 0/3] MdeModulePkg: use pool allocations for ACPI tables Ard Biesheuvel 2020-10-16 15:49 ` [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use pool allocations when possible Ard Biesheuvel 2020-10-19 19:47 ` [edk2-devel] " Laszlo Ersek 2020-10-22 2:01 ` 回复: " gaoliming 2020-10-22 12:55 ` Ard Biesheuvel 2020-10-26 1:35 ` 回复: " gaoliming 2020-10-26 6:25 ` Laszlo Ersek 2020-10-26 7:42 ` Ard Biesheuvel 2020-10-27 8:45 ` 回复: " gaoliming 2020-10-27 9:05 ` Ard Biesheuvel 2020-10-27 11:07 ` Laszlo Ersek 2020-10-16 15:49 ` [PATCH 2/3] MdeModulePkg/AcpiTableDxe: use pool allocation for RSDT/XSDT if possible Ard Biesheuvel 2020-10-19 20:06 ` [edk2-devel] " Laszlo Ersek 2020-10-19 20:11 ` Laszlo Ersek 2020-10-16 15:49 ` [PATCH 3/3] MdeModulePkg/AcpiTableDxe: use pool allocation for RSDP " Ard Biesheuvel 2020-10-19 20:13 ` [edk2-devel] " Laszlo Ersek 2020-10-19 1:28 ` FW: [PATCH 0/3] MdeModulePkg: use pool allocations for ACPI tables Wu, Hao A 2020-10-19 1:59 ` Wu, Hao A
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox