Re: [edk2-devel] [PATCH 2/3] MdeModulePkg/AcpiTableDxe: use pool allocation for RSDT/XSDT if possible

["Laszlo Ersek" <lersek@redhat.com>]

On 10/19/20 22:06, Laszlo Ersek wrote:
> On 10/16/20 17:49, Ard Biesheuvel wrote:
>> If no memory allocation limit is in effect for ACPI tables, prefer
>> pool allocations over page allocations, to avoid wasting memory on
>> systems where page based allocations are rounded up to 64 KB, such
>> as AArch64.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> ---
>>  MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 111 ++++++++++++--------
>>  1 file changed, 65 insertions(+), 46 deletions(-)
>>
>> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>> index b05e57e6584f..22f49a8489e7 100644
>> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
>> @@ -355,28 +355,35 @@ ReallocateAcpiTableBuffer (
>>                   NewMaxTableNumber * sizeof (UINT32);
>>    }
>>  
>> -  //
>> -  // Allocate memory in the lower 32 bit of address range for
>> -  // compatibility with ACPI 1.0 OS.
>> -  //
>> -  // This is done because ACPI 1.0 pointers are 32 bit values.
>> -  // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses.
>> -  // There is no architectural reason these should be below 4GB, it is purely
>> -  // for convenience of implementation that we force memory below 4GB.
>> -  //
>> -  PageAddress = 0xFFFFFFFF;
>> -  Status = gBS->AllocatePages (
>> -                  mAcpiTableAllocType,
>> -                  EfiACPIReclaimMemory,
>> -                  EFI_SIZE_TO_PAGES (TotalSize),
>> -                  &PageAddress
>> -                  );
>> +  if (mAcpiTableAllocType != AllocateAnyPages) {
>> +    //
>> +    // Allocate memory in the lower 32 bit of address range for
>> +    // compatibility with ACPI 1.0 OS.
>> +    //
>> +    // This is done because ACPI 1.0 pointers are 32 bit values.
>> +    // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses.
>> +    // There is no architectural reason these should be below 4GB, it is purely
>> +    // for convenience of implementation that we force memory below 4GB.
> 
> Hmmm. I first thought that the last two lines of the commend should not
> be re-instated. But I was wrong. Because supporting ACPI 1.0b causes us
> to allocate even areas pointed-to by XSDT entries under 4GB.
> 
>> +    //
>> +    PageAddress = 0xFFFFFFFF;
>> +    Status = gBS->AllocatePages (
>> +                    mAcpiTableAllocType,
>> +                    EfiACPIReclaimMemory,
>> +                    EFI_SIZE_TO_PAGES (TotalSize),
>> +                    &PageAddress
>> +                    );
>> +    Pointer = (UINT8 *)(UINTN)PageAddress;
> 
> (1) Same comment as before; we shouldn't convert PageAddress to a
> pointer unless the allocation succeeds.
> 
>> +  } else {
>> +    Status = gBS->AllocatePool (
>> +                    EfiACPIReclaimMemory,
>> +                    TotalSize,
>> +                    (VOID **)&Pointer);
>> +  }
>>  
>>    if (EFI_ERROR (Status)) {
>>      return EFI_OUT_OF_RESOURCES;
>>    }
>>  
>> -  Pointer = (UINT8 *) (UINTN) PageAddress;
>>    ZeroMem (Pointer, TotalSize);
>>  
>>    AcpiTableInstance->Rsdt1 = (EFI_ACPI_DESCRIPTION_HEADER *) Pointer;
>> @@ -406,21 +413,26 @@ ReallocateAcpiTableBuffer (
>>    }
>>    CopyMem (AcpiTableInstance->Xsdt, TempPrivateData.Xsdt, (sizeof (EFI_ACPI_DESCRIPTION_HEADER) + mEfiAcpiMaxNumTables * sizeof (UINT64)));
>>  
>> -  //
>> -  // Calculate orignal ACPI table buffer size
>> -  //
>> -  TotalSize = sizeof (EFI_ACPI_DESCRIPTION_HEADER) +         // for ACPI 2.0/3.0 XSDT
>> -              mEfiAcpiMaxNumTables * sizeof (UINT64);
>> +  if (mAcpiTableAllocType != AllocateAnyPages) {
>> +    //
>> +    // Calculate orignal ACPI table buffer size
>> +    //
>> +    TotalSize = sizeof (EFI_ACPI_DESCRIPTION_HEADER) +         // for ACPI 2.0/3.0 XSDT
>> +                mEfiAcpiMaxNumTables * sizeof (UINT64);
>>  
>> -  if ((PcdGet32 (PcdAcpiExposedTableVersions) & EFI_ACPI_TABLE_VERSION_1_0B) != 0) {
>> -    TotalSize += sizeof (EFI_ACPI_DESCRIPTION_HEADER) +         // for ACPI 1.0 RSDT
>> -                 mEfiAcpiMaxNumTables * sizeof (UINT32) +
>> -                 sizeof (EFI_ACPI_DESCRIPTION_HEADER) +         // for ACPI 2.0/3.0 RSDT
>> -                 mEfiAcpiMaxNumTables * sizeof (UINT32);
>> +    if ((PcdGet32 (PcdAcpiExposedTableVersions) & EFI_ACPI_TABLE_VERSION_1_0B) != 0) {
>> +      TotalSize += sizeof (EFI_ACPI_DESCRIPTION_HEADER) +         // for ACPI 1.0 RSDT
>> +                   mEfiAcpiMaxNumTables * sizeof (UINT32) +
>> +                   sizeof (EFI_ACPI_DESCRIPTION_HEADER) +         // for ACPI 2.0/3.0 RSDT
>> +                   mEfiAcpiMaxNumTables * sizeof (UINT32);
>> +    }
>> +
>> +    gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)TempPrivateData.Rsdt1,
>> +           EFI_SIZE_TO_PAGES (TotalSize));
>> +  } else {
>> +    gBS->FreePool (TempPrivateData.Rsdt1);
>>    }
>>  
>> -  gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)TempPrivateData.Rsdt1, EFI_SIZE_TO_PAGES (TotalSize));
>> -
>>    //
>>    // Update the Max ACPI table number
>>    //
>> @@ -1759,29 +1771,36 @@ AcpiTableAcpiTableConstructor (
>>                   mEfiAcpiMaxNumTables * sizeof (UINT32);
>>    }
>>  
>> -  //
>> -  // Allocate memory in the lower 32 bit of address range for
>> -  // compatibility with ACPI 1.0 OS.
>> -  //
>> -  // This is done because ACPI 1.0 pointers are 32 bit values.
>> -  // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses.
>> -  // There is no architectural reason these should be below 4GB, it is purely
>> -  // for convenience of implementation that we force memory below 4GB.
>> -  //
>> -  PageAddress = 0xFFFFFFFF;
>> -  Status = gBS->AllocatePages (
>> -                  mAcpiTableAllocType,
>> -                  EfiACPIReclaimMemory,
>> -                  EFI_SIZE_TO_PAGES (TotalSize),
>> -                  &PageAddress
>> -                  );
>> +  if (mAcpiTableAllocType != AllocateAnyPages) {
>> +    //
>> +    // Allocate memory in the lower 32 bit of address range for
>> +    // compatibility with ACPI 1.0 OS.
>> +    //
>> +    // This is done because ACPI 1.0 pointers are 32 bit values.
>> +    // ACPI 2.0 OS and all 64 bit OS must use the 64 bit ACPI table addresses.
>> +    // There is no architectural reason these should be below 4GB, it is purely
>> +    // for convenience of implementation that we force memory below 4GB.
>> +    //
>> +    PageAddress = 0xFFFFFFFF;
>> +    Status = gBS->AllocatePages (
>> +                    mAcpiTableAllocType,
>> +                    EfiACPIReclaimMemory,
>> +                    EFI_SIZE_TO_PAGES (TotalSize),
>> +                    &PageAddress
>> +                    );
>> +    Pointer = (UINT8 *)(UINTN)PageAddress;
> 
> (2) Same as (1).
> 
> Looks reasonable otherwise.

(3) Style: for both gBS->AllocatePool() calls added in this patch,
please either break the closing paren to a new line, or use the
"condensed" style (which you do use elsewhere in these patches).

Thanks!
Laszlo


> 
>> +  } else {
>> +    Status = gBS->AllocatePool (
>> +                    EfiACPIReclaimMemory,
>> +                    TotalSize,
>> +                    (VOID **)&Pointer);
>> +  }
>>  
>>    if (EFI_ERROR (Status)) {
>>      gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)AcpiTableInstance->Rsdp1, EFI_SIZE_TO_PAGES (RsdpTableSize));
>>      return EFI_OUT_OF_RESOURCES;
>>    }
>>  
>> -  Pointer = (UINT8 *) (UINTN) PageAddress;
>>    ZeroMem (Pointer, TotalSize);
>>  
>>    AcpiTableInstance->Rsdt1 = (EFI_ACPI_DESCRIPTION_HEADER *) Pointer;
>>
>