回复: [edk2-devel] [PATCH 1/3] MdeModulePkg/AcpiTableDxe: use pool allocations when possible

["gaoliming" <gaoliming@byosoft.com.cn>]

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);
> >    }
> >