From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, ard.biesheuvel@arm.com
Cc: 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>
Subject: Re: [edk2-devel] [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use pool allocations when possible
Date: Mon, 19 Oct 2020 21:47:51 +0200 [thread overview]
Message-ID: <f2c208c0-7680-c789-3ccd-d298b3963d3a@redhat.com> (raw)
In-Reply-To: <20201016154923.21260-2-ard.biesheuvel@arm.com>
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);
> }
>
next prev parent reply other threads:[~2020-10-19 19:48 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
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 ` Laszlo Ersek [this message]
2020-10-22 2:01 ` 回复: [edk2-devel] " 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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f2c208c0-7680-c789-3ccd-d298b3963d3a@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox