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

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

On 10/16/20 17:49, Ard Biesheuvel wrote:
> Use a pool allocation for the RSDP ACPI root pointer structure if no
> memory limit is in effect that forces us to use page based allocation,
> which may be wasteful if they get rounded up to 64 KB as is the case
> on AArch64.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com>
> ---
>  MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 30 ++++++++++++++------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> index 22f49a8489e7..fb939aa00f49 100644
> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> @@ -1737,19 +1737,26 @@ AcpiTableAcpiTableConstructor (
>      RsdpTableSize += sizeof (EFI_ACPI_1_0_ROOT_SYSTEM_DESCRIPTION_POINTER);
>    }
>  
> -  PageAddress = 0xFFFFFFFF;
> -  Status = gBS->AllocatePages (
> -                  mAcpiTableAllocType,
> -                  EfiACPIReclaimMemory,
> -                  EFI_SIZE_TO_PAGES (RsdpTableSize),
> -                  &PageAddress
> -                  );
> +  if (mAcpiTableAllocType != AllocateAnyPages) {
> +    PageAddress = 0xFFFFFFFF;
> +    Status = gBS->AllocatePages (
> +                    mAcpiTableAllocType,
> +                    EfiACPIReclaimMemory,
> +                    EFI_SIZE_TO_PAGES (RsdpTableSize),
> +                    &PageAddress
> +                    );
> +    Pointer = (UINT8 *)(UINTN)PageAddress;

(1) (same as earlier) please check for success before assigning the pointer

> +  } else {
> +    Status = gBS->AllocatePool (
> +                    EfiACPIReclaimMemory,
> +                    RsdpTableSize,
> +                    (VOID **)&Pointer);

(2) (same as earlier) style: please break off the closing paren, or
condense all the arguments.

Looks OK other than these.

Thanks
Laszlo

> +  }
>  
>    if (EFI_ERROR (Status)) {
>      return EFI_OUT_OF_RESOURCES;
>    }
>  
> -  Pointer = (UINT8 *) (UINTN) PageAddress;
>    ZeroMem (Pointer, RsdpTableSize);
>  
>    AcpiTableInstance->Rsdp1 = (EFI_ACPI_1_0_ROOT_SYSTEM_DESCRIPTION_POINTER *) Pointer;
> @@ -1797,7 +1804,12 @@ AcpiTableAcpiTableConstructor (
>    }
>  
>    if (EFI_ERROR (Status)) {
> -    gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)AcpiTableInstance->Rsdp1, EFI_SIZE_TO_PAGES (RsdpTableSize));
> +    if (mAcpiTableAllocType != AllocateAnyPages) {
> +      gBS->FreePages ((EFI_PHYSICAL_ADDRESS)(UINTN)AcpiTableInstance->Rsdp1,
> +             EFI_SIZE_TO_PAGES (RsdpTableSize));
> +    } else {
> +      gBS->FreePool (AcpiTableInstance->Rsdp1);
> +    }
>      return EFI_OUT_OF_RESOURCES;
>    }
>  
>