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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66324): https://edk2.groups.io/g/devel/message/66324
Mute This Topic: https://groups.io/mt/77553423/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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);
> }
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66400): https://edk2.groups.io/g/devel/message/66400
Mute This Topic: https://groups.io/mt/77553423/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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);
> > }
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66522): https://edk2.groups.io/g/devel/message/66522
Mute This Topic: https://groups.io/mt/77722442/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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);
>>> }
>>>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66535): https://edk2.groups.io/g/devel/message/66535
Mute This Topic: https://groups.io/mt/77722442/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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);
> >>> }
> >>>
> >
> >
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66579): https://edk2.groups.io/g/devel/message/66579
Mute This Topic: https://groups.io/mt/77805955/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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);
>>>>> }
>>>>>
>>>
>>>
>>>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#66585): https://edk2.groups.io/g/devel/message/66585
Mute This Topic: https://groups.io/mt/77805955/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#66592): https://edk2.groups.io/g/devel/message/66592 Mute This Topic: https://groups.io/mt/77805955/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#66648): https://edk2.groups.io/g/devel/message/66648 Mute This Topic: https://groups.io/mt/77805955/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#66643): https://edk2.groups.io/g/devel/message/66643 Mute This Topic: https://groups.io/mt/77834796/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#66644): https://edk2.groups.io/g/devel/message/66644 Mute This Topic: https://groups.io/mt/77834796/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.