Re: [PATCH v2 23/28] NXP/LS1043aRdbPkg/ArmPlatformLib: Use Allocate pool

["Leif Lindholm" <leif@nuviainc.com>]

On Fri, Mar 20, 2020 at 20:05:38 +0530, Pankaj Bansal wrote:
> From: Pankaj Bansal <pankaj.bansal@nxp.com>
> 
> Allocate Pages may allocate more memory than required for
> VirtualMemoryTable.
> There is no special requirement that VirtualMemoryTable size should be
> page size aligned.
> 
> Therefore, replace AllocatePages with AllocatePool.
> 
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>

I don't object to this as such (although one comment), but what is the
purpose of this change?

My comment is that most other platforms use AllocatePages for this. So
this is diverging from the norm. Secondly, while I don't necessarily
*like* the current design (copied across most ARM platforms), it's
somewhat mitigated by the AllocatePages giving you a minimum of 128
entries before you start overwriting things. I don't know what you'll
overwrite if you do go too far, but you will overwrite it *before* the
ASSERT ever gets evaluated.

/
    Leif

> ---
>  .../LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLib.inf  | 1 +
>  .../LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLibMem.c | 5 +++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLib.inf b/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLib.inf
> index 1faf99b99c54..c64032f32772 100644
> --- a/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLib.inf
> +++ b/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLib.inf
> @@ -25,6 +25,7 @@ [Packages]
>  
>  [LibraryClasses]
>    ArmLib
> +  DebugLib
>    SocLib
>  
>  [Sources.common]
> diff --git a/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLibMem.c b/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLibMem.c
> index f5fa308551aa..f8dd642e3cff 100644
> --- a/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLibMem.c
> +++ b/Platform/NXP/LS1043aRdbPkg/Library/ArmPlatformLib/ArmPlatformLibMem.c
> @@ -43,10 +43,11 @@ ArmPlatformGetVirtualMemoryMap (
>  
>    ASSERT (VirtualMemoryMap != NULL);
>  
> -  VirtualMemoryTable = (ARM_MEMORY_REGION_DESCRIPTOR*)AllocatePages (
> -          EFI_SIZE_TO_PAGES (sizeof (ARM_MEMORY_REGION_DESCRIPTOR) * MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS));
> +  VirtualMemoryTable = AllocatePool (sizeof (ARM_MEMORY_REGION_DESCRIPTOR) *
> +                                     MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS);
>  
>    if (VirtualMemoryTable == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: Error: Failed AllocatePool()\n", __FUNCTION__));
>      return;
>    }
>  
> -- 
> 2.17.1
>