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

["Pankaj Bansal" <pankaj.bansal@nxp.com>]

> -----Original Message-----
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Wednesday, April 1, 2020 11:34 PM
> To: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>
> Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Michael D Kinney
> <michael.d.kinney@intel.com>; devel@edk2.groups.io; Varun Sethi
> <V.Sethi@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>; Jon Nettleton <jon@solid-run.com>
> Subject: Re: [PATCH v2 23/28] NXP/LS1043aRdbPkg/ArmPlatformLib: Use
> Allocate pool
> 
> 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. 

I referred ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoLib.c

> 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.
> 

We can improve this by evaluating ASSERT after each entry like this :
  VirtualMemoryTable[Index].PhysicalBase = 0xXXXXXXXX;
  VirtualMemoryTable[Index].VirtualBase  = 0xXXXXXXXX;
  VirtualMemoryTable[Index].Length       = 0xXXXXXXXX;
  VirtualMemoryTable[Index++].Attributes   = 0xXXXXXXXX;

  ASSERT (Index < MAX_VIRTUAL_MEMORY_MAP_DESCRIPTORS);

> /
>     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
> >