* [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
* [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
* [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
* 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
* 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
* 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
* 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
* 回复: [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
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